diff --git a/crates/nargo_cli/tests/execution_success/brillig_schnorr/Prover.toml b/crates/nargo_cli/tests/execution_success/brillig_schnorr/Prover.toml index c5c3ab5101a..5fe6bd2546f 100644 --- a/crates/nargo_cli/tests/execution_success/brillig_schnorr/Prover.toml +++ b/crates/nargo_cli/tests/execution_success/brillig_schnorr/Prover.toml @@ -1,4 +1,5 @@ message = [0,1,2,3,4,5,6,7,8,9] +message_field = "0x010203040506070809" pub_key_x = "0x17cbd3ed3151ccfd170efe1d54280a6a4822640bf5c369908ad74ea21518a9c5" pub_key_y = "0x0e0456e3795c1a31f20035b741cd6158929eeccd320d299cfcac962865a6bc74" signature = [ diff --git a/crates/nargo_cli/tests/execution_success/brillig_schnorr/src/main.nr b/crates/nargo_cli/tests/execution_success/brillig_schnorr/src/main.nr index 0ffd6af6fcd..4212839601f 100644 --- a/crates/nargo_cli/tests/execution_success/brillig_schnorr/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/brillig_schnorr/src/main.nr @@ -2,9 +2,20 @@ use dep::std; // Note: If main has any unsized types, then the verifier will never be able // to figure out the circuit instance -unconstrained fn main(message: [u8; 10], pub_key_x: Field, pub_key_y: Field, signature: [u8; 64]) { +unconstrained fn main(message: [u8; 10], message_field: Field, pub_key_x: Field, pub_key_y: Field, signature: [u8; 64]) { + // Regression for issue #2421 + // We want to make sure that we can accurately verify a signature whose message is a slice vs. an array + let message_field_bytes = message_field.to_be_bytes(10); + for i in 0..10 { + assert(message[i] == message_field_bytes[i]); + } // Is there ever a situation where someone would want // to ensure that a signature was invalid? + // Check that passing a slice as the message is valid + let valid_signature = std::schnorr::verify_signature(pub_key_x,pub_key_y,signature, message_field_bytes); + assert(valid_signature); + + // Check that passing an array as the message is valid let valid_signature = std::schnorr::verify_signature(pub_key_x,pub_key_y,signature, message); assert(valid_signature); } diff --git a/crates/nargo_cli/tests/execution_success/schnorr/Prover.toml b/crates/nargo_cli/tests/execution_success/schnorr/Prover.toml index c5c3ab5101a..5fe6bd2546f 100644 --- a/crates/nargo_cli/tests/execution_success/schnorr/Prover.toml +++ b/crates/nargo_cli/tests/execution_success/schnorr/Prover.toml @@ -1,4 +1,5 @@ message = [0,1,2,3,4,5,6,7,8,9] +message_field = "0x010203040506070809" pub_key_x = "0x17cbd3ed3151ccfd170efe1d54280a6a4822640bf5c369908ad74ea21518a9c5" pub_key_y = "0x0e0456e3795c1a31f20035b741cd6158929eeccd320d299cfcac962865a6bc74" signature = [ diff --git a/crates/nargo_cli/tests/execution_success/schnorr/src/main.nr b/crates/nargo_cli/tests/execution_success/schnorr/src/main.nr index c0933b23029..3c8881b2f39 100644 --- a/crates/nargo_cli/tests/execution_success/schnorr/src/main.nr +++ b/crates/nargo_cli/tests/execution_success/schnorr/src/main.nr @@ -2,9 +2,20 @@ use dep::std; // Note: If main has any unsized types, then the verifier will never be able // to figure out the circuit instance -fn main(message: [u8; 10], pub_key_x: Field, pub_key_y: Field, signature: [u8; 64]) { +fn main(message: [u8; 10], message_field: Field, pub_key_x: Field, pub_key_y: Field, signature: [u8; 64]) { + // Regression for issue #2421 + // We want to make sure that we can accurately verify a signature whose message is a slice vs. an array + let message_field_bytes = message_field.to_be_bytes(10); + for i in 0..10 { + assert(message[i] == message_field_bytes[i]); + } // Is there ever a situation where someone would want // to ensure that a signature was invalid? + // Check that passing a slice as the message is valid + let valid_signature = std::schnorr::verify_signature(pub_key_x,pub_key_y,signature, message_field_bytes); + assert(valid_signature); + + // Check that passing an array as the message is valid let valid_signature = std::schnorr::verify_signature(pub_key_x,pub_key_y,signature, message); assert(valid_signature); } diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs index 1ee323e4c09..7180467aff7 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs @@ -16,12 +16,24 @@ pub(crate) fn convert_black_box_call( ) { match bb_func { BlackBoxFunc::SHA256 => { - if let ( - [RegisterOrMemory::HeapArray(message_array)], - [RegisterOrMemory::HeapArray(result_array)], - ) = (function_arguments, function_results) + if let ([..], [RegisterOrMemory::HeapArray(result_array)]) = + (function_arguments, function_results) { - let message_vector = brillig_context.array_to_vector(message_array); + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 1 { + &function_arguments[1] + } else { + &function_arguments[0] + }; + let message_vector = match message { + RegisterOrMemory::HeapArray(message_array) => { + brillig_context.array_to_vector(message_array) + } + RegisterOrMemory::HeapVector(message_vector) => *message_vector, + _ => unreachable!("ICE: SHA256 expects the message to be an array or a vector"), + }; brillig_context.black_box_op_instruction(BlackBoxOp::Sha256 { message: message_vector, output: *result_array, @@ -31,12 +43,26 @@ pub(crate) fn convert_black_box_call( } } BlackBoxFunc::Blake2s => { - if let ( - [RegisterOrMemory::HeapArray(message_array)], - [RegisterOrMemory::HeapArray(result_array)], - ) = (function_arguments, function_results) + if let ([..], [RegisterOrMemory::HeapArray(result_array)]) = + (function_arguments, function_results) { - let message_vector = brillig_context.array_to_vector(message_array); + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 1 { + &function_arguments[1] + } else { + &function_arguments[0] + }; + let message_vector = match message { + RegisterOrMemory::HeapArray(message_array) => { + brillig_context.array_to_vector(message_array) + } + RegisterOrMemory::HeapVector(message_vector) => *message_vector, + _ => { + unreachable!("ICE: Blake2s expects the message to be an array or a vector") + } + }; brillig_context.black_box_op_instruction(BlackBoxOp::Blake2s { message: message_vector, output: *result_array, @@ -47,12 +73,27 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::Keccak256 => { if let ( - [RegisterOrMemory::HeapArray(message_array), RegisterOrMemory::RegisterIndex(array_size)], + [.., RegisterOrMemory::RegisterIndex(array_size)], [RegisterOrMemory::HeapArray(result_array)], ) = (function_arguments, function_results) { - let message_vector = - HeapVector { size: *array_size, pointer: message_array.pointer }; + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 2 { + &function_arguments[1] + } else { + &function_arguments[0] + }; + let message_vector = match message { + RegisterOrMemory::HeapArray(message_array) => { + HeapVector { size: *array_size, pointer: message_array.pointer } + } + RegisterOrMemory::HeapVector(message_vector) => *message_vector, + _ => unreachable!( + "ICE: Keccak256 expects the message to be an array or a vector" + ), + }; brillig_context.black_box_op_instruction(BlackBoxOp::Keccak256 { message: message_vector, output: *result_array, @@ -62,12 +103,26 @@ pub(crate) fn convert_black_box_call( } } BlackBoxFunc::HashToField128Security => { - if let ( - [RegisterOrMemory::HeapArray(message_array)], - [RegisterOrMemory::RegisterIndex(result_register)], - ) = (function_arguments, function_results) + if let ([..], [RegisterOrMemory::RegisterIndex(result_register)]) = + (function_arguments, function_results) { - let message_vector = brillig_context.array_to_vector(message_array); + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 1 { + &function_arguments[1] + } else { + &function_arguments[0] + }; + let message_vector = match message { + RegisterOrMemory::HeapArray(message_array) => { + brillig_context.array_to_vector(message_array) + } + RegisterOrMemory::HeapVector(message_vector) => { + *message_vector + } + _ => unreachable!("ICE: HashToField128Security expects the message to be an array or a vector"), + }; brillig_context.black_box_op_instruction(BlackBoxOp::HashToField128Security { message: message_vector, output: *result_register, @@ -78,11 +133,27 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::EcdsaSecp256k1 => { if let ( - [RegisterOrMemory::HeapArray(public_key_x), RegisterOrMemory::HeapArray(public_key_y), RegisterOrMemory::HeapArray(signature), RegisterOrMemory::HeapArray(message_hash)], + [RegisterOrMemory::HeapArray(public_key_x), RegisterOrMemory::HeapArray(public_key_y), RegisterOrMemory::HeapArray(signature), ..], [RegisterOrMemory::RegisterIndex(result_register)], ) = (function_arguments, function_results) { - let message_hash_vector = brillig_context.array_to_vector(message_hash); + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 4 { + &function_arguments[4] + } else { + &function_arguments[3] + }; + let message_hash_vector = match message { + RegisterOrMemory::HeapArray(message_hash) => { + brillig_context.array_to_vector(message_hash) + } + RegisterOrMemory::HeapVector(message_hash_vector) => *message_hash_vector, + _ => unreachable!( + "ICE: EcdsaSecp256k1 expects the message to be an array or a vector" + ), + }; brillig_context.black_box_op_instruction(BlackBoxOp::EcdsaSecp256k1 { hashed_msg: message_hash_vector, public_key_x: *public_key_x, @@ -98,11 +169,27 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::Pedersen => { if let ( - [RegisterOrMemory::HeapArray(message_array), RegisterOrMemory::RegisterIndex(domain_separator)], + [.., RegisterOrMemory::RegisterIndex(domain_separator)], [RegisterOrMemory::HeapArray(result_array)], ) = (function_arguments, function_results) { - let message_vector = brillig_context.array_to_vector(message_array); + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 2 { + &function_arguments[1] + } else { + &function_arguments[0] + }; + let message_vector = match message { + RegisterOrMemory::HeapArray(message_array) => { + brillig_context.array_to_vector(message_array) + } + RegisterOrMemory::HeapVector(message_vector) => *message_vector, + _ => { + unreachable!("ICE: Pedersen expects the message to be an array or a vector") + } + }; brillig_context.black_box_op_instruction(BlackBoxOp::Pedersen { inputs: message_vector, domain_separator: *domain_separator, @@ -114,11 +201,27 @@ pub(crate) fn convert_black_box_call( } BlackBoxFunc::SchnorrVerify => { if let ( - [RegisterOrMemory::RegisterIndex(public_key_x), RegisterOrMemory::RegisterIndex(public_key_y), RegisterOrMemory::HeapArray(signature), RegisterOrMemory::HeapArray(message_hash)], + [RegisterOrMemory::RegisterIndex(public_key_x), RegisterOrMemory::RegisterIndex(public_key_y), RegisterOrMemory::HeapArray(signature), ..], [RegisterOrMemory::RegisterIndex(result_register)], ) = (function_arguments, function_results) { - let message_hash = brillig_context.array_to_vector(message_hash); + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if function_arguments.len() > 4 { + &function_arguments[4] + } else { + &function_arguments[3] + }; + let message_hash = match message { + RegisterOrMemory::HeapArray(message_hash) => { + brillig_context.array_to_vector(message_hash) + } + RegisterOrMemory::HeapVector(message_hash) => *message_hash, + _ => unreachable!( + "ICE: Schnorr verify expects the message to be an array or a vector" + ), + }; let signature = brillig_context.array_to_vector(signature); brillig_context.black_box_op_instruction(BlackBoxOp::SchnorrVerify { public_key_x: *public_key_x, diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 29ca4fa3892..3f12bda8da2 100644 --- a/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -147,47 +147,86 @@ impl GeneratedAcir { BlackBoxFuncCall::XOR { lhs: inputs[0][0], rhs: inputs[1][0], output: outputs[0] } } BlackBoxFunc::RANGE => BlackBoxFuncCall::RANGE { input: inputs[0][0] }, - BlackBoxFunc::SHA256 => BlackBoxFuncCall::SHA256 { inputs: inputs[0].clone(), outputs }, + BlackBoxFunc::SHA256 => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; + BlackBoxFuncCall::SHA256 { inputs, outputs } + } BlackBoxFunc::Blake2s => { - BlackBoxFuncCall::Blake2s { inputs: inputs[0].clone(), outputs } + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; + BlackBoxFuncCall::Blake2s { inputs, outputs } + } + BlackBoxFunc::HashToField128Security => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; + BlackBoxFuncCall::HashToField128Security { inputs, output: outputs[0] } + } + BlackBoxFunc::SchnorrVerify => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let message = if inputs.len() > 4 { inputs[4].clone() } else { inputs[3].clone() }; + BlackBoxFuncCall::SchnorrVerify { + public_key_x: inputs[0][0], + public_key_y: inputs[1][0], + // Schnorr signature is an r & s, 32 bytes each + signature: inputs[2].clone(), + message, + output: outputs[0], + } + } + BlackBoxFunc::Pedersen => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let inputs = if inputs.len() > 1 { inputs[1].clone() } else { inputs[0].clone() }; + BlackBoxFuncCall::Pedersen { + inputs, + outputs: (outputs[0], outputs[1]), + domain_separator: constants[0].to_u128() as u32, + } + } + BlackBoxFunc::EcdsaSecp256k1 => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let hashed_message = + if inputs.len() > 4 { inputs[4].clone() } else { inputs[3].clone() }; + BlackBoxFuncCall::EcdsaSecp256k1 { + // 32 bytes for each public key co-ordinate + public_key_x: inputs[0].clone(), + public_key_y: inputs[1].clone(), + // (r,s) are both 32 bytes each, so signature + // takes up 64 bytes + signature: inputs[2].clone(), + hashed_message, + output: outputs[0], + } + } + BlackBoxFunc::EcdsaSecp256r1 => { + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + let hashed_message = + if inputs.len() > 4 { inputs[4].clone() } else { inputs[3].clone() }; + BlackBoxFuncCall::EcdsaSecp256r1 { + // 32 bytes for each public key co-ordinate + public_key_x: inputs[0].clone(), + public_key_y: inputs[1].clone(), + // (r,s) are both 32 bytes each, so signature + // takes up 64 bytes + signature: inputs[2].clone(), + hashed_message, + output: outputs[0], + } } - BlackBoxFunc::HashToField128Security => BlackBoxFuncCall::HashToField128Security { - inputs: inputs[0].clone(), - output: outputs[0], - }, - BlackBoxFunc::SchnorrVerify => BlackBoxFuncCall::SchnorrVerify { - public_key_x: inputs[0][0], - public_key_y: inputs[1][0], - // Schnorr signature is an r & s, 32 bytes each - signature: inputs[2].clone(), - message: inputs[3].clone(), - output: outputs[0], - }, - BlackBoxFunc::Pedersen => BlackBoxFuncCall::Pedersen { - inputs: inputs[0].clone(), - outputs: (outputs[0], outputs[1]), - domain_separator: constants[0].to_u128() as u32, - }, - BlackBoxFunc::EcdsaSecp256k1 => BlackBoxFuncCall::EcdsaSecp256k1 { - // 32 bytes for each public key co-ordinate - public_key_x: inputs[0].clone(), - public_key_y: inputs[1].clone(), - // (r,s) are both 32 bytes each, so signature - // takes up 64 bytes - signature: inputs[2].clone(), - hashed_message: inputs[3].clone(), - output: outputs[0], - }, - BlackBoxFunc::EcdsaSecp256r1 => BlackBoxFuncCall::EcdsaSecp256r1 { - // 32 bytes for each public key co-ordinate - public_key_x: inputs[0].clone(), - public_key_y: inputs[1].clone(), - // (r,s) are both 32 bytes each, so signature - // takes up 64 bytes - signature: inputs[2].clone(), - hashed_message: inputs[3].clone(), - output: outputs[0], - }, BlackBoxFunc::FixedBaseScalarMul => BlackBoxFuncCall::FixedBaseScalarMul { input: inputs[0][0], outputs: (outputs[0], outputs[1]), @@ -203,11 +242,14 @@ impl GeneratedAcir { }); } }; - BlackBoxFuncCall::Keccak256VariableLength { - inputs: inputs[0].clone(), - var_message_size, - outputs, - } + + // Slices are represented as a tuple of (length, slice contents). + // We must check the number of inputs to differentiate between arrays and slices + // and make sure that we pass the correct inputs to the function call. + // `inputs` is cloned into a vector before being popped to find the `var_message_size` + // so we still check `inputs` against its original size passed into `call_black_box` + let inputs = if inputs.len() > 2 { inputs[1].clone() } else { inputs[0].clone() }; + BlackBoxFuncCall::Keccak256VariableLength { inputs, var_message_size, outputs } } BlackBoxFunc::RecursiveAggregation => { let has_previous_aggregation = self.opcodes.iter().any(|op| { diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 555f4b89275..86a0ca1f20c 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -886,8 +886,8 @@ impl<'interner> Monomorphizer<'interner> { } } let printable_type: PrintableType = typ.into(); - let abi_as_string = - serde_json::to_string(&printable_type).expect("ICE: expected PrintableType to serialize"); + let abi_as_string = serde_json::to_string(&printable_type) + .expect("ICE: expected PrintableType to serialize"); arguments.push(ast::Expression::Literal(ast::Literal::Str(abi_as_string))); }