From 838e0be28a129063ee53f2956960e94641e01e2a Mon Sep 17 00:00:00 2001 From: cyberthirst Date: Fri, 15 Mar 2024 15:07:11 +0100 Subject: [PATCH 1/5] fix slice bounds check --- vyper/builtins/functions.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/vyper/builtins/functions.py b/vyper/builtins/functions.py index 86b88442cf..e6f301ad99 100644 --- a/vyper/builtins/functions.py +++ b/vyper/builtins/functions.py @@ -440,12 +440,13 @@ def build_IR(self, expr, args, kwargs, context): # make sure we don't overrun the source buffer, checking for # overflow: - # `assert start+length <= src_len && start+length <= length` + # valid inputs satisfy: `assert start+length <= src_len && start+length > start` + bounds_check = [ "with", "end", ["add", start, length], - ["assert", ["iszero", ["or", ["gt", "end", src_len], ["gt", "end", length]]]], + ["assert", ["iszero", ["or", ["gt", "end", src_len], ["lt", "end", start]]]], ] ret = [ From e0ab57d59b2e9642037f9f24a5e6c5bdb9e79e0d Mon Sep 17 00:00:00 2001 From: cyberthirst Date: Sat, 16 Mar 2024 11:25:06 +0100 Subject: [PATCH 2/5] add overflow check to adhoc slice --- vyper/builtins/functions.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/vyper/builtins/functions.py b/vyper/builtins/functions.py index e6f301ad99..2ff03e4266 100644 --- a/vyper/builtins/functions.py +++ b/vyper/builtins/functions.py @@ -228,6 +228,13 @@ def build_IR(self, expr, context): ADHOC_SLICE_NODE_MACROS = ["~calldata", "~selfcode", "~extcode"] +def _make_slice_bounds_check(start, length, src_len): + return [ + "with", + "end", + ["add", start, length], + ["assert", ["iszero", ["or", ["gt", "end", src_len], ["lt", "end", start]]]], + ] def _build_adhoc_slice_node(sub: IRnode, start: IRnode, length: IRnode, context: Context) -> IRnode: assert length.is_literal, "typechecker failed" @@ -241,7 +248,7 @@ def _build_adhoc_slice_node(sub: IRnode, start: IRnode, length: IRnode, context: if sub.value == "~calldata": node = [ "seq", - ["assert", ["le", ["add", start, length], "calldatasize"]], # runtime bounds check + _make_slice_bounds_check(start, length, "calldatasize"), ["mstore", np, length], ["calldatacopy", np + 32, start, length], np, @@ -251,7 +258,7 @@ def _build_adhoc_slice_node(sub: IRnode, start: IRnode, length: IRnode, context: elif sub.value == "~selfcode": node = [ "seq", - ["assert", ["le", ["add", start, length], "codesize"]], # runtime bounds check + _make_slice_bounds_check(start, length, "codesize"), ["mstore", np, length], ["codecopy", np + 32, start, length], np, @@ -266,8 +273,7 @@ def _build_adhoc_slice_node(sub: IRnode, start: IRnode, length: IRnode, context: sub.args[0], [ "seq", - # runtime bounds check - ["assert", ["le", ["add", start, length], ["extcodesize", "_extcode_address"]]], + _make_slice_bounds_check(start, length, ["extcodesize", "_extcode_address"]), ["mstore", np, length], ["extcodecopy", "_extcode_address", np + 32, start, length], np, @@ -451,7 +457,7 @@ def build_IR(self, expr, args, kwargs, context): ret = [ "seq", - bounds_check, + _make_slice_bounds_check(start, length, src_len), do_copy, ["mstore", dst, length], # set length dst, # return pointer to dst From abd37254ac0aea0b4580c6e04dbc5533ebd38023 Mon Sep 17 00:00:00 2001 From: cyberthirst Date: Sat, 16 Mar 2024 11:27:30 +0100 Subject: [PATCH 3/5] add tests for slice oob access --- .../functional/builtins/codegen/test_slice.py | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/functional/builtins/codegen/test_slice.py b/tests/functional/builtins/codegen/test_slice.py index 0c5a8fc485..0bd2a295f1 100644 --- a/tests/functional/builtins/codegen/test_slice.py +++ b/tests/functional/builtins/codegen/test_slice.py @@ -435,3 +435,66 @@ def test_slice_bytes32_calldata_extended(get_contract, code, result): c.bar(3, "0x0001020304050607080910111213141516171819202122232425262728293031", 5).hex() == result ) + + +oob_fail_list = [ + """ +d: public(Bytes[256]) + +@external +def do_slice(): + x : uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639935 # 2**256-1 + self.d = b"\x01\x02\x03\x04\x05\x06" + assert len(slice(self.d, 1, x))==115792089237316195423570985008687907853269984665640564039457584007913129639935 + """, + """ +@external +def do_slice(): + x: uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639935 # 2**256 - 1 + y: uint256 = 22704331223003175573249212746801550559464702875615796870481879217237868556850 # 0x3232323232323232323232323232323232323232323232323232323232323232 + z: uint96 = 1 + if True: + placeholder : uint256[16] = [y, y, y, y, y, y, y, y, y, y, y, y, y, y, y, y] + s :String[32] = slice(uint2str(z), 1, x) # uint2str(z) == "1" + #print(len(s)) + assert slice(s, 1, 2) == "22" + """, + """ +x: public(Bytes[64]) +secret: uint256 + +@deploy +def __init__(): + self.x = empty(Bytes[64]) + self.secret = 42 + +@external +def do_slice() -> Bytes[64]: + # max - 63 + start: uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639872 + return slice(self.x, start, 64) + """, + # tests bounds check in adhoc location calldata + """ +interface IFace: + def choose_value(_x: uint256, _y: uint256, _z: uint256, idx: uint256) -> Bytes[32]: nonpayable + +@external +def choose_value(_x: uint256, _y: uint256, _z: uint256, idx: uint256) -> Bytes[32]: + assert idx % 32 == 4 + return slice(msg.data, idx, 32) + +@external +def do_slice(): + idx: uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639908 + ret: uint256 = _abi_decode(extcall IFace(self).choose_value(1, 2, 3, idx), uint256) + assert ret == 0 + """ +] + + +@pytest.mark.parametrize("bad_code", oob_fail_list) +def test_slice_buffer_oob_reverts(bad_code, get_contract, tx_failed): + c = get_contract(bad_code) + with tx_failed(): + c.do_slice() \ No newline at end of file From a78ba17658b07a7c1ee246629aac586ae9574d30 Mon Sep 17 00:00:00 2001 From: cyberthirst Date: Sat, 16 Mar 2024 11:34:23 +0100 Subject: [PATCH 4/5] add comment to oob check in slice --- vyper/builtins/functions.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/vyper/builtins/functions.py b/vyper/builtins/functions.py index 2ff03e4266..b5231d8734 100644 --- a/vyper/builtins/functions.py +++ b/vyper/builtins/functions.py @@ -228,6 +228,10 @@ def build_IR(self, expr, context): ADHOC_SLICE_NODE_MACROS = ["~calldata", "~selfcode", "~extcode"] + +# make sure we don't overrun the source buffer, checking for +# overflow: +# valid inputs satisfy: `assert start+length <= src_len && start+length > start` def _make_slice_bounds_check(start, length, src_len): return [ "with", @@ -444,17 +448,6 @@ def build_IR(self, expr, args, kwargs, context): do_copy = copy_bytes(copy_dst, copy_src, copy_len, copy_maxlen) - # make sure we don't overrun the source buffer, checking for - # overflow: - # valid inputs satisfy: `assert start+length <= src_len && start+length > start` - - bounds_check = [ - "with", - "end", - ["add", start, length], - ["assert", ["iszero", ["or", ["gt", "end", src_len], ["lt", "end", start]]]], - ] - ret = [ "seq", _make_slice_bounds_check(start, length, src_len), From d9a07402cf868b9fc9ec6db74ee02abacfc8513f Mon Sep 17 00:00:00 2001 From: cyberthirst Date: Sun, 17 Mar 2024 16:47:29 +0100 Subject: [PATCH 5/5] refactor oob slice tests --- .../functional/builtins/codegen/test_slice.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/functional/builtins/codegen/test_slice.py b/tests/functional/builtins/codegen/test_slice.py index 0bd2a295f1..a0fee779bc 100644 --- a/tests/functional/builtins/codegen/test_slice.py +++ b/tests/functional/builtins/codegen/test_slice.py @@ -437,26 +437,27 @@ def test_slice_bytes32_calldata_extended(get_contract, code, result): ) +# test cases crafted based on advisory GHSA-9x7f-gwxq-6f2c oob_fail_list = [ """ d: public(Bytes[256]) @external def do_slice(): - x : uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639935 # 2**256-1 + x : uint256 = max_value(uint256) self.d = b"\x01\x02\x03\x04\x05\x06" - assert len(slice(self.d, 1, x))==115792089237316195423570985008687907853269984665640564039457584007913129639935 + assert len(slice(self.d, 1, x)) == max_value(uint256) """, """ @external def do_slice(): - x: uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639935 # 2**256 - 1 - y: uint256 = 22704331223003175573249212746801550559464702875615796870481879217237868556850 # 0x3232323232323232323232323232323232323232323232323232323232323232 + x: uint256 = max_value(uint256) + # y == 0x3232323232323232323232323232323232323232323232323232323232323232 + y: uint256 = 22704331223003175573249212746801550559464702875615796870481879217237868556850 z: uint96 = 1 if True: placeholder : uint256[16] = [y, y, y, y, y, y, y, y, y, y, y, y, y, y, y, y] - s :String[32] = slice(uint2str(z), 1, x) # uint2str(z) == "1" - #print(len(s)) + s :String[32] = slice(uint2str(z), 1, x) assert slice(s, 1, 2) == "22" """, """ @@ -470,8 +471,7 @@ def __init__(): @external def do_slice() -> Bytes[64]: - # max - 63 - start: uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639872 + start: uint256 = max_value(uint256) - 63 return slice(self.x, start, 64) """, # tests bounds check in adhoc location calldata @@ -486,7 +486,7 @@ def choose_value(_x: uint256, _y: uint256, _z: uint256, idx: uint256) -> Bytes[3 @external def do_slice(): - idx: uint256 = 115792089237316195423570985008687907853269984665640564039457584007913129639908 + idx: uint256 = max_value(uint256) - 27 ret: uint256 = _abi_decode(extcall IFace(self).choose_value(1, 2, 3, idx), uint256) assert ret == 0 """