From 26fe525fc2290a81d0890024f28a2289f81bd05f Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Mon, 4 Nov 2024 14:01:32 +0100 Subject: [PATCH] fix: ecrecover validation (#1565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes validation of ecrecover `r` and `s` values. Fixes the library call 'mock' to throw error if an invalid values reaches this point. Fix for: https://github.com/code-423n4/2024-09-kakarot-findings/issues/13 - - - This change is [Reviewable](https://reviewable.io/reviews/kkrt-labs/kakarot/1565) Co-authored-by: Clément Walter --- .../kakarot/precompiles/ec_recover.cairo | 25 +++++++- .../kakarot/precompiles/test_ec_recover.py | 58 ++++++++++++++----- tests/utils/syscall_handler.py | 5 +- 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/cairo_zero/kakarot/precompiles/ec_recover.cairo b/cairo_zero/kakarot/precompiles/ec_recover.cairo index 84454d0a4..53b5e1ee7 100644 --- a/cairo_zero/kakarot/precompiles/ec_recover.cairo +++ b/cairo_zero/kakarot/precompiles/ec_recover.cairo @@ -4,14 +4,15 @@ from starkware.cairo.common.alloc import alloc from starkware.cairo.common.cairo_builtins import HashBuiltin, BitwiseBuiltin from starkware.cairo.common.cairo_keccak.keccak import finalize_keccak from starkware.cairo.common.bool import FALSE -from starkware.cairo.common.math_cmp import RC_BOUND +from starkware.cairo.common.math_cmp import RC_BOUND, is_nn from starkware.cairo.common.cairo_secp.ec import EcPoint from starkware.cairo.common.cairo_secp.bigint import BigInt3 from starkware.cairo.common.cairo_secp.signature import ( recover_public_key, public_key_point_to_eth_address, ) -from starkware.cairo.common.uint256 import Uint256, uint256_reverse_endian +from starkware.cairo.common.uint256 import Uint256, uint256_reverse_endian, uint256_lt +from utils.uint256 import uint256_eq from starkware.cairo.common.cairo_secp.bigint import bigint_to_uint256 from starkware.cairo.common.keccak_utils.keccak_utils import keccak_add_uint256s from starkware.cairo.common.memset import memset @@ -31,6 +32,9 @@ namespace PrecompileEcRecover { const PRECOMPILE_ADDRESS = 0x01; const GAS_COST_EC_RECOVER = 3000; + const SECP256K1N_HIGH = 0xfffffffffffffffffffffffffffffffe; + const SECP256K1N_LOW = 0xbaaedce6af48a03bbfd25e8cd0364141; + // @notice Run the precompile. // @param input_len The length of input array. // @param input The input array. @@ -62,6 +66,23 @@ namespace PrecompileEcRecover { let r = Helpers.bytes_to_uint256(32, input_padded + 32 * 2); let s = Helpers.bytes_to_uint256(32, input_padded + 32 * 3); + let SECP256K1N = Uint256(low=SECP256K1N_LOW, high=SECP256K1N_HIGH); + let (is_valid_upper_r) = uint256_lt(r, SECP256K1N); + let (is_valid_upper_s) = uint256_lt(s, SECP256K1N); + let is_valid_upper_bound = is_valid_upper_r * is_valid_upper_s; + if (is_valid_upper_bound == FALSE) { + let (output) = alloc(); + return (0, output, GAS_COST_EC_RECOVER, 0); + } + + let (is_invalid_lower_r) = uint256_eq(r, Uint256(low=0, high=0)); + let (is_invalid_lower_s) = uint256_eq(s, Uint256(low=0, high=0)); + let is_invalid_lower_bound = is_invalid_lower_r + is_invalid_lower_s; + if (is_invalid_lower_bound != FALSE) { + let (output) = alloc(); + return (0, output, GAS_COST_EC_RECOVER, 0); + } + // v - 27, see recover_public_key comment let (helpers_class) = Kakarot_cairo1_helpers_class_hash.read(); let (success, recovered_address) = ICairo1Helpers.library_call_recover_eth_address( diff --git a/cairo_zero/tests/src/kakarot/precompiles/test_ec_recover.py b/cairo_zero/tests/src/kakarot/precompiles/test_ec_recover.py index f5f11a851..af80c4933 100644 --- a/cairo_zero/tests/src/kakarot/precompiles/test_ec_recover.py +++ b/cairo_zero/tests/src/kakarot/precompiles/test_ec_recover.py @@ -3,6 +3,8 @@ from ethereum.crypto.elliptic_curve import SECP256K1N, secp256k1_recover from ethereum.crypto.hash import Hash32, keccak256 from ethereum.utils.byte import left_pad_zero_bytes +from hypothesis import given +from hypothesis import strategies as st from tests.utils.helpers import ec_sign, generate_random_private_key @@ -34,9 +36,11 @@ def ecrecover(data): @pytest.mark.EC_RECOVER class TestEcRecover: - def test_should_return_evm_address_in_bytes32(self, cairo_run): + @given(message=st.binary(min_size=1, max_size=1000)) + def test_valid_signature(self, message, cairo_run): + """Test with valid signatures generated from random messages.""" private_key = generate_random_private_key() - msg = keccak256(b"test message") + msg = keccak256(message) (v, r, s) = ec_sign(msg, private_key) input_data = [ @@ -47,25 +51,53 @@ def test_should_return_evm_address_in_bytes32(self, cairo_run): ] padded_address, _ = ecrecover(input_data) - [output] = cairo_run("test__ec_recover", input=input_data) - assert bytes(output) == bytes(padded_address) - def test_should_fail_when_input_len_is_not_128(self, cairo_run): - [output] = cairo_run("test__ec_recover", input=[]) + @given(input_length=st.integers(min_value=0, max_value=127)) + def test_invalid_input_length(self, input_length, cairo_run): + """Test with various invalid input lengths.""" + input_data = [0] * input_length + [output] = cairo_run("test__ec_recover", input=input_data) assert output == [] - def test_should_fail_when_recovery_identifier_is_neither_27_nor_28(self, cairo_run): - private_key = generate_random_private_key() - msg = keccak256(b"test message") - (_, r, s) = ec_sign(msg, private_key) - v = 1 + @given( + v=st.integers(min_value=0, max_value=26) | st.integers(min_value=29), + msg=st.binary(min_size=32, max_size=32), + r=st.integers(min_value=1, max_value=SECP256K1N - 1), + s=st.integers(min_value=1, max_value=SECP256K1N - 1), + ) + def test_invalid_v(self, v, msg, r, s, cairo_run): + """Test with invalid v values.""" input_data = [ *msg, *v.to_bytes(32, "big"), - *r, - *s, + *r.to_bytes(32, "big"), + *s.to_bytes(32, "big"), ] [output] = cairo_run("test__ec_recover", input=input_data) assert output == [] + + @given( + v=st.integers(min_value=27, max_value=28), + msg=st.binary(min_size=32, max_size=32), + r=st.integers(min_value=0, max_value=2**256 - 1), + s=st.integers(min_value=0, max_value=2**256 - 1), + ) + def test_parameter_boundaries(self, cairo_run, v, msg, r, s): + """Test `r` and `s` parameter validation including boundary conditions.""" + input_data = [ + *msg, + *v.to_bytes(32, "big"), + *r.to_bytes(32, "big"), + *s.to_bytes(32, "big"), + ] + + py_result = ecrecover(input_data) + [cairo_result] = cairo_run("test__ec_recover", input=input_data) + + if py_result is None: + assert cairo_result == [] + else: + py_address, _ = py_result + assert bytes(cairo_result) == bytes(py_address) diff --git a/tests/utils/syscall_handler.py b/tests/utils/syscall_handler.py index a33bca2b9..3300d90b8 100644 --- a/tests/utils/syscall_handler.py +++ b/tests/utils/syscall_handler.py @@ -44,10 +44,11 @@ def cairo_recover_eth_address(class_hash, calldata): s = U256(uint256_to_int(calldata[4], calldata[5])) y_parity = U256(calldata[6]) + # r and s must have been validated by the precompile preparation if 0 >= r or r >= SECP256K1N: - return [0, 0] + raise ValueError("Invalid r value") if 0 >= s or s >= SECP256K1N: - return [0, 0] + raise ValueError("Invalid s value") try: public_key = secp256k1_recover(r, s, y_parity, msg_hash) except Exception: