From 48506fdf60ab46240b918deccffc331e03bfd48e Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Fri, 2 Oct 2020 10:06:24 +0200 Subject: openpgp: Mark subpackets as authenticated when verifying signatures. - See #572. --- openpgp/src/packet/signature.rs | 113 +++++++++++++++++++++++++++++- openpgp/src/packet/signature/subpacket.rs | 4 ++ 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/openpgp/src/packet/signature.rs b/openpgp/src/packet/signature.rs index d0655ef6..6b2d62d2 100644 --- a/openpgp/src/packet/signature.rs +++ b/openpgp/src/packet/signature.rs @@ -2058,7 +2058,31 @@ impl Signature { "Signature has no creation time subpacket".into()).into()); } - key.verify(self, digest.as_ref()) + let result = key.verify(self, digest.as_ref()); + if result.is_ok() { + // Mark information in this signature as authenticated. + + // The hashed subpackets are authenticated by the + // signature. + self.hashed_area_mut().iter_mut().for_each(|p| { + p.set_authenticated(true); + }); + + // The self-authenticating unhashed subpackets are + // authenticated by the key's identity. + self.unhashed_area_mut().iter_mut().for_each(|p| { + use subpacket::SubpacketValue; + let authenticated = match p.value() { + SubpacketValue::Issuer(id) => + id == &key.keyid(), + SubpacketValue::IssuerFingerprint(fp) => + fp == &key.fingerprint(), + _ => false, + }; + p.set_authenticated(authenticated); + }); + } + result } /// Verifies the signature over text or binary documents using @@ -2262,12 +2286,20 @@ impl Signature { // The signature is good, but we may still need to verify the // back sig. if self.key_flags().map(|kf| kf.for_signing()).unwrap_or(false) { - if let Some(backsig) = self.embedded_signature_mut() { + let result = if let Some(backsig) = self.embedded_signature_mut() { backsig.verify_primary_key_binding(pk, subkey) } else { Err(Error::BadSignature( "Primary key binding signature missing".into()).into()) + }; + if result.is_ok() { + // Mark the subpacket as authenticated by the embedded + // signature. + self.subpacket_mut(subpacket::SubpacketTag::EmbeddedSignature) + .expect("must exist, we verified the signature above") + .set_authenticated(true); } + result } else { // No backsig required. Ok(()) @@ -3054,4 +3086,81 @@ mod test { there is a bug. Please get in contact.", 2 * SIG_BACKDATE_BY); } + + /// Checks that subpackets are marked as authentic on signature + /// verification. + #[test] + fn subpacket_authentication() -> Result<()> { + use subpacket::{Subpacket, SubpacketValue}; + + // We'll study this certificate, because it contains a + // signing-capable subkey. + let mut pp = crate::PacketPile::from_bytes(crate::tests::key( + "emmelie-dorothea-dina-samantha-awina-ed25519.pgp"))?; + assert_eq!(pp.children().count(), 5); + + // The signatures have not been verified, hence no subpacket + // is authenticated. + if let Some(Packet::Signature(sig)) = pp.path_ref_mut(&[4]) { + assert!(sig.hashed_area().iter().all(|p| ! p.authenticated())); + assert!(sig.unhashed_area().iter().all(|p| ! p.authenticated())); + + // Add a bogus issuer subpacket. + sig.unhashed_area_mut().add(Subpacket::new( + SubpacketValue::Issuer("AAAA BBBB CCCC DDDD".parse()?), + false)?)?; + } else { + panic!("expected a signature"); + } + + // Break the userid binding signature. + if let Some(Packet::Signature(sig)) = pp.path_ref_mut(&[2]) { + assert!(sig.hashed_area().iter().all(|p| ! p.authenticated())); + assert!(sig.unhashed_area().iter().all(|p| ! p.authenticated())); + + // Add a bogus issuer subpacket to the hashed area + // breaking the signature. + sig.hashed_area_mut().add(Subpacket::new( + SubpacketValue::Issuer("AAAA BBBB CCCC DDDD".parse()?), + false)?)?; + } else { + panic!("expected a signature"); + } + + // Parse into cert verifying the signatures. + use std::convert::TryFrom; + let cert = Cert::try_from(pp)?; + assert_eq!(cert.bad_signatures().len(), 1); + assert_eq!(cert.keys().subkeys().count(), 1); + let subkey = cert.keys().subkeys().nth(0).unwrap(); + assert_eq!(subkey.self_signatures().len(), 1); + + // All the authentic information in the self signature has + // been authenticated by the verification process. + let sig = &subkey.self_signatures()[0]; + assert!(sig.hashed_area().iter().all(|p| p.authenticated())); + // All but our fake issuer information. + assert!(sig.unhashed_area().iter().all(|p| { + if let SubpacketValue::Issuer(id) = p.value() { + if id == &"AAAA BBBB CCCC DDDD".parse().unwrap() { + // Our fake id... + true + } else { + p.authenticated() + } + } else { + p.authenticated() + } + })); + // Check the subpackets in the embedded signature. + let sig = sig.embedded_signature().unwrap(); + assert!(sig.hashed_area().iter().all(|p| p.authenticated())); + assert!(sig.unhashed_area().iter().all(|p| p.authenticated())); + + // No information in the bad signature has been authenticated. + let sig = &cert.bad_signatures()[0]; + assert!(sig.hashed_area().iter().all(|p| ! p.authenticated())); + assert!(sig.unhashed_area().iter().all(|p| ! p.authenticated())); + Ok(()) + } } diff --git a/openpgp/src/packet/signature/subpacket.rs b/openpgp/src/packet/signature/subpacket.rs index b084ebd4..65d0b815 100644 --- a/openpgp/src/packet/signature/subpacket.rs +++ b/openpgp/src/packet/signature/subpacket.rs @@ -647,6 +647,10 @@ impl SubpacketArea { self.packets.iter() } + pub(crate) fn iter_mut(&mut self) -> impl Iterator { + self.packets.iter_mut() + } + /// Returns a reference to the *last* instance of the specified /// subpacket, if any. /// -- cgit v1.2.3