fix(ecdsa): match upstream behavior to error on overflowing add (#1742)

In the case `Is_x_reduced()` is true, the addition of `C::ORDER` to `r`
is done in `Uint` and the upstream `ecdsa` crate errors if the addition
overflow `Uint`'s fixed size. To match upstream behavior, we switch back
to using the same `Uint` logic.

Also took some additional care in handling conversions when the
`Scalar<C>` num bytes differs from `Coordinate<C>` num bytes.

Added a synthetic test case that failed before and now passes.

Author: jonathanpwang

Cargo.lock

@@ -1,5 +1,5 @@
 use alloc::vec::Vec;
-use core::ops::{Add, AddAssign, Mul};
+use core::ops::{Add, Mul};
 
 use ecdsa_core::{
     self,
@@ -12,10 +12,11 @@ use ecdsa_core::{
     EncodedPoint, Error, RecoveryId, Result, Signature, SignatureSize,
 };
 use elliptic_curve::{
-    generic_array::ArrayLength,
+    bigint::CheckedAdd,
+    generic_array::{typenum::Unsigned, ArrayLength},
     sec1::{FromEncodedPoint, ModulusSize, Tag, ToEncodedPoint},
     subtle::{Choice, ConditionallySelectable, CtOption},
-    CurveArithmetic, FieldBytesSize, PrimeCurve,
+    CurveArithmetic, FieldBytes, FieldBytesEncoding, FieldBytesSize, PrimeCurve,
 };
 use openvm_algebra_guest::{DivUnsafe, IntMod, Reduce};
 
@@ -378,12 +379,14 @@ where
     Coordinate<C>: IntMod,
     C::Scalar: IntMod + Reduce,
 {
+    /// ## Assumption
+    /// To use this implementation, the `Signature<C>`, `Coordinate<C>`, and `FieldBytes<C>` should
+    /// all be encoded in big endian bytes. The implementation also assumes that
+    /// `Scalar::<C>::NUM_LIMBS <= FieldBytesSize::<C>::USIZE <= Coordinate::<C>::NUM_LIMBS`.
+    ///
     /// Ref: <https://github.com/RustCrypto/signatures/blob/85c984bcc9927c2ce70c7e15cbfe9c6936dd3521/ecdsa/src/recovery.rs#L297>
     ///
     /// Recovery does not require additional signature verification: <https://github.com/RustCrypto/signatures/pull/831>
-    ///
-    /// ## Panics
-    /// If the signature is invalid or public key cannot be recovered from the given input.
     #[allow(non_snake_case)]
     pub fn recover_from_prehash_noverify(
         prehash: &[u8],
@@ -411,16 +414,37 @@ where
         }
 
         // Perf: don't use bits2field from ::ecdsa
-        let z = Scalar::<C>::from_be_bytes(bits2field::<C>(prehash).unwrap().as_ref());
+        let prehash_bytes = bits2field::<C>(prehash)?;
+        // If prehash is longer than Scalar::NUM_LIMBS, take leftmost bytes
+        let trim = prehash_bytes.len().saturating_sub(Scalar::<C>::NUM_LIMBS);
+        // from_be_bytes still works if len < Scalar::NUM_LIMBS
+        // we don't need to reduce because IntMod is up to modular equivalence
+        let z = Scalar::<C>::from_be_bytes(&prehash_bytes[..prehash_bytes.len() - trim]);
 
         // `r` is in the Scalar field, we now possibly add C::ORDER to it to get `x`
         // in the Coordinate field.
-        let mut x = Coordinate::<C>::from_le_bytes(r.as_le_bytes());
+        // We take some extra care for the case when FieldBytesSize<C> may be larger than
+        // Scalar::<C>::NUM_LIMBS.
+        let mut r_bytes = {
+            let mut r_bytes = FieldBytes::<C>::default();
+            assert!(FieldBytesSize::<C>::USIZE >= Scalar::<C>::NUM_LIMBS);
+            let offset = r_bytes.len().saturating_sub(r_be.len());
+            r_bytes[offset..].copy_from_slice(r_be);
+            r_bytes
+        };
         if recovery_id.is_x_reduced() {
-            // Copy from slice in case Coordinate has more bytes than Scalar
-            let order = Coordinate::<C>::from_le_bytes(Scalar::<C>::MODULUS.as_ref());
-            x.add_assign(order);
+            match Option::<C::Uint>::from(
+                C::Uint::decode_field_bytes(&r_bytes).checked_add(&C::ORDER),
+            ) {
+                Some(restored) => r_bytes = restored.encode_field_bytes(),
+                // No reduction should happen here if r was reduced
+                None => {
+                    return Err(Error::new());
+                }
+            };
         }
+        assert!(FieldBytesSize::<C>::USIZE <= Coordinate::<C>::NUM_LIMBS);
+        let x = Coordinate::<C>::from_be_bytes(&r_bytes);
         let rec_id = recovery_id.to_byte();
         // The point R decompressed from x-coordinate `r`
         let R: C::Point = FromCompressed::decompress(x, &rec_id).ok_or_else(Error::new)?;
@@ -463,8 +487,12 @@ where
     }
 
     // Perf: don't use bits2field from ::ecdsa
-    let z =
-        <C as IntrinsicCurve>::Scalar::from_be_bytes(bits2field::<C>(prehash).unwrap().as_ref());
+    let prehash_bytes = bits2field::<C>(prehash)?;
+    // If prehash is longer than Scalar::NUM_LIMBS, take leftmost bytes
+    let trim = prehash_bytes.len().saturating_sub(Scalar::<C>::NUM_LIMBS);
+    // from_be_bytes still works if len < Scalar::NUM_LIMBS
+    // we don't need to reduce because IntMod is up to modular equivalence
+    let z = Scalar::<C>::from_be_bytes(&prehash_bytes[..prehash_bytes.len() - trim]);
 
     let u1 = z.div_unsafe(&s);
     let u2 = (&r).div_unsafe(&s);

extensions/ecc/guest/src/ecdsa.rs

@@ -11,14 +11,15 @@ repository.workspace = true
 openvm-stark-sdk.workspace = true
 openvm-circuit = { workspace = true, features = ["test-utils"] }
 openvm-transpiler.workspace = true
-openvm-algebra-circuit.workspace = true
 openvm-algebra-transpiler.workspace = true
 openvm-ecc-transpiler.workspace = true
 openvm-ecc-circuit.workspace = true
 openvm-rv32im-transpiler.workspace = true
-openvm-keccak256-transpiler.workspace = true
 openvm-toolchain-tests = { path = "../../../crates/toolchain/tests" }
 openvm-sdk.workspace = true
+serde.workspace = true
+serde_with.workspace = true
+toml.workspace = true
 eyre.workspace = true
 hex-literal.workspace = true
 num-bigint.workspace = true

extensions/ecc/tests/Cargo.toml

@@ -27,8 +27,13 @@ serde = { version = "1.0", default-features = false, features = [
     "alloc",
     "derive",
 ] }
+serde_with = { version = "3.13.0", default-features = false, features = [
+    "alloc",
+    "macros",
+] }
 hex = { version = "0.4.3", default-features = false, features = ["alloc"] }
 hex-literal = { version = "0.4.1", default-features = false }
+ecdsa-core = { version = "0.16.9", package = "ecdsa", default-features = false }
 
 [target.'cfg(not(target_os = "zkvm"))'.dependencies]
 num-bigint = "0.4.6"
@@ -64,6 +69,10 @@ required-features = ["k256"]
 name = "ecdsa"
 required-features = ["k256"]
 
+[[example]]
+name = "ecdsa_recover"
+required-features = ["p256"]
+
 [[example]]
 name = "invalid_setup"
 required-features = ["k256", "p256"]

extensions/ecc/tests/programs/Cargo.toml

@@ -0,0 +1,65 @@
+#![cfg_attr(not(feature = "std"), no_main)]
+#![cfg_attr(not(feature = "std"), no_std)]
+
+extern crate alloc;
+
+use alloc::vec::Vec;
+
+use ecdsa_core::RecoveryId;
+use openvm::io::read;
+#[allow(unused_imports)]
+use openvm_p256::{
+    ecdsa::{Signature, VerifyingKey},
+    P256Coord, P256Point,
+};
+use serde::{Deserialize, Serialize};
+use serde_with::{serde_as, Bytes};
+
+openvm::entry!(main);
+
+openvm::init!("openvm_init_ecdsa_recover_p256.rs");
+
+/// Signature recovery test vectors
+#[repr(C)]
+#[serde_as]
+#[derive(Serialize, Deserialize)]
+struct RecoveryTestVector {
+    #[serde_as(as = "Bytes")]
+    pk: [u8; 33],
+    #[serde_as(as = "Bytes")]
+    msg: [u8; 32],
+    #[serde_as(as = "Bytes")]
+    sig: [u8; 64],
+    recid: u8,
+    ok: bool,
+}
+
+pub fn main() {
+    let test_vectors: Vec<RecoveryTestVector> = read();
+    for vector in test_vectors {
+        let sig = match Signature::try_from(vector.sig.as_slice()) {
+            Ok(_v) => _v,
+            Err(_) => {
+                assert_eq!(vector.ok, false);
+                continue;
+            }
+        };
+        let recid = match RecoveryId::from_byte(vector.recid) {
+            Some(_v) => _v,
+            None => {
+                assert_eq!(vector.ok, false);
+                continue;
+            }
+        };
+        let _ = match VerifyingKey::recover_from_prehash(&vector.msg, &sig, recid) {
+            Ok(_v) => _v,
+            Err(_) => {
+                openvm::io::println("Recovery failed");
+                assert_eq!(vector.ok, false);
+                continue;
+            }
+        };
+        // If reached here, recovery succeeded
+        assert_eq!(vector.ok, true);
+    }
+}

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

@@ -0,0 +1,3 @@
+// This file is automatically generated by cargo openvm. Do not rename or edit.
+openvm_algebra_guest::moduli_macros::moduli_init! { "115792089210356248762697446949407573530086143415290314195533631308867097853951", "115792089210356248762697446949407573529996955224135760342422259061068512044369" }
+openvm_ecc_guest::sw_macros::sw_init! { P256Point }

extensions/ecc/tests/programs/openvm_init_ecdsa_recover_p256.rs

@@ -0,0 +1,17 @@
+[app_vm_config.rv32i]
+[app_vm_config.rv32m]
+[app_vm_config.io]
+[app_vm_config.keccak]
+
+[app_vm_config.modular]
+supported_moduli = [
+    "115792089237316195423570985008687907853269984665640564039457584007908834671663",
+    "115792089237316195423570985008687907852837564279074904382605163141518161494337",
+]
+
+[[app_vm_config.ecc.supported_curves]]
+struct_name = "Secp256k1Point"
+modulus = "115792089237316195423570985008687907853269984665640564039457584007908834671663"
+scalar = "115792089237316195423570985008687907852837564279074904382605163141518161494337"
+a = "0"
+b = "7"

extensions/ecc/tests/programs/openvm_k256_keccak.toml

@@ -0,0 +1,15 @@
+[app_vm_config.rv32i]
+[app_vm_config.rv32m]
+[app_vm_config.io]
+[app_vm_config.modular]
+supported_moduli = [
+    "115792089210356248762697446949407573530086143415290314195533631308867097853951",
+    "115792089210356248762697446949407573529996955224135760342422259061068512044369",
+]
+
+[[app_vm_config.ecc.supported_curves]]
+struct_name = "P256Point"
+modulus = "115792089210356248762697446949407573530086143415290314195533631308867097853951"
+scalar = "115792089210356248762697446949407573529996955224135760342422259061068512044369"
+a = "115792089210356248762697446949407573530086143415290314195533631308867097853948"
+b = "41058363725152142129326129780047268409114441015993725554835256314039467401291"

extensions/ecc/tests/programs/openvm_p256.toml

@@ -1,31 +1,34 @@
+mod test_vectors;
+
 #[cfg(test)]
 mod tests {
     use core::str::FromStr;
 
     use eyre::Result;
     use hex_literal::hex;
     use num_bigint::BigUint;
-    use openvm_algebra_circuit::ModularExtension;
     use openvm_algebra_transpiler::ModularTranspilerExtension;
     use openvm_circuit::{
-        arch::{instructions::exe::VmExe, SystemConfig},
+        arch::instructions::exe::VmExe,
         utils::{air_test, air_test_with_min_segments},
     };
-    use openvm_ecc_circuit::{
-        CurveConfig, Rv32WeierstrassConfig, WeierstrassExtension, P256_CONFIG, SECP256K1_CONFIG,
-    };
+    use openvm_ecc_circuit::{CurveConfig, Rv32WeierstrassConfig, P256_CONFIG, SECP256K1_CONFIG};
     use openvm_ecc_transpiler::EccTranspilerExtension;
-    use openvm_keccak256_transpiler::Keccak256TranspilerExtension;
     use openvm_rv32im_transpiler::{
         Rv32ITranspilerExtension, Rv32IoTranspilerExtension, Rv32MTranspilerExtension,
     };
-    use openvm_sdk::config::SdkVmConfig;
+    use openvm_sdk::{
+        config::{AppConfig, SdkVmConfig},
+        StdIn,
+    };
     use openvm_stark_backend::p3_field::FieldAlgebra;
     use openvm_stark_sdk::{openvm_stark_backend, p3_baby_bear::BabyBear};
     use openvm_toolchain_tests::{
         build_example_program_at_path_with_features, get_programs_dir, NoInitFile,
     };
     use openvm_transpiler::{transpiler::Transpiler, FromElf};
+
+    use crate::test_vectors::P256_RECOVERY_TEST_VECTORS;
     type F = BabyBear;
 
     #[test]
@@ -163,39 +166,39 @@ mod tests {
 
     #[test]
     fn test_ecdsa() -> Result<()> {
-        let config = SdkVmConfig::builder()
-            .system(SystemConfig::default().with_continuations().into())
-            .rv32i(Default::default())
-            .rv32m(Default::default())
-            .io(Default::default())
-            .modular(ModularExtension::new(vec![
-                SECP256K1_CONFIG.modulus.clone(),
-                SECP256K1_CONFIG.scalar.clone(),
-            ]))
-            .keccak(Default::default())
-            .ecc(WeierstrassExtension::new(vec![SECP256K1_CONFIG.clone()]))
-            .build();
-
+        let config = toml::from_str::<AppConfig<SdkVmConfig>>(include_str!(
+            "../programs/openvm_k256_keccak.toml"
+        ))?
+        .app_vm_config;
         let elf = build_example_program_at_path_with_features(
             get_programs_dir!(),
             "ecdsa",
             ["k256"],
             &config,
         )?;
-        let openvm_exe = VmExe::from_elf(
-            elf,
-            Transpiler::<F>::default()
-                .with_extension(Rv32ITranspilerExtension)
-                .with_extension(Rv32MTranspilerExtension)
-                .with_extension(Rv32IoTranspilerExtension)
-                .with_extension(Keccak256TranspilerExtension)
-                .with_extension(EccTranspilerExtension)
-                .with_extension(ModularTranspilerExtension),
-        )?;
+        let openvm_exe = VmExe::from_elf(elf, config.transpiler())?;
         air_test(config, openvm_exe);
         Ok(())
     }
 
+    #[test]
+    fn test_p256_ecdsa_recover() -> Result<()> {
+        let config =
+            toml::from_str::<AppConfig<SdkVmConfig>>(include_str!("../programs/openvm_p256.toml"))?
+                .app_vm_config;
+        let elf = build_example_program_at_path_with_features(
+            get_programs_dir!(),
+            "ecdsa_recover",
+            ["p256"],
+            &config,
+        )?;
+        let openvm_exe = VmExe::from_elf(elf, config.transpiler())?;
+        let mut input = StdIn::default();
+        input.write(&P256_RECOVERY_TEST_VECTORS.to_vec());
+        air_test_with_min_segments(config, openvm_exe, input, 1);
+        Ok(())
+    }
+
     #[test]
     #[should_panic]
     fn test_invalid_setup() {

extensions/ecc/tests/src/lib.rs

@@ -0,0 +1,27 @@
+use hex_literal::hex;
+use serde::{Deserialize, Serialize};
+use serde_with::{serde_as, Bytes};
+
+#[repr(C)]
+#[serde_as]
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct RecoveryTestVector {
+    #[serde_as(as = "Bytes")]
+    pub pk: [u8; 33],
+    #[serde_as(as = "Bytes")]
+    pub msg: [u8; 32],
+    #[serde_as(as = "Bytes")]
+    pub sig: [u8; 64],
+    pub recid: u8,
+    pub ok: bool,
+}
+
+#[allow(dead_code)]
+pub const P256_RECOVERY_TEST_VECTORS: &[RecoveryTestVector] = &[RecoveryTestVector {
+    pk: hex!("020000000000000000000000000000000000000000000000000000000000000000"),
+    msg: hex!("00000000000000000000FFFFFFFF03030BFFFFFFFFFF030BFFFFFFFFFFFFF8FC"),
+    sig: hex!("00000000ffffffff00000000000000004319055258e8617b0c46353d039cdaaf0000000000000000000000000000000000000000000000000000000000000001"
+    ),
+    recid: 2,
+    ok: false,
+}];