openpgp: Clamp the secret key in Key4::import_secret_cv25519.

  - Fixes #1087.

3 files changed, 52 insertions, 4 deletions

diff --git a/openpgp/NEWS b/openpgp/NEWS
index 8d8c4624..572dce4f 100644
--- a/openpgp/NEWS
+++ b/openpgp/NEWS
@@ -3,6 +3,12 @@
 #+TITLE: sequoia-openpgp NEWS – history of user-visible changes
 #+STARTUP: content hidestars
 
+* Changes in 1.19.0
+** Notable fixes
+   - Key4::import_secret_cv25519 will now clamp some bits of the given
+     secret scalar to make the generated secret key packet more
+     compatible with implementations that do not implicitly do the
+     clamping before decryption.
 * Changes in 1.18.0
 ** New functionality
    - ComponentAmalgamation::certifications_by_key
diff --git a/openpgp/src/crypto/backend/interface.rs b/openpgp/src/crypto/backend/interface.rs
index bf6f876a..864b2714 100644
--- a/openpgp/src/crypto/backend/interface.rs
+++ b/openpgp/src/crypto/backend/interface.rs
@@ -51,6 +51,22 @@ pub trait Asymmetric {
     /// Returns a tuple containing the secret and public key.
     fn x25519_generate_key() -> Result<(Protected, [u8; 32])>;
 
+    /// Clamp the X25519 secret key scalar.
+    ///
+    /// X25519 does the clamping implicitly, but OpenPGP's ECDH over
+    /// Curve25519 requires the secret to be clamped.  To increase
+    /// compatibility with OpenPGP implementations that do not
+    /// implicitly clamp the secrets before use, we do that before we
+    /// store the secrets in OpenPGP data structures.
+    ///
+    /// Note: like every function in this trait, this function expects
+    /// `secret` to be in native byte order.
+    fn x25519_clamp_secret(secret: &mut Protected) {
+        secret[0] &= 0b1111_1000;
+        secret[31] &= !0b1000_0000;
+        secret[31] |= 0b0100_0000;
+    }
+
     /// Computes the public key for a given secret key.
     fn x25519_derive_public(secret: &Protected) -> Result<[u8; 32]>;
 
diff --git a/openpgp/src/packet/key.rs b/openpgp/src/packet/key.rs
index 118428fe..787c5101 100644
--- a/openpgp/src/packet/key.rs
+++ b/openpgp/src/packet/key.rs
@@ -1139,6 +1139,10 @@ impl<R> Key4<SecretParts, R>
     /// algorithm `sym`.  If one or both are `None` secure defaults
     /// will be used.  The key will have it's creation date set to
     /// `ctime` or the current time if `None` is given.
+    ///
+    /// The given `private_key` is expected to be in the native X25519
+    /// representation, i.e. as opaque byte string of length 32.  It
+    /// is transformed into OpenPGP's representation during import.
     pub fn import_secret_cv25519<H, S, T>(private_key: &[u8],
                                           hash: H, sym: S, ctime: T)
         -> Result<Self> where H: Into<Option<HashAlgorithm>>,
@@ -1150,6 +1154,22 @@ impl<R> Key4<SecretParts, R>
         let mut private_key = Protected::from(private_key);
         let public_key = Backend::x25519_derive_public(&private_key)?;
 
+        // Clamp the X25519 secret key scalar.
+        //
+        // X25519 does the clamping implicitly, but OpenPGP's ECDH
+        // over Curve25519 requires the secret to be clamped.  To
+        // increase compatibility with OpenPGP implementations that do
+        // not implicitly clamp the secrets before use, we do that
+        // before we store the secrets in OpenPGP data structures.
+        Backend::x25519_clamp_secret(&mut private_key);
+
+        // Reverse the scalar.
+        //
+        // X25519 stores the secret as opaque byte string representing
+        // a little-endian scalar.  OpenPGP's ECDH over Curve25519 on
+        // the other hand stores it as big-endian scalar, as was
+        // customary in OpenPGP.  See
+        // https://lists.gnupg.org/pipermail/gnupg-devel/2018-February/033437.html.
         private_key.reverse();
 
         use crate::crypto::ecdh;
@@ -2441,10 +2461,14 @@ FwPoSAbbsLkNS/iNN2MDGAVYvezYn2QZ
     }
 
     #[test]
-    #[cfg(not(windows))] // see: https://gitlab.com/sequoia-pgp/sequoia/-/issues/958
-    fn cv25519_secret_is_reversed() {
-        let private_key: &[u8] = &crate::crypto::SessionKey::new(32);
-        let key: Key4<_, UnspecifiedRole> = Key4::import_secret_cv25519(private_key, None, None, None).unwrap();
+    fn cv25519_secret_is_reversed() -> Result<()> {
+        use crate::crypto::backend::{Backend, interface::Asymmetric};
+
+        let (mut private_key, _) = Backend::x25519_generate_key()?;
+        Backend::x25519_clamp_secret(&mut private_key);
+
+        let key: Key4<_, UnspecifiedRole> =
+            Key4::import_secret_cv25519(&private_key, None, None, None)?;
         if let crate::packet::key::SecretKeyMaterial::Unencrypted(key) = key.secret() {
             key.map(|secret| {
                 if let mpi::SecretKeyMaterial::ECDH { scalar } = secret {
@@ -2458,6 +2482,8 @@ FwPoSAbbsLkNS/iNN2MDGAVYvezYn2QZ
         } else {
             unreachable!();
         }
+
+        Ok(())
     }
 
     #[test]