Add X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS.
This change adds a new flag, X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS,
which causes basicConstraints with isCA to be required for intermediate
CA certificates. Without this, intermediates are also acceptable if
they're missing basicConstraints, but include either a certSign
keyUsage, or a CA Netscape certificate type.
This is a short-term change for patching. I'll undo a lot of it and make
this the default in the next change.
Change-Id: I7f42ffd76c57de3037f054108951e230c1b4e415
Reviewed-on: https://boringssl-review.googlesource.com/30184
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Matt Braithwaite <mab@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 0711992..df236ae 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -495,6 +495,137 @@
// /JKuuFGmzkG+rUbXFmo/Zg2ozVplw71NnQJ4znPsf7A=
// -----END RSA PRIVATE KEY-----
+// The following four certificates were generated with this Go program, varying
+// |includeNetscapeExtension| and defining rootKeyPEM and rootCertPEM to be
+// strings containing the kSANTypesRoot, above.
+
+// package main
+
+// import (
+// "crypto/ecdsa"
+// "crypto/elliptic"
+// "crypto/rand"
+// "crypto/x509"
+// "crypto/x509/pkix"
+// "encoding/asn1"
+// "encoding/pem"
+// "math/big"
+// "os"
+// "time"
+// )
+
+// const includeNetscapeExtension = true
+
+// func main() {
+// block, _ := pem.Decode([]byte(rootKeyPEM))
+// rootPriv, _ := x509.ParsePKCS1PrivateKey(block.Bytes)
+// block, _ = pem.Decode([]byte(rootCertPEM))
+// root, _ := x509.ParseCertificate(block.Bytes)
+
+// interTemplate := &x509.Certificate{
+// SerialNumber: big.NewInt(2),
+// Subject: pkix.Name{
+// CommonName: "No Basic Constraints (Netscape)",
+// },
+// NotBefore: time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC),
+// NotAfter: time.Date(2099, time.January, 1, 0, 0, 0, 0, time.UTC),
+// }
+
+// if includeNetscapeExtension {
+// interTemplate.ExtraExtensions = []pkix.Extension{
+// pkix.Extension{
+// Id: asn1.ObjectIdentifier([]int{2, 16, 840, 1, 113730, 1, 1}),
+// Value: []byte{0x03, 0x02, 2, 0x04},
+// },
+// }
+// } else {
+// interTemplate.KeyUsage = x509.KeyUsageCertSign
+// }
+
+// interKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+
+// interDER, err := x509.CreateCertificate(rand.Reader, interTemplate, root, &interKey.PublicKey, rootPriv)
+// if err != nil {
+// panic(err)
+// }
+
+// pem.Encode(os.Stdout, &pem.Block{Type: "CERTIFICATE", Bytes: interDER})
+
+// inter, _ := x509.ParseCertificate(interDER)
+
+// leafTemplate := &x509.Certificate{
+// SerialNumber: big.NewInt(3),
+// Subject: pkix.Name{
+// CommonName: "Leaf from CA with no Basic Constraints",
+// },
+// NotBefore: time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC),
+// NotAfter: time.Date(2099, time.January, 1, 0, 0, 0, 0, time.UTC),
+// BasicConstraintsValid: true,
+// }
+// leafKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+
+// leafDER, err := x509.CreateCertificate(rand.Reader, leafTemplate, inter, &leafKey.PublicKey, interKey)
+// if err != nil {
+// panic(err)
+// }
+
+// pem.Encode(os.Stdout, &pem.Block{Type: "CERTIFICATE", Bytes: leafDER})
+// }
+
+// kNoBasicConstraintsCertSignIntermediate doesn't have isCA set, but contains
+// certSign in the keyUsage.
+static const char kNoBasicConstraintsCertSignIntermediate[] =
+ "-----BEGIN CERTIFICATE-----\n"
+ "MIIBqjCCAROgAwIBAgIBAjANBgkqhkiG9w0BAQsFADArMRcwFQYDVQQKEw5Cb3Jp\n"
+ "bmdTU0wgVGVzdDEQMA4GA1UEAxMHUm9vdCBDQTAgFw0wMDAxMDEwMDAwMDBaGA8y\n"
+ "MDk5MDEwMTAwMDAwMFowHzEdMBsGA1UEAxMUTm8gQmFzaWMgQ29uc3RyYWludHMw\n"
+ "WTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAASEFMblfxIEDO8My7wHtHWTuDzNyID1\n"
+ "OsPkMGkn32O/pSyXxXuAqDeFoMVffUMTyfm8JcYugSEbrv2qEXXM4bZRoy8wLTAO\n"
+ "BgNVHQ8BAf8EBAMCAgQwGwYDVR0jBBQwEoAQQDfXAftAL7gcflQEJ4xZATANBgkq\n"
+ "hkiG9w0BAQsFAAOBgQC1Lh6hIAm3K5kRh5iIydU0YAEm7eV6ZSskERDUq3DLJyl9\n"
+ "ZUZCHUzvb464dkwZjeNzaUVS1pdElJslwX3DtGgeJLJGCnk8zUjBjaNrrDm0kzPW\n"
+ "xKt/6oif1ci/KCKqKNXJAIFbc4e+IiBpenwpxHk3If4NM+Ek0nKoO8Uj0NkgTQ==\n"
+ "-----END CERTIFICATE-----\n";
+
+static const char kNoBasicConstraintsCertSignLeaf[] =
+ "-----BEGIN CERTIFICATE-----\n"
+ "MIIBUDCB96ADAgECAgEDMAoGCCqGSM49BAMCMB8xHTAbBgNVBAMTFE5vIEJhc2lj\n"
+ "IENvbnN0cmFpbnRzMCAXDTAwMDEwMTAwMDAwMFoYDzIwOTkwMTAxMDAwMDAwWjAx\n"
+ "MS8wLQYDVQQDEyZMZWFmIGZyb20gQ0Egd2l0aCBubyBCYXNpYyBDb25zdHJhaW50\n"
+ "czBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABEsYPMwzdJKjB+2gpC90ib2ilHoB\n"
+ "w/arQ6ikUX0CNUDDaKaOu/jF39ogzVlg4lDFrjCKShSfCCcrwgONv70IZGijEDAO\n"
+ "MAwGA1UdEwEB/wQCMAAwCgYIKoZIzj0EAwIDSAAwRQIgbV7R99yM+okXSIs6Fp3o\n"
+ "eCOXiDL60IBxaTOcLS44ywcCIQDbn87Gj5cFgHBYAkzdHqDsyGXkxQTHDq9jmX24\n"
+ "Djy3Zw==\n"
+ "-----END CERTIFICATE-----\n";
+
+// kNoBasicConstraintsNetscapeCAIntermediate doesn't have isCA set, but contains
+// a Netscape certificate-type extension that asserts a type of "SSL CA".
+static const char kNoBasicConstraintsNetscapeCAIntermediate[] =
+ "-----BEGIN CERTIFICATE-----\n"
+ "MIIBuDCCASGgAwIBAgIBAjANBgkqhkiG9w0BAQsFADArMRcwFQYDVQQKEw5Cb3Jp\n"
+ "bmdTU0wgVGVzdDEQMA4GA1UEAxMHUm9vdCBDQTAgFw0wMDAxMDEwMDAwMDBaGA8y\n"
+ "MDk5MDEwMTAwMDAwMFowKjEoMCYGA1UEAxMfTm8gQmFzaWMgQ29uc3RyYWludHMg\n"
+ "KE5ldHNjYXBlKTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABCeMbmCaOtMzXBqi\n"
+ "PrCdNOH23CkaawUA+pAezitAN4RXS1O2CGK5sJjGPVVeogROU8G7/b+mU+ciZIzH\n"
+ "1PP8FJKjMjAwMBsGA1UdIwQUMBKAEEA31wH7QC+4HH5UBCeMWQEwEQYJYIZIAYb4\n"
+ "QgEBBAQDAgIEMA0GCSqGSIb3DQEBCwUAA4GBAAgNWjh7cfBTClTAk+Ml//5xb9Ju\n"
+ "tkBhG6Rm+kkMD+qiSMO6t7xS7CsA0+jIBjkdEYaLZ3oxtQCBdZsVNxUvRxZ0AUfF\n"
+ "G3DtRFTsrI1f7IQhpMuqEMF4shPW+5x54hrq0Fo6xMs6XoinJZcTUaaB8EeXRF6M\n"
+ "P9p6HuyLrmn0c/F0\n"
+ "-----END CERTIFICATE-----\n";
+
+static const char kNoBasicConstraintsNetscapeCALeaf[] =
+ "-----BEGIN CERTIFICATE-----\n"
+ "MIIBXDCCAQKgAwIBAgIBAzAKBggqhkjOPQQDAjAqMSgwJgYDVQQDEx9ObyBCYXNp\n"
+ "YyBDb25zdHJhaW50cyAoTmV0c2NhcGUpMCAXDTAwMDEwMTAwMDAwMFoYDzIwOTkw\n"
+ "MTAxMDAwMDAwWjAxMS8wLQYDVQQDEyZMZWFmIGZyb20gQ0Egd2l0aCBubyBCYXNp\n"
+ "YyBDb25zdHJhaW50czBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABDlJKolDu3R2\n"
+ "tPqSDycr0QJcWhxdBv76V0EEVflcHRxED6vAioTEcnQszt1OfKtBZvjlo0yp6i6Q\n"
+ "DaYit0ZInmWjEDAOMAwGA1UdEwEB/wQCMAAwCgYIKoZIzj0EAwIDSAAwRQIhAJsh\n"
+ "aZL6BHeEfoUBj1oZ2Ln91qzj3UCVMJ+vrmwAFdYyAiA3wp2JphgchvmoUFuzPXwj\n"
+ "XyPwWPbymSTpzKhB4xB7qQ==\n"
+ "-----END CERTIFICATE-----\n";
// CertFromPEM parses the given, NUL-terminated pem block and returns an
// |X509*|.
@@ -1341,3 +1472,55 @@
}
}
}
+
+TEST(X509Test, NoBasicConstraintsCertSign) {
+ bssl::UniquePtr<X509> root(CertFromPEM(kSANTypesRoot));
+ bssl::UniquePtr<X509> intermediate(
+ CertFromPEM(kNoBasicConstraintsCertSignIntermediate));
+ bssl::UniquePtr<X509> leaf(CertFromPEM(kNoBasicConstraintsCertSignLeaf));
+
+ ASSERT_TRUE(root);
+ ASSERT_TRUE(intermediate);
+ ASSERT_TRUE(leaf);
+
+ // The intermediate has keyUsage certSign, but is not marked as a CA in the
+ // basicConstraints. Sadly, since BoringSSL is based on OpenSSL 1.0.2, this is
+ // considered acceptable by default.
+ EXPECT_EQ(X509_V_OK,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, {}, 0));
+
+ // Setting either STRICT or REQUIRE_CA_BASIC_CONSTRAINTS should trigger an
+ // error.
+ EXPECT_EQ(X509_V_ERR_INVALID_CA,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, {},
+ X509_V_FLAG_X509_STRICT));
+ EXPECT_EQ(X509_V_ERR_INVALID_CA,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, {},
+ X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS));
+}
+
+TEST(X509Test, NoBasicConstraintsNetscapeCA) {
+ bssl::UniquePtr<X509> root(CertFromPEM(kSANTypesRoot));
+ bssl::UniquePtr<X509> intermediate(
+ CertFromPEM(kNoBasicConstraintsNetscapeCAIntermediate));
+ bssl::UniquePtr<X509> leaf(CertFromPEM(kNoBasicConstraintsNetscapeCALeaf));
+
+ ASSERT_TRUE(root);
+ ASSERT_TRUE(intermediate);
+ ASSERT_TRUE(leaf);
+
+ // The intermediate has a Netscape certificate type of "SSL CA", but is not
+ // marked as a CA in the basicConstraints. Sadly, since BoringSSL is based on
+ // OpenSSL 1.0.2, this is considered acceptable by default.
+ EXPECT_EQ(X509_V_OK,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, {}, 0));
+
+ // Setting either STRICT or REQUIRE_CA_BASIC_CONSTRAINTS should trigger an
+ // error.
+ EXPECT_EQ(X509_V_ERR_INVALID_CA,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, {},
+ X509_V_FLAG_X509_STRICT));
+ EXPECT_EQ(X509_V_ERR_INVALID_CA,
+ Verify(leaf.get(), {root.get()}, {intermediate.get()}, {},
+ X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS));
+}
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index d788c5e..fdce251 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -648,7 +648,11 @@
default:
if ((ret == 0)
|| ((ctx->param->flags & X509_V_FLAG_X509_STRICT)
- && (ret != 1))) {
+ && (ret != 1))
+ || ((ctx->param->flags &
+ X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS)
+ && (ret != 1))
+ ) {
ret = 0;
ctx->error = X509_V_ERR_INVALID_CA;
} else
diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h
index 97a07d5..6a7c554 100644
--- a/include/openssl/x509_vfy.h
+++ b/include/openssl/x509_vfy.h
@@ -410,6 +410,8 @@
#define X509_V_FLAG_SUITEB_192_LOS 0x20000
/* Suite B 128 bit mode allowing 192 bit algorithms */
#define X509_V_FLAG_SUITEB_128_LOS 0x30000
+/* Disable workarounds for broken certificates */
+#define X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS 0x40000
/* Allow partial chains if at least one certificate is in trusted store */
#define X509_V_FLAG_PARTIAL_CHAIN 0x80000
