From 69706a4361ec61c96405c415ab3fab900db4177d Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 20 Mar 2023 15:31:28 -0400 Subject: [PATCH 01/18] Updated to new curve25519 scalar API --- Cargo.lock | 2 -- Cargo.toml | 6 +++--- src/signature.rs | 2 ++ src/signing.rs | 15 +++++++++++++-- src/verifying.rs | 8 +++----- 5 files changed, 21 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e80fe13..e54ead0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -240,8 +240,6 @@ dependencies = [ [[package]] name = "curve25519-dalek" version = "4.0.0-rc.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8d4ba9852b42210c7538b75484f9daa0655e9a3ac04f693747bb0f02cf3cfe16" dependencies = [ "cfg-if", "digest", diff --git a/Cargo.toml b/Cargo.toml index 57bc2da..0b95c5e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,7 @@ rustdoc-args = [ features = ["batch", "pkcs8"] [dependencies] -curve25519-dalek = { version = "=4.0.0-rc.1", default-features = false, features = ["digest"] } +curve25519-dalek = { path = "../curve25519-dalek", default-features = false, features = ["digest"] } ed25519 = { version = ">=2.2, <2.3", default-features = false } signature = { version = ">=2.0, <2.1", optional = true, default-features = false } sha2 = { version = "0.10", default-features = false } @@ -38,7 +38,7 @@ serde_bytes = { version = "0.11", optional = true } zeroize = { version = "1.5", default-features = false, optional = true } [dev-dependencies] -curve25519-dalek = { version = "=4.0.0-rc.1", default-features = false, features = ["digest", "rand_core"] } +curve25519-dalek = { path = "../curve25519-dalek", default-features = false, features = ["digest", "rand_core"] } hex = "0.4" bincode = "1.0" serde_json = "1.0" @@ -64,7 +64,7 @@ batch = ["alloc", "merlin", "rand_core"] fast = ["curve25519-dalek/precomputed-tables"] digest = ["signature/digest"] # This features turns off stricter checking for scalar malleability in signatures -legacy_compatibility = [] +legacy_compatibility = ["curve25519-dalek/legacy_compatibility"] pkcs8 = ["ed25519/pkcs8"] pem = ["alloc", "ed25519/pem", "pkcs8"] rand_core = ["dep:rand_core"] diff --git a/src/signature.rs b/src/signature.rs index 72b7b0e..4b34e10 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -90,9 +90,11 @@ fn check_scalar(bytes: [u8; 32]) -> Result { // as the order of the basepoint is roughly a 2^(252.5) bit number. // // This succeed-fast trick should succeed for roughly half of all scalars. + /* if bytes[31] & 240 == 0 { return Ok(Scalar::from_bits(bytes)); } + */ match Scalar::from_canonical_bytes(bytes).into() { None => Err(InternalError::ScalarFormat.into()), diff --git a/src/signing.rs b/src/signing.rs index 28f7346..aee901f 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -29,7 +29,7 @@ use curve25519_dalek::digest::generic_array::typenum::U64; use curve25519_dalek::digest::Digest; use curve25519_dalek::edwards::CompressedEdwardsY; use curve25519_dalek::edwards::EdwardsPoint; -use curve25519_dalek::scalar::Scalar; +use curve25519_dalek::scalar::{clamp_integer, Scalar}; use ed25519::signature::{KeypairRef, Signer, Verifier}; @@ -688,6 +688,10 @@ impl<'d> Deserialize<'d> for SigningKey { // better-designed, Schnorr-based signature scheme, see Trevor Perrin's work on // "generalised EdDSA" and "VXEdDSA". pub(crate) struct ExpandedSecretKey { + // `key_bytes` and `key` are separate, because the public key is computed as an unreduced + // scalar multiplication (ie `mul_base_clamped`), whereas the signing operations are done + // modulo l. + pub(crate) key_bytes: [u8; 32], pub(crate) key: Scalar, pub(crate) nonce: [u8; 32], } @@ -695,6 +699,7 @@ pub(crate) struct ExpandedSecretKey { #[cfg(feature = "zeroize")] impl Drop for ExpandedSecretKey { fn drop(&mut self) { + self.key_bytes.zeroize(); self.key.zeroize(); self.nonce.zeroize() } @@ -707,9 +712,15 @@ impl From<&SecretKey> for ExpandedSecretKey { // TODO: Use bytes.split_array_ref once it’s in MSRV. let (lower, upper) = hash.split_at(32); + // Clamp the lower bytes. This is the key integer. // The try_into here converts to fixed-size array + let key_bytes = clamp_integer(lower.try_into().unwrap()); + // For signing, we'll need the integer as a Scalar + let key = Scalar::from_bytes_mod_order(key_bytes); + ExpandedSecretKey { - key: Scalar::from_bits_clamped(lower.try_into().unwrap()), + key_bytes, + key, nonce: upper.try_into().unwrap(), } } diff --git a/src/verifying.rs b/src/verifying.rs index 1ea9332..ae86ae0 100644 --- a/src/verifying.rs +++ b/src/verifying.rs @@ -66,7 +66,7 @@ pub struct VerifyingKey { } impl Debug for VerifyingKey { - fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { write!(f, "VerifyingKey({:?}), {:?})", self.compressed, self.point) } } @@ -92,8 +92,7 @@ impl PartialEq for VerifyingKey { impl From<&ExpandedSecretKey> for VerifyingKey { /// Derive this public key from its corresponding `ExpandedSecretKey`. fn from(expanded_secret_key: &ExpandedSecretKey) -> VerifyingKey { - let bits: [u8; 32] = expanded_secret_key.key.to_bytes(); - VerifyingKey::clamp_and_mul_base(bits) + VerifyingKey::clamp_and_mul_base(expanded_secret_key.key_bytes) } } @@ -183,8 +182,7 @@ impl VerifyingKey { /// Internal utility function for clamping a scalar representation and multiplying by the /// basepont to produce a public key. fn clamp_and_mul_base(bits: [u8; 32]) -> VerifyingKey { - let scalar = Scalar::from_bits_clamped(bits); - let point = EdwardsPoint::mul_base(&scalar); + let point = EdwardsPoint::mul_base_clamped(bits); let compressed = point.compress(); // Invariant: VerifyingKey.1 is always the decompression of VerifyingKey.0 From 7ea430090db27a3070c76ead5431907359522a95 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 20 Mar 2023 15:49:39 -0400 Subject: [PATCH 02/18] Switched curve25519-dalek dep to a test branch --- Cargo.lock | 1 + Cargo.toml | 4 ++-- src/signature.rs | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e54ead0..4125879 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -240,6 +240,7 @@ dependencies = [ [[package]] name = "curve25519-dalek" version = "4.0.0-rc.1" +source = "git+ssh://git@github.com/rozbb/curve25519-dalek.git?branch=scalar-always-reduced#5d1aae2b391d38c3a3abd7776edbe6499cc1dc8d" dependencies = [ "cfg-if", "digest", diff --git a/Cargo.toml b/Cargo.toml index 0b95c5e..39b1e49 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,7 @@ rustdoc-args = [ features = ["batch", "pkcs8"] [dependencies] -curve25519-dalek = { path = "../curve25519-dalek", default-features = false, features = ["digest"] } +curve25519-dalek = { git = "ssh://git@github.com/rozbb/curve25519-dalek.git", branch = "scalar-always-reduced", default-features = false, features = ["digest"] } ed25519 = { version = ">=2.2, <2.3", default-features = false } signature = { version = ">=2.0, <2.1", optional = true, default-features = false } sha2 = { version = "0.10", default-features = false } @@ -38,7 +38,7 @@ serde_bytes = { version = "0.11", optional = true } zeroize = { version = "1.5", default-features = false, optional = true } [dev-dependencies] -curve25519-dalek = { path = "../curve25519-dalek", default-features = false, features = ["digest", "rand_core"] } +curve25519-dalek = { git = "ssh://git@github.com/rozbb/curve25519-dalek.git", branch = "scalar-always-reduced", default-features = false, features = ["digest", "rand_core"] } hex = "0.4" bincode = "1.0" serde_json = "1.0" diff --git a/src/signature.rs b/src/signature.rs index 4b34e10..957e4e9 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -76,6 +76,8 @@ fn check_scalar(bytes: [u8; 32]) -> Result { return Err(InternalError::ScalarFormat.into()); } + // You cannot do arithmetic with scalars construct with Scalar::from_bits. We only use this + // scalar for EdwardsPoint::vartime_double_scalar_mul_basepoint, which is an accepted usecase. Ok(Scalar::from_bits(bytes)) } From 74cb956ea30a5b46e44d1ce7b1e46cd53ab89a5e Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 20 Mar 2023 15:51:04 -0400 Subject: [PATCH 03/18] ssh -> https --- Cargo.lock | 2 +- Cargo.toml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4125879..a0452f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -240,7 +240,7 @@ dependencies = [ [[package]] name = "curve25519-dalek" version = "4.0.0-rc.1" -source = "git+ssh://git@github.com/rozbb/curve25519-dalek.git?branch=scalar-always-reduced#5d1aae2b391d38c3a3abd7776edbe6499cc1dc8d" +source = "git+https://github.com/rozbb/curve25519-dalek.git?branch=scalar-always-reduced#5d1aae2b391d38c3a3abd7776edbe6499cc1dc8d" dependencies = [ "cfg-if", "digest", diff --git a/Cargo.toml b/Cargo.toml index 39b1e49..4c1baa5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,7 @@ rustdoc-args = [ features = ["batch", "pkcs8"] [dependencies] -curve25519-dalek = { git = "ssh://git@github.com/rozbb/curve25519-dalek.git", branch = "scalar-always-reduced", default-features = false, features = ["digest"] } +curve25519-dalek = { git = "https://github.com/rozbb/curve25519-dalek.git", branch = "scalar-always-reduced", default-features = false, features = ["digest"] } ed25519 = { version = ">=2.2, <2.3", default-features = false } signature = { version = ">=2.0, <2.1", optional = true, default-features = false } sha2 = { version = "0.10", default-features = false } @@ -38,7 +38,7 @@ serde_bytes = { version = "0.11", optional = true } zeroize = { version = "1.5", default-features = false, optional = true } [dev-dependencies] -curve25519-dalek = { git = "ssh://git@github.com/rozbb/curve25519-dalek.git", branch = "scalar-always-reduced", default-features = false, features = ["digest", "rand_core"] } +curve25519-dalek = { git = "https://github.com/rozbb/curve25519-dalek.git", branch = "scalar-always-reduced", default-features = false, features = ["digest", "rand_core"] } hex = "0.4" bincode = "1.0" serde_json = "1.0" From bf5d2cfb83302808879969c2ab2063dd2789e187 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Tue, 21 Mar 2023 03:01:27 -0400 Subject: [PATCH 04/18] Deleted 'succeed-fast' signature parsing trick --- src/signature.rs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/signature.rs b/src/signature.rs index 957e4e9..52d3769 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -84,20 +84,6 @@ fn check_scalar(bytes: [u8; 32]) -> Result { #[cfg(not(feature = "legacy_compatibility"))] #[inline(always)] fn check_scalar(bytes: [u8; 32]) -> Result { - // Since this is only used in signature deserialisation (i.e. upon - // verification), we can do a "succeed fast" trick by checking that the most - // significant 4 bits are unset. If they are unset, we can succeed fast - // because we are guaranteed that the scalar is fully reduced. However, if - // the 4th most significant bit is set, we must do the full reduction check, - // as the order of the basepoint is roughly a 2^(252.5) bit number. - // - // This succeed-fast trick should succeed for roughly half of all scalars. - /* - if bytes[31] & 240 == 0 { - return Ok(Scalar::from_bits(bytes)); - } - */ - match Scalar::from_canonical_bytes(bytes).into() { None => Err(InternalError::ScalarFormat.into()), Some(x) => Ok(x), From 92afd3ed78ce7f5c7e9fd52c4d87cbd59021719f Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Fri, 31 Mar 2023 17:04:29 -0400 Subject: [PATCH 05/18] Updated curve- dep to the new API --- Cargo.lock | 10 +++++----- Cargo.toml | 4 ++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6b7711b..d1e6e57 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -239,8 +239,8 @@ dependencies = [ [[package]] name = "curve25519-dalek" -version = "4.0.0-rc.1" -source = "git+https://github.com/rozbb/curve25519-dalek.git?branch=scalar-always-reduced#5d1aae2b391d38c3a3abd7776edbe6499cc1dc8d" +version = "4.0.0-rc.2" +source = "git+https://github.com/dalek-cryptography/curve25519-dalek.git?rev=f460ae149b0000695205cc78f560d74a2d3918eb#f460ae149b0000695205cc78f560d74a2d3918eb" dependencies = [ "cfg-if", "digest", @@ -286,7 +286,7 @@ dependencies = [ [[package]] name = "ed25519-dalek" -version = "2.0.0-pre.0" +version = "2.0.0-rc.2" dependencies = [ "bincode", "criterion", @@ -313,9 +313,9 @@ checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91" [[package]] name = "fiat-crypto" -version = "0.1.17" +version = "0.1.19" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a214f5bb88731d436478f3ae1f8a277b62124089ba9fb67f4f93fb100ef73c90" +checksum = "93ace6ec7cc19c8ed33a32eaa9ea692d7faea05006b5356b9e2b668ec4bc3955" [[package]] name = "generic-array" diff --git a/Cargo.toml b/Cargo.toml index d3ce138..c300cab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,3 +69,7 @@ pem = ["alloc", "ed25519/pem", "pkcs8"] rand_core = ["dep:rand_core"] serde = ["dep:serde", "ed25519/serde"] zeroize = ["dep:zeroize", "curve25519-dalek/zeroize"] + +[patch.crates-io.curve25519-dalek] +git = "https://github.com/dalek-cryptography/curve25519-dalek.git" +rev = "f460ae149b0000695205cc78f560d74a2d3918eb" From 06f5517b05a89c8e202868f1947be66fd9a4e5df Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sat, 1 Apr 2023 00:18:20 -0400 Subject: [PATCH 06/18] Made ExpandedSecretKey.scalar_bytes unclamped; clamping occurs in all scalar-point multiplication --- src/signing.rs | 34 ++++++++++++++++++++++------------ src/verifying.rs | 18 +++++++++++------- tests/x25519.rs | 16 ++++++++-------- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/signing.rs b/src/signing.rs index 243b594..d750abc 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -462,12 +462,23 @@ impl SigningKey { self.verifying_key.verify_strict(message, signature) } - /// Convert this signing key into a Curve25519 scalar. + /// Convert this signing key into a byte representation of a(n) (unreduced) Curve25519 scalar. /// - /// This is useful for e.g. performing X25519 Diffie-Hellman using - /// Ed25519 keys. - pub fn to_scalar(&self) -> Scalar { - ExpandedSecretKey::from(&self.secret_key).scalar + /// This can be used for performing X25519 Diffie-Hellman using Ed25519 keys. The bytes output + /// by this function are a valid secret key for the X25519 public key given by + /// `self.verifying_key().to_montgomery()`. + /// + /// # Note + /// + /// We do NOT recommend this usage of a signing/verifying key. Signing keys are usually + /// long-term keys, while keys used for key exchange should rather be ephemeral. If you can + /// help it, use a separate key for encryption. + /// + /// For more information on the security of systems which use the same keys for both signing + /// and Diffie-Hellman, see the paper + /// [On using the same key pair for Ed25519 and an X25519 based KEM](https://eprint.iacr.org/2021/509). + pub fn to_scalar_bytes(&self) -> [u8; 32] { + ExpandedSecretKey::from(&self.secret_key).scalar_bytes } } @@ -760,15 +771,14 @@ impl From<&SecretKey> for ExpandedSecretKey { // TODO: Use bytes.split_array_ref once it’s in MSRV. let (lower, upper) = hash.split_at(32); - // Clamp the lower bytes. This is the key integer. - // The try_into here converts to fixed-size array - let key_bytes = clamp_integer(lower.try_into().unwrap()); - // For signing, we'll need the integer as a Scalar - let key = Scalar::from_bytes_mod_order(key_bytes); + // The lower bytes are the scalar. The try_into here converts to fixed-size array + let scalar_bytes = lower.try_into().unwrap(); + // For signing, we'll need the integer, clamped, and converted to a Scalar + let scalar = Scalar::from_bytes_mod_order(clamp_integer(scalar_bytes)); ExpandedSecretKey { - scalar_bytes: key_bytes, - scalar: key, + scalar_bytes, + scalar, nonce: upper.try_into().unwrap(), } } diff --git a/src/verifying.rs b/src/verifying.rs index 4cc7eed..7b4f944 100644 --- a/src/verifying.rs +++ b/src/verifying.rs @@ -420,15 +420,19 @@ impl VerifyingKey { /// Convert this verifying key into Montgomery form. /// - /// This is useful for systems which perform X25519 Diffie-Hellman using - /// Ed25519 keys. + /// This can be used for performing X25519 Diffie-Hellman using Ed25519 keys. The output of + /// this function is a valid X25519 public key whose secret key is `sk.to_scalar_bytes()`, + /// where `sk` is a valid signing key for this `VerifyingKey`. /// - /// When possible, it's recommended to use separate keys for signing and - /// Diffie-Hellman. + /// # Note /// - /// For more information on the security of systems which use the same keys - /// for both signing and Diffie-Hellman, see the paper - /// [On using the same key pair for Ed25519 and an X25519 based KEM](https://eprint.iacr.org/2021/509.pdf). + /// We do NOT recommend this usage of a signing/verifying key. Signing keys are usually + /// long-term keys, while keys used for key exchange should rather be ephemeral. If you can + /// help it, use a separate key for encryption. + /// + /// For more information on the security of systems which use the same keys for both signing + /// and Diffie-Hellman, see the paper + /// [On using the same key pair for Ed25519 and an X25519 based KEM](https://eprint.iacr.org/2021/509). pub fn to_montgomery(&self) -> MontgomeryPoint { self.point.to_montgomery() } diff --git a/tests/x25519.rs b/tests/x25519.rs index bb588f7..c22a302 100644 --- a/tests/x25519.rs +++ b/tests/x25519.rs @@ -16,16 +16,16 @@ fn ed25519_to_x25519_dh() { let ed25519_signing_key_a = SigningKey::from_bytes(&ed25519_secret_key_a); let ed25519_signing_key_b = SigningKey::from_bytes(&ed25519_secret_key_b); - let scalar_a = ed25519_signing_key_a.to_scalar(); - let scalar_b = ed25519_signing_key_b.to_scalar(); + let scalar_a = ed25519_signing_key_a.to_scalar_bytes(); + let scalar_b = ed25519_signing_key_b.to_scalar_bytes(); assert_eq!( - scalar_a.to_bytes(), - hex!("307c83864f2833cb427a2ef1c00a013cfdff2768d980c0a3a520f006904de94f") + scalar_a, + hex!("357c83864f2833cb427a2ef1c00a013cfdff2768d980c0a3a520f006904de90f") ); assert_eq!( - scalar_b.to_bytes(), - hex!("68bd9ed75882d52815a97585caf4790a7f6c6b3b7f821c5e259a24b02e502e51") + scalar_b, + hex!("6ebd9ed75882d52815a97585caf4790a7f6c6b3b7f821c5e259a24b02e502e11") ); let x25519_public_key_a = ed25519_signing_key_a.verifying_key().to_montgomery(); @@ -44,11 +44,11 @@ fn ed25519_to_x25519_dh() { hex!("5166f24a6918368e2af831a4affadd97af0ac326bdf143596c045967cc00230e"); assert_eq!( - (x25519_public_key_a * scalar_b).to_bytes(), + x25519_public_key_a.mul_clamped(scalar_b).to_bytes(), expected_shared_secret ); assert_eq!( - (x25519_public_key_b * scalar_a).to_bytes(), + x25519_public_key_b.mul_clamped(scalar_a).to_bytes(), expected_shared_secret ); } From 652d151f27307b2097c3dee73b42dd560049b18d Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sat, 1 Apr 2023 00:36:27 -0400 Subject: [PATCH 07/18] Attempt CI fix for legacy_compatibility deprecation warnings --- .github/workflows/rust.yml | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index a70fef0..4ecdbad 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -39,7 +39,11 @@ jobs: - run: cargo test --target ${{ matrix.target }} --features digest,rand_core - run: cargo test --target ${{ matrix.target }} --features serde - run: cargo test --target ${{ matrix.target }} --features pem - - run: cargo test --target ${{ matrix.target }} --all-features + # Can't use --all-features because of legacy_compatibility + - run: cargo test --target ${{ matrix.target }} --features asm,batch,digest,pem,rand_core,serde + - env: + RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning + run: cargo test --target ${{ matrix.target }} --features legacy_compatibility build-simd: name: Test simd backend (nightly) @@ -82,7 +86,10 @@ jobs: - uses: taiki-e/install-action@cargo-hack # No default features build - run: cargo build --target thumbv7em-none-eabi --release --no-default-features - - run: cargo hack build --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std + - run: cargo hack build --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std,legacy_compatibility + - env: + RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning + run: cargo build --target thumbv7em-none-eabi --release --features "legacy_compatibility" bench: name: Check that benchmarks compile @@ -119,4 +126,6 @@ jobs: - uses: dtolnay/rust-toolchain@stable with: toolchain: stable - - run: cargo doc --all-features + - env: + RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning + run: cargo doc --all-features From da33aa42a8dab832e5e2a7f5ddb65e26e1d373ba Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sat, 1 Apr 2023 00:40:09 -0400 Subject: [PATCH 08/18] Fix 2 --- .github/workflows/rust.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 4ecdbad..56240d3 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -89,7 +89,7 @@ jobs: - run: cargo hack build --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std,legacy_compatibility - env: RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning - run: cargo build --target thumbv7em-none-eabi --release --features "legacy_compatibility" + run: cargo build --target thumbv7em-none-eabi --release --no-default-features --features "legacy_compatibility" bench: name: Check that benchmarks compile From bcee905ae27f45938d84033c1696daa523e75022 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Tue, 4 Apr 2023 13:14:31 -0400 Subject: [PATCH 09/18] Docs; added allow(deprecated) to legacy_compatibility check --- .github/workflows/rust.yml | 11 ++--------- src/signature.rs | 1 + src/signing.rs | 5 +++-- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 56240d3..18ec9f2 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -39,11 +39,7 @@ jobs: - run: cargo test --target ${{ matrix.target }} --features digest,rand_core - run: cargo test --target ${{ matrix.target }} --features serde - run: cargo test --target ${{ matrix.target }} --features pem - # Can't use --all-features because of legacy_compatibility - - run: cargo test --target ${{ matrix.target }} --features asm,batch,digest,pem,rand_core,serde - - env: - RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning - run: cargo test --target ${{ matrix.target }} --features legacy_compatibility + run: cargo test --target ${{ matrix.target }} --all-features build-simd: name: Test simd backend (nightly) @@ -86,10 +82,7 @@ jobs: - uses: taiki-e/install-action@cargo-hack # No default features build - run: cargo build --target thumbv7em-none-eabi --release --no-default-features - - run: cargo hack build --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std,legacy_compatibility - - env: - RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning - run: cargo build --target thumbv7em-none-eabi --release --no-default-features --features "legacy_compatibility" + - run: cargo hack build --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std bench: name: Check that benchmarks compile diff --git a/src/signature.rs b/src/signature.rs index 52d3769..4a58656 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -78,6 +78,7 @@ fn check_scalar(bytes: [u8; 32]) -> Result { // You cannot do arithmetic with scalars construct with Scalar::from_bits. We only use this // scalar for EdwardsPoint::vartime_double_scalar_mul_basepoint, which is an accepted usecase. + #[allow(deprecated)] Ok(Scalar::from_bits(bytes)) } diff --git a/src/signing.rs b/src/signing.rs index d750abc..849ad44 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -747,7 +747,7 @@ impl<'d> Deserialize<'d> for SigningKey { // better-designed, Schnorr-based signature scheme, see Trevor Perrin's work on // "generalised EdDSA" and "VXEdDSA". pub(crate) struct ExpandedSecretKey { - // `key_bytes` and `key` are separate, because the public key is computed as an unreduced + // `scalar_bytes` and `scalar` are separate, because the public key is computed as an unreduced // scalar multiplication (ie `mul_base_clamped`), whereas the signing operations are done // modulo l. pub(crate) scalar_bytes: [u8; 32], @@ -773,7 +773,8 @@ impl From<&SecretKey> for ExpandedSecretKey { // The lower bytes are the scalar. The try_into here converts to fixed-size array let scalar_bytes = lower.try_into().unwrap(); - // For signing, we'll need the integer, clamped, and converted to a Scalar + // For signing, we'll need the integer, clamped, and converted to a Scalar. See + // PureEdDSA.keygen in RFC 8032 Appendix A. let scalar = Scalar::from_bytes_mod_order(clamp_integer(scalar_bytes)); ExpandedSecretKey { From 9a2b2964c53ae4c26c6baac5a15c259aa9b18a43 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Tue, 4 Apr 2023 15:22:12 -0400 Subject: [PATCH 10/18] Added legacy compat deprecation notice --- src/signature.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/signature.rs b/src/signature.rs index 4a58656..c328cf0 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -63,6 +63,10 @@ impl Debug for InternalSignature { } } +#[deprecated( + since = "2.0.0", + note = "The legacy_compatibility feature permits signature malleability. See README." +)] #[cfg(feature = "legacy_compatibility")] #[inline(always)] fn check_scalar(bytes: [u8; 32]) -> Result { From 1cf99d7d06a11572f532871de583dc15c2cf79f4 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Tue, 4 Apr 2023 15:24:19 -0400 Subject: [PATCH 11/18] Fix CI --- .github/workflows/rust.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 18ec9f2..a70fef0 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -39,7 +39,7 @@ jobs: - run: cargo test --target ${{ matrix.target }} --features digest,rand_core - run: cargo test --target ${{ matrix.target }} --features serde - run: cargo test --target ${{ matrix.target }} --features pem - run: cargo test --target ${{ matrix.target }} --all-features + - run: cargo test --target ${{ matrix.target }} --all-features build-simd: name: Test simd backend (nightly) @@ -119,6 +119,4 @@ jobs: - uses: dtolnay/rust-toolchain@stable with: toolchain: stable - - env: - RUSTFLAGS: '-D warnings -A deprecated' # legacy_compatibility triggers a deprecation warning - run: cargo doc --all-features + - run: cargo doc --all-features From 36808fdfe24e5971566092845a72acc7679d6161 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 10 Apr 2023 02:09:37 -0400 Subject: [PATCH 12/18] Removed deprecation notice on check_scalar --- src/signature.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/signature.rs b/src/signature.rs index c328cf0..b2e8a6b 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -63,10 +63,9 @@ impl Debug for InternalSignature { } } -#[deprecated( - since = "2.0.0", - note = "The legacy_compatibility feature permits signature malleability. See README." -)] +/// Ensures that the scalar `s` of a signature is within the bounds [0, 2^253). +/// +/// **Unsafe**: This version of `check_scalar` permits signature malleability. See README. #[cfg(feature = "legacy_compatibility")] #[inline(always)] fn check_scalar(bytes: [u8; 32]) -> Result { @@ -82,10 +81,12 @@ fn check_scalar(bytes: [u8; 32]) -> Result { // You cannot do arithmetic with scalars construct with Scalar::from_bits. We only use this // scalar for EdwardsPoint::vartime_double_scalar_mul_basepoint, which is an accepted usecase. + // The `from_bits` method is deprecated because it's unsafe. We know this. #[allow(deprecated)] Ok(Scalar::from_bits(bytes)) } +/// Ensures that the scalar `s` of a signature is within the bounds [0, ℓ) #[cfg(not(feature = "legacy_compatibility"))] #[inline(always)] fn check_scalar(bytes: [u8; 32]) -> Result { From 997e4ed6f643c217a72388ac672f4794f5327043 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Fri, 9 Jun 2023 00:08:42 -0400 Subject: [PATCH 13/18] Fixing CI --- src/signing.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/signing.rs b/src/signing.rs index 7524717..62e73c6 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -20,10 +20,8 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use sha2::Sha512; -#[cfg(feature = "digest")] -use curve25519_dalek::digest::generic_array::typenum::U64; use curve25519_dalek::{ - digest::Digest, + digest::{generic_array::typenum::U64, Digest}, edwards::{CompressedEdwardsY, EdwardsPoint}, scalar::{clamp_integer, Scalar}, }; From c67428e2c6d8ccf3b3bf26dc65216eac580ff0fb Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Fri, 9 Jun 2023 00:26:11 -0400 Subject: [PATCH 14/18] Made clippy happy --- src/hazmat.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/hazmat.rs b/src/hazmat.rs index f84a80e..2c2869d 100644 --- a/src/hazmat.rs +++ b/src/hazmat.rs @@ -69,17 +69,15 @@ impl ExpandedSecretKey { } /// Construct an `ExpandedSecretKey` from an array of 64 bytes. + #[allow(clippy::unwrap_used)] pub fn from_bytes(bytes: &[u8; 64]) -> Self { // TODO: Use bytes.split_array_ref once it’s in MSRV. - let mut lower: [u8; 32] = [0u8; 32]; - let mut upper: [u8; 32] = [0u8; 32]; - - lower.copy_from_slice(&bytes[00..32]); - upper.copy_from_slice(&bytes[32..64]); + let (lower, upper) = bytes.split_at(32); - // The try_into here converts to fixed-size array - // The lower bytes are the scalar. The try_into here converts to fixed-size array + // Convert the slices to [u8; 32] let scalar_bytes = lower.try_into().unwrap(); + let hash_prefix = upper.try_into().unwrap(); + // For signing, we'll need the integer, clamped, and converted to a Scalar. See // PureEdDSA.keygen in RFC 8032 Appendix A. let scalar = Scalar::from_bytes_mod_order(clamp_integer(scalar_bytes)); @@ -87,7 +85,7 @@ impl ExpandedSecretKey { ExpandedSecretKey { scalar_bytes, scalar, - hash_prefix: upper.try_into().unwrap(), + hash_prefix, } } From df466f8094230744e0b4f111e3457a6e5d8f3c9a Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 11 Jun 2023 17:06:11 -0400 Subject: [PATCH 15/18] Misc cleanup --- Cargo.toml | 2 +- src/hazmat.rs | 21 ++++++--------------- src/signing.rs | 17 ++--------------- tests/x25519.rs | 12 ++++++------ 4 files changed, 15 insertions(+), 37 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index abaa11a..ec28d59 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,7 +66,7 @@ fast = ["curve25519-dalek/precomputed-tables"] digest = ["signature/digest"] # Exposes the hazmat module hazmat = [] -# This features turns off stricter checking for scalar malleability in signatures +# Turns off stricter checking for scalar malleability in signatures legacy_compatibility = ["curve25519-dalek/legacy_compatibility"] pkcs8 = ["ed25519/pkcs8"] pem = ["alloc", "ed25519/pem", "pkcs8"] diff --git a/src/hazmat.rs b/src/hazmat.rs index 2c2869d..c90baa3 100644 --- a/src/hazmat.rs +++ b/src/hazmat.rs @@ -68,7 +68,9 @@ impl ExpandedSecretKey { bytes } - /// Construct an `ExpandedSecretKey` from an array of 64 bytes. + /// Construct an `ExpandedSecretKey` from an array of 64 bytes. In the spec, the bytes are the + /// output of a SHA-512 hash. This clamps the first 32 bytes and uses it as a scalar, and uses + /// the second 32 bytes as a domain separator for hashing. #[allow(clippy::unwrap_used)] pub fn from_bytes(bytes: &[u8; 64]) -> Self { // TODO: Use bytes.split_array_ref once it’s in MSRV. @@ -223,7 +225,6 @@ where mod test { use super::*; - use curve25519_dalek::Scalar; use rand::{rngs::OsRng, CryptoRng, RngCore}; // Pick distinct, non-spec 512-bit hash functions for message and sig-context hashing @@ -234,19 +235,9 @@ mod test { // Make a random expanded secret key for testing purposes. This is NOT how you generate // expanded secret keys IRL. They're the hash of a seed. fn random(mut rng: R) -> Self { - // The usual signing algorithm clamps its scalars - let mut scalar_bytes = [0u8; 32]; - rng.fill_bytes(&mut scalar_bytes); - let scalar = Scalar::from_bytes_mod_order(clamp_integer(scalar_bytes)); - - let mut hash_prefix = [0u8; 32]; - rng.fill_bytes(&mut hash_prefix); - - ExpandedSecretKey { - scalar_bytes, - scalar, - hash_prefix, - } + let mut bytes = [0u8; 64]; + rng.fill_bytes(&mut bytes); + ExpandedSecretKey::from_bytes(&bytes) } } diff --git a/src/signing.rs b/src/signing.rs index 62e73c6..b0f0b49 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -23,7 +23,7 @@ use sha2::Sha512; use curve25519_dalek::{ digest::{generic_array::typenum::U64, Digest}, edwards::{CompressedEdwardsY, EdwardsPoint}, - scalar::{clamp_integer, Scalar}, + scalar::Scalar, }; use ed25519::signature::{KeypairRef, Signer, Verifier}; @@ -726,20 +726,7 @@ impl From<&SecretKey> for ExpandedSecretKey { #[allow(clippy::unwrap_used)] fn from(secret_key: &SecretKey) -> ExpandedSecretKey { let hash = Sha512::default().chain_update(secret_key).finalize(); - // TODO: Use bytes.split_array_ref once it’s in MSRV. - let (lower, upper) = hash.split_at(32); - - // The lower bytes are the scalar. The try_into here converts to fixed-size array - let scalar_bytes = lower.try_into().unwrap(); - // For signing, we'll need the integer, clamped, and converted to a Scalar. See - // PureEdDSA.keygen in RFC 8032 Appendix A. - let scalar = Scalar::from_bytes_mod_order(clamp_integer(scalar_bytes)); - - ExpandedSecretKey { - scalar_bytes, - scalar, - hash_prefix: upper.try_into().unwrap(), - } + ExpandedSecretKey::from_bytes(hash.as_ref()) } } diff --git a/tests/x25519.rs b/tests/x25519.rs index c22a302..18ae502 100644 --- a/tests/x25519.rs +++ b/tests/x25519.rs @@ -16,15 +16,15 @@ fn ed25519_to_x25519_dh() { let ed25519_signing_key_a = SigningKey::from_bytes(&ed25519_secret_key_a); let ed25519_signing_key_b = SigningKey::from_bytes(&ed25519_secret_key_b); - let scalar_a = ed25519_signing_key_a.to_scalar_bytes(); - let scalar_b = ed25519_signing_key_b.to_scalar_bytes(); + let scalar_a_bytes = ed25519_signing_key_a.to_scalar_bytes(); + let scalar_b_bytes = ed25519_signing_key_b.to_scalar_bytes(); assert_eq!( - scalar_a, + scalar_a_bytes, hex!("357c83864f2833cb427a2ef1c00a013cfdff2768d980c0a3a520f006904de90f") ); assert_eq!( - scalar_b, + scalar_b_bytes, hex!("6ebd9ed75882d52815a97585caf4790a7f6c6b3b7f821c5e259a24b02e502e11") ); @@ -44,11 +44,11 @@ fn ed25519_to_x25519_dh() { hex!("5166f24a6918368e2af831a4affadd97af0ac326bdf143596c045967cc00230e"); assert_eq!( - x25519_public_key_a.mul_clamped(scalar_b).to_bytes(), + x25519_public_key_a.mul_clamped(scalar_b_bytes).to_bytes(), expected_shared_secret ); assert_eq!( - x25519_public_key_b.mul_clamped(scalar_a).to_bytes(), + x25519_public_key_b.mul_clamped(scalar_a_bytes).to_bytes(), expected_shared_secret ); } From 651c04e1b31d9bcbdae85910fae9d248652168dd Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 11 Jun 2023 23:48:41 -0400 Subject: [PATCH 16/18] Removed unnecessary unwraps --- src/hazmat.rs | 23 +++++++++-------------- src/signature.rs | 12 ++++++++---- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/hazmat.rs b/src/hazmat.rs index c90baa3..56588b4 100644 --- a/src/hazmat.rs +++ b/src/hazmat.rs @@ -71,14 +71,12 @@ impl ExpandedSecretKey { /// Construct an `ExpandedSecretKey` from an array of 64 bytes. In the spec, the bytes are the /// output of a SHA-512 hash. This clamps the first 32 bytes and uses it as a scalar, and uses /// the second 32 bytes as a domain separator for hashing. - #[allow(clippy::unwrap_used)] pub fn from_bytes(bytes: &[u8; 64]) -> Self { // TODO: Use bytes.split_array_ref once it’s in MSRV. - let (lower, upper) = bytes.split_at(32); - - // Convert the slices to [u8; 32] - let scalar_bytes = lower.try_into().unwrap(); - let hash_prefix = upper.try_into().unwrap(); + let mut scalar_bytes: [u8; 32] = [0u8; 32]; + let mut hash_prefix: [u8; 32] = [0u8; 32]; + scalar_bytes.copy_from_slice(&bytes[00..32]); + hash_prefix.copy_from_slice(&bytes[32..64]); // For signing, we'll need the integer, clamped, and converted to a Scalar. See // PureEdDSA.keygen in RFC 8032 Appendix A. @@ -98,18 +96,15 @@ impl ExpandedSecretKey { /// A `Result` whose okay value is an EdDSA `ExpandedSecretKey` or whose error value is an /// `SignatureError` describing the error that occurred, namely that the given slice's length /// is not 64. - #[allow(clippy::unwrap_used)] pub fn from_slice(bytes: &[u8]) -> Result { - if bytes.len() != 64 { - Err(InternalError::BytesLength { + // Try to coerce bytes to a [u8; 64] + bytes.try_into().map(|b| Self::from_bytes(b)).map_err(|_| { + InternalError::BytesLength { name: "ExpandedSecretKey", length: 64, } - .into()) - } else { - // If the input is 64 bytes long, coerce it to a 64-byte array - Ok(Self::from_bytes(bytes.try_into().unwrap())) - } + .into() + }) } } diff --git a/src/signature.rs b/src/signature.rs index b2e8a6b..36174c8 100644 --- a/src/signature.rs +++ b/src/signature.rs @@ -148,13 +148,17 @@ impl InternalSignature { /// only checking the most significant three bits. (See also the /// documentation for [`crate::VerifyingKey::verify_strict`].) #[inline] - #[allow(clippy::unwrap_used)] + #[allow(non_snake_case)] pub fn from_bytes(bytes: &[u8; SIGNATURE_LENGTH]) -> Result { // TODO: Use bytes.split_array_ref once it’s in MSRV. - let (lower, upper) = bytes.split_at(32); + let mut R_bytes: [u8; 32] = [0u8; 32]; + let mut s_bytes: [u8; 32] = [0u8; 32]; + R_bytes.copy_from_slice(&bytes[00..32]); + s_bytes.copy_from_slice(&bytes[32..64]); + Ok(InternalSignature { - R: CompressedEdwardsY(lower.try_into().unwrap()), - s: check_scalar(upper.try_into().unwrap())?, + R: CompressedEdwardsY(R_bytes), + s: check_scalar(s_bytes)?, }) } } From 955739f76afa1b397e637528552b84fdac9e3433 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 11 Jun 2023 23:53:35 -0400 Subject: [PATCH 17/18] Made clippy happy --- src/hazmat.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hazmat.rs b/src/hazmat.rs index 56588b4..4414a84 100644 --- a/src/hazmat.rs +++ b/src/hazmat.rs @@ -98,7 +98,7 @@ impl ExpandedSecretKey { /// is not 64. pub fn from_slice(bytes: &[u8]) -> Result { // Try to coerce bytes to a [u8; 64] - bytes.try_into().map(|b| Self::from_bytes(b)).map_err(|_| { + bytes.try_into().map(Self::from_bytes).map_err(|_| { InternalError::BytesLength { name: "ExpandedSecretKey", length: 64, From c1c9b44c8111acf57bcb4c255d72a42c815a866d Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 11 Jun 2023 23:59:20 -0400 Subject: [PATCH 18/18] Removed another unwrap --- src/batch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/batch.rs b/src/batch.rs index d94008d..d5d1746 100644 --- a/src/batch.rs +++ b/src/batch.rs @@ -177,7 +177,7 @@ pub fn verify_batch( h.update(signatures[i].r_bytes()); h.update(verifying_keys[i].as_bytes()); h.update(&messages[i]); - h.finalize().try_into().unwrap() + *h.finalize().as_ref() }) .collect();