[go] crypto/x509: don't create certs with negative serials
356 views
Skip to first unread message
Roland Shoemaker (Gerrit)
Apr 14, 2022, 8:59:33 PM4/14/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Roland Shoemaker has uploaded this change for review.
crypto/x509: don't create certs with negative serials
Refuse to create certificates with negative serial numbers, as they
are explicitly disallowed by RFC 5280.
We still allow parsing certificates with negative serial numbers,
because in the past there were buggy CA implementations which would
produce them (although there are currently *no* trusted certificates
that have this issue). We may want to revisit this decision if we can
find metrics about the prevalence of this issue in enterprise settings.
Change-Id: I131262008db99b6354f542f335abc68775a2d6d0
---
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
2 files changed, 61 insertions(+), 6 deletions(-)
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 6d99191..a7deee2 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1478,13 +1478,17 @@
return nil, errors.New("x509: no SerialNumber given")
}
- // RFC 5280 Section 4.1.2.2: serial number must not be longer than 20 octets
+ // RFC 5280 Section 4.1.2.2: serial number must positive and should not be longer
+ // than 20 octets.
//
// We cannot simply check for len(serialBytes) > 20, because encoding/asn1 may
// pad the slice in order to prevent the integer being mistaken for a negative
// number (DER uses the high bit of the left-most byte to indicate the sign.),
// so we need to double check the composition of the serial if it is exactly
// 20 bytes.
+ if template.SerialNumber.Sign() == -1 {
+ return nil, errors.New("x509: serial must be positive")
+ }
serialBytes := template.SerialNumber.Bytes()
if len(serialBytes) > 20 || (len(serialBytes) == 20 && serialBytes[0]&0x80 != 0) {
return nil, errors.New("x509: serial number exceeds 20 octets")
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index c294f91..8dca12b 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -602,11 +602,7 @@
for _, test := range tests {
commonName := "test.example.com"
template := Certificate{
- // SerialNumber is negative to ensure that negative
- // values are parsed. This is due to the prevalence of
- // buggy code that produces certificates with negative
- // serial numbers.
- SerialNumber: big.NewInt(-1),
+ SerialNumber: big.NewInt(1),
Subject: pkix.Name{
CommonName: commonName,
Organization: []string{"Σ Acme Co"},
@@ -3628,3 +3624,40 @@
t.Errorf("CreateCertificate returned unexpected error: want %q, got %q", expectedErr, err)
}
}
+
+var negativeSerialCert = `-----BEGIN CERTIFICATE-----
+MIIBBTCBraADAgECAgH/MAoGCCqGSM49BAMCMA0xCzAJBgNVBAMTAjopMB4XDTIy
+MDQxNDIzNTYwNFoXDTIyMDQxNTAxNTYwNFowDTELMAkGA1UEAxMCOikwWTATBgcq
+hkjOPQIBBggqhkjOPQMBBwNCAAQ9ezsIsj+q17K87z/PXE/rfGRN72P/Wyn5d6oo
+5M0ZbSatuntMvfKdX79CQxXAxN4oXk3Aov4jVSG12AcDI8ShMAoGCCqGSM49BAMC
+A0cAMEQCIBzfBU5eMPT6m5lsR6cXaJILpAaiD9YxOl4v6dT3rzEjAiBHmjnHmAss
+RqUAyJKFzqZxOlK2q4j2IYnuj5+LrLGbQA==
+-----END CERTIFICATE-----`
+
+func TestParseNegativeSerial(t *testing.T) {
+ pemBlock, _ := pem.Decode([]byte(negativeSerialCert))
+ _, err := ParseCertificate(pemBlock.Bytes)
+ if err != nil {
+ t.Fatalf("failed to parse certificate: %s", err)
+ }
+}
+
+func TestCreateNegativeSerial(t *testing.T) {
+ k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+ if err != nil {
+ t.Fatal(err)
+ }
+ tmpl := &Certificate{
+ SerialNumber: big.NewInt(-1),
+ Subject: pkix.Name{
+ CommonName: ":)",
+ },
+ NotAfter: time.Now().Add(time.Hour),
+ NotBefore: time.Now().Add(-time.Hour),
+ }
+ expectedErr := "x509: serial must be positive"
+ _, err = CreateCertificate(rand.Reader, tmpl, tmpl, k.Public(), k)
+ if err == nil || err.Error() != expectedErr {
+ t.Errorf("CreateCertificate returned unexpected error: want %q, got %q", expectedErr, err)
+ }
+}
To view, visit change 400494. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 14, 2022, 9:00:48 PM4/14/22
to goph...@pubsubhelper.golang.org, Damien Neil, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil.
Patch set 1:Run-TryBot +1Auto-Submit +1Code-Review +1
1 comment:
Patchset:
Thought we already did this, but apparently not.
To view, visit change 400494. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Apr 15, 2022, 11:07:49 AM4/15/22
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Gopher Robot, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Patch set 1:Code-Review +2
1 comment:
File src/crypto/x509/x509.go:
Patch Set #1, Line 1490:
return nil, errors.New("x509: serial must be positive")Maybe "serial number must be positive" for consistency with the next error.
To view, visit change 400494. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 15, 2022, 12:09:20 PM4/15/22
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Roland Shoemaker uploaded patch set #2 to this change.
crypto/x509: don't create certs with negative serials
Refuse to create certificates with negative serial numbers, as they
are explicitly disallowed by RFC 5280.
We still allow parsing certificates with negative serial numbers,
because in the past there were buggy CA implementations which would
produce them (although there are currently *no* trusted certificates
that have this issue). We may want to revisit this decision if we can
find metrics about the prevalence of this issue in enterprise settings.
Change-Id: I131262008db99b6354f542f335abc68775a2d6d0
---
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
2 files changed, 61 insertions(+), 6 deletions(-)
To view, visit change 400494. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Apr 15, 2022, 12:09:30 PM4/15/22
to goph...@pubsubhelper.golang.org, Damien Neil, Gopher Robot, golang-co...@googlegroups.com
Patch set 2:Run-TryBot +1Auto-Submit +1Code-Review +1
1 comment:
File src/crypto/x509/x509.go:
Patch Set #1, Line 1490:
return nil, errors.New("x509: serial must be positive")Maybe "serial number must be positive" for consistency with the next error.
Done
To view, visit change 400494. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Apr 15, 2022, 12:25:58 PM4/15/22
to Roland Shoemaker, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Damien Neil, golang-co...@googlegroups.com
Gopher Robot submitted this change.
1 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: 1, Deletions: 1.
@@ -3655,7 +3655,7 @@
NotAfter: time.Now().Add(time.Hour),
NotBefore: time.Now().Add(-time.Hour),
}
- expectedErr := "x509: serial must be positive"
+ expectedErr := "x509: serial number must be positive"
_, err = CreateCertificate(rand.Reader, tmpl, tmpl, k.Public(), k)
if err == nil || err.Error() != expectedErr {
t.Errorf("CreateCertificate returned unexpected error: want %q, got %q", expectedErr, err)
```
```
The name of the file: src/crypto/x509/x509.go
Insertions: 1, Deletions: 1.
@@ -1487,7 +1487,7 @@
// so we need to double check the composition of the serial if it is exactly
// 20 bytes.
if template.SerialNumber.Sign() == -1 {
- return nil, errors.New("x509: serial must be positive")
+ return nil, errors.New("x509: serial number must be positive")
}
serialBytes := template.SerialNumber.Bytes()
if len(serialBytes) > 20 || (len(serialBytes) == 20 && serialBytes[0]&0x80 != 0) {
```
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: don't create certs with negative serials
Refuse to create certificates with negative serial numbers, as they
are explicitly disallowed by RFC 5280.
We still allow parsing certificates with negative serial numbers,
because in the past there were buggy CA implementations which would
produce them (although there are currently *no* trusted certificates
that have this issue). We may want to revisit this decision if we can
find metrics about the prevalence of this issue in enterprise settings.
Change-Id: I131262008db99b6354f542f335abc68775a2d6d0
Reviewed-on: https://go-review.googlesource.com/c/go/+/400494
Reviewed-by: Damien Neil <dn...@google.com>
Reviewed-by: Roland Shoemaker <rol...@golang.org>
Run-TryBot: Roland Shoemaker <rol...@golang.org>
Auto-Submit: Roland Shoemaker <rol...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
2 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 6d99191..8823ff8 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1478,13 +1478,17 @@
return nil, errors.New("x509: no SerialNumber given")
}
- // RFC 5280 Section 4.1.2.2: serial number must not be longer than 20 octets
+ // RFC 5280 Section 4.1.2.2: serial number must positive and should not be longer
+ // than 20 octets.
//
// We cannot simply check for len(serialBytes) > 20, because encoding/asn1 may
// pad the slice in order to prevent the integer being mistaken for a negative
// number (DER uses the high bit of the left-most byte to indicate the sign.),
// so we need to double check the composition of the serial if it is exactly
// 20 bytes.
+ if template.SerialNumber.Sign() == -1 {
+ return nil, errors.New("x509: serial number must be positive")
+ }
serialBytes := template.SerialNumber.Bytes()
if len(serialBytes) > 20 || (len(serialBytes) == 20 && serialBytes[0]&0x80 != 0) {
return nil, errors.New("x509: serial number exceeds 20 octets")
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index c294f91..4806ef3 100644+ expectedErr := "x509: serial number must be positive"
+ _, err = CreateCertificate(rand.Reader, tmpl, tmpl, k.Public(), k)
+ if err == nil || err.Error() != expectedErr {
+ t.Errorf("CreateCertificate returned unexpected error: want %q, got %q", expectedErr, err)
+ }
+}
To view, visit change 400494. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages