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 letsencrypt/boulder#7417
Related to, but a different implementation of, #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

[aaomidi]

nit, just for consistency sake here

The ReadASN1Element and ReadASN1 happens twice in a row, once for the initial input, and then once more for the second input. Could this be a separate function call, similar to the skipTime function below?

[pgporada]

The first ReadASN1Element and ReadASN1 pair on lines 70-75 operate on an outer cryptobyte.String whereas the second ReadASN1Element and ReadASN1 pair on lines 80-85 read from that outer cryptobyte.String and then operate on an inner cryptobyte.String. I think this is fine as-is and should be left alone.

[aaomidi]

Is there a chance that the CRL is so malformed that these fields don't exist? What happens in that case?

[pgporada]

I think there should be a separate lint that handles that case.

[pgporada]

For what it's worth, Boulder contains a lint that looks for a missing issuer field.

[aaomidi]

Looking good, but tests need a negative test case, this would need to be a malformed CRL generated outside of this code and thrown into the testdata directory.

In the past I've done this by temporarily removing the checks from the x509 code to allow me to generate malformed entries, and then storing them as PEMs here.

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

[aaomidi]

@zakird I think this is good to go? Maybe once this is merged we can get a release pushed out too?