Merge pull request #2662 from ethereum/fixEcrecover2

Fix ecrecover memory clear

Changelog.md

@@ -13,6 +13,7 @@ Features:
  * Type checker: Warn when existing symbols, including builtins, are overwritten.
 
 Bugfixes:
+ * Code Generator: Properly clear return memory area for ecrecover.
  * Type Checker: Fix crash for some assignment to non-lvalue.
  * Type Checker: Fix invalid "specify storage keyword" warning for reference members of structs.
  * Type Checker: Mark modifiers as internal.

docs/bugs.json

@@ -1,4 +1,11 @@
 [
+    {
+        "name": "ECRecoverMalformedInput",
+	"summary": "The ecrecover() builtin can return garbage for malformed input.",
+        "description": "The ecrecover precompile does not properly signal failure for malformed input (especially in the 'v' argument) and thus the Solidity function can return data that was previously present in the return area in memory.",
+        "fixed": "0.4.14",
+        "severity": "medium"
+    },
     {
         "name": "SkipEmptyStringLiteral",
 	"summary": "If \"\" is used in a function call, the following function arguments will not be correctly passed to the function.",

docs/bugs_by_version.json

@@ -1,6 +1,7 @@
 {
     "0.1.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -16,6 +17,7 @@
     }, 
     "0.1.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -31,6 +33,7 @@
     }, 
     "0.1.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -46,6 +49,7 @@
     }, 
     "0.1.3": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -61,6 +65,7 @@
     }, 
     "0.1.4": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -76,6 +81,7 @@
     }, 
     "0.1.5": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -91,6 +97,7 @@
     }, 
     "0.1.6": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -107,6 +114,7 @@
     }, 
     "0.1.7": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -123,6 +131,7 @@
     }, 
     "0.2.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -139,6 +148,7 @@
     }, 
     "0.2.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -155,6 +165,7 @@
     }, 
     "0.2.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -171,6 +182,7 @@
     }, 
     "0.3.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -186,6 +198,7 @@
     }, 
     "0.3.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -200,6 +213,7 @@
     }, 
     "0.3.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -214,6 +228,7 @@
     }, 
     "0.3.3": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -227,6 +242,7 @@
     }, 
     "0.3.4": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -240,6 +256,7 @@
     }, 
     "0.3.5": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -253,6 +270,7 @@
     }, 
     "0.3.6": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -264,6 +282,7 @@
     }, 
     "0.4.0": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -275,6 +294,7 @@
     }, 
     "0.4.1": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -286,6 +306,7 @@
     }, 
     "0.4.10": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
@@ -293,20 +314,26 @@
     }, 
     "0.4.11": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral"
         ], 
         "released": "2017-05-03"
     }, 
     "0.4.12": {
-        "bugs": [], 
+        "bugs": [
+            "ECRecoverMalformedInput"
+        ], 
         "released": "2017-07-03"
     }, 
     "0.4.13": {
-        "bugs": [], 
+        "bugs": [
+            "ECRecoverMalformedInput"
+        ], 
         "released": "2017-07-06"
     }, 
     "0.4.2": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -317,6 +344,7 @@
     }, 
     "0.4.3": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -326,6 +354,7 @@
     }, 
     "0.4.4": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored"
@@ -334,6 +363,7 @@
     }, 
     "0.4.5": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored", 
@@ -343,6 +373,7 @@
     }, 
     "0.4.6": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction", 
             "IdentityPrecompileReturnIgnored"
@@ -351,6 +382,7 @@
     }, 
     "0.4.7": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
@@ -358,6 +390,7 @@
     }, 
     "0.4.8": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 
@@ -365,6 +398,7 @@
     }, 
     "0.4.9": {
         "bugs": [
+            "ECRecoverMalformedInput", 
             "SkipEmptyStringLiteral", 
             "ConstantOptimizerSubtraction"
         ], 

libsolidity/codegen/ExpressionCompiler.cpp

@@ -1622,7 +1622,7 @@ void ExpressionCompiler::appendExternalFunctionCall(
 		// zero bytes (which we cannot detect).
 		solAssert(0 < retSize && retSize <= 32, "");
 		utils().fetchFreeMemoryPointer();
-		m_context << Instruction::DUP1 << u256(0) << Instruction::MSTORE;
+		m_context << u256(0) << Instruction::DUP2 << Instruction::MSTORE;
 		m_context << u256(32) << Instruction::ADD;
 		utils().storeFreeMemoryPointer();
 	}

test/libsolidity/SolidityEndToEndTest.cpp

@@ -8251,6 +8251,53 @@ BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input)
 	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
 }
 
+BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input_proper)
+{
+	char const* sourceCode = R"(
+		contract C {
+			function f() returns (address) {
+				return recover(
+					0x77e5189111eb6557e8a637b27ef8fbb15bc61d61c2f00cc48878f3a296e5e0ca,
+					0, // invalid v value
+					0x6944c77849b18048f6abe0db8084b0d0d0689cdddb53d2671c36967b58691ad4,
+					0xef4f06ba4f78319baafd0424365777241af4dfd3da840471b4b4b087b7750d0d,
+					0xca35b7d915458ef540ade6068dfe2f44e8fa733c,
+					0xca35b7d915458ef540ade6068dfe2f44e8fa733c
+				);
+			}
+			function recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s, uint blockExpired, bytes32 salt)
+				returns (address)
+			{
+				require(hash == keccak256(blockExpired, salt));
+				return ecrecover(hash, v, r, s);
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
+}
+
+BOOST_AUTO_TEST_CASE(failing_ecrecover_invalid_input_asm)
+{
+	char const* sourceCode = R"(
+		contract C {
+			function f() returns (address) {
+				assembly {
+					mstore(mload(0x40), 0xca35b7d915458ef540ade6068dfe2f44e8fa733c)
+				}
+				return ecrecover(
+					0x77e5189111eb6557e8a637b27ef8fbb15bc61d61c2f00cc48878f3a296e5e0ca,
+					0, // invalid v value
+					0x6944c77849b18048f6abe0db8084b0d0d0689cdddb53d2671c36967b58691ad4,
+					0xef4f06ba4f78319baafd0424365777241af4dfd3da840471b4b4b087b7750d0d
+				);
+			}
+		}
+	)";
+	compileAndRun(sourceCode, 0, "C");
+	BOOST_CHECK(callContractFunction("f()") == encodeArgs(u256(0)));
+}
+
 BOOST_AUTO_TEST_CASE(calling_nonexisting_contract_throws)
 {
 	char const* sourceCode = R"(