From c31ceb8dab94d2ea08879e36ad450547136ca2e1 Mon Sep 17 00:00:00 2001 From: "Neal H. Walfield" Date: Mon, 14 Dec 2020 22:45:43 +0100 Subject: openpgp: Simplify hash policies. - The standard policy currently has two policies related to hash algorithms: when a hash algorithm should be rejected for normal signatures, and when a hash algorithm should be rejected for revocation sigantures. - If we distinguish two security contexts, then we'll have four policies (the cross product). - If the currently state is not already unmanageable, then this certainly is. - Simplify this by using a single scalar to represent how long a revocation certificate using a broken hash should continue to be accepted. - This is probably sufficiently expressive in practice as this is a largely inexact science. And, if a more nuanced policy is required, it is always possible to wrap `StandardPolicy`. --- openpgp/src/cert.rs | 2 +- openpgp/src/policy.rs | 163 +++++++++++++++++++++++---------------- openpgp/src/policy/cutofflist.rs | 19 ++++- 3 files changed, 114 insertions(+), 70 deletions(-) diff --git a/openpgp/src/cert.rs b/openpgp/src/cert.rs index 648be37a..5a840ab5 100644 --- a/openpgp/src/cert.rs +++ b/openpgp/src/cert.rs @@ -5324,7 +5324,7 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g= // signatures. The standard policy reject SHA-1. So, use a // custom policy. let p = &P::new(); - let sha1 = p.hash_cutoffs(HashAlgorithm::SHA1).0.unwrap(); + let sha1 = p.hash_cutoff(HashAlgorithm::SHA1).unwrap(); let p = &P::at(sha1 - std::time::Duration::from_secs(1)); let cert = Cert::from_bytes( diff --git a/openpgp/src/policy.rs b/openpgp/src/policy.rs index 6aa027cd..4c66ef19 100644 --- a/openpgp/src/policy.rs +++ b/openpgp/src/policy.rs @@ -45,6 +45,7 @@ use crate::{ Tag, }, Result, + types, types::{ AEADAlgorithm, HashAlgorithm, @@ -555,8 +556,8 @@ pub struct StandardPolicy<'a> { time: Option, // Hash algorithms. - hash_algos_normal: NormalHashCutoffList, - hash_algos_revocation: RevocationHashCutoffList, + hash_algos: HashCutoffList, + hash_revocation_tolerance: types::Duration, // Critical subpacket tags. critical_subpackets: SubpacketTagCutoffList, @@ -591,7 +592,7 @@ impl<'a> From<&'a StandardPolicy<'a>> for Option<&'a dyn Policy> { } } -a_cutoff_list!(NormalHashCutoffList, HashAlgorithm, 12, +a_cutoff_list!(HashCutoffList, HashAlgorithm, 12, [ REJECT, // 0. Not assigned. Some(Timestamp::Y1997), // 1. MD5 @@ -606,21 +607,6 @@ a_cutoff_list!(NormalHashCutoffList, HashAlgorithm, 12, ACCEPT, // 10. SHA512 ACCEPT, // 11. SHA224 ]); -a_cutoff_list!(RevocationHashCutoffList, HashAlgorithm, 12, - [ - REJECT, // 0. Not assigned. - Some(Timestamp::Y2004), // 1. MD5 - Some(Timestamp::Y2020), // 2. SHA-1 - Some(Timestamp::Y2020), // 3. RIPE-MD/160 - REJECT, // 4. Reserved. - REJECT, // 5. Reserved. - REJECT, // 6. Reserved. - REJECT, // 7. Reserved. - ACCEPT, // 8. SHA256 - ACCEPT, // 9. SHA384 - ACCEPT, // 10. SHA512 - ACCEPT, // 11. SHA224 - ]); a_cutoff_list!(SubpacketTagCutoffList, SubpacketTag, 36, [ @@ -768,8 +754,11 @@ impl<'a> StandardPolicy<'a> { const EMPTY_LIST: &'static [&'static str] = &[]; Self { time: None, - hash_algos_normal: NormalHashCutoffList::Default(), - hash_algos_revocation: RevocationHashCutoffList::Default(), + hash_algos: HashCutoffList::Default(), + // There are 365.2425 days in a year. Use a reasonable + // approximation. + hash_revocation_tolerance: + types::Duration::seconds((7 * 365 + 2) * 24 * 60 * 60), critical_subpackets: SubpacketTagCutoffList::Default(), good_critical_notations: EMPTY_LIST, asymmetric_algos: AsymmetricAlgorithmCutoffList::Default(), @@ -826,14 +815,12 @@ impl<'a> StandardPolicy<'a> { /// Always considers `h` to be secure. pub fn accept_hash(&mut self, h: HashAlgorithm) { - self.hash_algos_normal.set(h, ACCEPT); - self.hash_algos_revocation.set(h, ACCEPT); + self.hash_algos.set(h, ACCEPT); } /// Always considers `h` to be insecure. pub fn reject_hash(&mut self, h: HashAlgorithm) { - self.hash_algos_normal.set(h, REJECT); - self.hash_algos_revocation.set(h, REJECT); + self.hash_algos.set(h, REJECT); } /// Considers `h` to be insecure starting at `normal` for normal @@ -893,25 +880,68 @@ impl<'a> StandardPolicy<'a> { /// /// Since RIPE-MD is structured similarly to SHA-1, we /// conservatively consider it to be broken as well. - pub fn reject_hash_at(&mut self, h: HashAlgorithm, - normal: N, revocation: R) - where N: Into>, - R: Into>, + pub fn reject_hash_at(&mut self, h: HashAlgorithm, t: T) + where T: Into>, { - self.hash_algos_normal.set( + self.hash_algos.set( h, - normal.into().and_then(system_time_cutoff_to_timestamp)); - self.hash_algos_revocation.set( - h, - revocation.into().and_then(system_time_cutoff_to_timestamp)); + t.into().and_then(system_time_cutoff_to_timestamp)); } /// Returns the cutoff times for the specified hash algorithm. - pub fn hash_cutoffs(&self, h: HashAlgorithm) - -> (Option, Option) + pub fn hash_cutoff(&self, h: HashAlgorithm) + -> Option { - (self.hash_algos_normal.cutoff(h).map(|t| t.into()), - self.hash_algos_revocation.cutoff(h).map(|t| t.into())) + self.hash_algos.cutoff(h).map(|t| t.into()) + } + + /// Sets the amount of time to continue to accept revocation + /// certificates after a hash algorithm should be rejected. + /// + /// Using [`StandardPolicy::reject_hash_at`], it is possible to + /// indicate when a hash algorithm's security has been + /// compromised, and, as such, should no longer be accepted. + /// + /// [`StandardPolicy::reject_hash_at`]: #method.reject_hash_at + /// + /// Applying this policy to revocation certificates can have some + /// unfortunate side effects. In particular, if a certificate has + /// been revoked using a revocation certificate that relies on a + /// broken hash algorithm, but the most recent self signature uses + /// a strong acceptable hash algorithm, then rejecting the + /// revocation certificate would mean considering the certificate + /// to not be revoked! This would be a catastrophe if the secret + /// key material were compromised. + /// + /// Unfortunately, this happens in practice. A common example + /// appears to be a certificate that has been updated many times, + /// and is then revoked using a revocation certificate that was + /// generated when the certificate was generated. + /// + /// Since the consequences of allowing an invalid revocation + /// certificate are significantly less severe (a denial of + /// service) than ignoring a valid revocation certificate + /// (compromised confidentiality, integrity, and authentication), + /// this option makes it possible to accept revocations using weak + /// hash algorithms longer than other types of signatures. + /// + /// By default, the standard policy accepts revocation + /// certificates seven years after the hash they are using was + /// initially compromised. + pub fn hash_revocation_tolerance(&mut self, d: D) + where D: Into + { + self.hash_revocation_tolerance = d.into(); + } + + /// Sets the amount of time to continue to accept revocation + /// certificates after a hash algorithm should be rejected. + /// + /// See [`StandardPolicy::hash_revocation_tolerance`] for details. + /// + /// [`StandardPolicy::hash_revocation_tolerance`]: #method.hash_revocation_tolerance + pub fn get_hash_revocation_tolerance(&self) -> types::Duration { + self.hash_revocation_tolerance } /// Always considers `s` to be secure. @@ -1119,25 +1149,29 @@ impl<'a> Policy for StandardPolicy<'a> { fn signature(&self, sig: &Signature, _sec: HashAlgoSecurity) -> Result<()> { let time = self.time.unwrap_or_else(Timestamp::now); - match sig.typ() { - t @ SignatureType::KeyRevocation - | t @ SignatureType::SubkeyRevocation - | t @ SignatureType::CertificationRevocation => - { - self.hash_algos_revocation.check(sig.hash_algo(), time) - .context(format!( - "Policy rejected revocation signature ({})", t))? - } - t => - { - self.hash_algos_normal.check(sig.hash_algo(), time) - .context(format!( - "Policy rejected non-revocation signature ({})", t))? - } + let rev = match sig.typ() { + SignatureType::KeyRevocation + | SignatureType::SubkeyRevocation + | SignatureType::CertificationRevocation => true, + _ => false, + }; + + if rev { + self + .hash_algos.check( + sig.hash_algo(), time, Some(self.hash_revocation_tolerance)) + .context(format!( + "Policy rejected revocation signature ({})", sig.typ()))? + } else { + self + .hash_algos.check( + sig.hash_algo(), time, None) + .context(format!( + "Policy rejected non-revocation signature ({})", sig.typ()))? } for csp in sig.hashed_area().iter().filter(|sp| sp.critical()) { - self.critical_subpackets.check(csp.tag(), time) + self.critical_subpackets.check(csp.tag(), time, None) .context("Policy rejected critical signature subpacket")?; if let SubpacketValue::NotationData(n) = csp.value() { if ! self.good_critical_notations.contains(&n.name()) { @@ -1221,25 +1255,25 @@ impl<'a> Policy for StandardPolicy<'a> { }; let time = self.time.unwrap_or_else(Timestamp::now); - self.asymmetric_algos.check(a, time) + self.asymmetric_algos.check(a, time, None) .context("Policy rejected encryption algorithm") } fn packet(&self, packet: &Packet) -> Result<()> { let time = self.time.unwrap_or_else(Timestamp::now); - self.packet_tags.check(packet.tag(), time) + self.packet_tags.check(packet.tag(), time, None) .context("Policy rejected packet type") } fn symmetric_algorithm(&self, algo: SymmetricAlgorithm) -> Result<()> { let time = self.time.unwrap_or_else(Timestamp::now); - self.symmetric_algos.check(algo, time) + self.symmetric_algos.check(algo, time, None) .context("Policy rejected symmetric encryption algorithm") } fn aead_algorithm(&self, algo: AEADAlgorithm) -> Result<()> { let time = self.time.unwrap_or_else(Timestamp::now); - self.aead_algos.check(algo, time) + self.aead_algos.check(algo, time, None) .context("Policy rejected authenticated encryption algorithm") } } @@ -1962,8 +1996,8 @@ mod test { let mut reject : StandardPolicy = StandardPolicy::new(); reject.reject_hash_at( algo, - SystemTime::now() + Duration::from_secs(SECS_IN_YEAR), SystemTime::now() + Duration::from_secs(SECS_IN_YEAR)); + reject.hash_revocation_tolerance(0); cert.primary_key().binding_signature(&reject, None)?; assert_match!(RevocationStatus::Revoked(_) = cert_revoked.revocation_status(&reject, None)); @@ -1972,8 +2006,8 @@ mod test { let mut reject : StandardPolicy = StandardPolicy::new(); reject.reject_hash_at( algo, - SystemTime::now() - Duration::from_secs(SECS_IN_YEAR), SystemTime::now() - Duration::from_secs(SECS_IN_YEAR)); + reject.hash_revocation_tolerance(0); assert!(cert.primary_key() .binding_signature(&reject, None).is_err()); assert_match!(RevocationStatus::NotAsFarAsWeKnow @@ -1984,8 +2018,8 @@ mod test { let mut reject : StandardPolicy = StandardPolicy::new(); reject.reject_hash_at( algo, - SystemTime::now() - Duration::from_secs(SECS_IN_YEAR), - SystemTime::now() + Duration::from_secs(SECS_IN_YEAR)); + SystemTime::now() - Duration::from_secs(SECS_IN_YEAR)); + reject.hash_revocation_tolerance(2 * SECS_IN_YEAR as u32); assert!(cert.primary_key() .binding_signature(&reject, None).is_err()); assert_match!(RevocationStatus::Revoked(_) @@ -1997,12 +2031,11 @@ mod test { assert!(algo_u8 != 0u8); reject.reject_hash_at( (algo_u8 - 1).into(), - SystemTime::now() - Duration::from_secs(SECS_IN_YEAR), SystemTime::now() - Duration::from_secs(SECS_IN_YEAR)); reject.reject_hash_at( (algo_u8 + 1).into(), - SystemTime::now() - Duration::from_secs(SECS_IN_YEAR), SystemTime::now() - Duration::from_secs(SECS_IN_YEAR)); + reject.hash_revocation_tolerance(0); cert.primary_key().binding_signature(&reject, None)?; assert_match!(RevocationStatus::Revoked(_) = cert_revoked.revocation_status(&reject, None)); @@ -2013,8 +2046,8 @@ mod test { let mut reject : StandardPolicy = StandardPolicy::new(); reject.reject_hash_at( algo, - SystemTime::UNIX_EPOCH - Duration::from_secs(SECS_IN_YEAR), SystemTime::UNIX_EPOCH - Duration::from_secs(SECS_IN_YEAR)); + reject.hash_revocation_tolerance(0); assert!(cert.primary_key() .binding_signature(&reject, None).is_err()); assert_match!(RevocationStatus::NotAsFarAsWeKnow @@ -2026,8 +2059,8 @@ mod test { let mut reject : StandardPolicy = StandardPolicy::new(); reject.reject_hash_at( algo, - SystemTime::UNIX_EPOCH + Duration::from_secs(500 * SECS_IN_YEAR), SystemTime::UNIX_EPOCH + Duration::from_secs(500 * SECS_IN_YEAR)); + reject.hash_revocation_tolerance(0); cert.primary_key().binding_signature(&reject, None)?; assert_match!(RevocationStatus::Revoked(_) = cert_revoked.revocation_status(&reject, None)); diff --git a/openpgp/src/policy/cutofflist.rs b/openpgp/src/policy/cutofflist.rs index 6c1933ea..47d1d3e5 100644 --- a/openpgp/src/policy/cutofflist.rs +++ b/openpgp/src/policy/cutofflist.rs @@ -6,6 +6,7 @@ use crate::{ Error, Result, types::Timestamp, + types::Duration, }; // A `const fn` function can only use a subset of Rust's @@ -152,9 +153,17 @@ impl CutoffList } // Checks whether the `a` is safe to use at time `time`. + // + // `tolerance` is added to the cutoff time. #[inline] - pub(super) fn check(&self, a: A, time: Timestamp) -> Result<()> { + pub(super) fn check(&self, a: A, time: Timestamp, + tolerance: Option) + -> Result<()> + { if let Some(cutoff) = self.cutoff(a.clone()) { + let cutoff = cutoff + .checked_add(tolerance.unwrap_or(Duration::seconds(0))) + .unwrap_or(Timestamp::MAX); if time >= cutoff { Err(Error::PolicyViolation( a.to_string(), Some(cutoff.into())).into()) @@ -226,7 +235,9 @@ macro_rules! a_cutoff_list { } } - fn check(&self, a: $algo, time: Timestamp) -> Result<()> { + fn check(&self, a: $algo, time: Timestamp, d: Option) + -> Result<()> + { use crate::policy::cutofflist::VecOrSlice; match self { @@ -237,10 +248,10 @@ macro_rules! a_cutoff_list { CutoffList { cutoffs: VecOrSlice::Slice(&Self::DEFAULTS[..]), _a: std::marker::PhantomData, - }.check(a, time) + }.check(a, time, d) } - $name::Custom(ref l) => l.check(a, time), + $name::Custom(ref l) => l.check(a, time, d), } } } -- cgit v1.2.3