diff options
| -rw-r--r-- | openpgp/NEWS | 1 | ||||
| -rw-r--r-- | openpgp/src/crypto/s2k.rs | 18 | ||||
| -rw-r--r-- | openpgp/src/parse.rs | 55 | ||||
| -rw-r--r-- | openpgp/src/serialize.rs | 30 | ||||
| -rw-r--r-- | openpgp/tests/data/keys/hardware-backed-secret.pgp | 14 |
5 files changed, 97 insertions, 21 deletions
diff --git a/openpgp/NEWS b/openpgp/NEWS index 7b0d62f9..95d629e4 100644 --- a/openpgp/NEWS +++ b/openpgp/NEWS @@ -5,6 +5,7 @@ * Changes in 1.20.0 ** New functionality + - S2K::Implicit - Signature::verify_signature * Changes in 1.19.0 ** Notable fixes diff --git a/openpgp/src/crypto/s2k.rs b/openpgp/src/crypto/s2k.rs index 9b7c24d6..fc4bdc90 100644 --- a/openpgp/src/crypto/s2k.rs +++ b/openpgp/src/crypto/s2k.rs @@ -78,6 +78,18 @@ pub enum S2K { hash: HashAlgorithm }, + /// Simply hashes the password using MD5 + /// + /// This mechanism uses neither iteration to increase the time it + /// takes to derive the key from the password nor does it salt the + /// password, as well as using a very weak and fast hash + /// algorithm. This makes dictionary attacks more feasible. + /// + /// This mechanism has been deprecated in RFC 2440. Do not use + /// this variant. + #[deprecated(note = "Use `S2K::Iterated`.")] + Implicit, + /// Private S2K algorithm. Private { /// Tag identifying the private algorithm. @@ -224,6 +236,7 @@ impl S2K { hash.update(&data[0..tail]); } } + S2K::Implicit | S2K::Unknown { .. } | &S2K::Private { .. } => unreachable!(), } @@ -234,6 +247,9 @@ impl S2K { Ok(ret.into()) }), + S2K::Implicit => S2K::Simple { + hash: HashAlgorithm::MD5, + }.derive_key(password, key_size), S2K::Unknown { tag, .. } | S2K::Private { tag, .. } => Err(Error::MalformedPacket( format!("Unknown S2K type {:#x}", tag)).into()), @@ -248,6 +264,7 @@ impl S2K { Simple { .. } | Salted { .. } | Iterated { .. } + | Implicit => true, S2K::Private { .. } | S2K::Unknown { .. } @@ -353,6 +370,7 @@ impl fmt::Display for S2K { salt[4], salt[5], salt[6], salt[7], hash_bytes)) } + S2K::Implicit => f.write_str("Implicit S2K"), S2K::Private { tag, parameters } => if let Some(p) = parameters.as_ref() { write!(f, "Private/Experimental S2K {}:{:?}", tag, p) diff --git a/openpgp/src/parse.rs b/openpgp/src/parse.rs index 6bcbf869..9ebea413 100644 --- a/openpgp/src/parse.rs +++ b/openpgp/src/parse.rs @@ -2358,14 +2358,19 @@ impl Key4<key::UnspecifiedParts, key::UnspecifiedRole> Some(mpi::SecretKeyChecksum::Sum16))); sec.into() } - // Encrypted & MD5 for key derivation: unsupported - 1..=253 => { - return php.fail("unsupported secret key encryption"); - } - // Encrypted, S2K & SHA-1 checksum - 254 | 255 => { - let sk: SymmetricAlgorithm = php_try!(php.parse_u8("sym_algo")).into(); - let s2k = php_try!(S2K::parse_v4(&mut php)); + // Encrypted, whether we support the S2K method or not. + _ => { + let sk: SymmetricAlgorithm = match s2k_usage { + 254 | 255 => + php_try!(php.parse_u8("sym_algo")).into(), + _ => s2k_usage.into(), + }; + let s2k = match s2k_usage { + 254 | 255 => php_try!(S2K::parse_v4(&mut php)), + _ => { + #[allow(deprecated)] S2K::Implicit + }, + }; let s2k_supported = s2k.is_supported(); let cipher = php_try!(php.parse_bytes_eof("encrypted_mpis")) @@ -2373,10 +2378,10 @@ impl Key4<key::UnspecifiedParts, key::UnspecifiedRole> crate::packet::key::Encrypted::new_raw( s2k, sk, - if s2k_usage == 254 { - Some(mpi::SecretKeyChecksum::SHA1) - } else { - Some(mpi::SecretKeyChecksum::Sum16) + match s2k_usage { + 254 => Some(mpi::SecretKeyChecksum::SHA1), + 255 => Some(mpi::SecretKeyChecksum::Sum16), + _ => Some(mpi::SecretKeyChecksum::Sum16), }, if s2k_supported { Ok(cipher) @@ -6757,4 +6762,30 @@ yCsBO81bKqlfklugX5yRX5qTopuXX6KbWpFZXKJXUlGSetb4dXm+gYFBCRcA Ok(()) } + /// Tests for issue 1095, parsing a secret key packet with an + /// unknown S2K mechanism. + #[test] + fn key_unknown_s2k() -> Result<()> { + let mut ppr = PacketParser::from_bytes( + crate::tests::key("hardware-backed-secret.pgp"))?; + let mut i = 0; + while let PacketParserResult::Some(pp) = ppr { + if i == 0 { + assert!(matches!(&pp.packet, Packet::SecretKey(_))); + } + if i == 3 { + assert!(matches!(&pp.packet, Packet::SecretSubkey(_))); + } + + // Make sure it roundtrips. + let p = &pp.packet; + let v = p.to_vec()?; + let q = Packet::from_bytes(&v)?; + assert_eq!(p, &q); + + ppr = pp.recurse()?.1; + i += 1; + } + Ok(()) + } } diff --git a/openpgp/src/serialize.rs b/openpgp/src/serialize.rs index 6dcc1845..89ca0edf 100644 --- a/openpgp/src/serialize.rs +++ b/openpgp/src/serialize.rs @@ -1319,6 +1319,7 @@ impl Marshal for S2K { w.write_all(&salt[..])?; w.write_all(&[S2K::encode_count(hash_bytes)?])?; } + S2K::Implicit => (), S2K::Private { tag, parameters } | S2K::Unknown { tag, parameters} => { w.write_all(&[*tag])?; @@ -1339,6 +1340,7 @@ impl MarshalInto for S2K { &S2K::Simple{ .. } => 2, &S2K::Salted{ .. } => 2 + 8, &S2K::Iterated{ .. } => 2 + 8 + 1, + S2K::Implicit => 0, S2K::Private { parameters, .. } | S2K::Unknown { parameters, .. } => 1 + parameters.as_ref().map(|p| p.len()).unwrap_or(0), @@ -2001,14 +2003,21 @@ impl<P, R> Marshal for Key4<P, R> mpis.serialize_with_checksum(o, SecretKeyChecksum::Sum16) })?, SecretKeyMaterial::Encrypted(ref e) => { - // S2K usage. - write_byte(o, match e.checksum() { - Some(SecretKeyChecksum::SHA1) => 254, - Some(SecretKeyChecksum::Sum16) => 255, - None => return Err(Error::InvalidOperation( - "In Key4 packets, encrypted secret keys must be \ - checksummed".into()).into()), - })?; + // S2K usage + #[allow(deprecated)] + if let S2K::Implicit = e.s2k() { + // When the legacy implicit S2K mechanism is + // in use, the symmetric algorithm octet below + // takes the place of the S2K usage octet. + } else { + write_byte(o, match e.checksum() { + Some(SecretKeyChecksum::SHA1) => 254, + Some(SecretKeyChecksum::Sum16) => 255, + None => return Err(Error::InvalidOperation( + "In Key4 packets, encrypted secret keys must be \ + checksummed".into()).into()), + })?; + } write_byte(o, e.algo().into())?; e.s2k().serialize(o)?; o.write_all(e.raw_ciphertext())?; @@ -2024,6 +2033,7 @@ impl<P, R> NetLength for Key4<P, R> where P: key::KeyParts, R: key::KeyRole, { + #[allow(deprecated)] fn net_len(&self) -> usize { let have_secret_key = P::significant_secrets() && self.has_secret(); @@ -2037,7 +2047,9 @@ impl<P, R> NetLength for Key4<P, R> u.map(|mpis| mpis.serialized_len()) + 2, // Two octet checksum. SecretKeyMaterial::Encrypted(ref e) => - 1 + e.s2k().serialized_len() + matches!(e.s2k(), S2K::Implicit) + .then_some(0).unwrap_or(1) + + e.s2k().serialized_len() + e.raw_ciphertext().len(), } } else { diff --git a/openpgp/tests/data/keys/hardware-backed-secret.pgp b/openpgp/tests/data/keys/hardware-backed-secret.pgp new file mode 100644 index 00000000..b211205a --- /dev/null +++ b/openpgp/tests/data/keys/hardware-backed-secret.pgp @@ -0,0 +1,14 @@ +-----BEGIN PGP PRIVATE KEY BLOCK----- +Comment: from https://gitlab.com/dkg/openpgp-hardware-secrets/-/merge_requests/2 + +xTQEZgWtcxYJKwYBBAHaRw8BAQdAlLK6UPQsVHR2ETk1SwVIG3tBmpiEtikYYlCy +1TIiqzb8zR08aGFyZHdhcmUtc2VjcmV0QGV4YW1wbGUub3JnPsKNBBAWCAA1AhkB +BQJmBa1zAhsDCAsJCAcKDQwLBRUKCQgLAhYCFiEEXlP8Tur0WZR+f0I33/i9Uh4O +HEkACgkQ3/i9Uh4OHEnryAD8CzH2ajJvASp46ApfI4pLPY57rjBX++d/2FQPRyqG +HJUA/RLsNNgxiFYmK5cjtQe2/DgzWQ7R6PxPC6oa3XM7xPcCxzkEZgWtcxIKKwYB +BAGXVQEFAQEHQE1YXOKeaklwG01Yab4xopP9wbu1E+pCrP1xQpiFZW5KAwEIB/zC +eAQYFggAIAUCZgWtcwIbDBYhBF5T/E7q9FmUfn9CN9/4vVIeDhxJAAoJEN/4vVIe +DhxJVTgA/1WaFrKdP3AgL0Ffdooc5XXbjQsj0uHo6FZSHRI4pchMAQCyJnKQ3RvW +/0gm41JCqImyg2fxWG4hY0N5Q7Rc6PyzDQ== +=3w/O +-----END PGP PRIVATE KEY BLOCK----- |