openpgp: handle ??25519 secret keys with leading 0

OpenPGP MPIs allow leading zeros to be omitted while Nettle expects the
leading zeros ot be present. Now, we handle this by padding all secret
keys before handing them off to nettle.

Fixes #96

3 files changed, 128 insertions, 15 deletions

diff --git a/openpgp/src/crypto/ecdh.rs b/openpgp/src/crypto/ecdh.rs
index e90aff18..129d23b9 100644
--- a/openpgp/src/crypto/ecdh.rs
+++ b/openpgp/src/crypto/ecdh.rs
@@ -90,6 +90,8 @@ pub fn wrap_session_key(recipient: &Key, session_key: &[u8])
 pub fn unwrap_session_key(recipient: &Key, recipient_sec: &SecretKey,
                           ciphertext: &Ciphertext)
                           -> Result<Box<[u8]>> {
+    use memsec;
+
     if let (&PublicKey::ECDH {
         ref curve, ref hash, ref sym, ..
     }, SecretKey::ECDH {
@@ -103,27 +105,32 @@ pub fn unwrap_session_key(recipient: &Key, recipient_sec: &SecretKey,
                 #[allow(non_snake_case)]
                 let V = e.decode_point(curve)?.0;
 
-                // Get the secret part r of our key.
-                if scalar.value.len() != curve25519::CURVE25519_SIZE {
-                    return Err(Error::MalformedPacket(
-                        format!("Bad size of Curve25519 private key: {} \
-                                 expected: {}", scalar.value.len(),
-                                curve25519::CURVE25519_SIZE)).into());
-                }
-
+                // Nettle expects the private key to be exactly
+                // CURVE25519_SIZE bytes long but OpenPGP allows leading
+                // zeros to be stripped.
+                // Padding has to be unconditionaly, otherwise we have a
+                // secret-dependant branch.
+                //
                 // Reverse the scalar.  See
                 // https://lists.gnupg.org/pipermail/gnupg-devel/2018-February/033437.html.
-                let mut r_reversed = Vec::with_capacity(scalar.value.len());
-                r_reversed.extend_from_slice(&scalar.value);
-                &mut r_reversed.reverse();
-                let r = &r_reversed;
+                let missing = curve25519::CURVE25519_SIZE
+                    .saturating_sub(scalar.value.len());
+                let mut r = [0u8; curve25519::CURVE25519_SIZE];
+
+                r[missing..].copy_from_slice(&scalar.value[..]);
+                r.reverse();
 
                 // Compute the shared point S = rV = rvG, where (r, R)
                 // is the recipient's key pair.
                 #[allow(non_snake_case)]
                 let mut S = [0; curve25519::CURVE25519_SIZE];
-                curve25519::mul(&mut S, r, V)
-                    .expect("buffers are of the wrong size");
+                let res = curve25519::mul(&mut S, &r[..], V);
+
+                unsafe {
+                    memsec::memzero(r.as_mut_ptr(),
+                                    curve25519::CURVE25519_SIZE);
+                }
+                res.expect("buffers are of the wrong size");
 
                 // Compute KDF input.
                 let param = make_param(recipient, curve, hash, sym);
diff --git a/openpgp/src/packet/pkesk.rs b/openpgp/src/packet/pkesk.rs
index 47eac6d5..d949503d 100644
--- a/openpgp/src/packet/pkesk.rs
+++ b/openpgp/src/packet/pkesk.rs
@@ -270,4 +270,53 @@ mod tests {
             panic!("secret key is encrypted/missing");
         }
     }
+
+    #[test]
+    fn decrypt_with_short_cv25519_secret_key() {
+        use conversions::Time;
+        use super::PKESK;
+        use crypto::SessionKey;
+        use crypto::mpis::{self, MPI};
+        use PublicKeyAlgorithm;
+        use SymmetricAlgorithm;
+        use HashAlgorithm;
+        use constants::Curve;
+        use packet::Key;
+        use nettle::{curve25519, Yarrow};
+        use time;
+
+        // 20 byte sec key
+        let mut sec = [
+            0x0,0x0,
+            0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
+            0x1,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x2,
+            0x1,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x0,0x0
+        ];
+        let mut pnt = [0x40u8; curve25519::CURVE25519_SIZE + 1];
+        curve25519::mul_g(&mut pnt[1..], &sec[..]).unwrap();
+        sec.reverse();
+
+        let public_mpis = mpis::PublicKey::ECDH {
+            curve: Curve::Cv25519,
+            q: MPI::new(&pnt[..]),
+            hash: HashAlgorithm::SHA256,
+            sym: SymmetricAlgorithm::AES256,
+        };
+        let private_mpis = mpis::SecretKey::ECDH {
+            scalar: MPI::new(&sec[..]),
+        };
+        let key = Key{
+            common: Default::default(),
+            version: 4,
+            creation_time: time::now().canonicalize(),
+            pk_algo: PublicKeyAlgorithm::ECDH,
+            mpis: public_mpis,
+            secret: None,
+        };
+        let mut rng = Yarrow::default();
+        let sess_key = SessionKey::new(&mut rng, 32);
+        let pkesk = PKESK::new(SymmetricAlgorithm::AES256, &sess_key, &key).unwrap();
+
+        pkesk.decrypt(&key, &private_mpis).unwrap();
+    }
 }
diff --git a/openpgp/src/packet/signature/mod.rs b/openpgp/src/packet/signature/mod.rs
index 701043de..0d674cbe 100644
--- a/openpgp/src/packet/signature/mod.rs
+++ b/openpgp/src/packet/signature/mod.rs
@@ -93,6 +93,7 @@ impl Builder {
                      -> Result<Signature> {
         use PublicKeyAlgorithm::*;
         use crypto::mpis::PublicKey;
+        use memsec;
 
         let mut rng = Yarrow::default();
 
@@ -155,7 +156,23 @@ impl Builder {
                     let public = q.decode_point(&Curve::Ed25519)?.0;
 
                     let mut sig = vec![0; ed25519::ED25519_SIGNATURE_SIZE];
-                    ed25519::sign(public, &scalar.value, &digest, &mut sig)?;
+
+                    // Nettle expects the private key to be exactly
+                    // ED25519_KEY_SIZE bytes long but OpenPGP allows leading
+                    // zeros to be stripped.
+                    // Padding has to be unconditionaly, otherwise we have a
+                    // secret-dependant branch.
+                    let missing = ed25519::ED25519_KEY_SIZE
+                        .saturating_sub(scalar.value.len());
+                    let mut sec = [0u8; ed25519::ED25519_KEY_SIZE];
+                    sec[missing..].copy_from_slice(&scalar.value[..]);
+
+                    let res = ed25519::sign(public, &sec[..], &digest, &mut sig);
+                    unsafe {
+                        memsec::memzero(sec.as_mut_ptr(),
+                        ed25519::ED25519_KEY_SIZE);
+                    }
+                    res?;
 
                     mpis::Signature::EdDSA {
                         r: MPI::new(&sig[..32]),
@@ -933,4 +950,44 @@ mod test {
             }
         }
     }
+
+    #[test]
+    fn sign_with_short_ed25519_secret_key() {
+        use conversions::Time;
+        use nettle;
+        use time;
+
+        // 20 byte sec key
+        let sec = [
+            0x0,0x0,
+            0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0,
+            0x1,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x2,
+            0x1,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x2,0x2
+        ];
+        let mut pnt = [0x40u8; nettle::ed25519::ED25519_KEY_SIZE + 1];
+        ed25519::public_key(&mut pnt[1..], &sec[..]).unwrap();
+
+        let public_mpis = mpis::PublicKey::EdDSA {
+            curve: Curve::Ed25519,
+            q: MPI::new(&pnt[..]),
+        };
+        let private_mpis = mpis::SecretKey::EdDSA {
+            scalar: MPI::new(&sec[..]),
+        };
+        let key = Key{
+            common: Default::default(),
+            version: 4,
+            creation_time: time::now().canonicalize(),
+            pk_algo: PublicKeyAlgorithm::EdDSA,
+            mpis: public_mpis,
+            secret: None,
+        };
+        let msg = b"Hello, World";
+        let mut hash = HashAlgorithm::SHA256.context().unwrap();
+
+        hash.update(&msg[..]);
+
+        Builder::new(SignatureType::Text)
+            .sign_hash(&key, &private_mpis, HashAlgorithm::SHA256, hash).unwrap();
+    }
 }