Do byte comparison in all verify_* functions (#269)

* Made all signature R comparisons byte-wise

* Use Scalar::from_bits_clamped rather than manually clamping

* Added clippy lints and comments for use of unwrap()

* Clarify use of unused

src/lib.rs

@@ -241,6 +241,7 @@
 #![no_std]
 #![warn(future_incompatible, rust_2018_idioms)]
 #![deny(missing_docs)] // refuse to compile if documentation is missing
+#![deny(clippy::unwrap_used)] // don't allow unwrap
 #![cfg_attr(not(test), forbid(unsafe_code))]
 #![cfg_attr(docsrs, feature(doc_auto_cfg, doc_cfg, doc_cfg_hide))]
 #![cfg_attr(docsrs, doc(cfg_hide(docsrs)))]

src/signature.rs

@@ -162,6 +162,7 @@ impl InternalSignature {
     /// only checking the most significant three bits.  (See also the
     /// documentation for [`crate::VerifyingKey::verify_strict`].)
     #[inline]
+    #[allow(clippy::unwrap_used)]
     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);
@@ -181,7 +182,10 @@ impl TryFrom<&ed25519::Signature> for InternalSignature {
 }
 
 impl From<InternalSignature> for ed25519::Signature {
+    #[allow(clippy::unwrap_used)]
     fn from(sig: InternalSignature) -> ed25519::Signature {
+        // This function only fails if the s half of the parsed input exceeds the scalar modulus.
+        // Since the bytes are coming straight from a Scalar, this is impossible.
         ed25519::Signature::from_bytes(&sig.as_bytes()).unwrap()
     }
 }

src/signing.rs

@@ -681,25 +681,16 @@ impl Drop for ExpandedSecretKey {
 }
 
 impl From<&SecretKey> for ExpandedSecretKey {
+    #[allow(clippy::unwrap_used)]
     fn from(secret_key: &SecretKey) -> ExpandedSecretKey {
-        let mut h: Sha512 = Sha512::default();
-        let mut hash: [u8; 64] = [0u8; 64];
-        let mut lower: [u8; 32] = [0u8; 32];
-        let mut upper: [u8; 32] = [0u8; 32];
-
-        h.update(secret_key);
-        hash.copy_from_slice(h.finalize().as_slice());
-
-        lower.copy_from_slice(&hash[00..32]);
-        upper.copy_from_slice(&hash[32..64]);
-
-        lower[0] &= 248;
-        lower[31] &= 63;
-        lower[31] |= 64;
+        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 {
-            key: Scalar::from_bits(lower),
-            nonce: upper,
+            key: Scalar::from_bits_clamped(lower.try_into().unwrap()),
+            nonce: upper.try_into().unwrap(),
         }
     }
 }

src/verifying.rs

@@ -51,9 +51,8 @@ use crate::signing::*;
 /// considered unequal to the other equivalent encoding, despite the two representing the same
 /// point. More encoding details can be found
 /// [here](https://hdevalence.ca/blog/2020-10-04-its-25519am).
-///
-/// If you don't care and/or don't want to deal with this, just make sure to use the
-/// [`VerifyingKey::verify_strict`] function.
+/// If you want to make sure that signatures produced with respect to those sorts of public keys
+/// are rejected, use [`VerifyingKey::verify_strict`].
 // Invariant: VerifyingKey.1 is always the decompression of VerifyingKey.0
 #[derive(Copy, Clone, Default, Eq)]
 pub struct VerifyingKey(pub(crate) CompressedEdwardsY, pub(crate) EdwardsPoint);
@@ -85,8 +84,8 @@ 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 mut bits: [u8; 32] = expanded_secret_key.key.to_bytes();
-        VerifyingKey::mangle_scalar_bits_and_multiply_by_basepoint_to_produce_public_key(&mut bits)
+        let bits: [u8; 32] = expanded_secret_key.key.to_bytes();
+        VerifyingKey::clamp_and_mul_base(bits)
     }
 }
 
@@ -154,17 +153,10 @@ impl VerifyingKey {
         Ok(VerifyingKey(compressed, point))
     }
 
-    /// Internal utility function for mangling the bits of a (formerly
-    /// mathematically well-defined) "scalar" and multiplying it to produce a
-    /// public key.
-    fn mangle_scalar_bits_and_multiply_by_basepoint_to_produce_public_key(
-        bits: &mut [u8; 32],
-    ) -> VerifyingKey {
-        bits[0] &= 248;
-        bits[31] &= 127;
-        bits[31] |= 64;
-
-        let scalar = Scalar::from_bits(*bits);
+    /// 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 compressed = point.compress();
 
@@ -198,17 +190,21 @@ impl VerifyingKey {
     // Helper function for verification. Computes the _expected_ R component of the signature. The
     // caller compares this to the real R component.  If `context.is_some()`, this does the
     // prehashed variant of the computation using its contents.
+    // Note that this returns the compressed form of R and the caller does a byte comparison. This
+    // means that all our verification functions do not accept non-canonically encoded R values.
+    // See the validation criteria blog post for more details:
+    //     https://hdevalence.ca/blog/2020-10-04-its-25519am
     #[allow(non_snake_case)]
     fn recompute_r(
         &self,
         context: Option<&[u8]>,
         signature: &InternalSignature,
         M: &[u8],
-    ) -> EdwardsPoint {
+    ) -> CompressedEdwardsY {
         let k = Self::compute_challenge(context, &signature.R, &self.0, M);
         let minus_A: EdwardsPoint = -self.1;
         // Recall the (non-batched) verification equation: -[k]A + [s]B = R
-        EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s)
+        EdwardsPoint::vartime_double_scalar_mul_basepoint(&k, &(minus_A), &signature.s).compress()
     }
 
     /// Verify a `signature` on a `prehashed_message` using the Ed25519ph algorithm.
@@ -249,7 +245,7 @@ impl VerifyingKey {
         let message = prehashed_message.finalize();
         let expected_R = self.recompute_r(Some(ctx), &signature, &message);
 
-        if expected_R.compress() == signature.R {
+        if expected_R == signature.R {
             Ok(())
         } else {
             Err(InternalError::Verify.into())
@@ -337,7 +333,7 @@ impl VerifyingKey {
         }
 
         let expected_R = self.recompute_r(None, &signature, message);
-        if expected_R == signature_R {
+        if expected_R == signature.R {
             Ok(())
         } else {
             Err(InternalError::Verify.into())
@@ -393,7 +389,7 @@ impl VerifyingKey {
         let message = prehashed_message.finalize();
         let expected_R = self.recompute_r(Some(ctx), &signature, &message);
 
-        if expected_R == signature_R {
+        if expected_R == signature.R {
             Ok(())
         } else {
             Err(InternalError::Verify.into())
@@ -412,7 +408,7 @@ impl Verifier<ed25519::Signature> for VerifyingKey {
         let signature = InternalSignature::try_from(signature)?;
 
         let expected_R = self.recompute_r(None, &signature, message);
-        if expected_R.compress() == signature.R {
+        if expected_R == signature.R {
             Ok(())
         } else {
             Err(InternalError::Verify.into())