From 2136bc74fdcf04fd361e7b209660bcf3bafde160 Mon Sep 17 00:00:00 2001 From: Dustin Brody Date: Mon, 6 Aug 2018 16:33:20 +0000 Subject: [PATCH] Sanitize memory addresses and lengths (#97) * add a helper function to ensure for memory addressing and length purposes, especially as applied to array indexing and bounds-checking, that non-negative UInt256 numbers remain non-negative when lossily converted to int's --- VMTests.md | 16 ++++++------- nimbus/vm/interpreter/opcodes_impl.nim | 32 ++++++++++++++++---------- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/VMTests.md b/VMTests.md index 31d1b1ef3..05370ca7f 100644 --- a/VMTests.md +++ b/VMTests.md @@ -295,15 +295,15 @@ OK: 5/5 Fail: 0/5 Skip: 0/5 + calldatacopyUnderFlow.json OK + calldatacopyZeroMemExpansion.json OK + calldatacopyZeroMemExpansion_return.json OK -- calldatacopy_DataIndexTooHigh.json Fail -- calldatacopy_DataIndexTooHigh2.json Fail -- calldatacopy_DataIndexTooHigh2_return.json Fail -- calldatacopy_DataIndexTooHigh_return.json Fail ++ calldatacopy_DataIndexTooHigh.json OK ++ calldatacopy_DataIndexTooHigh2.json OK ++ calldatacopy_DataIndexTooHigh2_return.json OK ++ calldatacopy_DataIndexTooHigh_return.json OK + calldatacopy_sec.json OK + calldataload0.json OK + calldataload1.json OK + calldataload2.json OK -- calldataloadSizeTooHigh.json Fail ++ calldataloadSizeTooHigh.json OK + calldataloadSizeTooHighPartial.json OK + calldataload_BigOffset.json OK + calldatasize0.json OK @@ -313,20 +313,20 @@ OK: 5/5 Fail: 0/5 Skip: 0/5 + callvalue.json OK + codecopy0.json OK + codecopyZeroMemExpansion.json OK -- codecopy_DataIndexTooHigh.json Fail ++ codecopy_DataIndexTooHigh.json OK + codesize.json OK + extcodecopy0.json OK + extcodecopy0AddressTooBigLeft.json OK + extcodecopy0AddressTooBigRight.json OK + extcodecopyZeroMemExpansion.json OK -- extcodecopy_DataIndexTooHigh.json Fail ++ extcodecopy_DataIndexTooHigh.json OK + extcodesize0.json OK + extcodesize1.json OK + extcodesizeUnderFlow.json OK + gasprice.json OK + origin.json OK ``` -OK: 44/51 Fail: 7/51 Skip: 0/51 +OK: 51/51 Fail: 0/51 Skip: 0/51 ## vmIOandFlowOperations ```diff + BlockNumberDynamicJump0_AfterJumpdest.json OK diff --git a/nimbus/vm/interpreter/opcodes_impl.nim b/nimbus/vm/interpreter/opcodes_impl.nim index 3c8fe37a3..ec5c1302b 100644 --- a/nimbus/vm/interpreter/opcodes_impl.nim +++ b/nimbus/vm/interpreter/opcodes_impl.nim @@ -224,6 +224,14 @@ proc writePaddedResult(mem: var Memory, # Note, we don't need to write padding bytes # mem.extend already pads with zero properly +func cleanMemRef(x: UInt256): int {.inline.} = + ## Sanitize memory addresses, catch negative or impossibly big offsets + # See https://github.com/status-im/nimbus/pull/97 for more info + const upperBound = high(int32).u256 + if x > upperBound: + return high(int32) + return x.toInt + op address, inline = true: ## 0x30, Get address of currently executing account. push: computation.msg.storageAddress @@ -249,7 +257,7 @@ op callValue, inline = true: op callDataLoad, inline = false, startPos: ## 0x35, Get input data of current environment # TODO simplification: https://github.com/status-im/nimbus/issues/67 - let dataPos = startPos.toInt + let dataPos = startPos.cleanMemRef if dataPos >= computation.msg.data.len: push: 0 return @@ -278,7 +286,7 @@ op callDataCopy, inline = false, memStartPos, copyStartPos, size: ## 0x37, Copy input data in current environment to memory. # TODO tests: https://github.com/status-im/nimbus/issues/67 - let (memPos, copyPos, len) = (memStartPos.toInt, copyStartPos.toInt, size.toInt) + let (memPos, copyPos, len) = (memStartPos.cleanMemRef, copyStartPos.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[CallDataCopy].m_handler(computation.memory.len, memPos, len), @@ -294,7 +302,7 @@ op codecopy, inline = false, memStartPos, copyStartPos, size: ## 0x39, Copy code running in current environment to memory. # TODO tests: https://github.com/status-im/nimbus/issues/67 - let (memPos, copyPos, len) = (memStartPos.toInt, copyStartPos.toInt, size.toInt) + let (memPos, copyPos, len) = (memStartPos.cleanMemRef, copyStartPos.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[CodeCopy].m_handler(computation.memory.len, memPos, len), @@ -316,7 +324,7 @@ op extCodeCopy, inline = true: ## 0x3c, Copy an account's code to memory. let account = computation.stack.popAddress() let (memStartPos, codeStartPos, size) = computation.stack.popInt(3) - let (memPos, codePos, len) = (memStartPos.toInt, codeStartPos.toInt, size.toInt) + let (memPos, codePos, len) = (memStartPos.cleanMemRef, codeStartPos.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[ExtCodeCopy].m_handler(computation.memory.len, memPos, len), @@ -331,7 +339,7 @@ op returnDataSize, inline = true: op returnDataCopy, inline = false, memStartPos, copyStartPos, size: ## 0x3e, Copy output data from the previous call to memory. - let (memPos, copyPos, len) = (memStartPos.toInt, copyStartPos.toInt, size.toInt) + let (memPos, copyPos, len) = (memStartPos.cleanMemRef, copyStartPos.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[CodeCopy].m_handler(memPos, copyPos, len), @@ -387,7 +395,7 @@ op pop, inline = true: op mload, inline = true, memStartPos: ## 0x51, Load word from memory - let memPos = memStartPos.toInt + let memPos = memStartPos.cleanMemRef computation.gasMeter.consumeGas( computation.gasCosts[MLoad].m_handler(computation.memory.len, memPos, 32), @@ -399,7 +407,7 @@ op mload, inline = true, memStartPos: op mstore, inline = true, memStartPos, value: ## 0x52, Save word to memory - let memPos = memStartPos.toInt + let memPos = memStartPos.cleanMemRef computation.gasMeter.consumeGas( computation.gasCosts[MStore].m_handler(computation.memory.len, memPos, 32), @@ -411,7 +419,7 @@ op mstore, inline = true, memStartPos, value: op mstore8, inline = true, memStartPos, value: ## 0x53, Save byte to memory - let memPos = memStartPos.toInt + let memPos = memStartPos.cleanMemRef computation.gasMeter.consumeGas( computation.gasCosts[MStore].m_handler(computation.memory.len, memPos, 1), @@ -501,7 +509,7 @@ op create, inline = false, value, startPosition, size: ## 0xf0, Create a new account with associated code. # TODO: Forked create for Homestead - let (memPos, len) = (startPosition.toInt, size.toInt) + let (memPos, len) = (startPosition.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[CodeCopy].m_handler(computation.memory.len, memPos, len), @@ -663,7 +671,7 @@ template genCall(callName: untyped): untyped = shouldTransferValue, isStatic) = `callName Params`(computation) - let (memInPos, memInLen, memOutPos, memOutLen) = (memoryInputStartPosition.toInt, memoryInputSize.toInt, memoryOutputStartPosition.toInt, memoryOutputSize.toInt) + let (memInPos, memInLen, memOutPos, memOutLen) = (memoryInputStartPosition.cleanMemRef, memoryInputSize.cleanMemRef, memoryOutputStartPosition.cleanMemRef, memoryOutputSize.cleanMemRef) let (gasCost, childMsgGas) = computation.gasCosts[Op.Call].c_handler( value, @@ -751,7 +759,7 @@ genCall(staticCall) op returnOp, inline = false, startPos, size: ## 0xf3, Halt execution returning output data. - let (pos, len) = (startPos.toInt, size.toInt) + let (pos, len) = (startPos.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[Return].m_handler(computation.memory.len, pos, len), @@ -763,7 +771,7 @@ op returnOp, inline = false, startPos, size: op revert, inline = false, startPos, size: ## 0xfd, Halt execution reverting state changes but returning data and remaining gas. - let (pos, len) = (startPos.toInt, size.toInt) + let (pos, len) = (startPos.cleanMemRef, size.cleanMemRef) computation.gasMeter.consumeGas( computation.gasCosts[Revert].m_handler(computation.memory.len, pos, len),