From 231ca3b24ec9084567988ae9f9e8644a9c72c768 Mon Sep 17 00:00:00 2001 From: Roland Bracewell Shoemaker Date: Tue, 25 Jan 2022 13:32:27 -0800 Subject: [PATCH] security/advancedtls: fix CRL issuer comparison (#5130) Fix CRL issuer comparison issue --- security/advancedtls/crl.go | 50 +++++++++++++++---- security/advancedtls/crl_test.go | 33 ++++++++---- security/advancedtls/examples/go.sum | 1 + security/advancedtls/go.mod | 1 + security/advancedtls/go.sum | 1 + security/advancedtls/testdata/crl/2f11f022.r0 | 7 +++ 6 files changed, 73 insertions(+), 20 deletions(-) create mode 100644 security/advancedtls/testdata/crl/2f11f022.r0 diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go index 3931c1ec6..7988cb271 100644 --- a/security/advancedtls/crl.go +++ b/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) - if err != nil { - return RevocationUndetermined, err - } + rawEntryIssuer := crl.RawIssuer // 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 +} diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go index ec4483304..ef3eb85da 100644 --- a/security/advancedtls/crl_test.go +++ b/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) - if err != nil { - t.Fatalf("parseCRLExtensions(%v) failed, err = %v", tt.crl.TBSCertList.Issuer, err) - } - err = verifyCRL(crlExt, tt.cert.RawIssuer, tt.certs) + err := verifyCRL(tt.crl, 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) + } +} diff --git a/security/advancedtls/examples/go.sum b/security/advancedtls/examples/go.sum index d926d5ffb..e84d5d32d 100644 --- a/security/advancedtls/examples/go.sum +++ b/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= diff --git a/security/advancedtls/go.mod b/security/advancedtls/go.mod index 75527018e..8d12b6275 100644 --- a/security/advancedtls/go.mod +++ b/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 ) diff --git a/security/advancedtls/go.sum b/security/advancedtls/go.sum index d926d5ffb..e84d5d32d 100644 --- a/security/advancedtls/go.sum +++ b/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= diff --git a/security/advancedtls/testdata/crl/2f11f022.r0 b/security/advancedtls/testdata/crl/2f11f022.r0 new file mode 100644 index 000000000..e570f17ee --- /dev/null +++ b/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-----