diff options
author | Justus Winter <justus@sequoia-pgp.org> | 2023-03-07 10:52:25 +0100 |
---|---|---|
committer | Justus Winter <justus@sequoia-pgp.org> | 2023-03-07 11:52:53 +0100 |
commit | 85af379944d11681895867324f51d9b4ac1e0258 (patch) | |
tree | 994f004bc6fd70d2bf4ea508aba33ed5ef3f6313 | |
parent | de5f99ba931d6d9d21450e0aa16b793bff1222e1 (diff) |
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).
-rw-r--r-- | openpgp/NEWS | 5 | ||||
-rw-r--r-- | openpgp/src/packet/key.rs | 16 | ||||
-rw-r--r-- | openpgp/src/parse/mpis.rs | 31 |
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<'_>, |