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 <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 6d99191fefa59bccfbb56a72de6675c83296306d..8823ff8a26298458a2d7609344e0f66437e05dc4 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1478,13 +1478,17 @@ func CreateCertificate(rand io.Reader, template, parent *Certificate, pub, priv
                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 c294f91ed645de840d30bf39f2d342bad8d3eb7a..4806ef3493efd54dda5c9ae246d27ef77eeb728d 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -602,11 +602,7 @@ func TestCreateSelfSignedCertificate(t *testing.T) {
        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 @@ func TestCreateCertificateLongSerial(t *testing.T) {
                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 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)
+       }
+}