4 files changed, 74 insertions, 58 deletions
diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 218d794cca4..a44f5d6326a 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -7,6 +7,7 @@ package x509
 import (
 	"bytes"
 	"crypto"
+	"crypto/x509/pkix"
 	"errors"
 	"fmt"
 	"net"
@@ -837,6 +838,50 @@ func appendToFreshChain(chain []*Certificate, cert *Certificate) []*Certificate
 	return n
 }
 
+// alreadyInChain checks whether a candidate certificate is present in a chain.
+// Rather than doing a direct byte for byte equivalency check, we check if the
+// subject, public key, and SAN, if present, are equal. This prevents loops that
+// are created by mutual cross-signatures, or other cross-signature bridge
+// oddities.
+func alreadyInChain(candidate *Certificate, chain []*Certificate) bool {
+	type pubKeyEqual interface {
+		Equal(crypto.PublicKey) bool
+	}
+
+	var candidateSAN *pkix.Extension
+	for _, ext := range candidate.Extensions {
+		if ext.Id.Equal(oidExtensionSubjectAltName) {
+			candidateSAN = &ext
+			break
+		}
+	}
+
+	for _, cert := range chain {
+		if !bytes.Equal(candidate.RawSubject, cert.RawSubject) {
+			continue
+		}
+		if !candidate.PublicKey.(pubKeyEqual).Equal(cert.PublicKey) {
+			continue
+		}
+		var certSAN *pkix.Extension
+		for _, ext := range cert.Extensions {
+			if ext.Id.Equal(oidExtensionSubjectAltName) {
+				certSAN = &ext
+				break
+			}
+		}
+		if candidateSAN == nil && certSAN == nil {
+			return true
+		} else if candidateSAN == nil || certSAN == nil {
+			return false
+		}
+		if bytes.Equal(candidateSAN.Value, certSAN.Value) {
+			return true
+		}
+	}
+	return false
+}
+
 // maxChainSignatureChecks is the maximum number of CheckSignatureFrom calls
 // that an invocation of buildChains will (transitively) make. Most chains are
 // less than 15 certificates long, so this leaves space for multiple chains and
@@ -849,18 +894,9 @@ func (c *Certificate) buildChains(currentChain []*Certificate, sigChecks *int, o
 		hintCert *Certificate
 	)
 
-	type pubKeyEqual interface {
-		Equal(crypto.PublicKey) bool
-	}
-
 	considerCandidate := func(certType int, candidate *Certificate) {
-		for _, cert := range currentChain {
-			// If a certificate already appeared in the chain we've built, don't
-			// reconsider it. This prevents loops, for isntance those created by
-			// mutual cross-signatures, or other cross-signature bridges oddities.
-			if bytes.Equal(cert.RawSubject, candidate.RawSubject) && cert.PublicKey.(pubKeyEqual).Equal(candidate.PublicKey) {
-				return
-			}
+		if alreadyInChain(candidate, currentChain) {
+			return
 		}
 
 		if sigChecks == nil {
diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index 1b2cbe34dd2..8a7b08ab58f 100644
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -2340,6 +2340,29 @@ func TestPathBuilding(t *testing.T) {
 				"CN=leaf -> CN=inter b -> CN=inter c -> CN=root",
 			},
 		},
+		{
+			// Build a simple two node graph, where the leaf is directly issued from
+			// the root and both certificates have matching subject and public key, but
+			// the leaf has SANs.
+			name: "leaf with same subject, key, as parent but with SAN",
+			graph: trustGraphDescription{
+				Roots: []string{"root"},
+				Leaf:  "root",
+				Graph: []trustGraphEdge{
+					{
+						Issuer:  "root",
+						Subject: "root",
+						Type:    leafCertificate,
+						MutateTemplate: func(c *Certificate) {
+							c.DNSNames = []string{"localhost"}
+						},
+					},
+				},
+			},
+			expectedChains: []string{
+				"CN=root -> CN=root",
+			},
+		},
 	}
 
 	for _, tc := range tests {
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index ceb04ae20e9..582e1b15195 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1478,21 +1478,14 @@ 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 positive and should not be longer
-	// than 20 octets.
+	// RFC 5280 Section 4.1.2.2: serial number must positive
 	//
-	// 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.
+	// We _should_ also restrict serials to <= 20 octets, but it turns out a lot of people
+	// get this wrong, in part because the encoding can itself alter the length of the
+	// serial. For now we accept these non-conformant serials.
 	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")
-	}
 
 	if template.BasicConstraintsValid && !template.IsCA && template.MaxPathLen != -1 && (template.MaxPathLen != 0 || template.MaxPathLenZero) {
 		return nil, errors.New("x509: only CAs are allowed to specify MaxPathLen")
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 486d6bf3d23..f68dd0299a6 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -3589,42 +3589,6 @@ func TestOmitEmptyExtensions(t *testing.T) {
 	}
 }
 
-func TestCreateCertificateLongSerial(t *testing.T) {
-	k, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	serialBytes := make([]byte, 21)
-	serialBytes[0] = 0x80
-	serialBytes[20] = 1
-	tooLong := big.NewInt(0).SetBytes(serialBytes)
-
-	tmpl := &Certificate{
-		SerialNumber: tooLong,
-		Subject: pkix.Name{
-			CommonName: ":)",
-		},
-		NotAfter:  time.Now().Add(time.Hour),
-		NotBefore: time.Now().Add(-time.Hour),
-	}
-
-	expectedErr := "x509: serial number exceeds 20 octets"
-
-	_, 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)
-	}
-
-	serialBytes = serialBytes[:20]
-	tmpl.SerialNumber = big.NewInt(0).SetBytes(serialBytes)
-
-	_, 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)
-	}
-}
-
 var negativeSerialCert = `-----BEGIN CERTIFICATE-----
 MIIBBTCBraADAgECAgH/MAoGCCqGSM49BAMCMA0xCzAJBgNVBAMTAjopMB4XDTIy
 MDQxNDIzNTYwNFoXDTIyMDQxNTAxNTYwNFowDTELMAkGA1UEAxMCOikwWTATBgcq