Don't canonicalise serial numbers in CRL entries

Certificate parsing doesn't do this, which means certificates with negative or
top-bit-set serials cannot be looked up in a CRL, and therefore were not
reported as revoked even if the CRL stated they were.

CRLs parsing outlawed negative serials, so this was a functionality gap
but not a security issue.

But for top-bit-set serials, this would mean the `cert::Cert::serial`
was (eg) `00 80 01` but the `crl::BorrowedRevokedCert::serial_number`
had `80 01`.

This commit also allows and tests negative serials in CRL entries.

src/crl.rs

@@ -12,6 +12,7 @@
 // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
+use crate::cert::lenient_certificate_serial_number;
 use crate::der::Tag;
 use crate::x509::{remember_extension, set_extension_once, Extension};
 use crate::{der, signed_data, Error, SignatureAlgorithm, Time};
@@ -448,9 +449,9 @@ impl<'a> BorrowedRevokedCert<'a> {
             //    gracefully handle such certificates.
             // Like the handling in cert.rs we choose to be lenient here, not enforcing the length
             // of a CRL revoked certificate's serial number is less than 20 octets in encoded form.
-            let serial_number = ring::io::der::positive_integer(der)
+            let serial_number = lenient_certificate_serial_number(der)
                 .map_err(|_| Error::InvalidSerialNumber)?
-                .big_endian_without_leading_zero();
+                .as_slice_less_safe();
 
             let revocation_date = der::time_choice(der)?;
 

tests/crl_tests.rs

@@ -2,6 +2,13 @@ use webpki::{BorrowedCertRevocationList, CertRevocationList, Error};
 
 const REVOKED_SERIAL: &[u8] = &[0x03, 0xAE, 0x51, 0xDB, 0x51, 0x15, 0x5A, 0x3C];
 
+const REVOKED_SERIAL_NEGATIVE: &[u8] = &[0xfd, 0x78, 0xa8, 0x4e];
+
+// because cert::Cert::serial is the raw DER integer encoding, this includes a leading
+// zero if one is required to make the twos complement representation positive.  in this case
+// one is required.
+const REVOKED_SERIAL_WITH_TOP_BIT_SET: &[u8] = &[0x00, 0x80, 0xfe, 0xed, 0xf0, 0x0d];
+
 #[test]
 fn parse_valid_crl() {
     // We should be able to parse a valid CRL without error, and find the revoked serial.
@@ -105,20 +112,49 @@ fn parse_too_long_crl_number_crl() {
 
 #[test]
 fn parse_entry_negative_serial_crl() {
-    // Parsing a CRL that includes a revoked entry with a negative serial number shouldn't error
-    // up-front since the error is with a revoked entry.
     let crl = include_bytes!("crls/crl.negative.serial.der");
     let crl = BorrowedCertRevocationList::from_der(&crl[..]).unwrap();
 
-    // but searching for a revoked cert should error due to the entry with the negative serial number.
-    let res = crl.find_serial(REVOKED_SERIAL);
-    assert!(matches!(res, Err(Error::InvalidSerialNumber)));
+    assert!(crl
+        .find_serial(REVOKED_SERIAL)
+        .expect("looking for REVOKED_SERIAL failed")
+        .is_none());
+    assert!(crl
+        .find_serial(REVOKED_SERIAL_NEGATIVE)
+        .expect("looking for REVOKED_SERIAL_NEGATIVE failed")
+        .is_some());
 
     #[cfg(feature = "alloc")]
     {
-        // Constructing an owned CRL should error up-front since it will process the revoked certs.
-        let res = crl.to_owned();
-        assert!(matches!(res, Err(Error::InvalidSerialNumber)));
+        let crl = crl.to_owned().unwrap();
+        assert!(crl
+            .find_serial(REVOKED_SERIAL)
+            .expect("looking for REVOKED_SERIAL failed")
+            .is_none());
+        assert!(crl
+            .find_serial(REVOKED_SERIAL_NEGATIVE)
+            .expect("looking for REVOKED_SERIAL_NEGATIVE failed")
+            .is_some());
+    }
+}
+
+#[test]
+fn parse_entry_topbit_serial_crl() {
+    let crl = include_bytes!("crls/crl.topbit.serial.der");
+    let crl = BorrowedCertRevocationList::from_der(&crl[..]).unwrap();
+
+    assert!(crl
+        .find_serial(REVOKED_SERIAL_WITH_TOP_BIT_SET)
+        .expect("failed to look for REVOKED_SERIAL_WITH_TOP_BIT_SET")
+        .is_some());
+
+    #[cfg(feature = "alloc")]
+    {
+        let crl = crl.to_owned().unwrap();
+        assert!(crl
+            .find_serial(REVOKED_SERIAL_WITH_TOP_BIT_SET)
+            .expect("failed to look for REVOKED_SERIAL_WITH_TOP_BIT_SET")
+            .is_some());
     }
 }
 

tests/crls/crl.topbit.serial.txt

@@ -0,0 +1,95 @@
+SEQUENCE {
+  SEQUENCE {
+    INTEGER { 1 }
+    SEQUENCE {
+      # ecdsa-with-SHA384
+      OBJECT_IDENTIFIER { 1.2.840.10045.4.3.3 }
+    }
+    SEQUENCE {
+      SET {
+        SEQUENCE {
+          # countryName
+          OBJECT_IDENTIFIER { 2.5.4.6 }
+          PrintableString { "XX" }
+        }
+      }
+      SET {
+        SEQUENCE {
+          # organizationName
+          OBJECT_IDENTIFIER { 2.5.4.10 }
+          PrintableString { "Boulder Test" }
+        }
+      }
+      SET {
+        SEQUENCE {
+          # commonName
+          OBJECT_IDENTIFIER { 2.5.4.3 }
+          PrintableString { "(TEST) Elegant Elephant E1" }
+        }
+      }
+    }
+    UTCTime { "221010201207Z" }
+    UTCTime { "221019201206Z" }
+    SEQUENCE {
+      SEQUENCE {
+        INTEGER { `0080feedf00d` }
+        UTCTime { "221010191207Z" }
+        SEQUENCE {
+          SEQUENCE {
+            # reasonCode
+            OBJECT_IDENTIFIER { 2.5.29.21 }
+            OCTET_STRING {
+              ENUMERATED { `01` }
+            }
+          }
+        }
+      }
+    }
+    [0] {
+      SEQUENCE {
+        SEQUENCE {
+          # authorityKeyIdentifier
+          OBJECT_IDENTIFIER { 2.5.29.35 }
+          OCTET_STRING {
+            SEQUENCE {
+              [0 PRIMITIVE] { `01dabb7acb25208e5e79d6f996422f02412907be` }
+            }
+          }
+        }
+        SEQUENCE {
+          # cRLNumber
+          OBJECT_IDENTIFIER { 2.5.29.20 }
+          OCTET_STRING {
+            INTEGER { `171cce3de482ba61` }
+          }
+        }
+        SEQUENCE {
+          # issuingDistributionPoint
+          OBJECT_IDENTIFIER { 2.5.29.28 }
+          BOOLEAN { TRUE }
+          OCTET_STRING {
+            SEQUENCE {
+              [0] {
+                [0] {
+                  [6 PRIMITIVE] { "http://c.boulder.test/66283756913588288/0.crl" }
+                }
+              }
+              [1 PRIMITIVE] { `ff` }
+            }
+          }
+        }
+      }
+    }
+  }
+  SEQUENCE {
+    # ecdsa-with-SHA384
+    OBJECT_IDENTIFIER { 1.2.840.10045.4.3.3 }
+  }
+  BIT_STRING {
+    `00`
+    SEQUENCE {
+      INTEGER { `2f0e42149d361abc9ea1f3ad3a303c85876bb0fdc17e3959cce2c13c9d4746cb45d88348467db478318fa44714506b22` }
+      INTEGER { `57903e972922bd33817a4fea5517534adc2d2bdab6e69e41256bdb89c68d9ecfcb4bdcf78c349fb89d67cc237b9fbf33` }
+    }
+  }
+}