Update to new Scalar API (#293)

* Updated to new curve25519 scalar API

* Made ExpandedSecretKey.scalar_bytes unclamped; clamping occurs in all scalar-point multiplication

* Added legacy compat deprecation notice

* Removed deprecation notice on check_scalar

* Removed unnecessary unwraps

Cargo.toml

@@ -67,9 +67,13 @@ digest = ["signature/digest"]
 # Exposes the hazmat module
 hazmat = []
 # 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"]
 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"

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();
 

src/hazmat.rs

@@ -15,7 +15,7 @@
 
 use crate::{InternalError, SignatureError};
 
-use curve25519_dalek::Scalar;
+use curve25519_dalek::scalar::{clamp_integer, Scalar};
 
 #[cfg(feature = "zeroize")]
 use zeroize::{Zeroize, ZeroizeOnDrop};
@@ -35,6 +35,10 @@ use curve25519_dalek::digest::{generic_array::typenum::U64, Digest};
 ///
 /// Instances of this secret are automatically overwritten with zeroes when they fall out of scope.
 pub struct ExpandedSecretKey {
+    // `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],
     /// The secret scalar used for signing
     pub scalar: Scalar,
     /// The domain separator used when hashing the message to generate the pseudorandom `r` value
@@ -64,18 +68,24 @@ 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.
     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];
+        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]);
 
-        lower.copy_from_slice(&bytes[00..32]);
-        upper.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.
+        let scalar = Scalar::from_bytes_mod_order(clamp_integer(scalar_bytes));
 
         ExpandedSecretKey {
-            scalar: Scalar::from_bytes_mod_order(lower),
-            hash_prefix: upper,
+            scalar_bytes,
+            scalar,
+            hash_prefix,
         }
     }
 
@@ -86,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<Self, SignatureError> {
-        if bytes.len() != 64 {
-            Err(InternalError::BytesLength {
+        // Try to coerce bytes to a [u8; 64]
+        bytes.try_into().map(Self::from_bytes).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()
+        })
     }
 }
 
@@ -213,7 +220,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
@@ -224,17 +230,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<R: RngCore + CryptoRng>(mut rng: R) -> Self {
-            // The usual signing algorithm clamps its scalars
-            let scalar_bytes = [0u8; 32];
-            let scalar = Scalar::from_bits_clamped(scalar_bytes);
-
-            let mut hash_prefix = [0u8; 32];
-            rng.fill_bytes(&mut hash_prefix);
-
-            ExpandedSecretKey {
-                scalar,
-                hash_prefix,
-            }
+            let mut bytes = [0u8; 64];
+            rng.fill_bytes(&mut bytes);
+            ExpandedSecretKey::from_bytes(&bytes)
         }
     }
 

src/signature.rs

@@ -63,6 +63,9 @@ impl Debug for InternalSignature {
     }
 }
 
+/// 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<Scalar, SignatureError> {
@@ -76,24 +79,17 @@ fn check_scalar(bytes: [u8; 32]) -> Result<Scalar, SignatureError> {
         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.
+    // 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<Scalar, SignatureError> {
-    // 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),
@@ -152,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<InternalSignature, SignatureError> {
         // 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)?,
         })
     }
 }

src/signing.rs

@@ -473,12 +473,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
     }
 }
 
@@ -715,14 +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 try_into here converts to fixed-size array
-        ExpandedSecretKey {
-            scalar: Scalar::from_bits_clamped(lower.try_into().unwrap()),
-            hash_prefix: upper.try_into().unwrap(),
-        }
+        ExpandedSecretKey::from_bytes(hash.as_ref())
     }
 }
 

src/verifying.rs

@@ -65,7 +65,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)
     }
 }
@@ -91,8 +91,7 @@ impl PartialEq<VerifyingKey> 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.scalar.to_bytes();
-        VerifyingKey::clamp_and_mul_base(bits)
+        VerifyingKey::clamp_and_mul_base(expanded_secret_key.scalar_bytes)
     }
 }
 
@@ -191,8 +190,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
@@ -501,15 +499,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
+    ///
+    /// 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.pdf).
+    /// 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()
     }

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_bytes = ed25519_signing_key_a.to_scalar_bytes();
+    let scalar_b_bytes = ed25519_signing_key_b.to_scalar_bytes();
 
     assert_eq!(
-        scalar_a.to_bytes(),
-        hex!("307c83864f2833cb427a2ef1c00a013cfdff2768d980c0a3a520f006904de94f")
+        scalar_a_bytes,
+        hex!("357c83864f2833cb427a2ef1c00a013cfdff2768d980c0a3a520f006904de90f")
     );
     assert_eq!(
-        scalar_b.to_bytes(),
-        hex!("68bd9ed75882d52815a97585caf4790a7f6c6b3b7f821c5e259a24b02e502e51")
+        scalar_b_bytes,
+        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_bytes).to_bytes(),
         expected_shared_secret
     );
     assert_eq!(
-        (x25519_public_key_b * scalar_a).to_bytes(),
+        x25519_public_key_b.mul_clamped(scalar_a_bytes).to_bytes(),
         expected_shared_secret
     );
 }