diff options
author | Justus Winter <justus@sequoia-pgp.org> | 2023-03-14 14:28:07 +0100 |
---|---|---|
committer | Justus Winter <justus@sequoia-pgp.org> | 2023-03-14 16:48:14 +0100 |
commit | 67819944a69a7faba0d1cf400facaffce6da01d5 (patch) | |
tree | 5621b075255d679c56f43de17d8123ab36e5df68 | |
parent | 4989669caddf46613d17ccc08b5471eeaa25ac43 (diff) |
openpgp: Avoid leaking secrets when parsing secret key material.
-rw-r--r-- | openpgp/src/crypto/mpi.rs | 8 | ||||
-rw-r--r-- | openpgp/src/parse.rs | 33 | ||||
-rw-r--r-- | openpgp/src/parse/mpis.rs | 46 |
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")?; |