crypto/x509: disallow negative path length

pathLenConstraint is restricted to unsigned integers.
Also the -1 value of cert.MaxPathLength has a special
meaning, so we shouldn't allow unmarshaling -1.

BasicConstraints ::= SEQUENCE {
     cA                      BOOLEAN DEFAULT FALSE,
     pathLenConstraint       INTEGER (0..MAX) OPTIONAL }

Change-Id: I485a6aa7223127becc86c423e1ef9ed2fbd48209
GitHub-Last-Rev: 75a11b47b963ac383d1ad67dfc001648632a05f0
GitHub-Pull-Request: golang/go#60706
Reviewed-on: https://go-review.googlesource.com/c/go/+/502076
Reviewed-by: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: David Chase <drchase@google.com>

diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index b99c776f09b93fa4fc3525e25356102bb92dfde3..4abcc1b7b590e2442d4b35989b0da84baa2b2d94 100644 (file)
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -17,6 +17,7 @@ import (
        "errors"
        "fmt"
        "internal/godebug"
+       "math"
        "math/big"
        "net"
        "net/url"
@@ -373,14 +374,16 @@ func parseBasicConstraintsExtension(der cryptobyte.String) (bool, int, error) {
                        return false, 0, errors.New("x509: invalid basic constraints")
                }
        }
+
        maxPathLen := -1
        if der.PeekASN1Tag(cryptobyte_asn1.INTEGER) {
-               if !der.ReadASN1Integer(&maxPathLen) {
+               var mpl uint
+               if !der.ReadASN1Integer(&mpl) || mpl > math.MaxInt {
                        return false, 0, errors.New("x509: invalid basic constraints")
                }
+               maxPathLen = int(mpl)
        }
 
-       // TODO: map out.MaxPathLen to 0 if it has the -1 default value? (Issue 19285)
        return isCA, maxPathLen, nil
 }
 

diff --git a/src/crypto/x509/parser_test.go b/src/crypto/x509/parser_test.go
index e7c1d87bfa8efe2a8f38f6da84082231b6d70e80..3b9d9aed826b01bbfbf4e2cab6f4a3d2c743f637 100644 (file)
--- a/src/crypto/x509/parser_test.go
+++ b/src/crypto/x509/parser_test.go
@@ -208,3 +208,46 @@ func TestParsePolicies(t *testing.T) {
                })
        }
 }
+
+func TestParseCertificateNegativeMaxPathLength(t *testing.T) {
+       certs := []string{
+               // Certificate with MaxPathLen set to -1.
+               `
+-----BEGIN CERTIFICATE-----
+MIIByTCCATKgAwIBAgIBATANBgkqhkiG9w0BAQsFADAPMQ0wCwYDVQQDEwRURVNU
+MB4XDTcwMDEwMTAwMTY0MFoXDTcwMDEwMjAzNDY0MFowDzENMAsGA1UEAxMEVEVT
+VDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAsaHglFuSicTT8TKfipgsSi3N
+Wb/TcvuAhanFF1VGB+vS95kO7yFqyfRgX3GgOwT0KlJVsVjPjghEGR9RGTSLqkTD
+UFbiBgm8+VEPMOrUtIHIHXhl+ye44AkOEStxfz7gjN/EAS2h8ffPKhvDTHOlShKw
+Y3LQlxR0LdeJXq3eSqUCAwEAAaM1MDMwEgYDVR0TAQH/BAgwBgEB/wIB/zAdBgNV
+HQ4EFgQUrbrk0tqQAEsce8uYifP0BIVhuFAwDQYJKoZIhvcNAQELBQADgYEAIkhV
+ZBj1ThT+eyh50XsoU570NUysTg3Nj/3lbkEolzdcE+wu0CPXvgxLRM6Y62u1ey82
+8d5VQHstzF4dXgc3W+O9UySa+CKdcHx/q7o7seOGXdysT0IJtAY3w66mFkuF7PIn
+y9b7M5t6pmWjb7N0QqGuWeNqi4ZvS8gLKmVEgGY=
+-----END CERTIFICATE-----
+`,
+               // Certificate with MaxPathLen set to -2.
+               `
+-----BEGIN CERTIFICATE-----
+MIIByTCCATKgAwIBAgIBATANBgkqhkiG9w0BAQsFADAPMQ0wCwYDVQQDEwRURVNU
+MB4XDTcwMDEwMTAwMTY0MFoXDTcwMDEwMjAzNDY0MFowDzENMAsGA1UEAxMEVEVT
+VDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAsaHglFuSicTT8TKfipgsSi3N
+Wb/TcvuAhanFF1VGB+vS95kO7yFqyfRgX3GgOwT0KlJVsVjPjghEGR9RGTSLqkTD
+UFbiBgm8+VEPMOrUtIHIHXhl+ye44AkOEStxfz7gjN/EAS2h8ffPKhvDTHOlShKw
+Y3LQlxR0LdeJXq3eSqUCAwEAAaM1MDMwEgYDVR0TAQH/BAgwBgEB/wIB/jAdBgNV
+HQ4EFgQUrbrk0tqQAEsce8uYifP0BIVhuFAwDQYJKoZIhvcNAQELBQADgYEAGjIr
+YGQc7Ods+BuKck7p+vpAMONM8SLEuUtKorCP3ecsO51MoA4/niLbgMHaOGNHwzMp
+ajg0zLbY0Dj6Ml0VZ+lS3rjgTEhYXc626eZkoQqgUzL1jhe3S0ZbSxxmHMBKjJFl
+d5l1tRhScKu2NBgm74nYmJxJYgvuTA38wGhRrGU=
+-----END CERTIFICATE-----
+`,
+       }
+
+       for _, cert := range certs {
+               b, _ := pem.Decode([]byte(cert))
+               _, err := ParseCertificate(b.Bytes)
+               if err == nil || err.Error() != "x509: invalid basic constraints" {
+                       t.Errorf(`ParseCertificate() = %v; want = "x509: invalid basic constraints"`, err)
+               }
+       }
+}

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 788b9aca9bd13d5716b9fedfb2c931b61545484c..b2543d0727eb812356ed6f19feef74436c6e98e1 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1690,6 +1690,10 @@ func CreateCertificate(rand io.Reader, template, parent *Certificate, pub, priv
                return nil, errors.New("x509: serial number must be positive")
        }
 
+       if template.BasicConstraintsValid && template.MaxPathLen < -1 {
+               return nil, errors.New("x509: invalid MaxPathLen, must be greater or equal to -1")
+       }
+
        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 7c8972eef4ddcd7b38860957c957dde8aae74e5c..98f3f7941c8d63b265673bfc5a28707eb12dff99 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -4238,3 +4238,28 @@ func TestMessageSigner(t *testing.T) {
                t.Fatalf("CheckSignatureFrom failed: %s", err)
        }
 }
+
+func TestCreateCertificateNegativeMaxPathLength(t *testing.T) {
+       template := Certificate{
+               SerialNumber:          big.NewInt(1),
+               Subject:               pkix.Name{CommonName: "TEST"},
+               NotBefore:             time.Unix(1000, 0),
+               NotAfter:              time.Unix(100000, 0),
+               BasicConstraintsValid: true,
+               IsCA:                  true,
+
+               // CreateCertificate treats -1 in the same way as: MaxPathLen == 0 && MaxPathLenZero == false.
+               MaxPathLen: -1,
+       }
+
+       _, err := CreateCertificate(rand.Reader, &template, &template, rsaPrivateKey.Public(), rsaPrivateKey)
+       if err != nil {
+               t.Fatalf("CreateCertificate() unexpected error: %v", err)
+       }
+
+       template.MaxPathLen = -2
+       _, err = CreateCertificate(rand.Reader, &template, &template, rsaPrivateKey.Public(), rsaPrivateKey)
+       if err == nil || err.Error() != "x509: invalid MaxPathLen, must be greater or equal to -1" {
+               t.Fatalf(`CreateCertificate() = %v; want = "x509: invalid MaxPathLen, must be greater or equal to -1"`, err)
+       }
+}