-
Notifications
You must be signed in to change notification settings - Fork 23
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
move verify
method to tests
#112
Comments
skaunov
changed the title
improve secret values handling in
move Jun 15, 2024
verify
verify
method to tests
skaunov
added a commit
that referenced
this issue
Jul 7, 2024
# Subtle I was really late to understand that Subtle crypto supports the different curve `secp256r`, *and* it doesn't provide a facility to store secret values. So implementation for `web_sys::SecretKey` turned out to be just extra miles leading nowhere. ```toml web-sys = { version = "0.3", features = ["CryptoKey", "SubtleCrypto", "Crypto", "EcKeyImportParams"] } wasm-bindgen-futures = "0.4" ``` ```rust #[wasm_bindgen] extern "C" { // Return type of js_sys::global() type Global; // // Web Crypto API: Crypto interface (https://www.w3.org/TR/WebCryptoAPI/) // type WebCrypto; // Getters for the WebCrypto API #[wasm_bindgen(method, getter)] fn crypto(this: &Global) -> web_sys::Crypto; } // `fn sign` if sk.type_() != "secret" {return Err(JsError::new("`sk` must be secret key"))} if !js_sys::Object::values(&sk.algorithm().map_err( |er| JsError::new(er.as_string().expect("TODO check this failing").as_str()) )?).includes(&JsValue::from_str("P-256"), 0) {return Err(JsError::new("`sk` must be from `secp256`"))} // this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs> // js_sys::global().entries().find(); // TODO throw if no Crypto in global let global_the: Global = js_sys::global().unchecked_into(); let crypto_the: web_sys::Crypto = global_the.crypto(); let subtle_the = crypto_the.subtle(); let sk = JsFuture::from(subtle_the.export_key("pkcs8", &sk)?).await?; // ... ::from_pkcs8_der(js_sys::ArrayBuffer::from(sk).try_into()?)?; zeroize::Zeroizing::new(js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &sk).map_err( |er| Err(JsError::new(er.as_string().expect("TODO check this failing").as_str())) )?).await?).to_vec()); // ... // `fn try_into` // ... // zeroization protection ommitted here due to deprecation // <#112> // mostly boilerplate from signing; also some excessive ops left for the same reason // TODO align error-handling in this part if self.c.type_() != "secret" {return Err(JsError::new("`c` must be secret key"))} if !js_sys::Object::values(&self.c.algorithm()?).includes(js_sys::JsString::from("P-256").into(), 0) {return Err(JsError::new("`c` must be from `secp256`"))} this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs> js_sys::global().entries().find(); // TODO throw if no Crypto in global let global_the: Global = js_sys::global().unchecked_into(); let crypto_the: web_sys::Crypto = global_the.crypto(); let subtle_the = crypto_the.subtle(); let c_pkcs = //zeroize::Zeroizing::new( js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &self.c)?).await?).to_vec(); // ); let c_scalar = &plume_rustcrypto::SecretKey::from_pkcs8_der(&c_pkcs)?.to_nonzero_scalar(); sk_z.zeroize(); // ... ``` # randomness Somehow I thought Wasm doesn't have access to RNG, so I used a seedable one and required the seed. Here's how `sign` `fn` was different. ```rust // Wasm environment doesn't have a suitable way to get randomness for the signing process, so this instantiates ChaCha20 RNG with the provided seed. // @throws a "crypto error" in case of a problem with the secret key, and a verbal error on a problem with `seed` // @param {Uint8Array} seed - must be exactly 32 bytes. pub fn sign(seed: &mut [u8], v1: bool, sk: &mut [u8], msg: &[u8]) -> Result<PlumeSignature, JsError> { // ... let seed_z: zeroize::Zeroizing<[u8; 32]> = zeroize::Zeroizing::new(seed.try_into()?); seed.zeroize(); // TODO switch to `wasi-random` when that will be ready for crypto let sig = match v1 { true => plume_rustcrypto::PlumeSignature::sign_v1( &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z) ), false => plume_rustcrypto::PlumeSignature::sign_v2( &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z) ), }; let sig = signer.sign_with_rng( &mut rand_chacha::ChaCha20Rng::from_seed(*seed_z), msg ); // ... } ``` # `BigInt` conversion It was appealing to leave `s` as `BigInt` (see the comments), but that seems to be confusing and hinder downstream code reusage. There's an util function left for anybody who would want to have it as `BigInt`, but leaving the contraty function makes less sense and also makes the thing larger. So let me left it here for reference. ```rust let scalar_from_bigint = |n: js_sys::BigInt| -> Result<plume_rustcrypto::NonZeroScalar, anyhow::Error> { let result = plume_rustcrypto::NonZeroScalar::from_repr(k256::FieldBytes::from_slice( hex::decode({ let hexstring_freelen = n.to_string(16).map_err( |er| anyhow::Error::msg(er.as_string().expect("`RangeError` can be printed out")) )?.as_string().expect("on `JsString` this always produce a `String`"); let l = hexstring_freelen.len(); if l > 32*2 {return Err(anyhow::Error::msg("too many digits"))} else {["0".repeat(64-l), hexstring_freelen].concat()} })?.as_slice() ).to_owned()); if result.is_none().into() {Err(anyhow::Error::msg("isn't valid `secp256` non-zero scalar"))} else {Ok(result.expect(EXPECT_NONEALREADYCHECKED))} }; ```
skaunov
added a commit
that referenced
this issue
Jul 17, 2024
Resolves #114 removes respective GA this one should be tested like a package I guess, ideas for such tests are welcome as issues Couple of things left out of the committed code. # Subtle I was really late to understand that Subtle crypto supports the different curve `secp256r`, *and* it doesn't provide a facility to store secret values. So implementation for `web_sys::SecretKey` turned out to be just extra miles leading nowhere. ```toml web-sys = { version = "0.3", features = ["CryptoKey", "SubtleCrypto", "Crypto", "EcKeyImportParams"] } wasm-bindgen-futures = "0.4" ``` ```rust #[wasm_bindgen] extern "C" { // Return type of js_sys::global() type Global; // // Web Crypto API: Crypto interface (https://www.w3.org/TR/WebCryptoAPI/) // type WebCrypto; // Getters for the WebCrypto API #[wasm_bindgen(method, getter)] fn crypto(this: &Global) -> web_sys::Crypto; } // `fn sign` if sk.type_() != "secret" {return Err(JsError::new("`sk` must be secret key"))} if !js_sys::Object::values(&sk.algorithm().map_err( |er| JsError::new(er.as_string().expect("TODO check this failing").as_str()) )?).includes(&JsValue::from_str("P-256"), 0) {return Err(JsError::new("`sk` must be from `secp256`"))} // this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs> // js_sys::global().entries().find(); // TODO throw if no Crypto in global let global_the: Global = js_sys::global().unchecked_into(); let crypto_the: web_sys::Crypto = global_the.crypto(); let subtle_the = crypto_the.subtle(); let sk = JsFuture::from(subtle_the.export_key("pkcs8", &sk)?).await?; // ... ::from_pkcs8_der(js_sys::ArrayBuffer::from(sk).try_into()?)?; zeroize::Zeroizing::new(js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &sk).map_err( |er| Err(JsError::new(er.as_string().expect("TODO check this failing").as_str())) )?).await?).to_vec()); // ... // `fn try_into` // ... // zeroization protection ommitted here due to deprecation // <#112> // mostly boilerplate from signing; also some excessive ops left for the same reason // TODO align error-handling in this part if self.c.type_() != "secret" {return Err(JsError::new("`c` must be secret key"))} if !js_sys::Object::values(&self.c.algorithm()?).includes(js_sys::JsString::from("P-256").into(), 0) {return Err(JsError::new("`c` must be from `secp256`"))} this was my approach, but seems I got what they did at <https://github.com/rust-random/getrandom/blob/master/src/js.rs> js_sys::global().entries().find(); // TODO throw if no Crypto in global let global_the: Global = js_sys::global().unchecked_into(); let crypto_the: web_sys::Crypto = global_the.crypto(); let subtle_the = crypto_the.subtle(); let c_pkcs = //zeroize::Zeroizing::new( js_sys::Uint8Array::from(JsFuture::from(subtle_the.export_key("pkcs8", &self.c)?).await?).to_vec(); // ); let c_scalar = &plume_rustcrypto::SecretKey::from_pkcs8_der(&c_pkcs)?.to_nonzero_scalar(); sk_z.zeroize(); // ... ``` # randomness Somehow I thought Wasm doesn't have access to RNG, so I used a seedable one and required the seed. Here's how `sign` `fn` was different. ```rust // Wasm environment doesn't have a suitable way to get randomness for the signing process, so this instantiates ChaCha20 RNG with the provided seed. // @throws a "crypto error" in case of a problem with the secret key, and a verbal error on a problem with `seed` // @param {Uint8Array} seed - must be exactly 32 bytes. pub fn sign(seed: &mut [u8], v1: bool, sk: &mut [u8], msg: &[u8]) -> Result<PlumeSignature, JsError> { // ... let seed_z: zeroize::Zeroizing<[u8; 32]> = zeroize::Zeroizing::new(seed.try_into()?); seed.zeroize(); // TODO switch to `wasi-random` when that will be ready for crypto let sig = match v1 { true => plume_rustcrypto::PlumeSignature::sign_v1( &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z) ), false => plume_rustcrypto::PlumeSignature::sign_v2( &sk_z, msg, &mut rand_chacha::ChaCha20Rng::from_seed(seed_z) ), }; let sig = signer.sign_with_rng( &mut rand_chacha::ChaCha20Rng::from_seed(*seed_z), msg ); // ... } ``` # `BigInt` conversion It was appealing to leave `s` as `BigInt` (see the comments), but that seems to be confusing and hinder downstream code reusage. There's an util function left for anybody who would want to have it as `BigInt`, but leaving the contraty function makes less sense and also makes the thing larger. So let me left it here for reference. ```rust let scalar_from_bigint = |n: js_sys::BigInt| -> Result<plume_rustcrypto::NonZeroScalar, anyhow::Error> { let result = plume_rustcrypto::NonZeroScalar::from_repr(k256::FieldBytes::from_slice( hex::decode({ let hexstring_freelen = n.to_string(16).map_err( |er| anyhow::Error::msg(er.as_string().expect("`RangeError` can be printed out")) )?.as_string().expect("on `JsString` this always produce a `String`"); let l = hexstring_freelen.len(); if l > 32*2 {return Err(anyhow::Error::msg("too many digits"))} else {["0".repeat(64-l), hexstring_freelen].concat()} })?.as_slice() ).to_owned()); if result.is_none().into() {Err(anyhow::Error::msg("isn't valid `secp256` non-zero scalar"))} else {Ok(result.expect(EXPECT_NONEALREADYCHECKED))} }; ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
leave a note that secret values handling isn't appropriately implemented there
The text was updated successfully, but these errors were encountered: