[go] crypto/x509: reject duplicate extensions

[Gopher Robot (Gerrit)]

Gopher Robot submitted this change.

View Change

2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: src/crypto/x509/x509_test.go
Insertions: 219, Deletions: 0.

The diff is too large to show. Please review the diff.
```

Approvals: Damien Neil: Looks good to me, approved Roland Shoemaker: Looks good to me, but someone else must approve; Run TryBots; Automatically submit change Gopher Robot: TryBots succeeded

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 <dn...@google.com>
Run-TryBot: Roland Shoemaker <rol...@golang.org>
Auto-Submit: Roland Shoemaker <rol...@golang.org>
Reviewed-by: Roland Shoemaker <rol...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/crypto/x509/parser.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
3 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index 9dd7c25..e0e8f61 100644
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -930,6 +930,7 @@
 				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 @@
 					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 8823ff8..085408a 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1825,12 +1825,18 @@
 	}
 
 	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 @@
 		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 4806ef3..486d6bf 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -3661,3 +3661,49 @@
 		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")
+	}
+}

To view, visit change 383215. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: go

Gerrit-Branch: master

Gerrit-Change-Id: I531e932cfcdde78f64c106e747a68270bd4f1d80

Gerrit-Change-Number: 383215

Gerrit-PatchSet: 4

Gerrit-Owner: Roland Shoemaker <rol...@golang.org>

Gerrit-Reviewer: Damien Neil <dn...@google.com>

Gerrit-Reviewer: Filippo Valsorda <fil...@golang.org>

Gerrit-Reviewer: Gopher Robot <go...@golang.org>

Gerrit-Reviewer: Julie Qiu <ju...@golang.org>

Gerrit-Reviewer: Roland Shoemaker <rol...@golang.org>

Gerrit-MessageType: merged