Merge pull request #11221 from ethereum/fixCalldataDecodingOverflowBug

Fix calldata decoding overflow bug

Changelog.md

@@ -1,5 +1,9 @@
 ### 0.8.4 (unreleased)
 
+Important Bugfixes:
+ * ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of ``abi.decode`` can depend on data elsewhere in memory. Calldata decoding is not affected.
+
+
 Language Features:
  * Assembly / Yul: Allow hex string literals.
  * Possibility to use ``bytes.concat`` with variable number of ``bytes`` and ``bytesNN`` arguments which behaves as a restricted version of `abi.encodePacked` with a more descriptive name.

docs/bugs.json

@@ -1,4 +1,15 @@
 [
+    {
+        "name": "ABIDecodeTwoDimensionalArrayMemory",
+        "summary": "If used on memory byte arrays, result of the function ``abi.decode`` can depend on the contents of memory outside of the actual byte array that is decoded.",
+        "description": "The ABI specification uses pointers to data areas for everything that is dynamically-sized. When decoding data from memory (instead of calldata), the ABI decoder did not properly validate some of these pointers. More specifically, it was possible to use large values for the pointers inside arrays such that computing the offset resulted in an undetected overflow. This could lead to these pointers targeting areas in memory outside of the actual area to be decoded. This way, it was possible for ``abi.decode`` to return different values for the same encoded byte array.",
+        "introduced": "0.4.16",
+        "fixed": "0.8.4",
+        "conditions": {
+            "ABIEncoderV2": true
+        },
+        "severity": "very low"
+    },
     {
         "name": "KeccakCaching",
         "summary": "The bytecode optimizer incorrectly re-used previously evaluated Keccak-256 hashes. You are unlikely to be affected if you do not compute Keccak-256 hashes in inline assembly.",

docs/bugs_by_version.json

@@ -595,6 +595,7 @@
     },
     "0.4.16": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -615,6 +616,7 @@
     },
     "0.4.17": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -636,6 +638,7 @@
     },
     "0.4.18": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -656,6 +659,7 @@
     },
     "0.4.19": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -699,6 +703,7 @@
     },
     "0.4.20": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -720,6 +725,7 @@
     },
     "0.4.21": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -741,6 +747,7 @@
     },
     "0.4.22": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -762,6 +769,7 @@
     },
     "0.4.23": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -782,6 +790,7 @@
     },
     "0.4.24": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -802,6 +811,7 @@
     },
     "0.4.25": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -820,6 +830,7 @@
     },
     "0.4.26": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -987,6 +998,7 @@
     },
     "0.5.0": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1005,6 +1017,7 @@
     },
     "0.5.1": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1023,6 +1036,7 @@
     },
     "0.5.10": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1037,6 +1051,7 @@
     },
     "0.5.11": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1050,6 +1065,7 @@
     },
     "0.5.12": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1063,6 +1079,7 @@
     },
     "0.5.13": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1076,6 +1093,7 @@
     },
     "0.5.14": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1091,6 +1109,7 @@
     },
     "0.5.15": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1105,6 +1124,7 @@
     },
     "0.5.16": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1118,6 +1138,7 @@
     },
     "0.5.17": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1130,6 +1151,7 @@
     },
     "0.5.2": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1148,6 +1170,7 @@
     },
     "0.5.3": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1166,6 +1189,7 @@
     },
     "0.5.4": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1184,6 +1208,7 @@
     },
     "0.5.5": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1204,6 +1229,7 @@
     },
     "0.5.6": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1224,6 +1250,7 @@
     },
     "0.5.7": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1242,6 +1269,7 @@
     },
     "0.5.8": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1259,6 +1287,7 @@
     },
     "0.5.9": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1275,6 +1304,7 @@
     },
     "0.6.0": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1289,6 +1319,7 @@
     },
     "0.6.1": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1302,6 +1333,7 @@
     },
     "0.6.10": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup"
@@ -1310,6 +1342,7 @@
     },
     "0.6.11": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup"
@@ -1318,6 +1351,7 @@
     },
     "0.6.12": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup"
@@ -1326,6 +1360,7 @@
     },
     "0.6.2": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1339,6 +1374,7 @@
     },
     "0.6.3": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1352,6 +1388,7 @@
     },
     "0.6.4": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1365,6 +1402,7 @@
     },
     "0.6.5": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1377,6 +1415,7 @@
     },
     "0.6.6": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1388,6 +1427,7 @@
     },
     "0.6.7": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1399,6 +1439,7 @@
     },
     "0.6.8": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup"
@@ -1407,6 +1448,7 @@
     },
     "0.6.9": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1416,6 +1458,7 @@
     },
     "0.7.0": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup"
@@ -1424,6 +1467,7 @@
     },
     "0.7.1": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup",
@@ -1433,6 +1477,7 @@
     },
     "0.7.2": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy",
             "DynamicArrayCleanup"
@@ -1441,6 +1486,7 @@
     },
     "0.7.3": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching",
             "EmptyByteArrayCopy"
         ],
@@ -1448,42 +1494,50 @@
     },
     "0.7.4": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching"
         ],
         "released": "2020-10-19"
     },
     "0.7.5": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching"
         ],
         "released": "2020-11-18"
     },
     "0.7.6": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching"
         ],
         "released": "2020-12-16"
     },
     "0.8.0": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching"
         ],
         "released": "2020-12-16"
     },
     "0.8.1": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching"
         ],
         "released": "2021-01-27"
     },
     "0.8.2": {
         "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory",
             "KeccakCaching"
         ],
         "released": "2021-03-02"
     },
     "0.8.3": {
-        "bugs": [],
+        "bugs": [
+            "ABIDecodeTwoDimensionalArrayMemory"
+        ],
         "released": "2021-03-23"
     }
 }

libsolidity/codegen/ABIFunctions.cpp

@@ -1182,12 +1182,23 @@ string ABIFunctions::abiDecodingFunctionArrayAvailableLength(ArrayType const& _t
 			function <functionName>(offset, length, end) -> array {
 				array := <allocate>(<allocationSize>(length))
 				let dst := array
-				<storeLength>
+				<?dynamic>
+					mstore(array, length)
+					dst := add(array, 0x20)
+				</dynamic>
 				let src := offset
-				<staticBoundsCheck>
+				if gt(add(src, mul(length, <stride>)), end) {
+					<revertInvalidStride>
+				}
 				for { let i := 0 } lt(i, length) { i := add(i, 1) }
 				{
-					let elementPos := <retrieveElementPos>
+					<?dynamicBase>
+						let innerOffset := <load>(src)
+						if gt(innerOffset, 0xffffffffffffffff) { <revertStringOffset> }
+						let elementPos := add(offset, innerOffset)
+					<!dynamicBase>
+						let elementPos := src
+					</dynamicBase>
 					mstore(dst, <decodingFun>(elementPos, end))
 					dst := add(dst, 0x20)
 					src := add(src, <stride>)
@@ -1198,28 +1209,15 @@ string ABIFunctions::abiDecodingFunctionArrayAvailableLength(ArrayType const& _t
 		templ("readableTypeName", _type.toString(true));
 		templ("allocate", m_utils.allocationFunction());
 		templ("allocationSize", m_utils.arrayAllocationSizeFunction(_type));
-		string calldataStride = toCompactHexWithPrefix(_type.calldataStride());
+		templ("stride", toCompactHexWithPrefix(_type.calldataStride()));
-		templ("stride", calldataStride);
+		templ("dynamic", _type.isDynamicallySized());
-		if (_type.isDynamicallySized())
+		templ("load", _fromMemory ? "mload" : "calldataload");
-			templ("storeLength", "mstore(array, length) dst := add(array, 0x20)");
+		templ("dynamicBase", _type.baseType()->isDynamicallyEncoded());
-		else
+		templ(
-			templ("storeLength", "");
+			"revertInvalidStride",
-		if (_type.baseType()->isDynamicallyEncoded())
+			revertReasonIfDebug("ABI decoding: invalid calldata array stride")
-		{
+		);
-			templ("staticBoundsCheck", "");
+		templ("revertStringOffset", revertReasonIfDebug("ABI decoding: invalid calldata array offset"));
-			string load = _fromMemory ? "mload" : "calldataload";
-			templ("retrieveElementPos", "add(offset, " + load + "(src))");
-		}
-		else
-		{
-			templ("staticBoundsCheck", "if gt(add(src, mul(length, " +
-				calldataStride +
-				")), end) { " +
-				revertReasonIfDebug("ABI decoding: invalid calldata array stride") +
-				" }"
-			);
-			templ("retrieveElementPos", "src");
-		}
 		templ("decodingFun", abiDecodingFunction(*_type.baseType(), _fromMemory, false));
 		return templ.render();
 	});

test/libsolidity/gasTests/abiv2.sol

@@ -14,9 +14,9 @@ contract C {
 }
 // ----
 // creation:
-//   codeDepositCost: 1170600
+//   codeDepositCost: 1211600
-//   executionCost: 1214
+//   executionCost: 1261
-//   totalCost: 1171814
+//   totalCost: 1212861
 // external:
 //   a(): 1130
 //   b(uint256): infinite

test/libsolidity/gasTests/abiv2_optimised.sol

@@ -17,9 +17,9 @@ contract C {
 // optimize-yul: true
 // ----
 // creation:
-//   codeDepositCost: 626600
+//   codeDepositCost: 664600
-//   executionCost: 657
+//   executionCost: 696
-//   totalCost: 627257
+//   totalCost: 665296
 // external:
 //   a(): 985
 //   b(uint256): 2052

test/libsolidity/semanticTests/abiencodedecode/offset_overflow_in_array_decoding.sol

@@ -0,0 +1,29 @@
+pragma abicoder v2;
+contract Test {
+	struct MemoryUint {
+		uint field;
+	}
+	function test() public pure returns (uint) {
+		uint[] memory before = new uint[](1); // at offset 0x80
+		// Two problems here: The first offset is zero, the second offset is missing.
+		bytes memory corrupt = abi.encode(uint(32), // offset to "tuple"
+										  uint(0)); // bogus first element
+		/*
+		  At this point the free pointer is 0x80 + 64 (size of before) + 32 (length field of corrupt) + 64 (two encoded words)
+
+		  Now let's put random junk into memory immediately after the bogus first element. Our goal is to overflow the read pointer to point to before.
+		  The value read out at this point will be added to beginning of the encoded tuple, AKA corrupt + 64. We need then to write x where:
+		  x + 0x80 + 64 (before) + 32 (length of corrupt) + 32 (first word of corrupt) = 0x80 (mod 2^256)
+		  that is MAX_UINT - 128
+		*/
+		MemoryUint memory afterCorrupt;
+		afterCorrupt.field = uint(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff80);
+		before[0] = 123456;
+		uint[][2] memory decoded = abi.decode(corrupt, (uint[][2]));
+		return decoded[1][0];
+	}
+}
+// ====
+// compileViaYul: also
+// ----
+// test() -> FAILURE

test/libsolidity/semanticTests/abiencodedecode/offset_overflow_in_array_decoding_2.sol

@@ -0,0 +1,30 @@
+pragma abicoder v2;
+contract Test {
+	struct MemoryTuple {
+		uint field1;
+		uint field2;
+	}
+	function withinArray() public pure returns (uint) {
+		uint[] memory before = new uint[](1);
+		bytes memory corrupt = abi.encode(uint(32),
+										  uint(2));
+		MemoryTuple memory afterCorrupt;
+		before[0] = 123456;
+		/*
+		  As above, but in this case we are adding to:
+		  0x80 + 64 (before) + 32 (length of corrupt) + 32 (offset) + 32 (field pointer)
+		  giving MAX_UINT - 96
+		*/
+		afterCorrupt.field1 = uint(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff60);
+		afterCorrupt.field2 = uint(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff60);
+		uint[][] memory decoded = abi.decode(corrupt, (uint[][]));
+		/*
+		  Will return 123456 * 2, AKA before has been copied twice
+		 */
+		return decoded[0][0] + decoded[1][0];
+	}
+}
+// ====
+// compileViaYul: also
+// ----
+// withinArray() -> FAILURE

test/libsolidity/semanticTests/abiencodedecode/offset_overflow_in_array_decoding_3.sol

@@ -0,0 +1,23 @@
+pragma abicoder v2;
+contract Test {
+	struct MemoryUint {
+		uint field;
+	}
+	function test() public pure returns (uint) {
+		uint[] memory before = new uint[](1); // at offset 0x80
+		bytes memory corrupt = abi.encode(
+            uint(32),
+            uint(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff80),
+            uint(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff80)
+        );
+		MemoryUint memory afterCorrupt;
+		afterCorrupt.field = uint(0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff80);
+		before[0] = 123456;
+		uint[][2] memory decoded = abi.decode(corrupt, (uint[][2]));
+		return decoded[1][0];
+	}
+}
+// ====
+// compileViaYul: also
+// ----
+// test() -> FAILURE