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`.

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<Timestamp>,
 
     // 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<N, R>(&mut self, h: HashAlgorithm,
-                                normal: N, revocation: R)
-        where N: Into<Option<SystemTime>>,
-              R: Into<Option<SystemTime>>,
+    pub fn reject_hash_at<T>(&mut self, h: HashAlgorithm, t: T)
+        where T: Into<Option<SystemTime>>,
     {
-        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<SystemTime>, Option<SystemTime>)
+    pub fn hash_cutoff(&self, h: HashAlgorithm)
+        -> Option<SystemTime>
     {
-        (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<D>(&mut self, d: D)
+        where D: Into<types::Duration>
+    {
+        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<A> CutoffList<A>
     }
 
     // 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<Duration>)
+        -> 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<types::Duration>)
+                -> 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),
                 }
             }
         }