LFDT-Lockness / generic-ec

Generic elliptic curve cryptography in Rust
Apache License 2.0
2 stars 2 forks source link

Different serialization of SecretScalar depending on default-features, leads to deserialization error #8

Closed OrestisAlpos closed 12 months ago

OrestisAlpos commented 1 year ago

Let the following code in a fresh repo:

let mut rng = rand_dev::DevRng::new();
    let key_share_plaintext1 = dfns_key_import_common::KeySharePlaintext {
        secret_share: generic_ec::SecretScalar::<dfns_key_import_common::Secp256k1>::random(
            &mut rng,
        ),
        version: dfns_key_import_common::utils::VersionGuard,
        public_shares: Vec::new(),
    };
    let key_share_plaintext1 = serde_json::to_string(&key_share_plaintext1).unwrap();
    println!("{:?}", &key_share_plaintext1);

We can import generic-ec in two ways. A: generic-ec = { git = "https://github.com/dfns-labs/generic-ec", branch = "m", features = ["serde", "curve-secp256k1", "wasm"] } B: generic-ec = { git = "https://github.com/dfns-labs/generic-ec", branch = "m", default-features = false, features = ["serde", "curve-secp256k1", "wasm"] }

The output is different in each case. A: "{\"version\":1,\"secret_share\":{\"curve\":\"secp256k1\",\"scalar\":\"a975cfe60a7649f55abcc2461f4082a81100d9509f0b9e6ee1b90933330bc003\"},\"public_shares\":[]}" B: "{\"version\":1,\"secret_share\":{\"curve\":\"secp256k1\",\"scalar\":[129,246,35,161,33,118,114,128,230,85,106,60,213,139,216,47,171,195,164,195,136,54,156,175,82,247,162,86,106,168,244,105]},\"public_shares\":[]}" The difference is in the serialization of secret-share.

The problem comes if we add the following code, which simply deserializes the serialized data:

    let _: dfns_key_import_common::KeySharePlaintext<dfns_key_import_common::Secp256k1> =
        serde_json::from_str(&key_share_plaintext1).unwrap();

then import option B gives:

thread 'test' panicked at 'called `Result::unwrap()` on an `Err` value: Error("invalid type: sequence, expected bytes", line: 1, column: 58)'
survived commented 12 months ago

It doesn't lead to deserialization panic, it leads to deserialization error. Panic is caused by .unwrap() you wrote that should never be used in prod code. I'll investigate the error

OrestisAlpos commented 12 months ago

Yes, the code is taken from a test actually. But you are right, it leads to an error.