From 0d78578981bfcc4aa021dcc0f0238548f6ff9ca0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 7 Feb 2025 12:24:23 -0300 Subject: [PATCH] fix!: check abi integer input is within signed range (#7316) Co-authored-by: Michael J Klein --- .../execution_success/signed_cmp/Prover.toml | 2 +- .../execution_success/signed_div/Prover.toml | 38 ++--- tooling/noir_js/test/node/cjs.test.cjs | 6 +- tooling/noir_js/test/node/smoke.test.ts | 6 +- tooling/noirc_abi/src/errors.rs | 19 ++- tooling/noirc_abi/src/input_parser/json.rs | 60 ++++++- tooling/noirc_abi/src/input_parser/mod.rs | 160 ++++++++++++++---- tooling/noirc_abi/src/input_parser/toml.rs | 60 ++++++- 8 files changed, 269 insertions(+), 82 deletions(-) diff --git a/test_programs/execution_success/signed_cmp/Prover.toml b/test_programs/execution_success/signed_cmp/Prover.toml index 4b719f83c16..2761799a9cc 100644 --- a/test_programs/execution_success/signed_cmp/Prover.toml +++ b/test_programs/execution_success/signed_cmp/Prover.toml @@ -1 +1 @@ -minus_one = 255 +minus_one = -1 diff --git a/test_programs/execution_success/signed_div/Prover.toml b/test_programs/execution_success/signed_div/Prover.toml index be93fec5cc3..ac0e08269cf 100644 --- a/test_programs/execution_success/signed_div/Prover.toml +++ b/test_programs/execution_success/signed_div/Prover.toml @@ -1,51 +1,51 @@ [[ops]] lhs = 4 -rhs = 255 # -1 -result = 252 # -4 +rhs = -1 +result = -4 [[ops]] lhs = 4 -rhs = 254 # -2 -result = 254 # -2 +rhs = -2 +result = -2 [[ops]] lhs = 4 -rhs = 253 # -3 -result = 255 # -1 +rhs = -3 +result = -1 [[ops]] lhs = 4 -rhs = 252 # -4 -result = 255 # -1 +rhs = -4 +result = -1 [[ops]] lhs = 4 -rhs = 251 # -5 +rhs = -5 result = 0 [[ops]] -lhs = 252 # -4 -rhs = 255 # -1 +lhs = -4 +rhs = -1 result = 4 [[ops]] -lhs = 252 # -4 -rhs = 254 # -2 +lhs = -4 +rhs = -2 result = 2 [[ops]] -lhs = 252 # -4 -rhs = 253 # -3 +lhs = -4 +rhs = -3 result = 1 [[ops]] -lhs = 252 # -4 -rhs = 252 # -4 +lhs = -4 +rhs = -4 result = 1 [[ops]] -lhs = 252 # -4 -rhs = 251 # -5 +lhs = -4 +rhs = -5 result = 0 diff --git a/tooling/noir_js/test/node/cjs.test.cjs b/tooling/noir_js/test/node/cjs.test.cjs index 8698f9dfe2b..bbedd748203 100644 --- a/tooling/noir_js/test/node/cjs.test.cjs +++ b/tooling/noir_js/test/node/cjs.test.cjs @@ -59,7 +59,7 @@ it('0x prefixed string input for inputs will throw', async () => { }); describe('input validation', () => { - it('x should be a uint64 not a string', async () => { + it('x should be a int64 not a string', async () => { const inputs = { x: 'foo', y: '3', @@ -67,13 +67,13 @@ describe('input validation', () => { try { await new Noir(assert_lt_json).execute(inputs); - chai.expect.fail('Expected generatedWitness to throw, due to x not being convertible to a uint64'); + chai.expect.fail('Expected generatedWitness to throw, due to x not being convertible to a int64'); } catch (error) { const knownError = error; chai .expect(knownError.message) .to.equal( - 'Expected witness values to be integers, provided value causes `invalid digit found in string` error', + 'The value passed for parameter `x` is invalid:\nExpected witness values to be integers, but `foo` failed with `invalid digit found in string`', ); } }); diff --git a/tooling/noir_js/test/node/smoke.test.ts b/tooling/noir_js/test/node/smoke.test.ts index 6993a44f66e..dc04388e7ca 100644 --- a/tooling/noir_js/test/node/smoke.test.ts +++ b/tooling/noir_js/test/node/smoke.test.ts @@ -58,7 +58,7 @@ it('0x prefixed string input for inputs will throw', async () => { }); describe('input validation', () => { - it('x should be a uint64 not a string', async () => { + it('x should be a int64 not a string', async () => { const inputs = { x: 'foo', y: '3', @@ -66,11 +66,11 @@ describe('input validation', () => { try { await new Noir(assert_lt_program).execute(inputs); - expect.fail('Expected generatedWitness to throw, due to x not being convertible to a uint64'); + expect.fail('Expected generatedWitness to throw, due to x not being convertible to a int64'); } catch (error) { const knownError = error as Error; expect(knownError.message).to.equal( - 'Expected witness values to be integers, provided value causes `invalid digit found in string` error', + 'The value passed for parameter `x` is invalid:\nExpected witness values to be integers, but `foo` failed with `invalid digit found in string`', ); } }); diff --git a/tooling/noirc_abi/src/errors.rs b/tooling/noirc_abi/src/errors.rs index 4209a9e218b..c46945d8ff2 100644 --- a/tooling/noirc_abi/src/errors.rs +++ b/tooling/noirc_abi/src/errors.rs @@ -2,17 +2,26 @@ use crate::{ input_parser::{InputTypecheckingError, InputValue}, AbiType, }; -use acvm::acir::native_types::Witness; +use acvm::{acir::native_types::Witness, AcirField, FieldElement}; use thiserror::Error; #[derive(Debug, Error)] pub enum InputParserError { #[error("input file is badly formed, could not parse, {0}")] ParseInputMap(String), - #[error("Expected witness values to be integers, provided value causes `{0}` error")] - ParseStr(String), - #[error("Could not parse hex value {0}")] - ParseHexStr(String), + #[error( + "The value passed for parameter `{arg_name}` is invalid:\nExpected witness values to be integers, but `{value}` failed with `{error}`" + )] + ParseStr { arg_name: String, value: String, error: String }, + #[error("The value passed for parameter `{arg_name}` is invalid:\nValue {value} is less than minimum allowed value of {min}")] + InputUnderflowsMinimum { arg_name: String, value: String, min: String }, + #[error("The value passed for parameter `{arg_name}` is invalid:\nValue {value} exceeds maximum allowed value of {max}")] + InputOverflowsMaximum { arg_name: String, value: String, max: String }, + #[error( + "The value passed for parameter `{arg_name}` is invalid:\nValue {value} exceeds field modulus. Values must fall within [0, {})", + FieldElement::modulus() + )] + InputExceedsFieldModulus { arg_name: String, value: String }, #[error("cannot parse value into {0:?}")] AbiTypeMismatch(AbiType), #[error("Expected argument `{0}`, but none was found")] diff --git a/tooling/noirc_abi/src/input_parser/json.rs b/tooling/noirc_abi/src/input_parser/json.rs index a8ba83a3a65..7b2e8be454b 100644 --- a/tooling/noirc_abi/src/input_parser/json.rs +++ b/tooling/noirc_abi/src/input_parser/json.rs @@ -1,4 +1,7 @@ -use super::{field_to_signed_hex, parse_str_to_field, parse_str_to_signed, InputValue}; +use super::{ + field_to_signed_hex, parse_integer_to_signed, parse_str_to_field, parse_str_to_signed, + InputValue, +}; use crate::{errors::InputParserError, Abi, AbiType, MAIN_RETURN_NAME}; use acvm::{AcirField, FieldElement}; use iter_extended::{try_btree_map, try_vecmap}; @@ -69,9 +72,9 @@ pub enum JsonTypes { // Just a regular integer, that can fit in 64 bits. // // The JSON spec does not specify any limit on the size of integer number types, - // however we restrict the allowable size. Values which do not fit in a u64 should be passed + // however we restrict the allowable size. Values which do not fit in a i64 should be passed // as a string. - Integer(u64), + Integer(i64), // Simple boolean flag Bool(bool), // Array of JsonTypes @@ -147,12 +150,20 @@ impl InputValue { let input_value = match (value, param_type) { (JsonTypes::String(string), AbiType::String { .. }) => InputValue::String(string), (JsonTypes::String(string), AbiType::Integer { sign: crate::Sign::Signed, width }) => { - InputValue::Field(parse_str_to_signed(&string, *width)?) + InputValue::Field(parse_str_to_signed(&string, *width, arg_name)?) } ( JsonTypes::String(string), AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean, - ) => InputValue::Field(parse_str_to_field(&string)?), + ) => InputValue::Field(parse_str_to_field(&string, arg_name)?), + + ( + JsonTypes::Integer(integer), + AbiType::Integer { sign: crate::Sign::Signed, width }, + ) => { + let new_value = parse_integer_to_signed(integer as i128, *width, arg_name)?; + InputValue::Field(new_value) + } ( JsonTypes::Integer(integer), @@ -166,8 +177,13 @@ impl InputValue { (JsonTypes::Bool(boolean), AbiType::Boolean) => InputValue::Field(boolean.into()), (JsonTypes::Array(array), AbiType::Array { typ, .. }) => { - let array_elements = - try_vecmap(array, |value| InputValue::try_from_json(value, typ, arg_name))?; + let mut index = 0; + let array_elements = try_vecmap(array, |value| { + let sub_name = format!("{arg_name}[{index}]"); + let value = InputValue::try_from_json(value, typ, &sub_name); + index += 1; + value + })?; InputValue::Vec(array_elements) } @@ -186,8 +202,12 @@ impl InputValue { } (JsonTypes::Array(array), AbiType::Tuple { fields }) => { + let mut index = 0; let tuple_fields = try_vecmap(array.into_iter().zip(fields), |(value, typ)| { - InputValue::try_from_json(value, typ, arg_name) + let sub_name = format!("{arg_name}[{index}]"); + let value = InputValue::try_from_json(value, typ, &sub_name); + index += 1; + value })?; InputValue::Vec(tuple_fields) } @@ -201,11 +221,13 @@ impl InputValue { #[cfg(test)] mod test { + use acvm::FieldElement; use proptest::prelude::*; use crate::{ arbitrary::arb_abi_and_input_map, input_parser::{arbitrary::arb_signed_integer_type_and_value, json::JsonTypes, InputValue}, + AbiType, }; use super::{parse_json, serialize_to_json}; @@ -234,4 +256,26 @@ mod test { prop_assert_eq!(output_number, value); } } + + #[test] + fn errors_on_integer_to_signed_integer_overflow() { + let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 8 }; + let input = JsonTypes::Integer(128); + assert!(InputValue::try_from_json(input, &typ, "foo").is_err()); + + let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 16 }; + let input = JsonTypes::Integer(32768); + assert!(InputValue::try_from_json(input, &typ, "foo").is_err()); + } + + #[test] + fn try_from_json_negative_integer() { + let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 8 }; + let input = JsonTypes::Integer(-1); + let InputValue::Field(field) = InputValue::try_from_json(input, &typ, "foo").unwrap() + else { + panic!("Expected field"); + }; + assert_eq!(field, FieldElement::from(255_u128)); + } } diff --git a/tooling/noirc_abi/src/input_parser/mod.rs b/tooling/noirc_abi/src/input_parser/mod.rs index b7732235eb2..565870b0835 100644 --- a/tooling/noirc_abi/src/input_parser/mod.rs +++ b/tooling/noirc_abi/src/input_parser/mod.rs @@ -304,25 +304,35 @@ mod serialization_tests { } } -fn parse_str_to_field(value: &str) -> Result { +fn parse_str_to_field(value: &str, arg_name: &str) -> Result { let big_num = if let Some(hex) = value.strip_prefix("0x") { BigUint::from_str_radix(hex, 16) } else { BigUint::from_str_radix(value, 10) }; - big_num.map_err(|err_msg| InputParserError::ParseStr(err_msg.to_string())).and_then(|bigint| { - if bigint < FieldElement::modulus() { - Ok(field_from_big_uint(bigint)) - } else { - Err(InputParserError::ParseStr(format!( - "Input exceeds field modulus. Values must fall within [0, {})", - FieldElement::modulus(), - ))) - } - }) + big_num + .map_err(|err_msg| InputParserError::ParseStr { + arg_name: arg_name.into(), + value: value.into(), + error: err_msg.to_string(), + }) + .and_then(|bigint| { + if bigint < FieldElement::modulus() { + Ok(field_from_big_uint(bigint)) + } else { + Err(InputParserError::InputExceedsFieldModulus { + arg_name: arg_name.into(), + value: value.to_string(), + }) + } + }) } -fn parse_str_to_signed(value: &str, width: u32) -> Result { +fn parse_str_to_signed( + value: &str, + width: u32, + arg_name: &str, +) -> Result { let big_num = if let Some(hex) = value.strip_prefix("-0x") { BigInt::from_str_radix(hex, 16).map(|value| -value) } else if let Some(hex) = value.strip_prefix("0x") { @@ -331,22 +341,75 @@ fn parse_str_to_signed(value: &str, width: u32) -> Result max { + return Err(InputParserError::InputOverflowsMaximum { + arg_name: arg_name.into(), + value: bigint.to_string(), + max: max.to_string(), + }); + } + + let modulus: BigInt = FieldElement::modulus().into(); + let bigint = if bigint.sign() == num_bigint::Sign::Minus { + BigInt::from(2).pow(width) + bigint + } else { + bigint + }; + if bigint.is_zero() || (bigint.sign() == num_bigint::Sign::Plus && bigint < modulus) { + Ok(field_from_big_int(bigint)) + } else { + Err(InputParserError::InputExceedsFieldModulus { + arg_name: arg_name.into(), + value: value.to_string(), + }) + } + }) +} + +fn parse_integer_to_signed( + integer: i128, + width: u32, + arg_name: &str, +) -> Result { + let min = -(1 << (width - 1)); + let max = (1 << (width - 1)) - 1; + + if integer < min { + return Err(InputParserError::InputUnderflowsMinimum { + arg_name: arg_name.into(), + value: integer.to_string(), + min: min.to_string(), + }); + } + + if integer > max { + return Err(InputParserError::InputOverflowsMaximum { + arg_name: arg_name.into(), + value: integer.to_string(), + max: max.to_string(), + }); + } + + let integer = if integer < 0 { (1 << width) + integer } else { integer }; + Ok(FieldElement::from(integer as u128)) } fn field_from_big_uint(bigint: BigUint) -> FieldElement { @@ -390,7 +453,7 @@ mod test { #[test] fn parse_empty_str_fails() { // Check that this fails appropriately rather than being treated as 0, etc. - assert!(parse_str_to_field("").is_err()); + assert!(parse_str_to_field("", "arg_name").is_err()); } #[test] @@ -405,11 +468,11 @@ mod test { for field in fields { let hex_field = format!("0x{}", field.to_hex()); - let field_from_hex = parse_str_to_field(&hex_field).unwrap(); + let field_from_hex = parse_str_to_field(&hex_field, "arg_name").unwrap(); assert_eq!(field_from_hex, field); let dec_field = big_uint_from_field(field).to_string(); - let field_from_dec = parse_str_to_field(&dec_field).unwrap(); + let field_from_dec = parse_str_to_field(&dec_field, "arg_name").unwrap(); assert_eq!(field_from_dec, field); } } @@ -417,19 +480,46 @@ mod test { #[test] fn rejects_noncanonical_fields() { let noncanonical_field = FieldElement::modulus().to_string(); - assert!(parse_str_to_field(&noncanonical_field).is_err()); + assert!(parse_str_to_field(&noncanonical_field, "arg_name").is_err()); } #[test] fn test_parse_str_to_signed() { - let value = parse_str_to_signed("1", 8).unwrap(); + let value = parse_str_to_signed("1", 8, "arg_name").unwrap(); assert_eq!(value, FieldElement::from(1_u128)); - let value = parse_str_to_signed("-1", 8).unwrap(); + let value = parse_str_to_signed("-1", 8, "arg_name").unwrap(); assert_eq!(value, FieldElement::from(255_u128)); - let value = parse_str_to_signed("-1", 16).unwrap(); + let value = parse_str_to_signed("-1", 16, "arg_name").unwrap(); assert_eq!(value, FieldElement::from(65535_u128)); + + assert_eq!( + parse_str_to_signed("127", 8, "arg_name").unwrap(), + FieldElement::from(127_i128) + ); + assert!(parse_str_to_signed("128", 8, "arg_name").is_err()); + assert_eq!( + parse_str_to_signed("-128", 8, "arg_name").unwrap(), + FieldElement::from(128_i128) + ); + assert_eq!(parse_str_to_signed("-1", 8, "arg_name").unwrap(), FieldElement::from(255_i128)); + assert!(parse_str_to_signed("-129", 8, "arg_name").is_err()); + + assert_eq!( + parse_str_to_signed("32767", 16, "arg_name").unwrap(), + FieldElement::from(32767_i128) + ); + assert!(parse_str_to_signed("32768", 16, "arg_name").is_err()); + assert_eq!( + parse_str_to_signed("-32768", 16, "arg_name").unwrap(), + FieldElement::from(32768_i128) + ); + assert_eq!( + parse_str_to_signed("-1", 16, "arg_name").unwrap(), + FieldElement::from(65535_i128) + ); + assert!(parse_str_to_signed("-32769", 16, "arg_name").is_err()); } } diff --git a/tooling/noirc_abi/src/input_parser/toml.rs b/tooling/noirc_abi/src/input_parser/toml.rs index 6f2be68a0c4..aaa3358f4fc 100644 --- a/tooling/noirc_abi/src/input_parser/toml.rs +++ b/tooling/noirc_abi/src/input_parser/toml.rs @@ -1,4 +1,7 @@ -use super::{field_to_signed_hex, parse_str_to_field, parse_str_to_signed, InputValue}; +use super::{ + field_to_signed_hex, parse_integer_to_signed, parse_str_to_field, parse_str_to_signed, + InputValue, +}; use crate::{errors::InputParserError, Abi, AbiType, MAIN_RETURN_NAME}; use acvm::{AcirField, FieldElement}; use iter_extended::{try_btree_map, try_vecmap}; @@ -68,7 +71,7 @@ enum TomlTypes { String(String), // Just a regular integer, that can fit in 64 bits // Note that the toml spec specifies that all numbers are represented as `i64`s. - Integer(u64), + Integer(i64), // Simple boolean flag Bool(bool), // Array of TomlTypes @@ -135,15 +138,23 @@ impl InputValue { AbiType::Field | AbiType::Integer { sign: crate::Sign::Unsigned, .. } | AbiType::Boolean, - ) => InputValue::Field(parse_str_to_field(&string)?), + ) => InputValue::Field(parse_str_to_field(&string, arg_name)?), (TomlTypes::String(string), AbiType::Integer { sign: crate::Sign::Signed, width }) => { - InputValue::Field(parse_str_to_signed(&string, *width)?) + InputValue::Field(parse_str_to_signed(&string, *width, arg_name)?) } + ( + TomlTypes::Integer(integer), + AbiType::Integer { sign: crate::Sign::Signed, width }, + ) => { + let new_value = parse_integer_to_signed(integer as i128, *width, arg_name)?; + InputValue::Field(new_value) + } + ( TomlTypes::Integer(integer), AbiType::Field | AbiType::Integer { .. } | AbiType::Boolean, ) => { - let new_value = FieldElement::from(u128::from(integer)); + let new_value = FieldElement::from(i128::from(integer)); InputValue::Field(new_value) } @@ -151,8 +162,13 @@ impl InputValue { (TomlTypes::Bool(boolean), AbiType::Boolean) => InputValue::Field(boolean.into()), (TomlTypes::Array(array), AbiType::Array { typ, .. }) => { - let array_elements = - try_vecmap(array, |value| InputValue::try_from_toml(value, typ, arg_name))?; + let mut index = 0; + let array_elements = try_vecmap(array, |value| { + let sub_name = format!("{arg_name}[{index}]"); + let value = InputValue::try_from_toml(value, typ, &sub_name); + index += 1; + value + })?; InputValue::Vec(array_elements) } @@ -171,8 +187,12 @@ impl InputValue { } (TomlTypes::Array(array), AbiType::Tuple { fields }) => { + let mut index = 0; let tuple_fields = try_vecmap(array.into_iter().zip(fields), |(value, typ)| { - InputValue::try_from_toml(value, typ, arg_name) + let sub_name = format!("{arg_name}[{index}]"); + let value = InputValue::try_from_toml(value, typ, &sub_name); + index += 1; + value })?; InputValue::Vec(tuple_fields) } @@ -186,11 +206,13 @@ impl InputValue { #[cfg(test)] mod test { + use acvm::FieldElement; use proptest::prelude::*; use crate::{ arbitrary::arb_abi_and_input_map, input_parser::{arbitrary::arb_signed_integer_type_and_value, toml::TomlTypes, InputValue}, + AbiType, }; use super::{parse_toml, serialize_to_toml}; @@ -219,4 +241,26 @@ mod test { prop_assert_eq!(output_number, value); } } + + #[test] + fn errors_on_integer_to_signed_integer_overflow() { + let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 8 }; + let input = TomlTypes::Integer(128); + assert!(InputValue::try_from_toml(input, &typ, "foo").is_err()); + + let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 16 }; + let input = TomlTypes::Integer(32768); + assert!(InputValue::try_from_toml(input, &typ, "foo").is_err()); + } + + #[test] + fn try_from_toml_negative_integer() { + let typ = AbiType::Integer { sign: crate::Sign::Signed, width: 8 }; + let input = TomlTypes::Integer(-1); + let InputValue::Field(field) = InputValue::try_from_toml(input, &typ, "foo").unwrap() + else { + panic!("Expected field"); + }; + assert_eq!(field, FieldElement::from(255_u128)); + } }