crypto/x509: reject negative serial numbers

There is only one trusted certificate I could find in the web pki which
has a negative serial number. Removing this exception seems reasonable.

Updates #65085

Change-Id: I55435b3d75479dcb41d523383e4ff7894a1496ad
Reviewed-on: https://go-review.googlesource.com/c/go/+/562343
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>

diff --git a/doc/godebug.md b/doc/godebug.md
index e03261c6ae52f44a465724e35ebd66bd373793c3..c3b88d3de6627c6b331b71ca18e3f330d4fe0003 100644 (file)
--- a/doc/godebug.md
+++ b/doc/godebug.md
@@ -183,6 +183,11 @@ Go 1.23 enabled the experimental post-quantum key exchange mechanism
 X25519Kyber768Draft00 by default. The default can be reverted using the
 [`tlskyber` setting](/pkg/crypto/tls/#Config.CurvePreferences).
 
+Go 1.23 changed the behavior of
+[crypto/x509.ParseCertificate](/pkg/crypto/x509/#ParseCertificate) to reject
+serial numbers that are negative. This change can be reverted with
+the the [`x509negativeserial` setting](/pkg/crypto/x509/#ParseCertificate).
+
 ### Go 1.22
 
 Go 1.22 adds a configurable limit to control the maximum acceptable RSA key size

diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index 4202991f47ef22db8c91792bd3eea0886111c8fc..bcaa33ec6c46a99ee1152566546d5d219fbef302 100644 (file)
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -16,6 +16,7 @@ import (
        "encoding/asn1"
        "errors"
        "fmt"
+       "internal/godebug"
        "math/big"
        "net"
        "net/url"
@@ -815,6 +816,8 @@ func processExtensions(out *Certificate) error {
        return nil
 }
 
+var x509negativeserial = godebug.New("x509negativeserial")
+
 func parseCertificate(der []byte) (*Certificate, error) {
        cert := &Certificate{}
 
@@ -858,11 +861,13 @@ func parseCertificate(der []byte) (*Certificate, error) {
        if !tbs.ReadASN1Integer(serial) {
                return nil, errors.New("x509: malformed serial number")
        }
-       // we ignore the presence of negative serial numbers because
-       // of their prevalence, despite them being invalid
-       // TODO(rolandshoemaker): revisit this decision, there are currently
-       // only 10 trusted certificates with negative serial numbers
-       // according to censys.io.
+       if serial.Sign() == -1 {
+               if x509negativeserial.Value() != "1" {
+                       return nil, errors.New("x509: negative serial number")
+               } else {
+                       x509negativeserial.IncNonDefault()
+               }
+       }
        cert.SerialNumber = serial
 
        var sigAISeq cryptobyte.String
@@ -999,6 +1004,10 @@ func parseCertificate(der []byte) (*Certificate, error) {
 }
 
 // ParseCertificate parses a single certificate from the given ASN.1 DER data.
+//
+// Before Go 1.23, ParseCertificate accepted certificates with negative serial
+// numbers. This behavior can be restored by including "x509negativeserial=1" in
+// the GODEBUG environment variable.
 func ParseCertificate(der []byte) (*Certificate, error) {
        cert, err := parseCertificate(der)
        if err != nil {

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 90c7ce9076b5a81ab5a5450642ed5345510a0142..0bf2b044399e8c8174081cbc76364faef1377da1 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -1930,9 +1930,8 @@ func TestRSAMissingNULLParameters(t *testing.T) {
        }
 }
 
-const certISOOID = `
------BEGIN CERTIFICATE-----
-MIIB5TCCAVKgAwIBAgIQtwyL3RPWV7dJQp34HwZG9DAJBgUrDgMCHQUAMBExDzAN
+const certISOOID = `-----BEGIN CERTIFICATE-----
+MIIB5TCCAVKgAwIBAgIQNwyL3RPWV7dJQp34HwZG9DAJBgUrDgMCHQUAMBExDzAN
 BgNVBAMTBm15dGVzdDAeFw0xNjA4MDkyMjExMDVaFw0zOTEyMzEyMzU5NTlaMBEx
 DzANBgNVBAMTBm15dGVzdDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEArzIH
 GsyDB3ohIGkkvijF2PTRUX1bvOtY1eUUpjwHyu0twpAKSuaQv2Ha+/63+aHe8O86
@@ -3564,7 +3563,7 @@ func TestLargeOID(t *testing.T) {
 }
 
 const uniqueIDPEM = `-----BEGIN CERTIFICATE-----
-MIIFsDCCBJigAwIBAgIIrOyC1ydafZMwDQYJKoZIhvcNAQEFBQAwgY4xgYswgYgG
+MIIFsDCCBJigAwIBAgIILOyC1ydafZMwDQYJKoZIhvcNAQEFBQAwgY4xgYswgYgG
 A1UEAx6BgABNAGkAYwByAG8AcwBvAGYAdAAgAEYAbwByAGUAZgByAG8AbgB0ACAA
 VABNAEcAIABIAFQAVABQAFMAIABJAG4AcwBwAGUAYwB0AGkAbwBuACAAQwBlAHIA
 dABpAGYAaQBjAGEAdABpAG8AbgAgAEEAdQB0AGgAbwByAGkAdAB5MB4XDTE0MDEx
@@ -3831,8 +3830,8 @@ RqUAyJKFzqZxOlK2q4j2IYnuj5+LrLGbQA==
 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)
+       if err == nil {
+               t.Fatal("parsed certificate with negative serial")
        }
 }
 

diff --git a/src/internal/godebugs/table.go b/src/internal/godebugs/table.go
index 27ac6b300bd7670fc6b184c9c219a9bd81045fba..c07109e611d813f48e6d62268324a666789495cd 100644 (file)
--- a/src/internal/godebugs/table.go
+++ b/src/internal/godebugs/table.go
@@ -53,6 +53,7 @@ var All = []Info{
        {Name: "tlsunsafeekm", Package: "crypto/tls", Changed: 22, Old: "1"},
        {Name: "winreadlinkvolume", Package: "os", Changed: 22, Old: "0"},
        {Name: "winsymlink", Package: "os", Changed: 22, Old: "0"},
+       {Name: "x509negativeserial", Package: "crypto/x509", Changed: 23, Old: "1"},
        {Name: "x509sha1", Package: "crypto/x509"},
        {Name: "x509usefallbackroots", Package: "crypto/x509"},
        {Name: "x509usepolicies", Package: "crypto/x509"},

diff --git a/src/runtime/metrics/doc.go b/src/runtime/metrics/doc.go
index fbbeb1a4754cb20f65e4345a88bc24a28b817986..30e8671c0ca70ee8c49de5ab97ec1820ad055bfb 100644 (file)
--- a/src/runtime/metrics/doc.go
+++ b/src/runtime/metrics/doc.go
@@ -322,6 +322,11 @@ Below is the full list of supported metrics, ordered lexicographically.
                The number of non-default behaviors executed by the os package
                due to a non-default GODEBUG=winsymlink=... setting.
 
+       /godebug/non-default-behavior/x509negativeserial:events
+               The number of non-default behaviors executed by the crypto/x509
+               package due to a non-default GODEBUG=x509negativeserial=...
+               setting.
+
        /godebug/non-default-behavior/x509sha1:events
                The number of non-default behaviors executed by the crypto/x509
                package due to a non-default GODEBUG=x509sha1=... setting.