diff --git a/.noir-sync-commit b/.noir-sync-commit index e7c73939ac6c..bec736647ac4 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -b541e793e20fa3c991e0328ec2ff7926bdcdfd45 +a5b7df12faf9d71ff24f8c5cde5e78da44558caf diff --git a/noir/noir-repo/.tokeignore b/noir/noir-repo/.tokeignore new file mode 100644 index 000000000000..55f24e41dbd5 --- /dev/null +++ b/noir/noir-repo/.tokeignore @@ -0,0 +1,12 @@ +docs +scripts + +# aztec_macros is explicitly considered OOS for Noir audit +aztec_macros + +# config files +*.toml +*.md +*.json +*.txt +*.config.mjs diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 0de0c28be75b..117804b370a6 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1741,13 +1741,16 @@ impl<'a> Context<'a> { // will expand the array if there is one. let return_acir_vars = self.flatten_value_list(return_values, dfg)?; let mut warnings = Vec::new(); - for acir_var in return_acir_vars { + for (acir_var, is_databus) in return_acir_vars { if self.acir_context.is_constant(&acir_var) { warnings.push(SsaReport::Warning(InternalWarning::ReturnConstant { call_stack: call_stack.clone(), })); } - self.acir_context.return_var(acir_var)?; + if !is_databus { + // We do not return value for the data bus. + self.acir_context.return_var(acir_var)?; + } } Ok(warnings) } @@ -2671,12 +2674,22 @@ impl<'a> Context<'a> { &mut self, arguments: &[ValueId], dfg: &DataFlowGraph, - ) -> Result, InternalError> { + ) -> Result, InternalError> { let mut acir_vars = Vec::with_capacity(arguments.len()); for value_id in arguments { + let is_databus = if let Some(return_databus) = self.data_bus.return_data { + dfg[*value_id] == dfg[return_databus] + } else { + false + }; let value = self.convert_value(*value_id, dfg); acir_vars.append( - &mut self.acir_context.flatten(value)?.iter().map(|(var, _)| *var).collect(), + &mut self + .acir_context + .flatten(value)? + .iter() + .map(|(var, _)| (*var, is_databus)) + .collect(), ); } Ok(acir_vars) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 77b9e545e03c..73dc38881840 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -393,10 +393,11 @@ impl<'function> PerFunctionContext<'function> { let function = &ssa.functions[&func_id]; // If we have not already finished the flattening pass, functions marked // to not have predicates should be marked as entry points unless we are inlining into brillig. + let entry_point = &ssa.functions[&self.context.entry_point]; let no_predicates_is_entry_point = self.context.no_predicates_is_entry_point && function.is_no_predicates() - && !matches!(self.source_function.runtime(), RuntimeType::Brillig); + && !matches!(entry_point.runtime(), RuntimeType::Brillig); if function.runtime().is_entry_point() || no_predicates_is_entry_point { self.push_instruction(*id); } else { diff --git a/noir/noir-repo/noir_stdlib/src/uint128.nr b/noir/noir-repo/noir_stdlib/src/uint128.nr index 9c61fc801f3c..0332c8ac865d 100644 --- a/noir/noir-repo/noir_stdlib/src/uint128.nr +++ b/noir/noir-repo/noir_stdlib/src/uint128.nr @@ -1,8 +1,9 @@ use crate::ops::{Add, Sub, Mul, Div, Rem, Not, BitOr, BitAnd, BitXor, Shl, Shr}; use crate::cmp::{Eq, Ord, Ordering}; +use crate::println; global pow64 : Field = 18446744073709551616; //2^64; - +global pow63 : Field = 9223372036854775808; // 2^63; struct U128 { lo: Field, hi: Field, @@ -20,6 +21,13 @@ impl U128 { U128::from_u64s_le(lo, hi) } + pub fn zero() -> U128 { + U128 { lo: 0, hi: 0 } + } + + pub fn one() -> U128 { + U128 { lo: 1, hi: 0 } + } pub fn from_le_bytes(bytes: [u8; 16]) -> U128 { let mut lo = 0; let mut base = 1; @@ -87,27 +95,44 @@ impl U128 { U128 { lo: lo as Field, hi: hi as Field } } + unconstrained fn uconstrained_check_is_upper_ascii(ascii: u8) -> bool { + ((ascii >= 65) & (ascii <= 90)) // Between 'A' and 'Z' + } + fn decode_ascii(ascii: u8) -> Field { if ascii < 58 { ascii - 48 - } else if ascii < 71 { - ascii - 55 } else { + let ascii = ascii + 32 * (U128::uconstrained_check_is_upper_ascii(ascii) as u8); + assert(ascii >= 97); // enforce >= 'a' + assert(ascii <= 102); // enforce <= 'f' ascii - 87 } as Field } + // TODO: Replace with a faster version. + // A circuit that uses this function can be slow to compute + // (we're doing up to 127 calls to compute the quotient) unconstrained fn unconstrained_div(self: Self, b: U128) -> (U128, U128) { - if self < b { - (U128::from_u64s_le(0, 0), self) + if b == U128::zero() { + // Return 0,0 to avoid eternal loop + (U128::zero(), U128::zero()) + } else if self < b { + (U128::zero(), self) + } else if self == b { + (U128::one(), U128::zero()) } else { - //TODO check if this can overflow? - let (q,r) = self.unconstrained_div(b * U128::from_u64s_le(2, 0)); + let (q,r) = if b.hi as u64 >= pow63 as u64 { + // The result of multiplication by 2 would overflow + (U128::zero(), self) + } else { + self.unconstrained_div(b * U128::from_u64s_le(2, 0)) + }; let q_mul_2 = q * U128::from_u64s_le(2, 0); if r < b { (q_mul_2, r) } else { - (q_mul_2 + U128::from_u64s_le(1, 0), r - b) + (q_mul_2 + U128::one(), r - b) } } } @@ -129,11 +154,7 @@ impl U128 { let low = self.lo * b.lo; let lo = low as u64 as Field; let carry = (low - lo) / pow64; - let high = if crate::field::modulus_num_bits() as u32 > 196 { - (self.lo + self.hi) * (b.lo + b.hi) - low + carry - } else { - self.lo * b.hi + self.hi * b.lo + carry - }; + let high = self.lo * b.hi + self.hi * b.lo + carry; let hi = high as u64 as Field; U128 { lo, hi } } @@ -294,8 +315,8 @@ impl Shr for U128 { } } -mod test { - use crate::uint128::{U128, pow64}; +mod tests { + use crate::uint128::{U128, pow64, pow63}; #[test] fn test_not() { @@ -309,4 +330,205 @@ mod test { let not_not_num = not_num.not(); assert_eq(num, not_not_num); } + + #[test] + fn test_construction() { + // Check little-endian u64 is inversed with big-endian u64 construction + let a = U128::from_u64s_le(2, 1); + let b = U128::from_u64s_be(1, 2); + assert_eq(a, b); + // Check byte construction is equivalent + let c = U128::from_le_bytes([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]); + let d = U128::from_u64s_le(0x0706050403020100, 0x0f0e0d0c0b0a0908); + assert_eq(c, d); + } + + #[test] + fn test_byte_decomposition() { + let a = U128::from_u64s_le(0x0706050403020100, 0x0f0e0d0c0b0a0908); + // Get big-endian and little-endian byte decompostions + let le_bytes_a= a.to_le_bytes(); + let be_bytes_a= a.to_be_bytes(); + + // Check equivalence + for i in 0..16 { + assert_eq(le_bytes_a[i], be_bytes_a[15 - i]); + } + // Reconstruct U128 from byte decomposition + let b= U128::from_le_bytes(le_bytes_a); + // Check that it's the same element + assert_eq(a, b); + } + + #[test] + fn test_hex_constuction() { + let a = U128::from_u64s_le(0x1, 0x2); + let b = U128::from_hex("0x20000000000000001"); + assert_eq(a, b); + + let c= U128::from_hex("0xffffffffffffffffffffffffffffffff"); + let d= U128::from_u64s_le(0xffffffffffffffff, 0xffffffffffffffff); + assert_eq(c, d); + + let e= U128::from_hex("0x00000000000000000000000000000000"); + let f= U128::from_u64s_le(0, 0); + assert_eq(e, f); + } + + // Ascii decode tests + + #[test] + fn test_ascii_decode_correct_range() { + // '0'..'9' range + for i in 0..10 { + let decoded= U128::decode_ascii(48 + i); + assert_eq(decoded, i as Field); + } + // 'A'..'F' range + for i in 0..6 { + let decoded = U128::decode_ascii(65 + i); + assert_eq(decoded, (i + 10) as Field); + } + // 'a'..'f' range + for i in 0..6 { + let decoded = U128::decode_ascii(97 + i); + assert_eq(decoded, (i + 10) as Field); + } + } + + #[test(should_fail)] + fn test_ascii_decode_range_less_than_48_fails_0() { + crate::println(U128::decode_ascii(0)); + } + + #[test(should_fail)] + fn test_ascii_decode_range_less_than_48_fails_1() { + crate::println(U128::decode_ascii(47)); + } + + #[test(should_fail)] + fn test_ascii_decode_range_58_64_fails_0() { + let _ = U128::decode_ascii(58); + } + + #[test(should_fail)] + fn test_ascii_decode_range_58_64_fails_1() { + let _ = U128::decode_ascii(64); + } + + #[test(should_fail)] + fn test_ascii_decode_range_71_96_fails_0() { + let _ = U128::decode_ascii(71); + } + + #[test(should_fail)] + fn test_ascii_decode_range_71_96_fails_1() { + let _ = U128::decode_ascii(96); + } + + #[test(should_fail)] + fn test_ascii_decode_range_greater_than_102_fails() { + let _ = U128::decode_ascii(103); + } + + #[test(should_fail)] + fn test_ascii_decode_regression() { + // This code will actually fail because of ascii_decode, + // but in the past it was possible to create a value > (1<<128) + let a = U128::from_hex("0x~fffffffffffffffffffffffffffffff"); + let b:Field= a.to_integer(); + let c= b.to_le_bytes(17); + assert(c[16] != 0); + } + + #[test] + fn test_unconstrained_div() { + // Test the potential overflow case + let a= U128::from_u64s_le(0x0, 0xffffffffffffffff); + let b= U128::from_u64s_le(0x0, 0xfffffffffffffffe); + let c= U128::one(); + let d= U128::from_u64s_le(0x0, 0x1); + let (q,r) = a.unconstrained_div(b); + assert_eq(q, c); + assert_eq(r, d); + + let a = U128::from_u64s_le(2, 0); + let b = U128::one(); + // Check the case where a is a multiple of b + let (c,d ) = a.unconstrained_div(b); + assert_eq((c, d), (a, U128::zero())); + + // Check where b is a multiple of a + let (c,d) = b.unconstrained_div(a); + assert_eq((c, d), (U128::zero(), b)); + + // Dividing by zero returns 0,0 + let a = U128::from_u64s_le(0x1, 0x0); + let b = U128::zero(); + let (c,d)= a.unconstrained_div(b); + assert_eq((c, d), (U128::zero(), U128::zero())); + + // Dividing 1<<127 by 1<<127 (special case) + let a = U128::from_u64s_le(0x0, pow63 as u64); + let b = U128::from_u64s_le(0x0, pow63 as u64); + let (c,d )= a.unconstrained_div(b); + assert_eq((c, d), (U128::one(), U128::zero())); + } + + #[test] + fn integer_conversions() { + // Maximum + let start:Field = 0xffffffffffffffffffffffffffffffff; + let a = U128::from_integer(start); + let end = a.to_integer(); + assert_eq(start, end); + + // Minimum + let start:Field = 0x0; + let a = U128::from_integer(start); + let end = a.to_integer(); + assert_eq(start, end); + + // Low limb + let start:Field = 0xffffffffffffffff; + let a = U128::from_integer(start); + let end = a.to_integer(); + assert_eq(start, end); + + // High limb + let start:Field = 0xffffffffffffffff0000000000000000; + let a = U128::from_integer(start); + let end = a.to_integer(); + assert_eq(start, end); + } + #[test] + fn test_wrapping_mul() { + // 1*0==0 + assert_eq(U128::zero(), U128::zero().wrapping_mul(U128::one())); + + // 0*1==0 + assert_eq(U128::zero(), U128::one().wrapping_mul(U128::zero())); + + // 1*1==1 + assert_eq(U128::one(), U128::one().wrapping_mul(U128::one())); + + // 0 * ( 1 << 64 ) == 0 + assert_eq(U128::zero(), U128::zero().wrapping_mul(U128::from_u64s_le(0, 1))); + + // ( 1 << 64 ) * 0 == 0 + assert_eq(U128::zero(), U128::from_u64s_le(0, 1).wrapping_mul(U128::zero())); + + // 1 * ( 1 << 64 ) == 1 << 64 + assert_eq(U128::from_u64s_le(0, 1), U128::from_u64s_le(0, 1).wrapping_mul(U128::one())); + + // ( 1 << 64 ) * 1 == 1 << 64 + assert_eq(U128::from_u64s_le(0, 1), U128::one().wrapping_mul(U128::from_u64s_le(0, 1))); + + // ( 1 << 64 ) * ( 1 << 64 ) == 1 << 64 + assert_eq(U128::zero(), U128::from_u64s_le(0, 1).wrapping_mul(U128::from_u64s_le(0, 1))); + // -1 * -1 == 1 + assert_eq( + U128::one(), U128::from_u64s_le(0xffffffffffffffff, 0xffffffffffffffff).wrapping_mul(U128::from_u64s_le(0xffffffffffffffff, 0xffffffffffffffff)) + ); + } } diff --git a/noir/noir-repo/scripts/count_loc.sh b/noir/noir-repo/scripts/count_loc.sh new file mode 100755 index 000000000000..91565aa6c4a1 --- /dev/null +++ b/noir/noir-repo/scripts/count_loc.sh @@ -0,0 +1,33 @@ +#!/usr/bin/env bash +set -eu + +# Run relative to repo root +cd $(dirname "$0")/../ + +if ! command -v "tokei" >/dev/null 2>&1; then + echo "Error: tokei is required but not installed." >&2 + echo "Error: Run \`cargo install --git https://github.com/TomAFrench/tokei --branch tf/add-noir-support tokei\`" >&2 + + exit 1 +fi + +echo "" +echo "Total:" + +tokei ./ --sort code + +echo "" +echo "ACIR/ACVM:" +tokei ./acvm-repo --sort code + +echo "" +echo "Compiler:" +tokei ./compiler --sort code + +echo "" +echo "Tooling:" +tokei ./tooling --sort code + +echo "" +echo "Standard Library:" +tokei ./noir_stdlib --sort code diff --git a/noir/noir-repo/security/insectarium/noir_stdlib.md b/noir/noir-repo/security/insectarium/noir_stdlib.md new file mode 100644 index 000000000000..5ec4eb5f6cd1 --- /dev/null +++ b/noir/noir-repo/security/insectarium/noir_stdlib.md @@ -0,0 +1,61 @@ +# Bugs found in Noir stdlib + +## U128 + +### decode_ascii +Old **decode_ascii** function didn't check that the values of individual bytes in the string were just in the range of [0-9a-f-A-F]. +```rust +fn decode_ascii(ascii: u8) -> Field { + if ascii < 58 { + ascii - 48 + } else if ascii < 71 { + ascii - 55 + } else { + ascii - 87 + } as Field +} +``` +Since the function used the assumption that decode_ascii returns values in range [0,15] to construct **lo** and **hi** it was possible to overflow these 64-bit limbs. + +### unconstrained_div +```rust + unconstrained fn unconstrained_div(self: Self, b: U128) -> (U128, U128) { + if self < b { + (U128::from_u64s_le(0, 0), self) + } else { + //TODO check if this can overflow? + let (q,r) = self.unconstrained_div(b * U128::from_u64s_le(2, 0)); + let q_mul_2 = q * U128::from_u64s_le(2, 0); + if r < b { + (q_mul_2, r) + } else { + (q_mul_2 + U128::from_u64s_le(1, 0), r - b) + } + } + } +``` +There were 2 issues in unconstrained_div: +1) Attempting to divide by zero resulted in an infinite loop, because there was no check. +2) $a >= 2^{127}$ cause the function to multiply b to such power of 2 that the result would be more than $2^{128}$ and lead to assertion failure even though it was a legitimate input + +N.B. initial fix by Rumata888 also had an edgecase missing for when a==b and b >= (1<<127). + +### wrapping_mul +```rust +fn wrapping_mul(self: Self, b: U128) -> U128 { + let low = self.lo * b.lo; + let lo = low as u64 as Field; + let carry = (low - lo) / pow64; + let high = if crate::field::modulus_num_bits() as u32 > 196 { + (self.lo + self.hi) * (b.lo + b.hi) - low + carry // Bug + } else { + self.lo * b.hi + self.hi * b.lo + carry + }; + let hi = high as u64 as Field; + U128 { lo, hi } + } +``` +Wrapping mul had the code copied from regular mul barring the assertion that the product of high limbs is zero. Because that check was removed, the optimized path for moduli > 196 bits was incorrect, since it included their product (as at least one of them was supposed to be zero originally, but not for wrapping multiplication) + + + diff --git a/noir/noir-repo/test_programs/execution_success/no_predicates_brillig/src/main.nr b/noir/noir-repo/test_programs/execution_success/no_predicates_brillig/src/main.nr index 1d088473aa7f..65e2e5d61fe2 100644 --- a/noir/noir-repo/test_programs/execution_success/no_predicates_brillig/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/no_predicates_brillig/src/main.nr @@ -1,4 +1,8 @@ unconstrained fn main(x: u32, y: pub u32) { + intermediate_function(x, y); +} + +fn intermediate_function(x: u32, y: u32) { basic_checks(x, y); } diff --git a/noir/noir-repo/tooling/noirc_abi/src/lib.rs b/noir/noir-repo/tooling/noirc_abi/src/lib.rs index 7e89a102a984..7a1d1787ca56 100644 --- a/noir/noir-repo/tooling/noirc_abi/src/lib.rs +++ b/noir/noir-repo/tooling/noirc_abi/src/lib.rs @@ -471,7 +471,15 @@ impl Abi { .copied() }) { - Some(decode_value(&mut return_witness_values.into_iter(), &return_type.abi_type)?) + // We do not return value for the data bus. + if return_type.visibility == AbiVisibility::DataBus { + None + } else { + Some(decode_value( + &mut return_witness_values.into_iter(), + &return_type.abi_type, + )?) + } } else { // Unlike for the circuit inputs, we tolerate not being able to find the witness values for the return value. // This is because the user may be decoding a partial witness map for which is hasn't been calculated yet.