issuance: Validate existence of a CRL's TBSCertList.revokedCertificates field (#7417)

[RFC 5280 Section
5.1.2.6](https://datatracker.ietf.org/doc/html/rfc5280#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]](https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/crypto/x509/x509.go;l=2263-2267)
[[2]](https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/crypto/x509/x509.go;l=2359-2369)
[[3]](https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/crypto/x509/x509.go;l=2453-2455)
and
[[4]](https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/crypto/x509/parser.go;l=1157-1163).

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.

Fixes https://github.com/letsencrypt/boulder/issues/7415

issuance/crl_test.go

@@ -2,12 +2,15 @@ package issuance
 
 import (
 	"crypto/x509"
+	"errors"
 	"math/big"
 	"testing"
 	"time"
 
 	"github.com/jmhodges/clock"
 	"github.com/zmap/zlint/v3/lint"
+	"golang.org/x/crypto/cryptobyte"
+	cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
 
 	"github.com/letsencrypt/boulder/config"
 	"github.com/letsencrypt/boulder/crl/idp"
@@ -139,6 +142,9 @@ func TestIssueCRL(t *testing.T) {
 	expectUpdate := req.ThisUpdate.Add(-time.Second).Add(defaultProfile.validityInterval).Truncate(time.Second).UTC()
 	test.AssertEquals(t, parsedRes.NextUpdate, expectUpdate)
 	test.AssertEquals(t, len(parsedRes.Extensions), 3)
+	found, err := revokedCertificatesFieldExists(res)
+	test.AssertNotError(t, err, "Should have been able to parse CRL")
+	test.Assert(t, found, "Expected the revokedCertificates field to exist")
 
 	idps, err := idp.GetIDPURIs(parsedRes.Extensions)
 	test.AssertNotError(t, err, "getting IDP URIs from test CRL")
@@ -151,4 +157,92 @@ func TestIssueCRL(t *testing.T) {
 	_, err = issuer.IssueCRL(&defaultProfile, &req)
 	test.AssertError(t, err, "crl issuance with no IDP should fail")
 	test.AssertContains(t, err.Error(), "must contain an issuingDistributionPoint")
+
+	// A CRL with no entries must not have the revokedCertificates field
+	req = defaultRequest
+	req.Entries = []x509.RevocationListEntry{}
+	res, err = issuer.IssueCRL(&defaultProfile, &req)
+	test.AssertNotError(t, err, "issuing crl with no entries")
+	parsedRes, err = x509.ParseRevocationList(res)
+	test.AssertNotError(t, err, "parsing test crl")
+	test.AssertEquals(t, parsedRes.Issuer.CommonName, issuer.Cert.Subject.CommonName)
+	test.AssertDeepEquals(t, parsedRes.Number, big.NewInt(123))
+	test.AssertEquals(t, len(parsedRes.RevokedCertificateEntries), 0)
+	found, err = revokedCertificatesFieldExists(res)
+	test.AssertNotError(t, err, "Should have been able to parse CRL")
+	test.Assert(t, !found, "Violation of RFC 5280 Section 5.1.2.6")
+}
+
+// revokedCertificatesFieldExists is a modified version of
+// x509.ParseRevocationList that takes a given sequence of bytes representing a
+// CRL and parses away layers until the optional `revokedCertificates` field of
+// a TBSCertList is found. It returns a boolean indicating whether the field was
+// found or an error if there was an issue processing a CRL.
+//
+// https://datatracker.ietf.org/doc/html/rfc5280#section-5.1.2.6
+//
+//	When there are no revoked certificates, the revoked certificates list
+//	MUST be absent.
+//
+// https://datatracker.ietf.org/doc/html/rfc5280#appendix-A.1 page 118
+//
+//		TBSCertList  ::=  SEQUENCE  {
+//			   ..
+//				revokedCertificates     SEQUENCE OF SEQUENCE  {
+//			   ..
+//		    } OPTIONAL,
+//	     }
+func revokedCertificatesFieldExists(der []byte) (bool, error) {
+	input := cryptobyte.String(der)
+	if !input.ReadASN1Element(&input, cryptobyte_asn1.SEQUENCE) {
+		return false, errors.New("x509: malformed crl")
+	}
+	if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {
+		return false, errors.New("x509: malformed crl")
+	}
+
+	var tbs cryptobyte.String
+	if !input.ReadASN1Element(&tbs, cryptobyte_asn1.SEQUENCE) {
+		return false, errors.New("x509: malformed tbs crl")
+	}
+	if !tbs.ReadASN1(&tbs, cryptobyte_asn1.SEQUENCE) {
+		return false, errors.New("x509: malformed tbs crl")
+	}
+
+	// Skip optional version
+	tbs.SkipOptionalASN1(cryptobyte_asn1.INTEGER)
+
+	// Skip the signature
+	tbs.SkipASN1(cryptobyte_asn1.SEQUENCE)
+
+	// Skip the issuer
+	tbs.SkipASN1(cryptobyte_asn1.SEQUENCE)
+
+	// SkipOptionalASN1 is identical to SkipASN1 except that it also does a
+	// peek. We'll handle the non-optional thisUpdate with these double peeks
+	// because there's no harm doing so.
+	skipTime := func(s *cryptobyte.String) {
+		switch {
+		case s.PeekASN1Tag(cryptobyte_asn1.UTCTime):
+			s.SkipOptionalASN1(cryptobyte_asn1.UTCTime)
+		case s.PeekASN1Tag(cryptobyte_asn1.GeneralizedTime):
+			s.SkipOptionalASN1(cryptobyte_asn1.GeneralizedTime)
+		}
+	}
+
+	// Skip thisUpdate
+	skipTime(&tbs)
+
+	// Skip optional nextUpdate
+	skipTime(&tbs)
+
+	// Finally, the field which we care about: revokedCertificates. This will
+	// not trigger on the next field `crlExtensions` because that has
+	// context-specific tag [0] and EXPLICIT encoding, not `SEQUENCE` and is
+	// therefore a safe place to end this venture.
+	if tbs.PeekASN1Tag(cryptobyte_asn1.SEQUENCE) {
+		return true, nil
+	}
+
+	return false, nil
 }