openpgp: Avoid leaking secrets when parsing secret key material.

3 files changed, 57 insertions, 30 deletions

diff --git a/openpgp/src/crypto/mpi.rs b/openpgp/src/crypto/mpi.rs
index fe1bebff..961398c5 100644
--- a/openpgp/src/crypto/mpi.rs
+++ b/openpgp/src/crypto/mpi.rs
@@ -46,6 +46,12 @@ assert_send_and_sync!(MPI);
 
 impl From<Vec<u8>> for MPI {
     fn from(v: Vec<u8>) -> Self {
+        // XXX: This will leak secrets in v into the heap.  But,
+        // eagerly clearing the memory may have a very high overhead,
+        // after all, most MPIs that we encounter will not contain
+        // secrets.  I think it is better to avoid creating MPIs that
+        // contain secrets in the first place.  In 2.0, we can remove
+        // the impl From<MPI> for ProtectedMPI.
         Self::new(&v)
     }
 }
@@ -353,6 +359,8 @@ impl From<Protected> for ProtectedMPI {
     }
 }
 
+// XXX: In 2.0, get rid of this conversion.  If the value has been
+// parsed into an MPI, it may have already leaked.
 impl From<MPI> for ProtectedMPI {
     fn from(m: MPI) -> Self {
         ProtectedMPI {
diff --git a/openpgp/src/parse.rs b/openpgp/src/parse.rs
index 64c5b531..42881aa4 100644
--- a/openpgp/src/parse.rs
+++ b/openpgp/src/parse.rs
@@ -226,7 +226,7 @@ use crate::types::{
     SymmetricAlgorithm,
     Timestamp,
 };
-use crate::crypto::{self, mpi::{PublicKey, MPI}};
+use crate::crypto::{self, mpi::{PublicKey, MPI, ProtectedMPI}};
 use crate::crypto::symmetric::{Decryptor, BufferedReaderDecryptor};
 use crate::message;
 use crate::message::MessageValidator;
@@ -2942,11 +2942,22 @@ impl MPI {
     /// See [Section 3.2 of RFC 4880] for details.
     ///
     ///   [Section 3.2 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-3.2
-    fn parse(
+    fn parse(name_len: &'static str,
+             name: &'static str,
+             php: &mut PacketHeaderParser<'_>) -> Result<Self> {
+        Ok(MPI::parse_common(name_len, name, php)?.into())
+    }
+
+    /// Parses an OpenPGP MPI.
+    ///
+    /// See [Section 3.2 of RFC 4880] for details.
+    ///
+    ///   [Section 3.2 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-3.2
+    fn parse_common(
         name_len: &'static str,
         name: &'static str,
         php: &mut PacketHeaderParser<'_>)
-                 -> Result<Self> {
+                 -> Result<Vec<u8>> {
         // This function is used to parse MPIs from unknown
         // algorithms, which may use an encoding unknown to us.
         // Therefore, we need to be extra careful only to consume the
@@ -2958,7 +2969,7 @@ impl MPI {
         if bits == 0 {
             // Now consume the data.
             php.parse_be_u16(name_len).expect("worked before");
-            return Ok(vec![].into());
+            return Ok(vec![]);
         }
 
         let bytes = (bits + 7) / 8;
@@ -2999,7 +3010,7 @@ impl MPI {
         php.field(name_len, 2);
         php.field(name, bytes);
 
-        Ok(value.into())
+        Ok(value)
     }
 }
 
@@ -3013,6 +3024,18 @@ impl<'a> Parse<'a, MPI> for MPI {
     }
 }
 
+impl ProtectedMPI {
+    /// Parses an OpenPGP MPI containing secrets.
+    ///
+    /// See [Section 3.2 of RFC 4880] for details.
+    ///
+    ///   [Section 3.2 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-3.2
+    fn parse(name_len: &'static str,
+             name: &'static str,
+             php: &mut PacketHeaderParser<'_>) -> Result<Self> {
+        Ok(MPI::parse_common(name_len, name, php)?.into())
+    }
+}
 impl PKESK {
     /// Parses the body of an PK-ESK packet.
     fn parse(mut php: PacketHeaderParser) -> Result<PacketParser> {
diff --git a/openpgp/src/parse/mpis.rs b/openpgp/src/parse/mpis.rs
index 6e2121a8..b1351ee8 100644
--- a/openpgp/src/parse/mpis.rs
+++ b/openpgp/src/parse/mpis.rs
@@ -10,7 +10,7 @@ use crate::{
     HashAlgorithm,
 };
 use crate::types::Curve;
-use crate::crypto::mpi::{self, MPI};
+use crate::crypto::mpi::{self, MPI, ProtectedMPI};
 use crate::parse::{
     PacketHeaderParser,
     Cookie,
@@ -222,62 +222,58 @@ impl mpi::SecretKeyMaterial {
         #[allow(deprecated)]
         let mpis: Result<Self> = match algo {
             RSAEncryptSign | RSAEncrypt | RSASign => {
-                let d = MPI::parse("rsa_secret_d_len", "rsa_secret_d", php)?;
-                let p = MPI::parse("rsa_secret_p_len", "rsa_secret_p", php)?;
-                let q = MPI::parse("rsa_secret_q_len", "rsa_secret_q", php)?;
-                let u = MPI::parse("rsa_secret_u_len", "rsa_secret_u", php)?;
-
                 Ok(mpi::SecretKeyMaterial::RSA {
-                    d: d.into(),
-                    p: p.into(),
-                    q: q.into(),
-                    u: u.into(),
+                    d: ProtectedMPI::parse(
+                        "rsa_secret_d_len", "rsa_secret_d", php)?,
+                    p: ProtectedMPI::parse(
+                        "rsa_secret_p_len", "rsa_secret_p", php)?,
+                    q: ProtectedMPI::parse(
+                        "rsa_secret_q_len", "rsa_secret_q", php)?,
+                    u: ProtectedMPI::parse(
+                        "rsa_secret_u_len", "rsa_secret_u", php)?,
                 })
             }
 
             DSA => {
-                let x = MPI::parse("dsa_secret_len", "dsa_secret", php)?;
-
                 Ok(mpi::SecretKeyMaterial::DSA {
-                    x: x.into(),
+                    x: ProtectedMPI::parse(
+                        "dsa_secret_len", "dsa_secret", php)?,
                 })
             }
 
             ElGamalEncrypt | ElGamalEncryptSign => {
-                let x = MPI::parse("elgamal_secret_len", "elgamal_secret",
-                                   php)?;
-
                 Ok(mpi::SecretKeyMaterial::ElGamal {
-                    x: x.into(),
+                    x: ProtectedMPI::parse(
+                        "elgamal_secret_len", "elgamal_secret", php)?,
                 })
             }
 
             EdDSA => {
                 Ok(mpi::SecretKeyMaterial::EdDSA {
-                    scalar: MPI::parse("eddsa_secret_len", "eddsa_secret", php)?
-                                .into()
+                    scalar: ProtectedMPI::parse(
+                        "eddsa_secret_len", "eddsa_secret", php)?,
                 })
             }
 
             ECDSA => {
                 Ok(mpi::SecretKeyMaterial::ECDSA {
-                    scalar: MPI::parse("ecdsa_secret_len", "ecdsa_secret", php)?
-                                .into()
+                    scalar: ProtectedMPI::parse(
+                        "ecdsa_secret_len", "ecdsa_secret", php)?,
                 })
             }
 
             ECDH => {
                 Ok(mpi::SecretKeyMaterial::ECDH {
-                    scalar: MPI::parse("ecdh_secret_len", "ecdh_secret", php)?
-                                .into()
+                    scalar: ProtectedMPI::parse(
+                        "ecdh_secret_len", "ecdh_secret", php)?,
                 })
             }
 
             Unknown(_) | Private(_) => {
                 let mut mpis = Vec::new();
-                while let Ok(mpi) = MPI::parse("unknown_len",
+                while let Ok(mpi) = ProtectedMPI::parse("unknown_len",
                                                "unknown", php) {
-                    mpis.push(mpi.into());
+                    mpis.push(mpi);
                 }
                 let rest = php.parse_bytes_eof("rest")?;