openpgp: Rework protection of mpis::SecretKey.

  - Introduce a new type, ProtectedMPI, that uses
    crypto::mem::Protected for storing the MPI.  Change
    mpis::SecretKey to use this.

  - Fixes #181.

7 files changed, 131 insertions, 83 deletions

diff --git a/openpgp/src/crypto/mem.rs b/openpgp/src/crypto/mem.rs
index f08b9f68..45447719 100644
--- a/openpgp/src/crypto/mem.rs
+++ b/openpgp/src/crypto/mem.rs
@@ -10,7 +10,7 @@ use memsec;
 /// Holds a session key.
 ///
 /// The session key is cleared when dropped.
-#[derive(Clone, Eq)]
+#[derive(Clone, Eq, Hash)]
 pub struct Protected(Pin<Box<[u8]>>);
 
 impl PartialEq for Protected {
diff --git a/openpgp/src/crypto/mpis.rs b/openpgp/src/crypto/mpis.rs
index 418a58dd..0db4e1a5 100644
--- a/openpgp/src/crypto/mpis.rs
+++ b/openpgp/src/crypto/mpis.rs
@@ -14,7 +14,7 @@ use constants::{
     SymmetricAlgorithm,
 };
 use crypto::Hash;
-use crypto::mem::secure_cmp;
+use crypto::mem::{secure_cmp, Protected};
 use serialize::Serialize;
 
 use Error;
@@ -227,6 +227,61 @@ impl PartialEq for MPI {
 
 impl Eq for MPI {}
 
+/// Holds a single MPI containing secrets.
+///
+/// The memory will be cleared when the object is dropped.
+#[derive(Clone, Hash)]
+pub struct ProtectedMPI {
+    /// Integer value as big-endian.
+    value: Protected,
+}
+
+impl From<Vec<u8>> for ProtectedMPI {
+    fn from(m: Vec<u8>) -> Self {
+        MPI::from(m).into()
+    }
+}
+
+impl From<Protected> for ProtectedMPI {
+    fn from(m: Protected) -> Self {
+        MPI::new(&m).into()
+    }
+}
+
+impl From<MPI> for ProtectedMPI {
+    fn from(m: MPI) -> Self {
+        ProtectedMPI {
+            value: m.value.into(),
+        }
+    }
+}
+
+impl ProtectedMPI {
+    /// Returns the length of the MPI in bits.
+    pub fn bits(&self) -> usize {
+        self.value.len() * 8
+            - self.value.get(0).map(|&b| b.leading_zeros() as usize)
+                  .unwrap_or(0)
+    }
+
+    /// Returns the value of this MPI.
+    pub fn value(&self) -> &[u8] {
+        &self.value
+    }
+}
+
+impl fmt::Debug for ProtectedMPI {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        if cfg!(debug_assertions) {
+            f.write_fmt(format_args!(
+                "{} bits: {}", self.bits(),
+                ::conversions::to_hex(&*self.value, true)))
+        } else {
+            f.write_str("<Redacted>")
+        }
+    }
+}
+
 /// Holds a public key.
 ///
 /// Provides a typed and structured way of storing multiple MPIs (and
@@ -427,51 +482,51 @@ pub enum SecretKey {
     /// RSA secret key.
     RSA {
         /// Secret exponent, inverse of e in Phi(N).
-        d: MPI,
+        d: ProtectedMPI,
         /// Larger secret prime.
-        p: MPI,
+        p: ProtectedMPI,
         /// Smaller secret prime.
-        q: MPI,
+        q: ProtectedMPI,
         /// Inverse of p mod q.
-        u: MPI,
+        u: ProtectedMPI,
     },
 
     /// NIST DSA secret key.
     DSA {
         /// Secret key log_g(y) in Zp.
-        x: MPI,
+        x: ProtectedMPI,
     },
 
     /// Elgamal secret key.
     Elgamal {
         /// Secret key log_g(y) in Zp.
-        x: MPI,
+        x: ProtectedMPI,
     },
 
     /// DJBs "Twisted" Edwards curve DSA secret key.
     EdDSA {
         /// Secret scalar.
-        scalar: MPI,
+        scalar: ProtectedMPI,
     },
 
     /// NISTs Elliptic curve DSA secret key.
     ECDSA {
         /// Secret scalar.
-        scalar: MPI,
+        scalar: ProtectedMPI,
     },
 
     /// Elliptic curve Elgamal secret key.
     ECDH {
         /// Secret scalar.
-        scalar: MPI,
+        scalar: ProtectedMPI,
     },
 
     /// Unknown number of MPIs for an unknown algorithm.
     Unknown {
         /// The successfully parsed MPIs.
-        mpis: Box<[MPI]>,
+        mpis: Box<[ProtectedMPI]>,
         /// Any data that failed to parse.
-        rest: Box<[u8]>,
+        rest: Protected,
     },
 }
 
@@ -515,7 +570,7 @@ impl fmt::Debug for SecretKey {
     }
 }
 
-fn secure_mpi_cmp(a: &MPI, b: &MPI) -> Ordering {
+fn secure_mpi_cmp(a: &ProtectedMPI, b: &ProtectedMPI) -> Ordering {
     let ord1 = a.bits().cmp(&b.bits());
     let ord2 = secure_cmp(&a.value, &b.value);
 
@@ -609,36 +664,6 @@ impl PartialEq for SecretKey {
 
 impl Eq for SecretKey {}
 
-impl Drop for SecretKey {
-    fn drop(&mut self) {
-        use self::SecretKey::*;
-        match self {
-            RSA { ref mut d, ref mut p, ref mut q, ref mut u } => {
-                d.secure_memzero();
-                p.secure_memzero();
-                q.secure_memzero();
-                u.secure_memzero();
-            },
-            DSA { ref mut x } =>
-                x.secure_memzero(),
-            Elgamal { ref mut x } =>
-                x.secure_memzero(),
-            EdDSA { ref mut scalar } =>
-                scalar.secure_memzero(),
-            ECDSA { ref mut scalar } =>
-                scalar.secure_memzero(),
-            ECDH { ref mut scalar } =>
-                scalar.secure_memzero(),
-            Unknown { ref mut mpis, ref mut rest } => {
-                mpis.iter_mut().for_each(|m| m.secure_memzero());
-                unsafe {
-                    ::memsec::memzero(rest.as_mut_ptr(), rest.len());
-                }
-            },
-        }
-    }
-}
-
 impl SecretKey {
     /// Number of octets all MPIs of this instance occupy when serialized.
     pub fn serialized_len(&self) -> usize {
@@ -680,30 +705,30 @@ impl Arbitrary for SecretKey {
     fn arbitrary<G: Gen>(g: &mut G) -> Self {
         match g.gen_range(0, 6) {
             0 => SecretKey::RSA {
-                d: MPI::arbitrary(g),
-                p: MPI::arbitrary(g),
-                q: MPI::arbitrary(g),
-                u: MPI::arbitrary(g),
+                d: MPI::arbitrary(g).into(),
+                p: MPI::arbitrary(g).into(),
+                q: MPI::arbitrary(g).into(),
+                u: MPI::arbitrary(g).into(),
             },
 
             1 => SecretKey::DSA {
-                x: MPI::arbitrary(g),
+                x: MPI::arbitrary(g).into(),
             },
 
             2 => SecretKey::Elgamal {
-                x: MPI::arbitrary(g),
+                x: MPI::arbitrary(g).into(),
             },
 
             3 => SecretKey::EdDSA {
-                scalar: MPI::arbitrary(g),
+                scalar: MPI::arbitrary(g).into(),
             },
 
             4 => SecretKey::ECDSA {
-                scalar: MPI::arbitrary(g),
+                scalar: MPI::arbitrary(g).into(),
             },
 
             5 => SecretKey::ECDH {
-                scalar: MPI::arbitrary(g),
+                scalar: MPI::arbitrary(g).into(),
             },
 
             _ => unreachable!(),
diff --git a/openpgp/src/packet/key.rs b/openpgp/src/packet/key.rs
index 62ccbb22..20016769 100644
--- a/openpgp/src/packet/key.rs
+++ b/openpgp/src/packet/key.rs
@@ -9,7 +9,7 @@ use nettle::Hash as NettleHash;
 use nettle::hash::insecure_do_not_use::Sha1;
 
 use Error;
-use crypto::{mpis, Hash, KeyPair, SessionKey};
+use crypto::{mem::Protected, mpis, Hash, KeyPair};
 use packet::Tag;
 use packet;
 use Packet;
@@ -159,7 +159,7 @@ impl Key4 {
                 q: mpis::MPI::new(&public_key),
             },
             secret: Some(mpis::SecretKey::ECDH {
-                scalar: mpis::MPI::new(&private_key)
+                scalar: private_key.into(),
             }.into()),
         })
     }
@@ -211,7 +211,7 @@ impl Key4 {
                 q: mpis::MPI::new(&public_key),
             },
             secret: Some(mpis::SecretKey::EdDSA {
-                scalar: mpis::MPI::new(&private_key)
+                scalar: mpis::MPI::new(private_key).into(),
             }.into()),
         })
     }
@@ -259,11 +259,11 @@ impl Key4 {
                 n: mpis::MPI::new(&key.n()[..]),
             },
             secret: Some(mpis::SecretKey::RSA {
-                    d: mpis::MPI::new(d),
-                    p: mpis::MPI::new(&a[..]),
-                    q: mpis::MPI::new(&b[..]),
-                    u: mpis::MPI::new(&c[..]),
-                }.into()),
+                d: mpis::MPI::new(d).into(),
+                p: mpis::MPI::new(&a[..]).into(),
+                q: mpis::MPI::new(&b[..]).into(),
+                u: mpis::MPI::new(&c[..]).into(),
+            }.into()),
         })
     }
 
@@ -276,14 +276,14 @@ impl Key4 {
         let (public, private) = rsa::generate_keypair(&mut rng, bits as u32)?;
         let (p, q, u) = private.as_rfc4880();
         let public_mpis = PublicKey::RSA {
-            e: MPI::new(&*public.e()),
-            n: MPI::new(&*public.n()),
+            e: MPI::new(&*public.e()).into(),
+            n: MPI::new(&*public.n()).into(),
         };
         let private_mpis = mpis::SecretKey::RSA {
-            d: MPI::new(&*private.d()),
-            p: MPI::new(&*p),
-            q: MPI::new(&*q),
-            u: MPI::new(&*u),
+            d: MPI::new(&*private.d()).into(),
+            p: MPI::new(&*p).into(),
+            q: MPI::new(&*q).into(),
+            u: MPI::new(&*u).into(),
         };
         let sec = Some(private_mpis.into());
 
@@ -320,7 +320,8 @@ impl Key4 {
         let (mpis, secret, pk_algo) = match (curve.clone(), for_signing) {
             (Curve::Ed25519, true) => {
                 let mut public = [0u8; ED25519_KEY_SIZE + 1];
-                let mut private: SessionKey = ed25519::private_key(&mut rng).into();
+                let mut private: Protected =
+                    ed25519::private_key(&mut rng).into();
 
                 public[0] = 0x40;
                 ed25519::public_key(&mut public[1..], &private)?;
@@ -330,7 +331,7 @@ impl Key4 {
                     q: MPI::new(&public),
                 };
                 let private_mpis = mpis::SecretKey::EdDSA {
-                    scalar: MPI::new(&private),
+                    scalar: private.into(),
                 };
                 let sec = Some(private_mpis.into());
 
@@ -339,7 +340,8 @@ impl Key4 {
 
             (Curve::Cv25519, false) => {
                 let mut public = [0u8; CURVE25519_SIZE + 1];
-                let mut private: SessionKey = curve25519::private_key(&mut rng).into();
+                let mut private: Protected =
+                    curve25519::private_key(&mut rng).into();
 
                 public[0] = 0x40;
 
@@ -356,7 +358,7 @@ impl Key4 {
                     sym: SymmetricAlgorithm::AES256,
                 };
                 let private_mpis = mpis::SecretKey::ECDH {
-                    scalar: MPI::new(&private),
+                    scalar: private.into(),
                 };
                 let sec = Some(private_mpis.into());
 
@@ -389,7 +391,7 @@ impl Key4 {
                     q: MPI::new_weierstrass(&pub_x, &pub_y, field_sz),
                 };
                 let private_mpis = mpis::SecretKey::ECDSA{
-                    scalar: MPI::new(&private.as_bytes()),
+                    scalar: MPI::new(&private.as_bytes()).into(),
                 };
                 let sec = Some(private_mpis.into());
 
@@ -428,7 +430,7 @@ impl Key4 {
                         sym: SymmetricAlgorithm::AES256,
                     };
                     let private_mpis = mpis::SecretKey::ECDH{
-                        scalar: MPI::new(&private.as_bytes()),
+                        scalar: MPI::new(&private.as_bytes()).into(),
                     };
                     let sec = Some(private_mpis.into());
 
diff --git a/openpgp/src/packet/pkesk.rs b/openpgp/src/packet/pkesk.rs
index 012c940e..735908a4 100644
--- a/openpgp/src/packet/pkesk.rs
+++ b/openpgp/src/packet/pkesk.rs
@@ -355,7 +355,7 @@ mod tests {
             sym: SymmetricAlgorithm::AES256,
         };
         let private_mpis = mpis::SecretKey::ECDH {
-            scalar: MPI::new(&sec[..]),
+            scalar: MPI::new(&sec[..]).into(),
         };
         let mut key: Key = Key4::new(time::now().canonicalize(),
                                      PublicKeyAlgorithm::ECDH,
diff --git a/openpgp/src/packet/signature/mod.rs b/openpgp/src/packet/signature/mod.rs
index 04bb81e9..67144716 100644
--- a/openpgp/src/packet/signature/mod.rs
+++ b/openpgp/src/packet/signature/mod.rs
@@ -1169,7 +1169,7 @@ mod test {
             q: MPI::new(&pnt[..]),
         };
         let private_mpis = mpis::SecretKey::EdDSA {
-            scalar: MPI::new(&sec[..]),
+            scalar: MPI::new(&sec[..]).into(),
         };
         let key = Key4::new(time::now().canonicalize(),
                             PublicKeyAlgorithm::EdDSA,
diff --git a/openpgp/src/parse/mpis.rs b/openpgp/src/parse/mpis.rs
index 3b78755b..d5c7c455 100644
--- a/openpgp/src/parse/mpis.rs
+++ b/openpgp/src/parse/mpis.rs
@@ -218,10 +218,10 @@ impl mpis::SecretKey {
                 let u = MPI::parse("rsa_secret_u_len", "rsa_secret_u", php)?;
 
                 Ok(mpis::SecretKey::RSA {
-                    d: d,
-                    p: p,
-                    q: q,
-                    u: u,
+                    d: d.into(),
+                    p: p.into(),
+                    q: q.into(),
+                    u: u.into(),
                 })
             }
 
@@ -229,7 +229,7 @@ impl mpis::SecretKey {
                 let x = MPI::parse("dsa_secret_len", "dsa_secret", php)?;
 
                 Ok(mpis::SecretKey::DSA {
-                    x: x,
+                    x: x.into(),
                 })
             }
 
@@ -238,25 +238,28 @@ impl mpis::SecretKey {
                                    php)?;
 
                 Ok(mpis::SecretKey::Elgamal {
-                    x: x,
+                    x: x.into(),
                 })
             }
 
             EdDSA => {
                 Ok(mpis::SecretKey::EdDSA {
                     scalar: MPI::parse("eddsa_secret_len", "eddsa_secret", php)?
+                                .into()
                 })
             }
 
             ECDSA => {
                 Ok(mpis::SecretKey::ECDSA {
                     scalar: MPI::parse("ecdsa_secret_len", "ecdsa_secret", php)?
+                                .into()
                 })
             }
 
             ECDH => {
                 Ok(mpis::SecretKey::ECDH {
                     scalar: MPI::parse("ecdh_secret_len", "ecdh_secret", php)?
+                                .into()
                 })
             }
 
@@ -264,13 +267,13 @@ impl mpis::SecretKey {
                 let mut mpis = Vec::new();
                 while let Ok(mpi) = MPI::parse("unknown_parameter_len",
                                                "unknown_parameter", php) {
-                    mpis.push(mpi);
+                    mpis.push(mpi.into());
                 }
                 let mut rest = php.parse_bytes_eof("rest")?;
 
                 Ok(mpis::SecretKey::Unknown {
                     mpis: mpis.into_boxed_slice(),
-                    rest: rest.into_boxed_slice(),
+                    rest: rest.into(),
                 })
             }
         }
diff --git a/openpgp/src/serialize/mod.rs b/openpgp/src/serialize/mod.rs
index 454a484d..5ea93659 100644
--- a/openpgp/src/serialize/mod.rs
+++ b/openpgp/src/serialize/mod.rs
@@ -423,6 +423,24 @@ impl SerializeInto for crypto::mpis::MPI {
     }
 }
 
+impl Serialize for crypto::mpis::ProtectedMPI {
+    fn serialize(&self, w: &mut dyn std::io::Write) -> Result<()> {
+        write_be_u16(w, self.bits() as u16)?;
+        w.write_all(self.value())?;
+        Ok(())
+    }
+}
+
+impl SerializeInto for crypto::mpis::ProtectedMPI {
+    fn serialized_len(&self) -> usize {
+        2 + self.value().len()
+    }
+
+    fn serialize_into(&self, buf: &mut [u8]) -> Result<usize> {
+        generic_serialize_into(self, buf)
+    }
+}
+
 impl Serialize for crypto::mpis::PublicKey {
     fn serialize(&self, w: &mut dyn std::io::Write) -> Result<()> {
         use crypto::mpis::PublicKey::*;