crypto/x509: reject critical SKI extensions

Updates #65085

Change-Id: I8a00fff6b2af4e55bcb88456813b5ee1f7b1c01d
Reviewed-on: https://go-review.googlesource.com/c/go/+/562344
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

2 files changed, 29 insertions, 0 deletions

diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go
index 726409e988..001b001775 100644
--- a/src/crypto/x509/parser.go
+++ b/src/crypto/x509/parser.go
@@ -741,6 +741,10 @@ func processExtensions(out *Certificate) error {
 				}
 			case 14:
 				// RFC 5280, 4.2.1.2
+				if e.Critical {
+					// Conforming CAs MUST mark this extension as non-critical
+					return errors.New("x509: subject key identifier incorrectly marked critical")
+				}
 				val := cryptobyte.String(e.Value)
 				var skid cryptobyte.String
 				if !val.ReadASN1(&skid, cryptobyte_asn1.OCTET_STRING) {
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 0f528d4cc2..a29f914c8e 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -4035,3 +4035,28 @@ func TestRejectCriticalAIA(t *testing.T) {
 		t.Fatalf("ParseCertificate() unexpected error: %v, want: %s", err, expectedErr)
 	}
 }
+
+func TestRejectCriticalSKI(t *testing.T) {
+	template := Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject:      pkix.Name{CommonName: "Cert"},
+		NotBefore:    time.Unix(1000, 0),
+		NotAfter:     time.Unix(100000, 0),
+		ExtraExtensions: []pkix.Extension{
+			{
+				Id:       asn1.ObjectIdentifier{2, 5, 29, 14},
+				Critical: true,
+				Value:    []byte{1, 2, 3},
+			},
+		},
+	}
+	certDER, err := CreateCertificate(rand.Reader, &template, &template, rsaPrivateKey.Public(), rsaPrivateKey)
+	if err != nil {
+		t.Fatalf("CreateCertificate() unexpected error: %v", err)
+	}
+	expectedErr := "x509: subject key identifier incorrectly marked critical"
+	_, err = ParseCertificate(certDER)
+	if err == nil || err.Error() != expectedErr {
+		t.Fatalf("ParseCertificate() unexpected error: %v, want: %s", err, expectedErr)
+	}
+}