Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

accept "0" in Secp256k1Scalar::from #71

Closed
wants to merge 1 commit into from
Closed

Conversation

oleiba
Copy link
Contributor

@oleiba oleiba commented Nov 13, 2019

"0" is a valid input.
Previously, ECScalar::from(&BigInt::from("0")) has panicked.
This PR returns ECScalar::zero() in such case.

Motivation for this change:
In https://github.com/KZen-networks/centipede the struct Witness is defined:

pub struct Witness {
    pub x_vec: Vec<FE>,
    pub r_vec: Vec<FE>,
}

where x_vec is a vector of octets, often containing '0'. Therefore, the deserialization of the serialization of such instance of the struct, panics.

@omershlo
Copy link
Contributor

Looks good.

  1. Please do the same change for other curves as well
  2. this pr raised my concerns again for not being constant time. Concretely there's not much to do for this pr but I will open an issue on this topic

@0xmountaintop
Copy link

0xmountaintop commented Jan 11, 2020

I think putting this check at the end of the function can avoid not being constant time?
i.e.,

fn from(n: &BigInt) -> Secp256k1Scalar {
    let curve_order = FE::q();
    let n_reduced = BigInt::mod_add(n, &BigInt::from(0), &curve_order);
    let mut v = BigInt::to_vec(&n_reduced);

    if v.len() < SECRET_KEY_SIZE {
        let mut template = vec![0; SECRET_KEY_SIZE - v.len()];
        template.extend_from_slice(&v);
        v = template;
    }

    if n.eq(&BigInt::zero()) {
        Self::zero()
    } else {
        Secp256k1Scalar {
            purpose: "from_big_int",
            fe: SK::from_slice(&v).unwrap(),
        }
    }
}

The above code passes cargo build --features ec_secp256k1

@0xmountaintop
Copy link

  1. Please do the same change for other curves as well

Other relevant codes:

  • fn from(n: &BigInt) -> JubjubScalar in src/elliptic/curves/curve_jubjub.rs
  • fn from(n: &BigInt) -> RistrettoScalar in src/elliptic/curves/curve_ristretto.rs
  • fn from(n: &BigInt) -> Ed25519Scalar in src/elliptic/curves/ed25519.rs

@elichai
Copy link
Contributor

elichai commented Sep 10, 2021

This is no longer a problem since #120

@elichai elichai closed this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants