security/advancedtls: fix CRL issuer comparison (#5130)

Fix CRL issuer comparison issue

security/advancedtls/crl.go

@@ -27,6 +27,7 @@ import (
 	"encoding/asn1"
 	"encoding/binary"
 	"encoding/hex"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -34,6 +35,8 @@ import (
 	"strings"
 	"time"
 
+	"golang.org/x/crypto/cryptobyte"
+	cbasn1 "golang.org/x/crypto/cryptobyte/asn1"
 	"google.golang.org/grpc/grpclog"
 )
 
@@ -83,6 +86,7 @@ type certificateListExt struct {
 	CertList *pkix.CertificateList
 	// RFC5280, 5.2.1, all conforming CRLs must have a AKID with the ID method.
 	AuthorityKeyID []byte
+	RawIssuer      []byte
 }
 
 const tagDirectoryName = 4
@@ -99,6 +103,11 @@ var (
 )
 
 // x509NameHash implements the OpenSSL X509_NAME_hash function for hashed directory lookups.
+//
+// NOTE: due to the behavior of asn1.Marshal, if the original encoding of the RDN sequence
+// contains strings which do not use the ASN.1 PrintableString type, the name will not be
+// re-encoded using those types, resulting in a hash which does not match that produced
+// by OpenSSL.
 func x509NameHash(r pkix.RDNSequence) string {
 	var canonBytes []byte
 	// First, canonicalize all the strings.
@@ -277,10 +286,7 @@ func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revoca
 func checkCertRevocation(c *x509.Certificate, crl *certificateListExt) (RevocationStatus, error) {
 	// Per section 5.3.3 we prime the certificate issuer with the CRL issuer.
 	// Subsequent entries use the previous entry's issuer.
-	rawEntryIssuer, err := asn1.Marshal(crl.CertList.TBSCertList.Issuer)
+	rawEntryIssuer := crl.RawIssuer
-	if err != nil {
-		return RevocationUndetermined, err
-	}
 
 	// Loop through all the revoked certificates.
 	for _, revCert := range crl.CertList.TBSCertList.RevokedCertificates {
@@ -456,10 +462,11 @@ func fetchCRL(loc string, rawIssuer []byte, cfg RevocationConfig) (*certificateL
 			continue
 		}
 
-		rawCRLIssuer, err := asn1.Marshal(certList.CertList.TBSCertList.Issuer)
+		rawCRLIssuer, err := extractCRLIssuer(crlBytes)
 		if err != nil {
-			return nil, fmt.Errorf("asn1.Marshal(%v) failed err = %v", certList.CertList.TBSCertList.Issuer, err)
+			return nil, err
 		}
+		certList.RawIssuer = rawCRLIssuer
 		// RFC5280, 6.3.3 (b) Verify the issuer and scope of the complete CRL.
 		if bytes.Equal(rawIssuer, rawCRLIssuer) {
 			parsedCRL = certList
@@ -478,10 +485,6 @@ func verifyCRL(crl *certificateListExt, rawIssuer []byte, chain []*x509.Certific
 	// RFC5280, 6.3.3 (f) Obtain and validateate the certification path for the issuer of the complete CRL
 	// We intentionally limit our CRLs to be signed with the same certificate path as the certificate
 	// so we can use the chain from the connection.
-	rawCRLIssuer, err := asn1.Marshal(crl.CertList.TBSCertList.Issuer)
-	if err != nil {
-		return fmt.Errorf("asn1.Marshal(%v) failed err = %v", crl.CertList.TBSCertList.Issuer, err)
-	}
 
 	for _, c := range chain {
 		// Use the key where the subject and KIDs match.
@@ -490,10 +493,35 @@ func verifyCRL(crl *certificateListExt, rawIssuer []byte, chain []*x509.Certific
 		// "Conforming CRL issuers MUST use the key identifier method, and MUST
 		// include this extension in all CRLs issued."
 		// So, this is much simpler than RFC4158 and should be compatible.
-		if bytes.Equal(c.SubjectKeyId, crl.AuthorityKeyID) && bytes.Equal(c.RawSubject, rawCRLIssuer) {
+		if bytes.Equal(c.SubjectKeyId, crl.AuthorityKeyID) && bytes.Equal(c.RawSubject, crl.RawIssuer) {
 			// RFC5280, 6.3.3 (g) Validate signature.
 			return c.CheckCRLSignature(crl.CertList)
 		}
 	}
 	return fmt.Errorf("verifyCRL: No certificates mached CRL issuer (%v)", crl.CertList.TBSCertList.Issuer)
 }
+
+var crlPemPrefix = []byte("-----BEGIN X509 CRL")
+
+// extractCRLIssuer extracts the raw ASN.1 encoding of the CRL issuer. Due to the design of
+// pkix.CertificateList and pkix.RDNSequence, it is not possible to reliably marshal the
+// parsed Issuer to it's original raw encoding.
+func extractCRLIssuer(crlBytes []byte) ([]byte, error) {
+	if bytes.HasPrefix(crlBytes, crlPemPrefix) {
+		block, _ := pem.Decode(crlBytes)
+		if block != nil && block.Type == "X509 CRL" {
+			crlBytes = block.Bytes
+		}
+	}
+
+	der := cryptobyte.String(crlBytes)
+	var issuer cryptobyte.String
+	if !der.ReadASN1(&der, cbasn1.SEQUENCE) ||
+		!der.ReadASN1(&der, cbasn1.SEQUENCE) ||
+		!der.SkipOptionalASN1(cbasn1.INTEGER) ||
+		!der.SkipASN1(cbasn1.SEQUENCE) ||
+		!der.ReadASN1Element(&issuer, cbasn1.SEQUENCE) {
+		return nil, errors.New("extractCRLIssuer: invalid ASN.1 encoding")
+	}
+	return issuer, nil
+}

security/advancedtls/crl_test.go

@@ -337,7 +337,7 @@ func makeChain(t *testing.T, name string) []*x509.Certificate {
 	return certChain
 }
 
-func loadCRL(t *testing.T, path string) *pkix.CertificateList {
+func loadCRL(t *testing.T, path string) *certificateListExt {
 	b, err := ioutil.ReadFile(path)
 	if err != nil {
 		t.Fatalf("readFile(%v) failed err = %v", path, err)
@@ -346,7 +346,15 @@ func loadCRL(t *testing.T, path string) *pkix.CertificateList {
 	if err != nil {
 		t.Fatalf("ParseCrl(%v) failed err = %v", path, err)
 	}
-	return crl
+	crlExt, err := parseCRLExtensions(crl)
+	if err != nil {
+		t.Fatalf("parseCRLExtensions(%v) failed err = %v", path, err)
+	}
+	crlExt.RawIssuer, err = extractCRLIssuer(b)
+	if err != nil {
+		t.Fatalf("extractCRLIssuer(%v) failed err= %v", path, err)
+	}
+	return crlExt
 }
 
 func TestCachedCRL(t *testing.T) {
@@ -450,11 +458,11 @@ func TestGetIssuerCRLCache(t *testing.T) {
 func TestVerifyCrl(t *testing.T) {
 	tampered := loadCRL(t, testdata.Path("crl/1.crl"))
 	// Change the signature so it won't verify
-	tampered.SignatureValue.Bytes[0]++
+	tampered.CertList.SignatureValue.Bytes[0]++
 
 	verifyTests := []struct {
 		desc    string
-		crl     *pkix.CertificateList
+		crl     *certificateListExt
 		certs   []*x509.Certificate
 		cert    *x509.Certificate
 		errWant string
@@ -498,11 +506,7 @@ func TestVerifyCrl(t *testing.T) {
 
 	for _, tt := range verifyTests {
 		t.Run(tt.desc, func(t *testing.T) {
-			crlExt, err := parseCRLExtensions(tt.crl)
+			err := verifyCRL(tt.crl, tt.cert.RawIssuer, tt.certs)
-			if err != nil {
-				t.Fatalf("parseCRLExtensions(%v) failed, err = %v", tt.crl.TBSCertList.Issuer, err)
-			}
-			err = verifyCRL(crlExt, tt.cert.RawIssuer, tt.certs)
 			switch {
 			case tt.errWant == "" && err != nil:
 				t.Errorf("Valid CRL did not verify err = %v", err)
@@ -716,3 +720,14 @@ func TestVerifyConnection(t *testing.T) {
 		})
 	}
 }
+
+func TestIssuerNonPrintableString(t *testing.T) {
+	rawIssuer, err := hex.DecodeString("300c310a300806022a030c023a29")
+	if err != nil {
+		t.Fatalf("failed to decode issuer: %s", err)
+	}
+	_, err = fetchCRL("", rawIssuer, RevocationConfig{RootDir: testdata.Path("crl")})
+	if err != nil {
+		t.Fatalf("fetchCRL failed: %s", err)
+	}
+}

security/advancedtls/examples/go.sum

@@ -39,6 +39,7 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqeYNgFYFoEGnI=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
+golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 h1:psW17arqaxU48Z5kZ0CQnkZWQJsqcURM6tKiBApRjXI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=

security/advancedtls/go.mod

@@ -5,6 +5,7 @@ go 1.14
 require (
 	github.com/google/go-cmp v0.5.1 // indirect
 	github.com/hashicorp/golang-lru v0.5.4
+	golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9
 	google.golang.org/grpc v1.38.0
 	google.golang.org/grpc/examples v0.0.0-20201112215255-90f1b3ee835b
 )

security/advancedtls/go.sum

@@ -39,6 +39,7 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqeYNgFYFoEGnI=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
+golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 h1:psW17arqaxU48Z5kZ0CQnkZWQJsqcURM6tKiBApRjXI=
 golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
 golang.org/x/lint v0.0.0-20190313153728-d0100b6bd8b3/go.mod h1:6SW0HCj/g11FgYtHlgUYUwCkIfeOF89ocIRzGO/8vkc=
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=

security/advancedtls/testdata/crl/2f11f022.r0

@@ -0,0 +1,7 @@
+-----BEGIN X509 CRL-----
+MIHnMFICAQEwDQYJKoZIhvcNAQEMBQAwDDEKMAgGAioDDAI6KRcNMDkxMTEwMjMw
+MDAwWhcNMDkxMTExMDAwMDAwWqASMBAwDgYDVR0jBAcwBYADAQIDMA0GCSqGSIb3
+DQEBDAUAA4GBAMl2sjOjtOQ+OCsRyjM0IvqTn7lmdGJMvpYAym367JBamJPCbYrL
+MifCjCA1ra7gG0MweZbpm4SG2YLakwi1/B+XhApQ5VVv5SwDn6Yy5zr9ePLEF7Iy
+sP86e9s5XfOusLTW+Spre8q1vi7pJrRvUxhJGuUuLoM6Uhvh65ViilDJ
+-----END X509 CRL-----