From 3a7a856951c69e6c279b4305030c5da6ca8af913 Mon Sep 17 00:00:00 2001 From: Mateusz Poliwczak Date: Tue, 20 May 2025 16:21:22 +0000 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI Auto-Submit: Roland Shoemaker Reviewed-by: David Chase --- src/crypto/x509/parser.go | 7 ++++-- src/crypto/x509/parser_test.go | 43 ++++++++++++++++++++++++++++++++++ src/crypto/x509/x509.go | 4 ++++ src/crypto/x509/x509_test.go | 25 ++++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go index b99c776f09..4abcc1b7b5 100644 --- 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 e7c1d87bfa..3b9d9aed82 100644 --- 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 788b9aca9b..b2543d0727 100644 --- 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 7c8972eef4..98f3f7941c 100644 --- 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) + } +} -- 2.50.0