Add lint to verify CRL TBSCertList.revokedCertificates field is absent when there are no revoked certificates

[pgporada]

RFC 5280 Section 5.1.2.6 states:

When there are no revoked certificates, the revoked certificates list MUST be absent.

The golang x509 library does parse CRLs and by virtue of zero values, will correctly omit the revokedCertificates field from the DER-encoded bytes of an ASN.1 data structure. The important bits that crypto/x509 uses to determine whether revokedCertificates exists are here: [1] [2] [3] and [4].

This code is a validation that golang is doing the correct thing with respect to omitting this field. I chose this method over asn1.Unmarshal and building out a struct representation of a CRL to reduce complexity and avoid potential future issues in golang's handling of asn1 encoding/decoding.

Related to my previous work https://github.com/letsencrypt/boulder/pull/7417 Related to, but a different implementation of, https://github.com/zmap/zlint/pull/831

[aaomidi]

I like that this PR does not depend on x509.RevocationList parsing to work, effectively making it so it can catch issues with the go stdlib as well. +1

[defacto64]

I have some doubts on the above comments. This alternative implementation by Phil is certainly OK, however I tested my own implementation with a few hundreds CRLs of all types, taken randomly from the list of CDPs obtainable from CCADB, and I found a completely correct behaviour in all cases.

[aaomidi]

Honestly, I don't see why two implementations can't exist for the same lint. Just consolidate it into a single file and :shipit: Argubly multiple implementations create stronger guarantees of behavior validation.

[pgporada]

The negative test was created using a throwaway openssl CA which issued a certificate and then revoked that certificate. I then used this tool in a Windows VM to manually delete asn1 chunks inside the revokedCertificates container, but left the container in place.

[aarongable]

I found a completely correct behaviour in all cases.

I'm sure that your implementation is wholly correct, and I have no objections to landing both side-by-side. My concern isn't with regards to the asn1 package's current behavior -- my concern is that it does not make a documented API guarantee to preserve that behavior in the future, and an asn1-based implementation could unexpectedly become broken.

[pgporada]

Whoops, sorry @aaomidi wrong button.

[defacto64]

If I understand correctly, Phil, the "negative test" was created by deleting asn1 chunks from an existing CRL. The result is a CRL that contains the revokedCertificates element with zero length (which is the wanted flaw), however this CRL at this point has a broken signature, as well as an empty issuer field. Therefore it is "triply broken", so to speak. Without any controversy intended, it does not seem to me to be a very representative example of what is actually found in the wild.

[pgporada]

No controversy at all, this is the first time that I've ever put up an alternate solution to an existing PR. I hope you don't feel like I'm stepping on your toes, that was not my intention.

I don't believe the test CRL needs to be representative, it just needs bytes that can be parsed far enough to fail in the specific way that the lint covers. Would a more realistic CRL be nice to use, of course. Other lints should catch the missing issuer field given that section 5.1.2.3 states

The issuer field MUST contain a non-empty X.500 distinguished name (DN).

Internally we have been discussing upstreaming the Boulder CRL lints to zlint, one of which does catch the missing issuer field. We don't yet have a lint that catches a mismatched signature.

In my code, I'm purposefully skipping fields to get to the field I do want to test. I mentioned the same thing to Amir here.

Using mississued CRLs [1] and [2] found in the wild such as from this Bugzilla bug lint correctly with my code. This is not meant to pick on Entrust. Another recent example of this same problem can be found here, but the actual problematic CRLs were not uploaded, just linked. Using the negative test you submitted in PR 831 crl_empty_revoked_certificates_ko.pem also lints correctly with my code.

[zakird]

If I understand correctly, Phil, the "negative test" was created by deleting asn1 chunks from an existing CRL. The result is a CRL that contains the revokedCertificates element with zero length (which is the wanted flaw), however this CRL at this point has a broken signature, as well as an empty issuer field. Therefore it is "triply broken", so to speak. Without any controversy intended, it does not seem to me to be a very representative example of what is actually found in the wild.

I think that this is likely true for many of our test certificates. I don't think that this necessarily needs to representative in terms of triggering only one lint, if we believe it does indeed test what's expected in this specific lint.

[CBonnell]

I think @defacto64 raises a valid concern regarding the empty Issuer DN field. RFC 5280 requires that this field be non-empty, so the current test case may fail if the parser enforces this rule from 5280 in the future.

[CBonnell]

Here's a slightly modified version of @pgporada's test case with a non-empty Issuer DN:

-----BEGIN X509 CRL-----
MIIBYDBKAgEBMA0GCSqGSIb3DQEBCwUAMBYxFDASBgNVBAoTC0NlcnRzICdyIFVz
Fw0yNDA0MTUxNDMzMDBaFw0yNDA1MTUxNDMzMDBaMAAwDQYJKoZIhvcNAQELBQAD
ggEBAGhq9yTTM2ZjzAxyNvXpVbOI4xQhC0L6pdjsZ13d3QFi41QvRFib13fHgcBm
+hWXFSmOT8qgMlIk74y01DBCmrVyn6mTznr49Vy9k6eBEs34F9EtQrJ5MlYNghX2
8UNNTMbQS/T7aYQuVWp4VRZsM2ZFRC1XxDdj85qraRhhc6fDGS3PS6m5vnRuZlVv
3wVB2N2zutQeZcxHDbAa68rSS3fK8jdKjC8uzbYhCvWYIc/ZUB0c+o9clwbZdkl4
eC6gxZ1/uD98+GilFUdX9JNVsi6Il1x9Upm+Oz6JZ43Ly2+yuQZu2rohZNxEzv/f
rzDRkyHn2a+5mqqc2J9asb6RFUs=
-----END X509 CRL-----

[pgporada]

@CBonnell May I use that to add to the negative test please?

@defacto64 May I use your negative test CRL too?

[CBonnell]

@CBonnell May I use that to add to the negative test please?

Definitely, glad it's of use.