Require CRL number extension

Reject CRLs that lack id-ce-cRLNumber during parsing and retain the parsed CRL number.

Author: xyzzyz

src/crl/types.rs

@@ -175,6 +175,8 @@ pub struct OwnedCertRevocationList {
     signed_data: OwnedSignedData,
 
     next_update: UnixTime,
+
+    crl_number: Vec<u8>,
 }
 
 #[cfg(feature = "alloc")]
@@ -225,6 +227,8 @@ pub struct BorrowedCertRevocationList<'a> {
     revoked_certs: untrusted::Input<'a>,
 
     next_update: UnixTime,
+
+    crl_number: untrusted::Input<'a>,
 }
 
 impl<'a> BorrowedCertRevocationList<'a> {
@@ -263,6 +267,7 @@ impl<'a> BorrowedCertRevocationList<'a> {
                 .map(|idp| idp.as_slice_less_safe().to_vec()),
             revoked_certs,
             next_update: self.next_update,
+            crl_number: self.crl_number.as_slice_less_safe().to_vec(),
         })
     }
 
@@ -273,22 +278,26 @@ impl<'a> BorrowedCertRevocationList<'a> {
             match id {
                 // id-ce-cRLNumber 2.5.29.20 - RFC 5280 §5.2.3
                 Standard(20) => {
+                    // Duplicate cRLNumber extensions are invalid.
+                    if !self.crl_number.as_slice_less_safe().is_empty() {
+                        return Err(Error::ExtensionValueInvalid);
+                    }
+
                     // RFC 5280 §5.2.3:
                     //   CRL verifiers MUST be able to handle CRLNumber values
                     //   up to 20 octets.  Conforming CRL issuers MUST NOT use CRLNumber
                     //   values longer than 20 octets.
                     //
-                    extension.value.read_all(Error::InvalidCrlNumber, |der| {
-                        let crl_number = der::nonnegative_integer(der)
-                            .map_err(|_| Error::InvalidCrlNumber)?
-                            .as_slice_less_safe();
-                        if crl_number.len() <= 20 {
+                    self.crl_number = extension.value.read_all(Error::InvalidCrlNumber, |der| {
+                        let crl_number =
+                            der::nonnegative_integer(der).map_err(|_| Error::InvalidCrlNumber)?;
+                        if crl_number.as_slice_less_safe().len() <= 20 {
                             Ok(crl_number)
                         } else {
                             Err(Error::InvalidCrlNumber)
                         }
                     })?;
-                    // We enforce the cRLNumber is sensible, but don't retain the value for use.
+
                     Ok(())
                 }
 
@@ -408,6 +417,7 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> {
                 revoked_certs,
                 issuing_distribution_point: None,
                 next_update,
+                crl_number: untrusted::Input::from(&[]),
             };
 
             // RFC 5280 §5.1.2.7:
@@ -441,6 +451,13 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> {
                 },
             )?;
 
+            // From RFC 5280, Section 5.2.3:
+            //    CRL issuers conforming to this profile MUST include this extension in all
+            //    CRLs and MUST mark this extension as non-critical.
+            if crl.crl_number.as_slice_less_safe().is_empty() {
+                return Err(Error::MissingCrlNumber);
+            }
+
             Ok(crl)
         })?;
 
@@ -473,6 +490,7 @@ impl core::hash::Hash for BorrowedCertRevocationList<'_> {
             issuing_distribution_point,
             revoked_certs,
             next_update,
+            crl_number,
         } = self;
         signed_data.hash(state);
         issuer.as_slice_less_safe().hash(state);
@@ -481,6 +499,7 @@ impl core::hash::Hash for BorrowedCertRevocationList<'_> {
             .hash(state);
         revoked_certs.as_slice_less_safe().hash(state);
         next_update.hash(state);
+        crl_number.as_slice_less_safe().hash(state);
     }
 }
 
@@ -1313,21 +1332,67 @@ mod tests {
         assert!(OwnedCertRevocationList::from_der(crl).is_ok())
     }
 
+    #[test]
+    fn test_crl_missing_crl_number() {
+        assert_eq!(
+            BorrowedCertRevocationList::from_der(CRL_MISSING_CRL_NUMBER).err(),
+            Some(Error::MissingCrlNumber)
+        );
+    }
+
+    #[test]
+    fn test_crl_duplicate_crl_number() {
+        assert_eq!(
+            BorrowedCertRevocationList::from_der(CRL_DUPLICATE_CRL_NUMBER).err(),
+            Some(Error::ExtensionValueInvalid)
+        );
+    }
+
     #[test]
     fn test_crl_issuing_distribution_point_illegal_bit_string() {
-        let crl = &[
-            0x30, 0x65, 0x30, 0x50, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48,
-            0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00, 0x30, 0x0c, 0x31, 0x0a, 0x30, 0x08,
-            0x06, 0x03, 0x55, 0x04, 0x03, 0x13, 0x01, 0x41, 0x17, 0x0d, 0x32, 0x30, 0x30, 0x31,
-            0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0x17, 0x0d, 0x32, 0x31, 0x30,
-            0x31, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0xa0, 0x10, 0x30, 0x0e,
-            0x30, 0x0c, 0x06, 0x03, 0x55, 0x1d, 0x1c, 0x04, 0x05, 0x30, 0x03, 0x83, 0x01, 0x00,
-            0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b, 0x05,
-            0x00, 0x03, 0x02, 0x00, 0x00,
-        ];
         assert_eq!(
-            BorrowedCertRevocationList::from_der(crl).err(),
+            BorrowedCertRevocationList::from_der(CRL_WITH_REASON_PARTITIONED_IDP).err(),
             Some(Error::UnsupportedRevocationReasonsPartitioning)
         );
     }
+
+    const CRL_MISSING_CRL_NUMBER: &[u8] = &[
+        0x30, 0x81, 0xd0, 0x30, 0x79, 0x02, 0x01, 0x01, 0x30, 0x0a, 0x06, 0x08, 0x2a, 0x86, 0x48,
+        0xce, 0x3d, 0x04, 0x03, 0x02, 0x30, 0x25, 0x31, 0x23, 0x30, 0x21, 0x06, 0x03, 0x55, 0x04,
+        0x03, 0x0c, 0x1a, 0x63, 0x72, 0x6c, 0x2d, 0x6d, 0x69, 0x73, 0x73, 0x69, 0x6e, 0x67, 0x2d,
+        0x6e, 0x75, 0x6d, 0x62, 0x65, 0x72, 0x2e, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x17,
+        0x0d, 0x32, 0x30, 0x30, 0x31, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0x17,
+        0x0d, 0x32, 0x31, 0x30, 0x31, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0xa0,
+        0x23, 0x30, 0x21, 0x30, 0x1f, 0x06, 0x03, 0x55, 0x1d, 0x23, 0x04, 0x18, 0x30, 0x16, 0x80,
+        0x14, 0x40, 0x0e, 0x46, 0x72, 0x43, 0xeb, 0x8b, 0xac, 0x02, 0xdd, 0x09, 0xb6, 0xcc, 0x9d,
+        0x98, 0xe9, 0x5c, 0x34, 0xdc, 0xee, 0x30, 0x0a, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d,
+        0x04, 0x03, 0x02, 0x03, 0x47, 0x00, 0x30, 0x44, 0x02, 0x20, 0x44, 0xdb, 0x2f, 0x86, 0xb9,
+        0x9e, 0xb8, 0x9d, 0x4a, 0x4b, 0x42, 0xe3, 0x37, 0x97, 0xb5, 0x7a, 0x1f, 0x50, 0x8a, 0xae,
+        0x8a, 0x5c, 0x55, 0x29, 0xba, 0x5a, 0x5c, 0x4d, 0x53, 0x1d, 0x44, 0xce, 0x02, 0x20, 0x10,
+        0x13, 0x77, 0x62, 0x16, 0x7a, 0x89, 0x69, 0x2e, 0x76, 0xed, 0xb7, 0xde, 0x62, 0x03, 0x78,
+        0xc9, 0x50, 0xfb, 0xe7, 0xcd, 0xb8, 0x12, 0xa3, 0x83, 0x40, 0x2f, 0xd6, 0xf0, 0x5c, 0xc1,
+        0xb6,
+    ];
+
+    const CRL_DUPLICATE_CRL_NUMBER: &[u8] = &[
+        0x30, 0x6f, 0x30, 0x5a, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86,
+        0xf7, 0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00, 0x30, 0x0c, 0x31, 0x0a, 0x30, 0x08, 0x06, 0x03,
+        0x55, 0x04, 0x03, 0x13, 0x01, 0x41, 0x17, 0x0d, 0x32, 0x30, 0x30, 0x31, 0x30, 0x31, 0x30,
+        0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0x17, 0x0d, 0x32, 0x31, 0x30, 0x31, 0x30, 0x31, 0x30,
+        0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0xa0, 0x1a, 0x30, 0x18, 0x30, 0x0a, 0x06, 0x03, 0x55,
+        0x1d, 0x14, 0x04, 0x03, 0x02, 0x01, 0x01, 0x30, 0x0a, 0x06, 0x03, 0x55, 0x1d, 0x14, 0x04,
+        0x03, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01,
+        0x01, 0x0b, 0x05, 0x00, 0x03, 0x02, 0x00, 0x00,
+    ];
+
+    const CRL_WITH_REASON_PARTITIONED_IDP: &[u8] = &[
+        0x30, 0x71, 0x30, 0x5c, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86,
+        0xf7, 0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00, 0x30, 0x0c, 0x31, 0x0a, 0x30, 0x08, 0x06, 0x03,
+        0x55, 0x04, 0x03, 0x13, 0x01, 0x41, 0x17, 0x0d, 0x32, 0x30, 0x30, 0x31, 0x30, 0x31, 0x30,
+        0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0x17, 0x0d, 0x32, 0x31, 0x30, 0x31, 0x30, 0x31, 0x30,
+        0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0xa0, 0x1c, 0x30, 0x1a, 0x30, 0x0c, 0x06, 0x03, 0x55,
+        0x1d, 0x1c, 0x04, 0x05, 0x30, 0x03, 0x83, 0x01, 0x00, 0x30, 0x0a, 0x06, 0x03, 0x55, 0x1d,
+        0x14, 0x04, 0x03, 0x02, 0x01, 0x01, 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7,
+        0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00, 0x03, 0x02, 0x00, 0x00,
+    ];
 }

src/error.rs

@@ -90,6 +90,9 @@ pub enum Error {
     ///  - it was too long
     InvalidCrlNumber,
 
+    /// A CRL number extension was missing from a CRL.
+    MissingCrlNumber,
+
     /// A iPAddress name constraint was invalid:
     /// - it had a sparse network mask (ie, cannot be written in CIDR form).
     /// - it was too long or short
@@ -254,7 +257,7 @@ impl Error {
             Self::InvalidCertValidity => 190,
             Self::InvalidNetworkMaskConstraint => 180,
             Self::InvalidSerialNumber => 170,
-            Self::InvalidCrlNumber => 160,
+            Self::InvalidCrlNumber | Self::MissingCrlNumber => 160,
 
             // Errors related to unsupported features.
             Self::UnsupportedCrlSignatureAlgorithmForPublicKey(_)

tests/x509_limbo.rs

@@ -118,15 +118,14 @@ fn run_validation(tc: &Testcase) -> Result<(), String> {
         .crls
         .iter()
         .map(|pem| {
-            OwnedCertRevocationList::from_der(
-                CertificateRevocationListDer::from_pem_slice(pem.as_bytes())
-                    .expect("CRL PEM parse failed")
-                    .as_ref(),
-            )
-            .expect("CRL DER parse failed")
-            .into()
+            let der = CertificateRevocationListDer::from_pem_slice(pem.as_bytes())
+                .map_err(|e| format!("CRL PEM parse failed: {e}"))?;
+
+            OwnedCertRevocationList::from_der(der.as_ref())
+                .map(Into::into)
+                .map_err(|e| format!("CRL DER parse failed: {e}"))
         })
-        .collect::<Vec<_>>();
+        .collect::<Result<Vec<_>, _>>()?;
     let crls = crls.iter().collect::<Vec<_>>();
 
     let builder = if !crls.is_empty() {

third-party/x509-limbo/exceptions.json

@@ -24,11 +24,6 @@
     "actual": "FAILURE",
     "reason": "webpki intentionally rejects CA certificates in leaf position (CaUsedAsEndEntity)"
   },
-  "crl::crlnumber-missing": {
-    "expected": "FAILURE",
-    "actual": "SUCCESS",
-    "reason": "webpki does not enforce RFC 5280 requirement for CRLNumber extension presence"
-  },
   "crl::crlnumber-critical": {
     "expected": "FAILURE",
     "actual": "SUCCESS",