fix: ECDSA verify should not panic when signature not reduced

extensions/algebra/guest/src/lib.rs

@@ -140,21 +140,27 @@ pub trait IntMod:
     const ONE: Self;
 
     /// Creates a new IntMod from an instance of Repr.
+    /// Does not enforce the integer value of `bytes` must be less than the modulus.
     fn from_repr(repr: Self::Repr) -> Self;
 
     /// Creates a new IntMod from an array of bytes, little endian.
+    /// Does not enforce the integer value of `bytes` must be less than the modulus.
     fn from_le_bytes(bytes: &[u8]) -> Self;
 
     /// Creates a new IntMod from an array of bytes, big endian.
+    /// Does not enforce the integer value of `bytes` must be less than the modulus.
     fn from_be_bytes(bytes: &[u8]) -> Self;
 
     /// Creates a new IntMod from a u8.
+    /// Does not enforce the integer value of `bytes` must be less than the modulus.
     fn from_u8(val: u8) -> Self;
 
     /// Creates a new IntMod from a u32.
+    /// Does not enforce the integer value of `bytes` must be less than the modulus.
     fn from_u32(val: u32) -> Self;
 
     /// Creates a new IntMod from a u64.
+    /// Does not enforce the integer value of `bytes` must be less than the modulus.
     fn from_u64(val: u64) -> Self;
 
     /// Value of this IntMod as an array of bytes, little endian.
@@ -204,31 +210,17 @@ pub trait IntMod:
         ret
     }
 
-    /// zkVM specific concept: the in-memory values of `Self` will normally
-    /// be in their canonical unique form (e.g., less than modulus) but the
-    /// zkVM circuit does not constrain it. In cases where uniqueness is
-    /// essential for security, this function should be called to constrain
-    /// uniqueness.
+    /// VM specific concept: during guest execution, it is not enforced that the representation
+    /// of `Self` must be the unique integer less than the modulus. The guest code may sometimes
+    /// want to enforce that the representation is the canonical one less than the modulus.
+    /// the host to an honest host to provide the canonical representation less than the modulus.
     ///
-    /// Note that this is done automatically in [PartialEq] and [Eq] implementations.
-    ///
-    /// If `self` is not in its canonical form, the proof will fail to verify.
-    fn assert_unique(&self) {
-        // This must not be optimized out
-        let _ = core::hint::black_box(PartialEq::eq(self, self));
-    }
+    /// This function should enforce that guest execution proceeds **if and only if** `self`
+    /// is in the unique representation less than the modulus.
+    fn assert_reduced(&self);
 
-    /// This function is mostly for internal use in other internal implementations.
-    /// Normal users are not advised to use it.
-    ///
-    /// If `self` was directly constructed from a raw representation
-    /// and not in its canonical unique form (e.g., less than the modulus),
-    /// this function will "reduce" `self` to its canonical form and also
-    /// call `assert_unique`.
-    fn reduce(&mut self) {
-        self.add_assign(&Self::ZERO);
-        self.assert_unique();
-    }
+    /// Is the integer representation of `self` less than the modulus?
+    fn is_reduced(&self) -> bool;
 }
 
 // Ref: https://docs.rs/elliptic-curve/latest/elliptic_curve/ops/trait.Reduce.html

extensions/algebra/moduli-macros/src/lib.rs

@@ -106,6 +106,17 @@ pub fn moduli_declare(input: TokenStream) -> TokenStream {
         let module_name = format_ident!("algebra_impl_{}", mod_idx);
 
         let result = TokenStream::from(quote::quote_spanned! { span.into() =>
+            /// An element of the ring of integers modulo a positive integer.
+            /// The element is internally represented as a fixed size array of bytes.
+            ///
+            /// ## Caution
+            /// It is not guaranteed that the integer representation is less than the modulus.
+            /// After any arithmetic operation, the honest host should normalize the result
+            /// to its canonical representation less than the modulus, but guest execution does not
+            /// require it.
+            ///
+            /// See [`assert_reduced`](openvm_algebra_guest::IntMod::assert_reduced) and
+            /// [`is_reduced`](openvm_algebra_guest::IntMod::is_reduced).
             #[derive(Clone, Eq, serde::Serialize, serde::Deserialize)]
             #[repr(C, align(#block_size))]
             pub struct #struct_name(#[serde(with = "openvm_algebra_guest::BigArray")] [u8; #limbs]);
@@ -126,6 +137,8 @@ pub fn moduli_declare(input: TokenStream) -> TokenStream {
                     Self(bytes)
                 }
 
+                /// Constructor from little-endian bytes. Does not enforce the integer value of `bytes`
+                /// must be less than the modulus.
                 pub const fn from_const_bytes(bytes: [u8; #limbs]) -> Self {
                     Self(bytes)
                 }
@@ -430,6 +443,28 @@ pub fn moduli_declare(input: TokenStream) -> TokenStream {
                     fn cube(&self) -> Self {
                         &self.square() * self
                     }
+
+                    /// If `self` is not in its canonical form, the proof will fail to verify.
+                    /// This means guest execution will never terminate (either successfully or
+                    /// unsuccessfully) if `self` is not in its canonical form.
+                    // is_eq_mod enforces `self` is less than `modulus`
+                    fn assert_reduced(&self) {
+                        // This must not be optimized out
+                        let _ = core::hint::black_box(PartialEq::eq(self, self));
+                    }
+
+                    fn is_reduced(&self) -> bool {
+                        // limbs are little endian
+                        for (x_limb, p_limb) in self.0.iter().rev().zip(Self::MODULUS.iter().rev()) {
+                            if x_limb < p_limb {
+                                return true;
+                            } else if x_limb > p_limb {
+                                return false;
+                            }
+                        }
+                        // At this point, all limbs are equal
+                        false
+                    }
                 }
 
                 impl<'a> core::ops::AddAssign<&'a #struct_name> for #struct_name {

extensions/algebra/tests/programs/examples/moduli_setup.rs

@@ -36,4 +36,15 @@ pub fn main() {
         res += res.clone();
     }
     assert_eq!(res, Mersenne61::from_u32(1));
+
+    let mut non_reduced = Mersenne61::from_le_bytes(&[0xff; 32]);
+    assert!(!non_reduced.is_reduced());
+
+    non_reduced = Mersenne61::from_le_bytes(&Mersenne61::MODULUS);
+    assert!(!non_reduced.is_reduced());
+
+    let mut bytes = [0u8; 32];
+    bytes[7] = 1 << 5; // 2^61 = 2^{8*7 + 5} = modulus + 1
+    non_reduced = Mersenne61::from_le_bytes(&bytes);
+    assert!(!non_reduced.is_reduced());
 }

extensions/ecc/guest/src/ecdsa.rs

@@ -167,10 +167,12 @@ where
         // Note: Scalar internally stores using little endian
         let r = Scalar::<C>::from_be_bytes(r_be);
         let s = Scalar::<C>::from_be_bytes(s_be);
-        // The PartialEq implementation of Scalar: IntMod will constrain `r, s`
-        // are in the canonical unique form (i.e., less than the modulus).
-        assert_ne!(r, Scalar::<C>::ZERO);
-        assert_ne!(s, Scalar::<C>::ZERO);
+        if !r.is_reduced() || !s.is_reduced() {
+            return Err(Error::new());
+        }
+        if r == Scalar::<C>::ZERO || s == Scalar::<C>::ZERO {
+            return Err(Error::new());
+        }
 
         // Perf: don't use bits2field from ::ecdsa
         let z = Scalar::<C>::from_be_bytes(bits2field::<C>(prehash).unwrap().as_ref());
@@ -212,10 +214,12 @@ where
         // Note: Scalar internally stores using little endian
         let r = Scalar::<C>::from_be_bytes(r_be);
         let s = Scalar::<C>::from_be_bytes(s_be);
-        // The PartialEq implementation of Scalar: IntMod will constrain `r, s`
-        // are in the canonical unique form (i.e., less than the modulus).
-        assert_ne!(r, Scalar::<C>::ZERO);
-        assert_ne!(s, Scalar::<C>::ZERO);
+        if !r.is_reduced() || !s.is_reduced() {
+            return Err(Error::new());
+        }
+        if r == Scalar::<C>::ZERO || s == Scalar::<C>::ZERO {
+            return Err(Error::new());
+        }
 
         // Perf: don't use bits2field from ::ecdsa
         let z = <C as IntrinsicCurve>::Scalar::from_be_bytes(
@@ -233,6 +237,7 @@ where
             return Err(Error::new());
         }
         let (x_1, _) = R.into_coords();
+        // Scalar and Coordinate may be different byte lengths, so we use an inefficient reduction
         let x_mod_n = Scalar::<C>::reduce_le_bytes(x_1.as_le_bytes());
         if x_mod_n == r {
             Ok(())

extensions/ecc/sw-macros/src/lib.rs

@@ -347,7 +347,7 @@ pub fn sw_declare(input: TokenStream) -> TokenStream {
 
                         if hint.possible {
                             // ensure y < modulus
-                            hint.sqrt.assert_unique();
+                            hint.sqrt.assert_reduced();
 
                             if hint.sqrt.as_le_bytes()[0] & 1 != *rec_id & 1 {
                                 None
@@ -357,7 +357,7 @@ pub fn sw_declare(input: TokenStream) -> TokenStream {
                             }
                         } else {
                             // ensure sqrt < modulus
-                            hint.sqrt.assert_unique();
+                            hint.sqrt.assert_reduced();
 
                             let alpha = (x * x * x) + (x * &<#struct_name as ::openvm_ecc_guest::weierstrass::WeierstrassPoint>::CURVE_A) + &<#struct_name as ::openvm_ecc_guest::weierstrass::WeierstrassPoint>::CURVE_B;
                             if &hint.sqrt * &hint.sqrt == alpha * Self::get_non_qr() {
@@ -386,11 +386,11 @@ pub fn sw_declare(input: TokenStream) -> TokenStream {
                                 non_qr = non_qr_uninit.assume_init();
                             }
                             // ensure non_qr < modulus
-                            non_qr.assert_unique();
+                            non_qr.assert_reduced();
 
                             // construct exp = (p-1)/2 as an integer by first constraining exp = (p-1)/2 (mod p) and then exp < p
                             let exp = -<#intmod_type as openvm_algebra_guest::IntMod>::ONE.div_unsafe(#intmod_type::from_const_u8(2));
-                            exp.assert_unique();
+                            exp.assert_reduced();
 
                             if non_qr.exp_bytes(true, &exp.to_be_bytes()) != -<#intmod_type as openvm_algebra_guest::IntMod>::ONE
                             {

extensions/ecc/tests/programs/examples/decompress_invalid_hint.rs

@@ -107,7 +107,7 @@ impl Secp256k1PointWrapper {
 
         if hint.possible {
             // ensure y < modulus
-            hint.sqrt.assert_unique();
+            hint.sqrt.assert_reduced();
 
             if hint.sqrt.as_le_bytes()[0] & 1 != *rec_id & 1 {
                 None
@@ -117,7 +117,7 @@ impl Secp256k1PointWrapper {
             }
         } else {
             // ensure sqrt < modulus
-            hint.sqrt.assert_unique();
+            hint.sqrt.assert_reduced();
 
             let alpha = (x * x * x) + (x * &Secp256k1Point::CURVE_A) + &Secp256k1Point::CURVE_B;
             if &hint.sqrt * &hint.sqrt == alpha * Secp256k1Point::get_non_qr() {
@@ -184,7 +184,7 @@ impl MyCurvePointWrapper {
 
         if hint.possible {
             // ensure proof fails if y >= modulus
-            hint.sqrt.assert_unique();
+            hint.sqrt.assert_reduced();
 
             if hint.sqrt.as_le_bytes()[0] & 1 != *rec_id & 1 {
                 None
@@ -194,7 +194,7 @@ impl MyCurvePointWrapper {
             }
         } else {
             // ensure proof fails if sqrt * sqrt != alpha * non_qr
-            hint.sqrt.assert_unique();
+            hint.sqrt.assert_reduced();
 
             let alpha = (x * x * x) + (x * &MyCurvePoint::CURVE_A) + &MyCurvePoint::CURVE_B;
             if &hint.sqrt * &hint.sqrt == alpha * MyCurvePoint::get_non_qr() {

extensions/ecc/tests/programs/examples/ecdsa.rs

@@ -75,6 +75,27 @@ pub fn main() {
 
     // Test verification
     recovered_key
+        .clone()
         .verify_prehashed(&prehash, &signature)
         .unwrap();
+
+    // Test bad signature
+    let mut bad_sig1 = signature;
+    bad_sig1[..32].copy_from_slice(&[0u8; 32]);
+    let mut bad_sig2 = signature;
+    bad_sig2[32..].copy_from_slice(&[0u8; 32]);
+    let mut bad_sig3 = signature;
+    bad_sig3[..32].copy_from_slice(&[0xff; 32]);
+    let mut bad_sig4 = signature;
+    bad_sig4[32..].copy_from_slice(&[0xff; 32]);
+    for bad_sig in [bad_sig1, bad_sig2, bad_sig3, bad_sig4] {
+        assert!(VerifyingKey::<Secp256k1>::recover_from_prehash_noverify(
+            &prehash, &bad_sig, recid
+        )
+        .is_err());
+        assert!(recovered_key
+            .clone()
+            .verify_prehashed(&prehash, &bad_sig)
+            .is_err());
+    }
 }