From 21f7172274e551c721e9e35ab3c9d8322a2455d0 Mon Sep 17 00:00:00 2001 From: Charles Cooper Date: Tue, 11 Jun 2024 14:04:33 -0700 Subject: [PATCH] fix[codegen]: recursive dynarray oob check (#4091) this commit fixes more edge cases in `abi_decode` dynarray validation. these are bugs which were missed (or regressions) in 1f6b9433fbd524, which itself was a continuation of eb011367cc769. there are multiple fixes contained in this commit. - similar conceptual error as in 1f6b9433fbd524. when the length word is out-of-bounds and its runtime is value is zero, `make_setter` does not enter recursion and therefore there is no oob check. an example payload which demonstrates this is in `test_nested_invalid_dynarray_head()`. the fix is to check the size of the static section ("embedded static size") before entering the recursion, rather than child_type.static_size (which could be zero). essentially, this checks that the end of the static section is in bounds, rather than the beginning. - the fallback case in `complex_make_setter` could be referring to a tuple of dynamic types, which makes the tuple itself dynamic, so there needs to be an oob check there as well. - `static_size()` is more appropriate than `min_size()` for abi payload validation, because you can have "valid" ABI payloads where the runtime length of the dynamic section is zero, because the heads in the static section all point back into the static section. this commit replaces the `static_size()` check with `min_size()` check, everywhere. - remove `returndatasize` check in external calls, because it gets checked anyways during `make_setter` oob checks. - add a comment clarifying that payloads larger than `size_bound()` get rejected by `abi_decode` but not calldata decoding. tests for each case, contributed by @trocher --------- Co-authored-by: trocher --- .../builtins/codegen/test_abi_decode.py | 98 +++++++++++++++++++ .../test_external_contract_calls.py | 8 +- vyper/builtins/functions.py | 6 +- vyper/codegen/core.py | 15 ++- vyper/codegen/external_call.py | 12 +-- .../function_definitions/external_function.py | 2 +- 6 files changed, 119 insertions(+), 22 deletions(-) diff --git a/tests/functional/builtins/codegen/test_abi_decode.py b/tests/functional/builtins/codegen/test_abi_decode.py index d77bb1b5ae..5773636add 100644 --- a/tests/functional/builtins/codegen/test_abi_decode.py +++ b/tests/functional/builtins/codegen/test_abi_decode.py @@ -1323,3 +1323,101 @@ def run(x: Bytes[2 * 32 + 3 * 32 + 3 * 32 * 4]): with tx_failed(): c.run(data) + + +def test_nested_invalid_dynarray_head(get_contract, tx_failed): + code = """ +@nonpayable +@external +def foo(x:Bytes[320]): + if True: + a: Bytes[320-32] = b'' + + # make the word following the buffer x_mem dirty to make a potential + # OOB revert + fake_head: uint256 = 32 + x_mem: Bytes[320] = x + + y: DynArray[DynArray[uint256, 2], 2] = _abi_decode(x_mem,DynArray[DynArray[uint256, 2], 2]) + +@nonpayable +@external +def bar(x:Bytes[320]): + x_mem: Bytes[320] = x + + y:DynArray[DynArray[uint256, 2], 2] = _abi_decode(x_mem,DynArray[DynArray[uint256, 2], 2]) + """ + c = get_contract(code) + + encoded = (0x20, 0x02) # head of the dynarray # len of outer + inner = ( + 0x0, # head1 + # 0x0, # head2 + ) + + encoded = _abi_payload_from_tuple(encoded + inner) + with tx_failed(): + c.foo(encoded) # revert + with tx_failed(): + c.bar(encoded) # return [[],[]] + + +def test_static_outer_type_invalid_heads(get_contract, tx_failed): + code = """ +@nonpayable +@external +def foo(x:Bytes[320]): + x_mem: Bytes[320] = x + y:DynArray[uint256, 2][2] = _abi_decode(x_mem,DynArray[uint256, 2][2]) + +@nonpayable +@external +def bar(x:Bytes[320]): + if True: + a: Bytes[160] = b'' + # write stuff here to make the call revert in case decode do + # an out of bound access: + fake_head: uint256 = 32 + x_mem: Bytes[320] = x + y:DynArray[uint256, 2][2] = _abi_decode(x_mem,DynArray[uint256, 2][2]) + """ + c = get_contract(code) + + encoded = (0x20,) # head of the static array + inner = ( + 0x00, # head of the first dynarray + # 0x00, # head of the second dynarray + ) + + encoded = _abi_payload_from_tuple(encoded + inner) + + with tx_failed(): + c.foo(encoded) + with tx_failed(): + c.bar(encoded) + + +def test_abi_decode_max_size(get_contract, tx_failed): + # test case where the payload is "too large" than the max size + # of abi encoding the type. this can happen when the payload is + # "sparse" and has garbage bytes in between the static and dynamic + # sections + code = """ +@external +def foo(a:Bytes[1000]): + v: DynArray[uint256, 1] = _abi_decode(a,DynArray[uint256, 1]) + """ + c = get_contract(code) + + payload = ( + 0xA0, # head + 0x00, # garbage + 0x00, # garbage + 0x00, # garbage + 0x00, # garbage + 0x01, # len + 0x12, # elem1 + ) + + with tx_failed(): + c.foo(_abi_payload_from_tuple(payload)) diff --git a/tests/functional/codegen/calling_convention/test_external_contract_calls.py b/tests/functional/codegen/calling_convention/test_external_contract_calls.py index 2192e6b6ab..e6616a1481 100644 --- a/tests/functional/codegen/calling_convention/test_external_contract_calls.py +++ b/tests/functional/codegen/calling_convention/test_external_contract_calls.py @@ -2519,13 +2519,13 @@ def foo(a: DynArray[{typ}, 3], b: String[5]): encoded = abi.encode(f"({typ}[],string)", val).hex() data = f"0x{sig}{encoded}" - # Dynamic size is short by 1 byte - malformed = data[:264] + # Static size is short by 1 byte + malformed = data[:136] with tx_failed(): env.message_call(c1.address, data=malformed) - # Dynamic size is at least minimum (132 bytes * 2 + 2 (for 0x) = 266) - valid = data[:266] + # Static size is at least minimum ((4 + 64) bytes * 2 + 2 (for 0x) = 138) + valid = data[:138] env.message_call(c1.address, data=valid) diff --git a/vyper/builtins/functions.py b/vyper/builtins/functions.py index 1944d32125..2564329b65 100644 --- a/vyper/builtins/functions.py +++ b/vyper/builtins/functions.py @@ -2482,7 +2482,7 @@ def build_IR(self, expr, args, kwargs, context): wrapped_typ = calculate_type_for_external_return(output_typ) abi_size_bound = wrapped_typ.abi_type.size_bound() - abi_min_size = wrapped_typ.abi_type.min_size() + abi_min_size = wrapped_typ.abi_type.static_size() # Get the size of data input_max_len = data.typ.maxlen @@ -2506,6 +2506,10 @@ def build_IR(self, expr, args, kwargs, context): ret = ["seq"] + # NOTE: we could replace these 4 lines with + # `[assert [le, abi_min_size, data_len]]`. it depends on + # what we consider a "valid" payload. + # cf. test_abi_decode_max_size() if abi_min_size == abi_size_bound: ret.append(["assert", ["eq", abi_min_size, data_len]]) else: diff --git a/vyper/codegen/core.py b/vyper/codegen/core.py index 5d4621518f..ff0f801d74 100644 --- a/vyper/codegen/core.py +++ b/vyper/codegen/core.py @@ -895,10 +895,7 @@ def _abi_payload_size(ir_node): # the amount of size each value occupies in static section # (the amount of size it occupies in the dynamic section is handled in # make_setter recursion) - item_size = ir_node.typ.value_type.abi_type.static_size() - if item_size == 0: - # manual optimization; the mload cannot currently be optimized out - return ["add", OFFSET, 0] + item_size = ir_node.typ.value_type.abi_type.embedded_static_size() return ["add", OFFSET, ["mul", get_dyn_array_count(ir_node), item_size]] if isinstance(ir_node.typ, _BytestringT): @@ -982,7 +979,15 @@ def make_setter(left, right, hi=None): # Complex Types assert isinstance(left.typ, (SArrayT, TupleT, StructT)) - return _complex_make_setter(left, right, hi=hi) + with right.cache_when_complex("c_right") as (b1, right): + ret = ["seq"] + if hi is not None: + item_end = add_ofst(right, right.typ.abi_type.static_size()) + len_check = ["assert", ["le", item_end, hi]] + ret.append(len_check) + + ret.append(_complex_make_setter(left, right, hi=hi)) + return b1.resolve(IRnode.from_list(ret)) # locations with no dedicated copy opcode diff --git a/vyper/codegen/external_call.py b/vyper/codegen/external_call.py index f3cd4e7a44..b6ac180722 100644 --- a/vyper/codegen/external_call.py +++ b/vyper/codegen/external_call.py @@ -86,9 +86,8 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp abi_return_t = wrapped_return_t.abi_type - min_return_size = abi_return_t.min_size() max_return_size = abi_return_t.size_bound() - assert 0 < min_return_size <= max_return_size + assert 0 <= max_return_size ret_ofst = buf ret_len = max_return_size @@ -103,15 +102,6 @@ def _unpack_returndata(buf, fn_type, call_kwargs, contract_address, context, exp unpacker = ["seq"] - # revert when returndatasize is not in bounds - # (except when return_override is provided.) - if not call_kwargs.skip_contract_check: - assertion = IRnode.from_list( - ["assert", ["ge", "returndatasize", min_return_size]], - error_msg="returndatasize too small", - ) - unpacker.append(assertion) - assert isinstance(wrapped_return_t, TupleT) # unpack strictly diff --git a/vyper/codegen/function_definitions/external_function.py b/vyper/codegen/function_definitions/external_function.py index a9b4a93025..4c733ee851 100644 --- a/vyper/codegen/function_definitions/external_function.py +++ b/vyper/codegen/function_definitions/external_function.py @@ -84,7 +84,7 @@ def handler_for(calldata_kwargs, default_kwargs): # ensure calldata is at least of minimum length args_abi_t = calldata_args_t.abi_type - calldata_min_size = args_abi_t.min_size() + 4 + calldata_min_size = args_abi_t.static_size() + 4 # TODO optimize make_setter by using # TupleT(list(arg.typ for arg in calldata_kwargs + default_kwargs))