crypto/x509: reject duplicate extensions

When parsing certificates and CSRs, reject duplicate extensions (and
additionally duplicate requested extensions in CSRs.)

Fixes #50988

Change-Id: I531e932cfcdde78f64c106e747a68270bd4f1d80
Reviewed-on: https://go-review.googlesource.com/c/go/+/383215
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index 9dd7c2564e12b7cf37085263a401e1e9145d6afa..e0e8f6125fdca577aa29844444f8b923343280f5 100644 (file)
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -930,6 +930,7 @@ func parseCertificate(der []byte) (*Certificate, error) {
                                return nil, errors.New("x509: malformed extensions")
                        }
                        if present {
+                               seenExts := make(map[string]bool)
                                if !extensions.ReadASN1(&extensions, cryptobyte_asn1.SEQUENCE) {
                                        return nil, errors.New("x509: malformed extensions")
                                }
@@ -942,6 +943,11 @@ func parseCertificate(der []byte) (*Certificate, error) {
                                        if err != nil {
                                                return nil, err
                                        }
+                                       oidStr := ext.Id.String()
+                                       if seenExts[oidStr] {
+                                               return nil, errors.New("x509: certificate contains duplicate extensions")
+                                       }
+                                       seenExts[oidStr] = true
                                        cert.Extensions = append(cert.Extensions, ext)
                                }
                                err = processExtensions(cert)

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 8823ff8a26298458a2d7609344e0f66437e05dc4..085408a0f80b52ba1fa1df348635f7765c1a0b51 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1825,12 +1825,18 @@ func parseCSRExtensions(rawAttributes []asn1.RawValue) ([]pkix.Extension, error)
        }
 
        var ret []pkix.Extension
+       seenExts := make(map[string]bool)
        for _, rawAttr := range rawAttributes {
                var attr pkcs10Attribute
                if rest, err := asn1.Unmarshal(rawAttr.FullBytes, &attr); err != nil || len(rest) != 0 || len(attr.Values) == 0 {
                        // Ignore attributes that don't parse.
                        continue
                }
+               oidStr := attr.Id.String()
+               if seenExts[oidStr] {
+                       return nil, errors.New("x509: certificate request contains duplicate extensions")
+               }
+               seenExts[oidStr] = true
 
                if !attr.Id.Equal(oidExtensionRequest) {
                        continue
@@ -1840,6 +1846,14 @@ func parseCSRExtensions(rawAttributes []asn1.RawValue) ([]pkix.Extension, error)
                if _, err := asn1.Unmarshal(attr.Values[0].FullBytes, &extensions); err != nil {
                        return nil, err
                }
+               requestedExts := make(map[string]bool)
+               for _, ext := range extensions {
+                       oidStr := ext.Id.String()
+                       if requestedExts[oidStr] {
+                               return nil, errors.New("x509: certificate request contains duplicate requested extensions")
+                       }
+                       requestedExts[oidStr] = true
+               }
                ret = append(ret, extensions...)
        }
 

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 4806ef3493efd54dda5c9ae246d27ef77eeb728d..486d6bf3d238b4d2396cf677a0f09febf248d52f 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -3661,3 +3661,49 @@ func TestCreateNegativeSerial(t *testing.T) {
                t.Errorf("CreateCertificate returned unexpected error: want %q, got %q", expectedErr, err)
        }
 }
+
+const dupExtCert = `-----BEGIN CERTIFICATE-----
+MIIBrjCCARegAwIBAgIBATANBgkqhkiG9w0BAQsFADAPMQ0wCwYDVQQDEwR0ZXN0
+MCIYDzAwMDEwMTAxMDAwMDAwWhgPMDAwMTAxMDEwMDAwMDBaMA8xDTALBgNVBAMT
+BHRlc3QwgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBAMiFchnHms9l9NninAIz
+SkY9acwl9Bk2AtmJrNCenFpiA17AcOO5q8DJYwdXi6WPKlVgcyH+ysW8XMWkq+CP
+yhtF/+LMzl9odaUF2iUy3vgTC5gxGLWH5URVssx21Und2Pm2f4xyou5IVxbS9dxy
+jLvV9PEY9BIb0H+zFthjhihDAgMBAAGjFjAUMAgGAioDBAIFADAIBgIqAwQCBQAw
+DQYJKoZIhvcNAQELBQADgYEAlhQ4TQQKIQ8GUyzGiN/75TCtQtjhMGemxc0cNgre
+d9rmm4DjydH0t7/sMCB56lQrfhJNplguzsbjFW4l245KbNKHfLiqwEGUgZjBNKur
+ot6qX/skahLtt0CNOaFIge75HVKe/69OrWQGdp18dkay/KS4Glu8YMKIjOhfrUi1
+NZA=
+-----END CERTIFICATE-----`
+
+func TestDuplicateExtensionsCert(t *testing.T) {
+       b, _ := pem.Decode([]byte(dupExtCert))
+       if b == nil {
+               t.Fatalf("couldn't decode test certificate")
+       }
+       _, err := ParseCertificate(b.Bytes)
+       if err == nil {
+               t.Fatal("ParseCertificate should fail when parsing certificate with duplicate extensions")
+       }
+}
+
+const dupExtCSR = `-----BEGIN CERTIFICATE REQUEST-----
+MIIBczCB3QIBADAPMQ0wCwYDVQQDEwR0ZXN0MIGfMA0GCSqGSIb3DQEBAQUAA4GN
+ADCBiQKBgQC5PbxMGVJ8aLF9lq/EvGObXTRMB7ieiZL9N+DJZg1n/ECCnZLIvYrr
+ZmmDV7YZsClgxKGfjJB0RQFFyZElFM9EfHEs8NJdidDKCRdIhDXQWRyhXKevHvdm
+CQNKzUeoxvdHpU/uscSkw6BgUzPyLyTx9A6ye2ix94z8Y9hGOBO2DQIDAQABoCUw
+IwYJKoZIhvcNAQkOMRYwFDAIBgIqAwQCBQAwCAYCKgMEAgUAMA0GCSqGSIb3DQEB
+CwUAA4GBAHROEsE7URk1knXmBnQtIHwoq663vlMcX3Hes58pUy020rWP8QkocA+X
+VF18/phg3p5ILlS4fcbbP2bEeV0pePo2k00FDPsJEKCBAX2LKxbU7Vp2OuV2HM2+
+VLOVx0i+/Q7fikp3hbN1JwuMTU0v2KL/IKoUcZc02+5xiYrnOIt5
+-----END CERTIFICATE REQUEST-----`
+
+func TestDuplicateExtensionsCSR(t *testing.T) {
+       b, _ := pem.Decode([]byte(dupExtCSR))
+       if b == nil {
+               t.Fatalf("couldn't decode test certificate")
+       }
+       _, err := ParseCertificateRequest(b.Bytes)
+       if err == nil {
+               t.Fatal("ParseCertificate should fail when parsing certificate with duplicate extensions")
+       }
+}