Add lint to verify CRL TBSCertList.revokedCertificates field is absen…

…t when there are no revoked certificates (#832)

* Working lint and tests

* Add negative test

* Rename test crl

* DER, PEM, vim smuggled inside testdata just like xz, you pick

* Add more negative test cases and run through all of the files

---------

Co-authored-by: Zakir Durumeric <[email protected]>

v3/lints/rfc/lint_crl_revoked_certificates_field_empty.go

@@ -0,0 +1,123 @@
+package rfc
+
+/*
+ * ZLint Copyright 2024 Regents of the University of Michigan
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy
+ * of the License at http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+import (
+	"github.com/zmap/zcrypto/x509"
+	"github.com/zmap/zlint/v3/lint"
+	"github.com/zmap/zlint/v3/util"
+	"golang.org/x/crypto/cryptobyte"
+	cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
+)
+
+type revokedCertificates struct{}
+
+/*
+RFC 5280: 5.1.2.6
+
+	When there are no revoked certificates, the revoked certificates list
+	MUST be absent.
+*/
+func init() {
+	lint.RegisterRevocationListLint(&lint.RevocationListLint{
+		LintMetadata: lint.LintMetadata{
+			Name:          "e_crl_revoked_certificates_field_must_be_empty",
+			Description:   "When the revokedCertificates field is empty, it MUST be absent from the DER-encoded ASN.1 data structure.",
+			Citation:      "RFC 5280: 5.1.2.6",
+			Source:        lint.RFC5280,
+			EffectiveDate: util.RFC5280Date,
+		},
+		Lint: NewEmptyRevokedCertificates,
+	})
+}
+
+func NewEmptyRevokedCertificates() lint.RevocationListLintInterface {
+	return &revokedCertificates{}
+}
+
+func (l *revokedCertificates) CheckApplies(c *x509.RevocationList) bool {
+	// This lint is to verify that the TBSCertList.revokedCertificates field,
+	// when empty, is indeed missing from the DER-encoded ASN.1 bytes.
+	if c != nil && len(c.RevokedCertificates) == 0 {
+		return true
+	}
+
+	return false
+}
+
+func (l *revokedCertificates) Execute(c *x509.RevocationList) *lint.LintResult {
+	// This is a modified version of x509.ParseRevocationList that extracts the
+	// raw DER-encoded bytes that comprise a CRL and parses away layers until
+	// the optional `revokedCertificates` field of a TBSCertList is either found
+	// or confirmed to be missing from the ASN.1 data structure.
+	input := cryptobyte.String(c.Raw)
+
+	// From crypto/x509/parser.go: we read the SEQUENCE including length and tag
+	// bytes so that we can populate RevocationList.Raw, before unwrapping the
+	// SEQUENCE so it can be operated on
+	if !input.ReadASN1Element(&input, cryptobyte_asn1.SEQUENCE) {
+		return &lint.LintResult{Status: lint.Fatal, Details: "malformed CRL"}
+	}
+	if !input.ReadASN1(&input, cryptobyte_asn1.SEQUENCE) {
+		return &lint.LintResult{Status: lint.Fatal, Details: "malformed CRL"}
+	}
+
+	var tbs cryptobyte.String
+	// From crypto/x509/parser.go: do the same trick again as above to extract
+	// the raw bytes for Certificate.RawTBSCertificate
+	if !input.ReadASN1Element(&tbs, cryptobyte_asn1.SEQUENCE) {
+		return &lint.LintResult{Status: lint.Fatal, Details: "malformed TBS CRL"}
+	}
+	if !tbs.ReadASN1(&tbs, cryptobyte_asn1.SEQUENCE) {
+		return &lint.LintResult{Status: lint.Fatal, Details: "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 &lint.LintResult{Status: lint.Error, Details: "When there are no revoked certificates, the revoked certificates list	MUST be absent."}
+	}
+
+	return &lint.LintResult{Status: lint.Pass}
+}

v3/lints/rfc/lint_crl_revoked_certificates_field_empty_test.go

@@ -0,0 +1,167 @@
+/*
+ * ZLint Copyright 2024 Regents of the University of Michigan
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy
+ * of the License at http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package rfc
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
+	"math/big"
+	"testing"
+	"time"
+
+	"github.com/zmap/zcrypto/x509"
+	"github.com/zmap/zcrypto/x509/pkix"
+	"github.com/zmap/zlint/v3/lint"
+	"github.com/zmap/zlint/v3/test"
+)
+
+const lintUnderTest = "e_crl_revoked_certificates_field_must_be_empty"
+
+// generateCRLFromTemplate takes a CRL template and creates an in-memory issuer
+// capable of signing the CRL and returns the resulting CRL or an error.
+func generateCRLFromTemplate(crlTemplate *x509.RevocationList) (*x509.RevocationList, error) {
+	signer, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		return nil, err
+	}
+
+	issuerTemplate := &x509.Certificate{
+		SerialNumber:          big.NewInt(666),
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+		Subject: pkix.Name{
+			CommonName: "Big CA",
+		},
+		SubjectKeyId: []byte{1, 2, 3, 4, 5, 6},
+		KeyUsage:     x509.KeyUsageCRLSign | x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature,
+	}
+
+	issuerBytes, err := x509.CreateCertificate(rand.Reader, issuerTemplate, issuerTemplate, signer.Public(), signer)
+	if err != nil {
+		return nil, err
+	}
+
+	issuer, err := x509.ParseCertificate(issuerBytes)
+	if err != nil {
+		return nil, err
+	}
+	crlBytes, err := x509.CreateRevocationList(rand.Reader, crlTemplate, issuer, signer)
+	if err != nil {
+		return nil, err
+	}
+
+	// We're not going to trust that x509.ParseRevocationList is doing the
+	// correct thing here and will instead parse the DER-encoded ASN.1 bytes of
+	// this CRL later on in the lint itself.
+	actualCRL, err := x509.ParseRevocationList(crlBytes)
+	if err != nil {
+		return nil, err
+	}
+
+	return actualCRL, nil
+}
+
+var defaultTemplate = x509.RevocationList{
+	Number:     big.NewInt(1),
+	ThisUpdate: time.Now().Add(-time.Second),
+	NextUpdate: time.Now().Add(10 * time.Second),
+}
+
+func TestEmptyRevokedCertificatesField(t *testing.T) {
+	crlTemplate := defaultTemplate
+	crlTemplate.RevokedCertificates = []x509.RevokedCertificate{}
+
+	crl, err := generateCRLFromTemplate(&crlTemplate)
+	if err != nil {
+		t.Error(err)
+	}
+
+	out := test.TestLintRevocationList(t, lintUnderTest, crl, lint.NewEmptyConfig())
+	expected := lint.Pass
+	if out.Status != expected {
+		t.Errorf("expected %s, got %s", expected, out.Status)
+	}
+
+	expectedRevokedCerts := 0
+	if len(crl.RevokedCertificates) != expectedRevokedCerts {
+		t.Errorf("expected %d revoked certificates in CRL, got %d", expectedRevokedCerts, len(crl.RevokedCertificates))
+	}
+}
+
+func TestNilRevokedCertificatesField(t *testing.T) {
+	crlTemplate := defaultTemplate
+	crlTemplate.RevokedCertificates = nil
+
+	crl, err := generateCRLFromTemplate(&crlTemplate)
+	if err != nil {
+		t.Error(err)
+	}
+
+	out := test.TestLintRevocationList(t, lintUnderTest, crl, lint.NewEmptyConfig())
+	expected := lint.Pass
+	if out.Status != expected {
+		t.Errorf("expected %s, got %s", expected, out.Status)
+	}
+
+	expectedRevokedCerts := 0
+	if len(crl.RevokedCertificates) != expectedRevokedCerts {
+		t.Errorf("expected %d revoked certificates in CRL, got %d", expectedRevokedCerts, len(crl.RevokedCertificates))
+	}
+}
+
+func TestPopulatedRevokedCertificatesField(t *testing.T) {
+	crlTemplate := defaultTemplate
+	crlTemplate.RevokedCertificates = append(crlTemplate.RevokedCertificates, x509.RevokedCertificate{
+		SerialNumber:   big.NewInt(876),
+		RevocationTime: time.Now().Add(-24 * time.Hour),
+	})
+
+	crl, err := generateCRLFromTemplate(&crlTemplate)
+	if err != nil {
+		t.Error(err)
+	}
+
+	// The lint should not run for this case because we populated the
+	// TBSCertList.revokedCertificates field.
+	expected := lint.NA
+	out := test.TestLintRevocationList(t, lintUnderTest, crl, lint.NewEmptyConfig())
+	if out.Status != expected {
+		t.Errorf("expected %s, got %s", expected, out.Status)
+	}
+
+	expectedRevokedCerts := 1
+	if len(crl.RevokedCertificates) != expectedRevokedCerts {
+		t.Errorf("expected %d revoked certificates in CRL, got %d", expectedRevokedCerts, len(crl.RevokedCertificates))
+	}
+}
+
+func TestRevokedCertificatesContainerExistsButIsEmpty(t *testing.T) {
+	// Negative test data created outside the purview of Golang.
+	badCRLFiles := []string{
+		"crlWithRevokedCertificatesContainerButNoActualRevokedCerts-ReallyReallyBroken.pem",
+		"crlWithRevokedCertificatesContainerButNoActualRevokedCerts-CBonnell.pem",
+		"crlEntrustNoRevokedCerts01.pem", // https://bugzilla.mozilla.org/show_bug.cgi?id=1889217
+		"crlEntrustNoRevokedCerts02.pem",
+	}
+
+	expected := lint.Error
+	for _, crl := range badCRLFiles {
+		out := test.TestRevocationListLint(t, lintUnderTest, crl)
+		if out.Status != expected {
+			t.Errorf("expected %s, got %s", expected, out.Status)
+		}
+	}
+}

v3/testdata/crlEntrustNoRevokedCerts01.pem

@@ -0,0 +1,14 @@
+-----BEGIN X509 CRL-----
+MIICGTCCAQECAQEwDQYJKoZIhvcNAQELBQAwgYAxMDAuBgNVBAMMJ0VudHJ1c3Qg
+Q2VydGlmaWNhdGlvbiBBdXRob3JpdHkgLSBRVFNQMTEYMBYGA1UEYQwPVkFURVMt
+QjgxMTg4MDQ3MSUwIwYDVQQKDBxFbnRydXN0IERhdGFjYXJkIEV1cm9wZSBTLkwu
+MQswCQYDVQQGEwJFUxcNMjQwNDA0MDUyMTE0WhcNMjQwNDExMDUyMTEzWjAAoEow
+SDAfBgNVHSMEGDAWgBQcrT+c1y0iGaGcS+na8Soz9/u6DTALBgNVHRQEBAICDxsw
+GAYDVR08BBEYDzIwMjQwNDAyMDUyMTE0WjANBgkqhkiG9w0BAQsFAAOCAQEASWnR
+tZt0Hvn4jVsYmtQ8QTxnN8GK2QFqlm4Y0jQHFKE4ONHYqQ2/QaV1fVVc8TGX6apk
+CdyDjLlVW7eQsta4p45Sgu8lvORtf/i1NEkO3ZcxerFmLAsOkFRwd9p/KjsDwM44
+q1dX9o1SNqXxUcC0skyOx+1mze6c/hKN/A7Nb8uwbwcsF36TYYN5zwqN4onbWo83
+fBs3MG48yj6FwqH3Jq+LBT2LDuOg7Ut47qsPlry4QdQI7K5L9Zyjc4Hg3//Bp6rv
+vFyNWs1dtMg05H7nryguyQ52mJ4JjBYzWhnazZYb2vkGdM9GilnOTeoOwEWLS5zd
+ivQAcDeOyqsA4TdEhg==
+-----END X509 CRL-----

v3/testdata/crlEntrustNoRevokedCerts02.pem

@@ -0,0 +1,13 @@
+-----BEGIN X509 CRL-----
+MIIB6DCB0QIBATANBgkqhkiG9w0BAQsFADBRMTAwLgYDVQQDDCdTaWVtZW5zIElz
+c3VpbmcgQ0EgSW50ZXJuZXQgU2VydmVyIDIwMjAxEDAOBgNVBAoMB1NpZW1lbnMx
+CzAJBgNVBAYTAkRFFw0yNDA0MDQwNjU5MTRaFw0yNDA0MTEwNjU5MTNaMACgSjBI
+MB8GA1UdIwQYMBaAFMmnV8uGyWEHxsK0hmWpHsHK4QKbMAsGA1UdFAQEAgIKazAY
+BgNVHTwEERgPMjAyNDA0MDIwNjU5MTRaMA0GCSqGSIb3DQEBCwUAA4IBAQCLI/au
+Ypv+dVoPAsyF5+iQ6mKugBah8qle8umSFbH42H+ngVwRyYzyOo4IGvi3FAHD8dXR
+0E0GuH47zLIhU2GUqku0O699UA/qoLngTG13DVYqOIDWIW+fQduXKezMVc26rtjH
+3U0OjYxHaUlcsix7e0BjRzeh+ubU3jPQMHKatJabImCkZeG7xPnhq6i1Hfb+4b3F
+RA2nUA+IT3VQs/2tAoGcbqOgviS0AzaRgs2WUYm1PDm8YsML9Gt0d2LBe2o2Ov6n
+V5iXcKbtTzy+aq5ac7o9pwkuYLBnCELurMdTjUg0HpTRJMJm+lR2G+zmVhTO4azs
+7n9Pv3Y7qkCujQ3j
+-----END X509 CRL-----

v3/testdata/crlWithRevokedCertificatesContainerButNoActualRevokedCerts-CBonnell.pem

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

v3/testdata/crlWithRevokedCertificatesContainerButNoActualRevokedCerts-ReallyReallyBroken.pem

@@ -0,0 +1,9 @@
+-----BEGIN X509 CRL-----
+MIIBSjA0AgEBMA0GCSqGSIb3DQEBCwUAMAAXDTI0MDQxNTE0MzMwMFoXDTI0MDUx
+NTE0MzMwMFowADANBgkqhkiG9w0BAQsFAAOCAQEAaGr3JNMzZmPMDHI29elVs4jj
+FCELQvql2OxnXd3dAWLjVC9EWJvXd8eBwGb6FZcVKY5PyqAyUiTvjLTUMEKatXKf
+qZPOevj1XL2Tp4ESzfgX0S1CsnkyVg2CFfbxQ01MxtBL9PtphC5VanhVFmwzZkVE
+LVfEN2PzmqtpGGFzp8MZLc9Lqbm+dG5mVW/fBUHY3bO61B5lzEcNsBrrytJLd8ry
+N0qMLy7NtiEK9Zghz9lQHRz6j1yXBtl2SXh4LqDFnX+4P3z4aKUVR1f0k1WyLoiX
+XH1Smb47PolnjcvLb7K5Bm7auiFk3ETO/9+vMNGTIefZr7maqpzYn1qxvpEVSw==
+-----END X509 CRL-----