OpenVM

OpenVM v1.2.1-rc.0

Cantina Security Report

Organization

@OpenVM

Engagement Type

Cantina Reviews

Period

-

Repositories

GitHubopenvm-org/openvm

Researchers

Guido Vranken

Om Parikh

zigtur

Findings

Medium Risk

5 findings

5 fixed

0 acknowledged

Medium Risk5 findings

  1. 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:

    https://github.com/RustCrypto/signatures/blob/85c984bcc9927c2ce70c7e15cbfe9c6936dd3521/ecdsa/src/recovery.rs#L306-L310

    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.

  2. 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.

  3. 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.

    https://github.com/RustCrypto/signatures/blob/89232d6a962a199fd8211a117db74408353e4383/ecdsa/src/recovery.rs#L312-L313

    // 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.

  4. 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,349

    which 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 to p256 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.

  5. 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.