From bd6251fc3ba5a5a80b8d50b0069d3f899b2dee11 Mon Sep 17 00:00:00 2001 From: Yonatan Iluz Date: Tue, 9 Jun 2026 18:35:04 +0300 Subject: [PATCH] blockifier: add secp256r1 syscall point validation Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/blockifier/src/execution.rs | 1 + .../src/execution/native/syscall_handler.rs | 29 ++- crates/blockifier/src/execution/secp.rs | 32 +++ .../blockifier/src/execution/secp_shadow.rs | 232 ++++++++++++++++++ .../blockifier/src/execution/syscalls/secp.rs | 19 +- .../execution/syscalls/syscall_tests/secp.rs | 95 +++++++ 6 files changed, 396 insertions(+), 12 deletions(-) create mode 100644 crates/blockifier/src/execution/secp_shadow.rs diff --git a/crates/blockifier/src/execution.rs b/crates/blockifier/src/execution.rs index 82f5bc4c8e8..6c69a61cd20 100644 --- a/crates/blockifier/src/execution.rs +++ b/crates/blockifier/src/execution.rs @@ -11,6 +11,7 @@ pub mod errors; pub mod execution_utils; pub mod hint_code; pub mod secp; +pub mod secp_shadow; #[cfg(feature = "cairo_native")] pub mod native; diff --git a/crates/blockifier/src/execution/native/syscall_handler.rs b/crates/blockifier/src/execution/native/syscall_handler.rs index 573425c0de4..6c37bf69dcf 100644 --- a/crates/blockifier/src/execution/native/syscall_handler.rs +++ b/crates/blockifier/src/execution/native/syscall_handler.rs @@ -48,7 +48,6 @@ use crate::execution::native::utils::{ default_tx_v2_info, tx_v2_info_to_tx_v3_info, }; -use crate::execution::secp; use crate::execution::syscalls::common_syscall_logic::base_keccak; use crate::execution::syscalls::hint_processor::{SyscallExecutionError, OUT_OF_GAS_ERROR_FELT}; use crate::execution::syscalls::syscall_base::SyscallHandlerBase; @@ -58,6 +57,7 @@ use crate::execution::syscalls::vm_syscall_utils::{ SyscallSelector, TryExtractRevert, }; +use crate::execution::{secp, secp_shadow}; use crate::state::state_api::State; use crate::transaction::objects::TransactionInfo; use crate::utils::u64_from_usize; @@ -762,7 +762,14 @@ impl StarknetSyscallHandler for &mut NativeSyscallHandler<'_> { SyscallSelector::Secp256r1Add, )?; - Ok(Secp256Point::add(p0.into(), p1.into()).into()) + let p0_point: Secp256Point = p0.into(); + let p1_point: Secp256Point = p1.into(); + secp_shadow::reject_zero_x_in_add(&p0_point.0, &p1_point.0) + .map_err(|err| self.handle_error(remaining_gas, err.into()))?; + Secp256Point::add(p0_point, p1_point) + .reject_zero_x() + .map(Into::into) + .map_err(|err| self.handle_error(remaining_gas, err)) } fn secp256r1_mul( @@ -777,7 +784,13 @@ impl StarknetSyscallHandler for &mut NativeSyscallHandler<'_> { SyscallSelector::Secp256r1Mul, )?; - Ok(Secp256Point::mul(p.into(), m).into()) + let p_point: Secp256Point = p.into(); + secp_shadow::reject_zero_x_in_mul(&p_point.0, &u256_to_biguint(m)) + .map_err(|err| self.handle_error(remaining_gas, err.into()))?; + Secp256Point::mul(p_point, m) + .reject_zero_x() + .map(Into::into) + .map_err(|err| self.handle_error(remaining_gas, err)) } fn secp256r1_get_point_from_x( @@ -883,7 +896,7 @@ impl From for Secp256Point { } } -impl Secp256Point +impl Secp256Point where Curve::BaseField: PrimeField, // constraint for get_point_by_id { @@ -895,11 +908,17 @@ where { match result { Ok(None) => Ok(None), - Ok(Some(point)) => Ok(Some(Secp256Point(point.into()))), + Ok(Some(point)) => Ok(Some(Secp256Point(point.into()).reject_zero_x()?)), Err(error) => Err(error), } } + /// Rejects secp256r1 points with x-coordinate 0. + fn reject_zero_x(self) -> Result { + secp::reject_zero_x_point(&self.0)?; + Ok(self) + } + /// Given an (x, y) pair, this function: /// - Returns the point at infinity for (0, 0). /// - Returns `Err` if either `x` or `y` is outside the modulus. diff --git a/crates/blockifier/src/execution/secp.rs b/crates/blockifier/src/execution/secp.rs index 3601704f002..0e83afa32f7 100644 --- a/crates/blockifier/src/execution/secp.rs +++ b/crates/blockifier/src/execution/secp.rs @@ -1,9 +1,41 @@ use ark_ec::short_weierstrass::{Affine, SWCurveConfig}; use ark_ff::{BigInteger, PrimeField, Zero}; +use starknet_types_core::felt::Felt; use crate::execution::syscalls::hint_processor::INVALID_ARGUMENT_FELT; use crate::execution::syscalls::vm_syscall_utils::SyscallExecutorBaseError; +/// Whether points with x-coordinate 0 are rejected. Enabled for secp256r1 only. +pub trait SecpZeroXPolicy { + const REJECT_ZERO_X_POINT: bool; +} + +impl SecpZeroXPolicy for ark_secp256r1::Config { + const REJECT_ZERO_X_POINT: bool = true; +} + +impl SecpZeroXPolicy for ark_secp256k1::Config { + const REJECT_ZERO_X_POINT: bool = false; +} + +/// The error returned when a secp256r1 point with x-coordinate 0 is rejected. +pub(crate) fn zero_x_point_error() -> SyscallExecutorBaseError { + SyscallExecutorBaseError::InvalidSyscallInput { + input: Felt::ZERO, + info: "secp256r1 points with x-coordinate 0 are not allowed".to_string(), + } +} + +/// Rejects secp256r1 points with x-coordinate 0. Shared by the VM and Cairo Native syscall paths. +pub fn reject_zero_x_point( + point: &Affine, +) -> Result<(), SyscallExecutorBaseError> { + if Curve::REJECT_ZERO_X_POINT && !point.infinity && point.x.is_zero() { + return Err(zero_x_point_error()); + } + Ok(()) +} + pub fn get_point_from_x( x: num_bigint::BigUint, y_parity: bool, diff --git a/crates/blockifier/src/execution/secp_shadow.rs b/crates/blockifier/src/execution/secp_shadow.rs new file mode 100644 index 00000000000..6fce2ab9085 --- /dev/null +++ b/crates/blockifier/src/execution/secp_shadow.rs @@ -0,0 +1,232 @@ +//! Extra input validation for the secp256r1 `add` / `mul` syscalls: rejects an operation that +//! involves a point with x-coordinate 0. +//! +//! The functions below mirror the secp256r1 EC routines in +//! `starkware/cairo/common/secp256r1/ec.cairo` so they visit the same sequence of points; they only +//! inspect those points and never affect the syscall's computed result. Each `fn` here mirrors the +//! cairo `func` of the same name — keep them in sync. + +use ark_ec::short_weierstrass::{Affine, Projective, SWCurveConfig}; +use ark_ff::{PrimeField, Zero}; +use num_bigint::BigUint; + +use crate::execution::secp::{zero_x_point_error, SecpZeroXPolicy}; +use crate::execution::syscalls::vm_syscall_utils::SyscallExecutorBaseError; + +type ShadowResult = Result; + +/// Whether `point` has affine x-coordinate 0 and is not the point at infinity. Tested on the +/// projective `X` coordinate to avoid an inversion: the affine `x` is `X` times a nonzero factor, +/// so it is zero iff `X == 0` and the point is not at infinity. +fn is_zero_x_point(point: &Projective) -> bool { + !point.is_zero() && point.x.is_zero() +} + +/// Rejects a non-infinity point with x-coordinate 0. +fn reject_zero_x_operand( + point: &Projective, +) -> ShadowResult<()> +where + Curve::BaseField: PrimeField, +{ + if is_zero_x_point(point) { + return Err(zero_x_point_error()); + } + Ok(()) +} + +/// Mirror of `ec_double`. +fn ec_double( + point: Projective, +) -> ShadowResult> +where + Curve::BaseField: PrimeField, +{ + reject_zero_x_operand(&point)?; + Ok(point + point) +} + +/// Mirror of both `ec_add` and `fast_ec_add`, which are identical here: only the two operands are +/// inspected. Every `ec_add` branch (`fast_ec_add`, `ec_double`, or the point at infinity) operates +/// on `point0` / `point1`, and the point at infinity is naturally distinguished in projective form +/// (`Z == 0` vs `X == 0`). +fn ec_add( + point0: Projective, + point1: Projective, +) -> ShadowResult> +where + Curve::BaseField: PrimeField, +{ + reject_zero_x_operand(&point0)?; + reject_zero_x_operand(&point1)?; + Ok(point0 + point1) +} + +/// Mirror of `build_ec_mul_table`: returns `table[i] = i * point` for `i` in `0..=15`. +fn build_ec_mul_table( + point: Projective, +) -> ShadowResult<[Projective; 16]> +where + Curve::BaseField: PrimeField, +{ + // table[0] is the point at infinity. + let mut table = [Projective::::zero(); 16]; + table[1] = point; + table[2] = ec_double(table[1])?; + // The cairo unrolls table[3..=15]; table[i] = fast_ec_add(table[i - 1], point). + for i in 3..16 { + table[i] = ec_add(table[i - 1], point)?; + } + Ok(table) +} + +/// Mirror of `fast_ec_mul_inner`: for each nibble (consumed most significant first), multiplies the +/// accumulator by 16 (four doublings) and adds the nibble's table entry. +fn fast_ec_mul_inner( + table: &[Projective; 16], + mut point: Projective, + nibbles: impl Iterator, +) -> ShadowResult> +where + Curve::BaseField: PrimeField, +{ + for nibble in nibbles { + for _ in 0..4 { + point = ec_double(point)?; + } + point = ec_add(point, table[nibble])?; + } + Ok(point) +} + +/// The 64 nibbles (4 bits each) of the 256-bit scalar, `nibbles[0]` least significant. +fn scalar_nibbles(scalar: &BigUint) -> [usize; 64] { + let bytes = scalar.to_bytes_le(); + core::array::from_fn(|k| { + let byte = bytes.get(k / 2).copied().unwrap_or(0); + usize::from(if k.is_multiple_of(2) { byte & 0x0f } else { byte >> 4 }) + }) +} + +/// Mirror of `ec_mul_by_uint256`: a 16-entry precompute table plus a windowed double-and-add over +/// the scalar's 64 nibbles, most significant first. +fn ec_mul_by_uint256( + point: &Affine, + scalar: &BigUint, +) -> ShadowResult> +where + Curve::BaseField: PrimeField, +{ + let table = build_ec_mul_table(Projective::from(*point))?; + let nibbles = scalar_nibbles(scalar); + + // first_nibble = nibbles[63], last_nibble = nibbles[0]. + let mut res = table[nibbles[63]]; + // First inner call (cairo m = 124): nibbles 62..=32, most significant first. + res = fast_ec_mul_inner(&table, res, (32..=62).rev().map(|k| nibbles[k]))?; + // Second inner call (cairo m = 124): nibbles 31..=1. + res = fast_ec_mul_inner(&table, res, (1..=31).rev().map(|k| nibbles[k]))?; + // Final window for the least-significant nibble: 16 * res, then ec_add with its table entry. + for _ in 0..4 { + res = ec_double(res)?; + } + ec_add(res, table[nibbles[0]]) +} + +/// Rejects a `secp256r1_add(point0, point1)` syscall that involves a point with x-coordinate 0. +/// No-op for curves without [`SecpZeroXPolicy::REJECT_ZERO_X_POINT`]. +pub fn reject_zero_x_in_add( + point0: &Affine, + point1: &Affine, +) -> ShadowResult<()> +where + Curve::BaseField: PrimeField, +{ + if !Curve::REJECT_ZERO_X_POINT { + return Ok(()); + } + // Only the rejection matters; the computed point is discarded. + ec_add(Projective::from(*point0), Projective::from(*point1)).map(|_| ()) +} + +/// Rejects a `secp256r1_mul(point, scalar)` syscall that involves a point with x-coordinate 0 at +/// any point in the computation. No-op for curves without [`SecpZeroXPolicy::REJECT_ZERO_X_POINT`]. +pub fn reject_zero_x_in_mul( + point: &Affine, + scalar: &BigUint, +) -> ShadowResult<()> +where + Curve::BaseField: PrimeField, +{ + if !Curve::REJECT_ZERO_X_POINT { + return Ok(()); + } + // Only the rejection matters; the computed point is discarded. + ec_mul_by_uint256(point, scalar).map(|_| ()) +} + +#[cfg(test)] +mod tests { + use ark_ec::short_weierstrass::Affine; + use num_bigint::BigUint; + + use super::{reject_zero_x_in_add, reject_zero_x_in_mul}; + + fn secp256r1_point(x_hex: &str, y_hex: &str) -> Affine { + let parse = |hex: &str| BigUint::parse_bytes(hex.as_bytes(), 16).unwrap(); + Affine::new_unchecked(parse(x_hex).into(), parse(y_hex).into()) + } + + fn zero_x_point() -> Affine { + secp256r1_point("0", "66485c780e2f83d72433bd5d84a06bb6541c2af31dae871728bf856a174f93f4") + } + + fn generator() -> Affine { + secp256r1_point( + "6b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c296", + "4fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f5", + ) + } + + /// mul rejects a point with x-coordinate 0 that is reached early in the computation. + #[test] + fn mul_rejects_zero_x_point() { + let half = secp256r1_point( + "81bfb55b010b1bdf08b8d9d8590087aa278e28febff3b05632eeff09011c5579", + "8cd2f199d9815d7585073034eb76c93d50799b354b0fb1e77eb75eba8bff3d58", + ); + for scalar in [3_u32, 5, 7] { + assert!(reject_zero_x_in_mul(&half, &BigUint::from(scalar)).is_err()); + } + let quarter = secp256r1_point( + "02495ac9f43e45aae30d3366e351cc08828cf3e11cc3b7209fbd1730c4a14f4e", + "b55567231f26b356bbac703086b614bb21448433dd75ab263264c3d12206b9ee", + ); + assert!(reject_zero_x_in_mul(&quarter, &BigUint::from(7_u32)).is_err()); + } + + /// mul rejects a point with x-coordinate 0 reached only deep in the computation. + #[test] + fn mul_rejects_zero_x_point_large_scalar() { + let sixteenth = secp256r1_point( + "776aef1acb82b628e132cc29440988f0a15d4cc2b4f328aecb063c9b86e5018e", + "6e44dfc60444faa9c4e36bc217451f7ac2956cb3b2e9bbd655eba297163d1f34", + ); + let scalar = BigUint::from(1_u8) << 252_u32; + assert!(reject_zero_x_in_mul(&sixteenth, &scalar).is_err()); + } + + /// mul accepts a multiplication that never involves a point with x-coordinate 0. + #[test] + fn mul_accepts_regular_point() { + assert!(reject_zero_x_in_mul(&generator(), &BigUint::from(5_u32)).is_ok()); + } + + /// add rejects an operand with x-coordinate 0 but not regular points. + #[test] + fn add_rejects_zero_x_operand() { + assert!(reject_zero_x_in_add(&generator(), &generator()).is_ok()); + assert!(reject_zero_x_in_add(&zero_x_point(), &generator()).is_err()); + assert!(reject_zero_x_in_add(&generator(), &zero_x_point()).is_err()); + } +} diff --git a/crates/blockifier/src/execution/syscalls/secp.rs b/crates/blockifier/src/execution/syscalls/secp.rs index 637b3bdd507..68128c72ba2 100644 --- a/crates/blockifier/src/execution/syscalls/secp.rs +++ b/crates/blockifier/src/execution/syscalls/secp.rs @@ -13,7 +13,8 @@ use crate::execution::execution_utils::{ write_maybe_relocatable, write_u256, }; -use crate::execution::secp::new_affine; +use crate::execution::secp::{new_affine, reject_zero_x_point, SecpZeroXPolicy}; +use crate::execution::secp_shadow::{reject_zero_x_in_add, reject_zero_x_in_mul}; use crate::execution::syscalls::hint_processor::felt_to_bool; use crate::execution::syscalls::vm_syscall_utils::{ SyscallBaseResult, @@ -30,7 +31,7 @@ pub struct SecpHintProcessor { pub points: HashMap>, } -impl SecpHintProcessor +impl SecpHintProcessor where Curve::BaseField: PrimeField, { @@ -45,9 +46,10 @@ where points_segment_base: Relocatable, id: usize, ) -> SyscallBaseResult { - let lhs = self.get_point_by_ptr(request.lhs_ptr)?; - let rhs = self.get_point_by_ptr(request.rhs_ptr)?; - let result = *lhs + *rhs; + let lhs = *self.get_point_by_ptr(request.lhs_ptr)?; + let rhs = *self.get_point_by_ptr(request.rhs_ptr)?; + reject_zero_x_in_add(&lhs, &rhs)?; + let result = lhs + rhs; let ec_point_ptr = self.allocate_point(result.into(), vm, &mut Some(points_segment_base), id)?; Ok(SecpOpRespone { ec_point_ptr }) @@ -60,8 +62,9 @@ where points_segment_base: Relocatable, id: usize, ) -> SyscallBaseResult { - let ec_point = self.get_point_by_ptr(request.ec_point_ptr)?; - let result = *ec_point * Curve::ScalarField::from(request.multiplier); + let ec_point = *self.get_point_by_ptr(request.ec_point_ptr)?; + reject_zero_x_in_mul(&ec_point, &request.multiplier)?; + let result = ec_point * Curve::ScalarField::from(request.multiplier); let ec_point_ptr = self.allocate_point(result.into(), vm, &mut Some(points_segment_base), id)?; Ok(SecpOpRespone { ec_point_ptr }) @@ -115,6 +118,8 @@ where points_segment_base: &mut Option, id: usize, ) -> SyscallBaseResult { + reject_zero_x_point(&ec_point)?; + if points_segment_base.is_none() { *points_segment_base = Some(vm.add_memory_segment()); } diff --git a/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs b/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs index a60281626c3..f708e5c9850 100644 --- a/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs +++ b/crates/blockifier/src/execution/syscalls/syscall_tests/secp.rs @@ -1,12 +1,19 @@ +use ark_ec::short_weierstrass::Affine; use blockifier_test_utils::cairo_versions::{CairoVersion, RunnableCairo1}; use blockifier_test_utils::contracts::FeatureContract; +use cairo_vm::types::relocatable::Relocatable; +use cairo_vm::vm::vm_core::VirtualMachine; use expect_test::expect; +use num_bigint::BigUint; use starknet_api::abi::abi_utils::selector_from_name; +use starknet_api::felt; use starknet_api::transaction::fields::Calldata; +use starknet_types_core::felt::Felt; use test_case::test_case; use crate::context::ChainInfo; use crate::execution::entry_point::CallEntryPoint; +use crate::execution::syscalls::secp::{SecpHintProcessor, SecpMulRequest}; use crate::test_utils::initial_test_state::test_state; use crate::test_utils::{trivial_external_entry_point_new, BALANCE}; @@ -107,3 +114,91 @@ fn test_secp256r1(runnable_version: RunnableCairo1) { "#]] .assert_debug_eq(&execution); } + +/// Runs `entry_point` (with `calldata`) and asserts the secp256r1 point with x-coordinate 0 was +/// rejected with a hard error (not a catchable revert). +fn assert_secp256r1_zero_x_point_rejected( + runnable_version: RunnableCairo1, + entry_point: &str, + calldata: Vec, +) { + let test_contract = FeatureContract::TestContract(CairoVersion::Cairo1(runnable_version)); + let chain_info = &ChainInfo::create_for_testing(); + let mut state = test_state(chain_info, BALANCE, &[(test_contract, 1)]); + + let entry_point_call = CallEntryPoint { + calldata: Calldata(calldata.into()), + entry_point_selector: selector_from_name(entry_point), + ..trivial_external_entry_point_new(test_contract) + }; + let error = entry_point_call.execute_directly(&mut state).unwrap_err(); + assert!( + error.to_string().contains("secp256r1 points with x-coordinate 0 are not allowed"), + "Unexpected error: {error}" + ); +} + +// The following tests verify that a secp256r1 point with x-coordinate 0 is rejected with a hard +// error across the secp256r1 syscalls. u256 arguments are passed as (low, high) 128-bit limbs. + +/// The y-coordinate of a secp256r1 point with x-coordinate 0. +const ZERO_X_POINT_Y_LOW: &str = "0x541c2af31dae871728bf856a174f93f4"; +const ZERO_X_POINT_Y_HIGH: &str = "0x66485c780e2f83d72433bd5d84a06bb6"; + +/// `secp256r1_new` with the zero-x point. +#[cfg_attr(feature = "cairo_native", test_case(RunnableCairo1::Native; "Native"))] +#[test_case(RunnableCairo1::Casm; "VM")] +fn test_secp256r1_new_zero_x_point_rejected(runnable_version: RunnableCairo1) { + // `test_getter_secp256r1` calls `secp256r1_new_syscall(x, y)`. + let calldata = + vec![felt!("0x0"), felt!("0x0"), felt!(ZERO_X_POINT_Y_LOW), felt!(ZERO_X_POINT_Y_HIGH)]; + assert_secp256r1_zero_x_point_rejected(runnable_version, "test_getter_secp256r1", calldata); +} + +/// `secp256r1_get_point_from_x` with x == 0. +#[cfg_attr(feature = "cairo_native", test_case(RunnableCairo1::Native; "Native"))] +#[test_case(RunnableCairo1::Casm; "VM")] +fn test_secp256r1_get_point_from_x_zero_x_point_rejected(runnable_version: RunnableCairo1) { + // `test_new_point_secp256r1(x)` calls `secp256r1_get_point_from_x_syscall(x, ..)`; with x == 0 + // it derives the zero-x point. + let calldata = vec![felt!("0x0"), felt!("0x0")]; + assert_secp256r1_zero_x_point_rejected(runnable_version, "test_new_point_secp256r1", calldata); +} + +/// `secp256r1_add` that produces a point with x-coordinate 0. +#[cfg_attr(feature = "cairo_native", test_case(RunnableCairo1::Native; "Native"))] +#[test_case(RunnableCairo1::Casm; "VM")] +fn test_secp256r1_add_zero_x_point_rejected(runnable_version: RunnableCairo1) { + // `test_add_secp256r1(x, y)` computes `secp256r1_new(x, y) + generator`. + let calldata = vec![ + felt!("0x6211691a4a6c250a993ae2a93a66db1f"), + felt!("0xfd92d626016b41a6cad9cd497a821cc0"), + felt!("0x3baa0c70538a1a79a3669e9c26191d08"), + felt!("0x4dc1e96ddb4fb7f4cd7f08b7f2085bcb"), + ]; + assert_secp256r1_zero_x_point_rejected(runnable_version, "test_add_secp256r1", calldata); +} + +/// `secp_mul` of a point `p` (with x-coordinate != 0) that involves a point with x-coordinate 0 +/// during the computation, even though the final result has x-coordinate != 0. Driven directly, +/// since no feature-contract entry point multiplies a caller-provided point. +#[test] +fn test_secp256r1_mul_zero_x_intermediate_rejected() { + let parse = |hex: &str| BigUint::parse_bytes(hex.as_bytes(), 16).unwrap(); + let p = Affine::::new_unchecked( + parse("81bfb55b010b1bdf08b8d9d8590087aa278e28febff3b05632eeff09011c5579").into(), + parse("8cd2f199d9815d7585073034eb76c93d50799b354b0fb1e77eb75eba8bff3d58").into(), + ); + let mut secp_processor = SecpHintProcessor::::new(); + let point_ptr = Relocatable::from((1, 0)); + secp_processor.points.insert(point_ptr, p); + + let mut vm = VirtualMachine::new(false, false); + let request = SecpMulRequest { ec_point_ptr: point_ptr, multiplier: BigUint::from(3_u8) }; + let error = + secp_processor.secp_mul(request, &mut vm, Relocatable::from((2, 0)), 0).unwrap_err(); + assert!( + error.to_string().contains("secp256r1 points with x-coordinate 0 are not allowed"), + "Unexpected error: {error}" + ); +}