From 034d436933806c204b84b0500e2116938cb6a030 Mon Sep 17 00:00:00 2001 From: chriseth Date: Sun, 7 Aug 2016 19:40:19 +0200 Subject: [PATCH 1/2] Make ecrecover throw for malformed input. --- libsolidity/codegen/ExpressionCompiler.cpp | 45 +++++++++++++++++++--- test/libsolidity/SolidityEndToEndTest.cpp | 18 +++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 1d5745560..50148901c 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1445,6 +1445,19 @@ void ExpressionCompiler::appendExternalFunctionCall( argumentTypes.push_back(_arguments[i]->annotation().type); } + if (funKind == FunctionKind::ECRecover) + { + // Clears 32 bytes of currently free memory and advances free memory pointer. + // Output area will be "start of input area" - 32. + // The reason is that a failing ECRecover cannot be detected, it will just return + // zero bytes (which we cannot detect). + solAssert(0 < retSize && retSize <= 32, ""); + utils().fetchFreeMemoryPointer(); + m_context << Instruction::DUP1 << u256(0) << Instruction::MSTORE; + m_context << u256(32) << Instruction::ADD; + utils().storeFreeMemoryPointer(); + } + // Copy function identifier to memory. utils().fetchFreeMemoryPointer(); if (!_functionType.isBareCall() || manualFunctionId) @@ -1453,7 +1466,7 @@ void ExpressionCompiler::appendExternalFunctionCall( utils().storeInMemoryDynamic(IntegerType(8 * CompilerUtils::dataStartOffset), false); } // If the function takes arbitrary parameters, copy dynamic length data in place. - // Move argumenst to memory, will not update the free memory pointer (but will update the memory + // Move arguments to memory, will not update the free memory pointer (but will update the memory // pointer on the stack). utils().encodeToMemory( argumentTypes, @@ -1471,12 +1484,24 @@ void ExpressionCompiler::appendExternalFunctionCall( // function identifier [unless bare] // contract address - // Output data will replace input data. + // Output data will replace input data, unless we have ECRecover (then, output + // area will be 32 bytes just before input area). // put on stack: m_context << u256(retSize); - utils().fetchFreeMemoryPointer(); - m_context << Instruction::DUP1 << Instruction::DUP4 << Instruction::SUB; - m_context << Instruction::DUP2; + utils().fetchFreeMemoryPointer(); // This is the start of input + if (funKind == FunctionKind::ECRecover) + { + // In this case, output is 32 bytes before input and has already been cleared. + m_context << u256(32) << Instruction::DUP2 << Instruction::SUB << Instruction::SWAP1; + // Here: + m_context << Instruction::DUP1 << Instruction::DUP5 << Instruction::SUB; + m_context << Instruction::SWAP1; + } + else + { + m_context << Instruction::DUP1 << Instruction::DUP4 << Instruction::SUB; + m_context << Instruction::DUP2; + } // CALL arguments: outSize, outOff, inSize, inOff (already present up to here) // [value,] addr, gas (stack top) @@ -1539,6 +1564,16 @@ void ExpressionCompiler::appendExternalFunctionCall( utils().loadFromMemoryDynamic(IntegerType(160), false, true, false); utils().convertType(IntegerType(160), FixedBytesType(20)); } + else if (funKind == FunctionKind::ECRecover) + { + // Output is 32 bytes before input / free mem pointer. + // Failing ecrecover cannot be detected, so we clear output before the call. + m_context << u256(32); + utils().fetchFreeMemoryPointer(); + m_context << Instruction::SUB << Instruction::MLOAD; + m_context << Instruction::DUP1 << Instruction::ISZERO; + m_context.appendConditionalJumpTo(m_context.errorTag()); + } else if (!_functionType.returnParameterTypes().empty()) { utils().fetchFreeMemoryPointer(); diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 1b7c5ea46..338a47da0 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -6853,6 +6853,24 @@ BOOST_AUTO_TEST_CASE(create_dynamic_array_with_zero_length) BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(7))); } +BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input) +{ + // ecrecover should throw for malformed input + // (v should be 27 or 28, not 1) + // This is quite hard to test because the precompiled does NOT throw, instead it just + // does not write to its output area, we have to check that and currently do it + // by checking whether ecrecover "returns" zero. + char const* sourceCode = R"( + contract C { + function f() returns (address) { + return ecrecover(bytes32(uint(-1)), 1, 2, 3); + } + } + )"; + compileAndRun(sourceCode, 0, "C"); + BOOST_CHECK(callContractFunction("f()") == encodeArgs()); +} + BOOST_AUTO_TEST_SUITE_END() } From d731225d02c9b8d6dc9f2ba632923b765d0e111d Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 12 Aug 2016 15:54:17 +0200 Subject: [PATCH 2/2] Actually better to return zero on error. --- docs/miscellaneous.rst | 2 +- docs/units-and-global-variables.rst | 2 +- libsolidity/codegen/ExpressionCompiler.cpp | 2 -- test/libsolidity/SolidityEndToEndTest.cpp | 8 +++----- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/docs/miscellaneous.rst b/docs/miscellaneous.rst index ca0cf5930..804d69ef0 100644 --- a/docs/miscellaneous.rst +++ b/docs/miscellaneous.rst @@ -286,7 +286,7 @@ Global Variables - ``sha3(...) returns (bytes32)``: compute the Ethereum-SHA-3 (KECCAK-256) hash of the (tightly packed) arguments - ``sha256(...) returns (bytes32)``: compute the SHA-256 hash of the (tightly packed) arguments - ``ripemd160(...) returns (bytes20)``: compute the RIPEMD-160 hash of the (tightly packed) arguments -- ``ecrecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) returns (address)``: recover address associated with the public key from elliptic curve signature +- ``ecrecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) returns (address)``: recover address associated with the public key from elliptic curve signature, return zero on error - ``addmod(uint x, uint y, uint k) returns (uint)``: compute ``(x + y) % k`` where the addition is performed with arbitrary precision and does not wrap around at ``2**256`` - ``mulmod(uint x, uint y, uint k) returns (uint)``: compute ``(x * y) % k`` where the multiplication is performed with arbitrary precision and does not wrap around at ``2**256`` - ``this`` (current contract's type): the current contract, explicitly convertible to ``address`` diff --git a/docs/units-and-global-variables.rst b/docs/units-and-global-variables.rst index 62b9158d2..d1d578ed2 100644 --- a/docs/units-and-global-variables.rst +++ b/docs/units-and-global-variables.rst @@ -95,7 +95,7 @@ Mathematical and Cryptographic Functions ``ripemd160(...) returns (bytes20)``: compute RIPEMD-160 hash of the (tightly packed) arguments ``ecrecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) returns (address)``: - recover the address associated with the public key from elliptic curve signature + recover the address associated with the public key from elliptic curve signature or return zero on error In the above, "tightly packed" means that the arguments are concatenated without padding. This means that the following are all identical:: diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 50148901c..65326669b 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1571,8 +1571,6 @@ void ExpressionCompiler::appendExternalFunctionCall( m_context << u256(32); utils().fetchFreeMemoryPointer(); m_context << Instruction::SUB << Instruction::MLOAD; - m_context << Instruction::DUP1 << Instruction::ISZERO; - m_context.appendConditionalJumpTo(m_context.errorTag()); } else if (!_functionType.returnParameterTypes().empty()) { diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 338a47da0..345bac80c 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -6855,11 +6855,9 @@ BOOST_AUTO_TEST_CASE(create_dynamic_array_with_zero_length) BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input) { - // ecrecover should throw for malformed input + // ecrecover should return zero for malformed input // (v should be 27 or 28, not 1) - // This is quite hard to test because the precompiled does NOT throw, instead it just - // does not write to its output area, we have to check that and currently do it - // by checking whether ecrecover "returns" zero. + // Note that the precompile does not return zero but returns nothing. char const* sourceCode = R"( contract C { function f() returns (address) { @@ -6868,7 +6866,7 @@ BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input) } )"; compileAndRun(sourceCode, 0, "C"); - BOOST_CHECK(callContractFunction("f()") == encodeArgs()); + BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0))); } BOOST_AUTO_TEST_SUITE_END()