From 7e203652ef2f0e6dd9f8892d04f6643d3ef12ff2 Mon Sep 17 00:00:00 2001
From: Sun Yimin <emmansun@users.noreply.github.com>
Date: Mon, 26 May 2025 13:18:51 +0800
Subject: [PATCH] smx509: disallow negative path length #329

---
 smx509/parser.go      |  7 +++++--
 smx509/parser_test.go | 44 +++++++++++++++++++++++++++++++++++++++++++
 smx509/x509.go        |  4 ++++
 smx509/x509_test.go   | 25 ++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/smx509/parser.go b/smx509/parser.go
index 95671a5..bbae447 100644
--- a/smx509/parser.go
+++ b/smx509/parser.go
@@ -14,6 +14,7 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
+	"math"
 	"math/big"
 	"net"
 	"net/url"
@@ -93,7 +94,7 @@ func parseASN1String(tag cryptobyte_asn1.Tag, value []byte) (string, error) {
 		// treat a UCS-2 encoded string as a UTF-16 encoded string, as long as
 		// we reject out the UTF-16 specific code points. This matches the
 		// BoringSSL behavior.
-			
+
 		if len(value)%2 != 0 {
 			return "", errors.New("invalid BMPString")
 		}
@@ -434,9 +435,11 @@ func parseBasicConstraintsExtension(der cryptobyte.String) (bool, int, error) {
 	}
 	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)
diff --git a/smx509/parser_test.go b/smx509/parser_test.go
index 0ac9ad6..e422e75 100644
--- a/smx509/parser_test.go
+++ b/smx509/parser_test.go
@@ -3,6 +3,7 @@ package smx509
 import (
 	"encoding/asn1"
 	"encoding/hex"
+	"encoding/pem"
 	"testing"
 
 	cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
@@ -150,3 +151,46 @@ func TestParseSM2PublicKeyWithNistP256(t *testing.T) {
 		t.Fatal("should throw x509: unsupported SM2 curve")
 	}
 }
+
+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/smx509/x509.go b/smx509/x509.go
index 9df8d07..61c06ee 100644
--- a/smx509/x509.go
+++ b/smx509/x509.go
@@ -1565,6 +1565,10 @@ func CreateCertificate(rand io.Reader, template, parent, pub, priv any) ([]byte,
 		return nil, errors.New("x509: serial number must be positive")
 	}
 
+	if realTemplate.BasicConstraintsValid && realTemplate.MaxPathLen < -1 {
+		return nil, errors.New("x509: invalid MaxPathLen, must be greater or equal to -1")
+	}
+
 	if realTemplate.BasicConstraintsValid && !realTemplate.IsCA && realTemplate.MaxPathLen != -1 && (realTemplate.MaxPathLen != 0 || realTemplate.MaxPathLenZero) {
 		return nil, errors.New("x509: only CAs are allowed to specify MaxPathLen")
 	}
diff --git a/smx509/x509_test.go b/smx509/x509_test.go
index c2a2287..810537c 100644
--- a/smx509/x509_test.go
+++ b/smx509/x509_test.go
@@ -3654,3 +3654,28 @@ func TestRejectCriticalSKI(t *testing.T) {
 		t.Fatalf("ParseCertificate() unexpected error: %v, want: %s", err, expectedErr)
 	}
 }
+
+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)
+	}
+}