openpgp: Prevent secrets from leaking into the BufferedReader stack.

  - When parsing secrets using the BufferedReader protocol, they may
    leak into buffers of the readers in the BufferedReader stack.
    This is is most problematic when parsing SecretKeyMaterial.

  - Deprecate SecretKeyMaterial::parse* in favor of variants that
    operate on bytes.  Then, we can use the memory-backed
    BufferedReader which does not introduce additional buffering (and
    neither does the Dub reader used in the PackedHeaderParser).

3 files changed, 47 insertions, 5 deletions

diff --git a/openpgp/NEWS b/openpgp/NEWS
index 99ba887e..7a6080ed 100644
--- a/openpgp/NEWS
+++ b/openpgp/NEWS
@@ -6,7 +6,12 @@
 * Changes in 1.14.0
 ** New functionality
    - crypto::mem::Protected::new
+   - crypto::mpi::SecretKeyMaterial::from_bytes
+   - crypto::mpi::SecretKeyMaterial::from_bytes_with_checksum
    - policy::AsymmetricAlgorithm::BrainpoolP384
+** Deprecated functionality
+   - crypto::mpi::SecretKeyMaterial::parse
+   - crypto::mpi::SecretKeyMaterial::parse_with_checksum
 * Changes in 1.13.0
 ** New cryptographic backends
    - We added a backend that uses OpenSSL.
diff --git a/openpgp/src/packet/key.rs b/openpgp/src/packet/key.rs
index 83d563f4..23d6f6f8 100644
--- a/openpgp/src/packet/key.rs
+++ b/openpgp/src/packet/key.rs
@@ -1452,7 +1452,7 @@ impl Unencrypted {
     {
         self.mpis.map(|plaintext| {
             let algo: PublicKeyAlgorithm = plaintext[0].into();
-            let mpis = mpi::SecretKeyMaterial::parse(algo, &plaintext[1..])
+            let mpis = mpi::SecretKeyMaterial::from_bytes(algo, &plaintext[1..])
                 .expect("Decrypted secret key is malformed");
             fun(&mpis)
         })
@@ -1626,15 +1626,21 @@ impl Encrypted {
         use crate::crypto::symmetric::Decryptor;
 
         let key = self.s2k.derive_key(password, self.algo.key_size()?)?;
-        let cur = Cursor::new(self.ciphertext()?);
+        let ciphertext = self.ciphertext()?;
+        let cur = Cursor::new(ciphertext);
         let mut dec = Decryptor::new(self.algo, &key, cur)?;
 
         // Consume the first block.
-        let mut trash = vec![0u8; self.algo.block_size()?];
+        let block_size = self.algo.block_size()?;
+        let mut trash = mem::Protected::new(block_size);
         dec.read_exact(&mut trash)?;
 
-        mpi::SecretKeyMaterial::parse_with_checksum(
-            pk_algo, &mut dec, self.checksum.unwrap_or_default())
+        // Read the secret key.
+        let mut secret = mem::Protected::new(ciphertext.len() - block_size);
+        dec.read_exact(&mut secret)?;
+
+        mpi::SecretKeyMaterial::from_bytes_with_checksum(
+            pk_algo, &secret, self.checksum.unwrap_or_default())
             .map(|m| m.into())
     }
 }
diff --git a/openpgp/src/parse/mpis.rs b/openpgp/src/parse/mpis.rs
index 504f08ce..6e2121a8 100644
--- a/openpgp/src/parse/mpis.rs
+++ b/openpgp/src/parse/mpis.rs
@@ -151,6 +151,9 @@ impl mpi::SecretKeyMaterial {
     /// Parses secret key MPIs for `algo` plus their SHA1 checksum.
     ///
     /// Fails if the checksum is wrong.
+    #[deprecated(
+        since = "1.14.0",
+        note = "Leaks secrets into the heap, use [`SecretKeyMaterial::from_bytes_with_checksum`]")]
     pub fn parse_with_checksum<R: Read + Send + Sync>(algo: PublicKeyAlgorithm,
                                         reader: R,
                                         checksum: mpi::SecretKeyChecksum)
@@ -161,11 +164,27 @@ impl mpi::SecretKeyMaterial {
         Self::_parse(algo, &mut php, Some(checksum))
     }
 
+    /// Parses secret key MPIs for `algo` plus their SHA1 checksum.
+    ///
+    /// Fails if the checksum is wrong.
+    pub fn from_bytes_with_checksum(algo: PublicKeyAlgorithm,
+                                    bytes: &[u8],
+                                    checksum: mpi::SecretKeyChecksum)
+                                    -> Result<Self> {
+        let bio = buffered_reader::Memory::with_cookie(
+            bytes, Cookie::default());
+        let mut php = PacketHeaderParser::new_naked(bio.as_boxed());
+        Self::_parse(algo, &mut php, Some(checksum))
+    }
+
     /// Parses a set of OpenPGP MPIs representing a secret key.
     ///
     /// See [Section 3.2 of RFC 4880] for details.
     ///
     ///   [Section 3.2 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-3.2
+    #[deprecated(
+        since = "1.14.0",
+        note = "Leaks secrets into the heap, use [`SecretKeyMaterial::from_bytes`]")]
     pub fn parse<R: Read + Send + Sync>(algo: PublicKeyAlgorithm, reader: R) -> Result<Self>
     {
         let bio = buffered_reader::Generic::with_cookie(
@@ -179,6 +198,18 @@ impl mpi::SecretKeyMaterial {
     /// See [Section 3.2 of RFC 4880] for details.
     ///
     ///   [Section 3.2 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-3.2
+    pub fn from_bytes(algo: PublicKeyAlgorithm, buf: &[u8]) -> Result<Self> {
+        let bio = buffered_reader::Memory::with_cookie(
+            buf, Cookie::default());
+        let mut php = PacketHeaderParser::new_naked(bio.as_boxed());
+        Self::_parse(algo, &mut php, None)
+    }
+
+    /// Parses a set of OpenPGP MPIs representing a secret key.
+    ///
+    /// See [Section 3.2 of RFC 4880] for details.
+    ///
+    ///   [Section 3.2 of RFC 4880]: https://tools.ietf.org/html/rfc4880#section-3.2
     pub(crate) fn _parse(
         algo: PublicKeyAlgorithm,
         php: &mut PacketHeaderParser<'_>,