Organization
- @OpenVM
Engagement Type
Cantina Reviews
Period
-
Repositories
Researchers
Findings
Medium Risk
5 findings
5 fixed
0 acknowledged
Medium Risk5 findings
recover_from_prehash succeeds when it should fail due to missing overflow check
State
- Fixed
PR #1742
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Guido Vranken
Description
The upstream code performs a
checked_add
and returns Error if an overflow occurs:Option::<C::Uint>::from( C::Uint::decode_field_bytes(&r.to_repr()).checked_add(&C::ORDER), ) .ok_or_else(Error::new)? .encode_field_bytes()
But openvm does
add_assign
unconditionally.Proof of Concept
use hex_literal::hex; use ecdsa::RecoveryId;use p256::ecdsa::{VerifyingKey, Signature}; /// Signature recovery test vectorsstruct RecoveryTestVector { pk: [u8; 33], msg: [u8; 32], sig: [u8; 64], recid: u8, ok: bool,} const RECOVERY_TEST_VECTORS: &[RecoveryTestVector] = &[ RecoveryTestVector{pk:hex!("020000000000000000000000000000000000000000000000000000000000000000"),msg:hex!("00000000000000000000FFFFFFFF03030BFFFFFFFFFF030BFFFFFFFFFFFFF8FC"),sig:hex!("ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2606325510000000000000000000000000000000000ffffffffffff2728d948e8857fffff"),recid:0,ok:false},]; fn main() { for vector in RECOVERY_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) => { /* openvm p256 takes this code path */ _v }, Err(_) => { /* Original p256 crate takes this code path */ assert_eq!(vector.ok, false); continue; } }; }}
Recommendation
Implement the upstream logic to return error when the addition overflows for full compatibility.
recover_from_prehash succeeds when it should fail due to missing public key validation
State
- Fixed
PR #1743
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Guido Vranken
Description
The upstream code returns error if the recovered public key is invalid:
https://github.com/RustCrypto/signatures/blob/ecdsa/v0.16.9/ecdsa/src/recovery.rs#L310
let vk = Self::from_affine(pk.into())?;
But openvm returns the public key unconditionally.
Proof of Concept
use hex_literal::hex; use ecdsa::RecoveryId;use p256::ecdsa::{VerifyingKey, Signature}; /// Signature recovery test vectorsstruct RecoveryTestVector { pk: [u8; 33], msg: [u8; 32], sig: [u8; 64], recid: u8, ok: bool,} const RECOVERY_TEST_VECTORS: &[RecoveryTestVector] = &[ RecoveryTestVector{pk:hex!("020000000000000000000000000000000000000000000000000000000000000000"),msg:hex!("000000000000000000000000000000000000000000000000000CFD5E267CBB5E"),sig:hex!("6b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c296000000000000000000000000000000000000000000000000000cfd5e267cbb5e"),recid:1,ok:false},]; fn main() { for vector in RECOVERY_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) => { /* openvm p256 takes this code path */ _v }, Err(_) => { /* Original p256 crate takes this code path */ assert_eq!(vector.ok, false); continue; } }; }}
Recommendation
Return error when the recovered public key is invalid for full compatibility.
recover_from_prehash succeeds when it should fail due to missing ECDSA verification
State
- Fixed
PR #1744
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Guido Vranken
Description
The upstream code returns error if the recovered public key and signature combination does not pass ECDSA verification.
// Ensure signature verifies with the recovered key vk.verify_prehash(prehash, signature)?;
Proof of Concept
use hex_literal::hex; use ecdsa::RecoveryId;use k256::ecdsa::{VerifyingKey, Signature}; /// Signature recovery test vectorsstruct RecoveryTestVector { pk: [u8; 33], msg: [u8; 32], sig: [u8; 64], recid: u8, ok: bool,} const RECOVERY_TEST_VECTORS: &[RecoveryTestVector] = &[ RecoveryTestVector{pk:hex!("020000000000000000000000000000000000000000000000000000000000000000"),msg:hex!("0000000000000000000000000000000000000000000000000000000000000000"),sig:hex!("0000000000000000000000000000000000000000000000000000000000000001ffffffffbffffffffffffffffeffbaffaeff6f7000000100000000dbd0364140"),recid:1,ok:false},]; fn main() { for vector in RECOVERY_TEST_VECTORS { let mut sig = match Signature::try_from(vector.sig.as_slice()) { Ok(_v) => _v, Err(_) => { assert_eq!(vector.ok, false); continue; } }; let mut recid = vector.recid; /* Change false to true to make recover_from_prehash in k256 crate */ if false { if let Some(sig_normalized) = sig.normalize_s() { sig = sig_normalized; recid ^= 1; } } let recid = match RecoveryId::from_byte(recid) { Some(_v) => _v, None => { assert_eq!(vector.ok, false); continue; } }; let _ = match VerifyingKey::recover_from_prehash(&vector.msg, &sig, recid) { Ok(_v) => { println!("recover_from_prehash: ok"); /* openvm k256 takes this code path */ _v }, Err(_) => { println!("recover_from_prehash: err"); /* k256 crate takes this code path */ assert_eq!(vector.ok, false); continue; } }; }}
Recommendation
Return error when the recovered public key and signature combination does not pass ECDSA verification for full compatibility.
VerifyingKey::from_sec1_bytes panics when parsing unreduced coordinates
State
- Fixed
PR #1746
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Guido Vranken
Description
If
VerifyingKey::from_sec1_bytes
is called when a point representation whose coordinate(s) are equal or greater than the curve prime, a panic (assert failure) will occur due to field arithmetic assuming reduced input: https://cantina.xyz/code/87bec2b6-70e6-4b0b-bc4e-03235103de32/extensions/algebra/circuit/src/modular_chip/is_eq.rs?lines=349,349which for the examples below is invoked from https://cantina.xyz/code/87bec2b6-70e6-4b0b-bc4e-03235103de32/extensions/ecc/guest/src/weierstrass.rs?lines=94,94
The proof of concept demonstrates this for
k256
but it applies top256
as well.Proof of Concept
use hex_literal::hex;use k256::ecdsa::VerifyingKey as VkSecp256k1; fn main() { { /* Secp256k1 */ /* Fails because point is invalid and coordinate (Y) exceeds prime * * This panics in openvm: * thread 'guest_tests::test_pubkey' panicked at /home/jhg/openvm-june/v1.2.1-rc.0/openvm/extensions/algebra/circuit/src/modular_chip/is_eq.rs:349:13: * [255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 255, 255, 255, 255] >= [255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 255, 255, 255, 255] */ assert!( VkSecp256k1::from_sec1_bytes( &hex!("04" "0000000000000000000000000000000000000000000000000000000000000000" "fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f") ).is_err() ); /* Succeeds because it is a valid point * openvm behavior is equivalent (e.g. no bug) */ assert!( VkSecp256k1::from_sec1_bytes( &hex!("04" "0000000000000000000000000000000000000000000000000000000000000001" "4218f20ae6c646b363db68605822fb14264ca8d2587fdd6fbc750d587e76a7ee") ).is_ok() ); /* Same point as above but with prime added to X. * Fails because from_sec1_bytes rejects points with oversized coordinates, * even if the point is valid after reduction. * * This also panics in openvm in is_eq.rs. */ assert!( VkSecp256k1::from_sec1_bytes( &hex!("04" "fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc30" "4218f20ae6c646b363db68605822fb14264ca8d2587fdd6fbc750d587e76a7ee") ).is_err() ); }}
Recommendation
Reject coordinates >= prime in
from_sec1_bytes
by returning error, and do this before invoking any arithmetic that assumes reduced inputs.VerifyingKey::from_sec1_bytes accepts infinity point whereas upstream does not
State
- Fixed
PR #1747
Severity
- Severity: Medium
≈
Likelihood: Medium×
Impact: Medium Submitted by
Guido Vranken
Description
The infinity point is not a valid VerifyingKey. Upstream correctly rejects it whereas openvm does not.
Proof of Concept
use hex_literal::hex;use k256::ecdsa::VerifyingKey; fn main() { assert_eq!( VerifyingKey::from_sec1_bytes( &hex!("04" "0000000000000000000000000000000000000000000000000000000000000000" "0000000000000000000000000000000000000000000000000000000000000000") ).is_ok(), false);}
Recommendation
Reject the infinity point as a VerifyingKey.