From 6dd53473092c9b3a9eabb20beaafd849057093ad Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 31 Jul 2018 16:19:17 -0700 Subject: 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. Bug: 111893041 Test: cts -m CtsLibcoreTestCases Merged-In: I47a45e6b6f46b19fcbcb6c917895867d56dcd2ca Change-Id: I9eaf0fa61141627da607a329974111c0f8841508 --- src/crypto/x509/x509_test.cc | 184 +++++++++++++++++++++++++++++++++++++++++ src/crypto/x509/x509_vfy.c | 6 +- src/include/openssl/x509_vfy.h | 2 + 3 files changed, 191 insertions(+), 1 deletion(-) diff --git a/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc index b4cecca2..b66f3650 100644 --- a/src/crypto/x509/x509_test.cc +++ b/src/crypto/x509/x509_test.cc @@ -422,6 +422,138 @@ static const char kEd25519CertNull[] = "recgVPpVS7B+d9g4EwtZXIh4lodTBDHBBw==\n" "-----END CERTIFICATE-----\n"; +// 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*|. static bssl::UniquePtr CertFromPEM(const char *pem) { @@ -1035,3 +1167,55 @@ TEST(X509Test, PrettyPrintIntegers) { } } } + +TEST(X509Test, NoBasicConstraintsCertSign) { + bssl::UniquePtr root(CertFromPEM(kSANTypesRoot)); + bssl::UniquePtr intermediate( + CertFromPEM(kNoBasicConstraintsCertSignIntermediate)); + bssl::UniquePtr 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 root(CertFromPEM(kSANTypesRoot)); + bssl::UniquePtr intermediate( + CertFromPEM(kNoBasicConstraintsNetscapeCAIntermediate)); + bssl::UniquePtr 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/src/crypto/x509/x509_vfy.c b/src/crypto/x509/x509_vfy.c index aff2ee95..8fd8f9a3 100644 --- a/src/crypto/x509/x509_vfy.c +++ b/src/crypto/x509/x509_vfy.c @@ -647,7 +647,11 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) 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/src/include/openssl/x509_vfy.h b/src/include/openssl/x509_vfy.h index 208a3807..97375374 100644 --- a/src/include/openssl/x509_vfy.h +++ b/src/include/openssl/x509_vfy.h @@ -394,6 +394,8 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #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 -- cgit v1.2.3 From b010c2ec82901588f818573f418e07a3217f3f7a Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Tue, 31 Jul 2018 16:42:31 -0700 Subject: Require basicConstraints cA flag in intermediate certs. OpenSSL 1.0.2 (and thus BoringSSL) accepts keyUsage certSign or a Netscape CA certificate-type in lieu of basicConstraints in an intermediate certificate (unless X509_V_FLAG_X509_STRICT) is set. Bug: 111893041 Test: cts -m CtsLibcoreTestCases Merged-In: I47a45e6b6f46b19fcbcb6c917895867d56dcd2ca Change-Id: I8adaa7547fdffd849087e4401bc3c258bbcd82f2 Update-Note: This change tightens the code so that basicConstraints is required for intermediate certificates when verifying chains. This was previously only enabled if X509_V_FLAG_X509_STRICT was set, but that flag also has other effects. --- src/crypto/x509/x509_test.cc | 28 +++------------ src/crypto/x509/x509_vfy.c | 60 +++++++++++++++----------------- src/crypto/x509v3/v3_purp.c | 78 +++++++++--------------------------------- src/include/openssl/x509_vfy.h | 5 ++- 4 files changed, 50 insertions(+), 121 deletions(-) diff --git a/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc index b66f3650..ecb6e7fb 100644 --- a/src/crypto/x509/x509_test.cc +++ b/src/crypto/x509/x509_test.cc @@ -1179,19 +1179,9 @@ TEST(X509Test, NoBasicConstraintsCertSign) { 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. + // basicConstraints. 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)); + Verify(leaf.get(), {root.get()}, {intermediate.get()}, {}, 0)); } TEST(X509Test, NoBasicConstraintsNetscapeCA) { @@ -1205,17 +1195,7 @@ TEST(X509Test, NoBasicConstraintsNetscapeCA) { 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. + // marked as a CA in the basicConstraints. 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)); + Verify(leaf.get(), {root.get()}, {intermediate.get()}, {}, 0)); } diff --git a/src/crypto/x509/x509_vfy.c b/src/crypto/x509/x509_vfy.c index 8fd8f9a3..ef3d0f4c 100644 --- a/src/crypto/x509/x509_vfy.c +++ b/src/crypto/x509/x509_vfy.c @@ -578,7 +578,7 @@ static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x) static int check_chain_extensions(X509_STORE_CTX *ctx) { - int i, ok = 0, must_be_ca, plen = 0; + int i, ok = 0, plen = 0; X509 *x; int (*cb) (int xok, X509_STORE_CTX *xctx); int proxy_path_length = 0; @@ -586,15 +586,13 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) int allow_proxy_certs; cb = ctx->verify_cb; - /* - * must_be_ca can have 1 of 3 values: -1: we accept both CA and non-CA - * certificates, to allow direct use of self-signed certificates (which - * are marked as CA). 0: we only accept non-CA certificates. This is - * currently not used, but the possibility is present for future - * extensions. 1: we only accept CA certificates. This is currently used - * for all certificates in the chain except the leaf certificate. - */ - must_be_ca = -1; + enum { + // ca_or_leaf allows either type of certificate so that direct use of + // self-signed certificates works. + ca_or_leaf, + must_be_ca, + must_not_be_ca, + } ca_requirement; /* CRL path validation */ if (ctx->parent) { @@ -606,6 +604,8 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) purpose = ctx->param->purpose; } + ca_requirement = ca_or_leaf; + /* Check all untrusted certificates */ for (i = 0; i < ctx->last_untrusted; i++) { int ret; @@ -627,37 +627,30 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) if (!ok) goto end; } - ret = X509_check_ca(x); - switch (must_be_ca) { - case -1: - if ((ctx->param->flags & X509_V_FLAG_X509_STRICT) - && (ret != 1) && (ret != 0)) { - ret = 0; - ctx->error = X509_V_ERR_INVALID_CA; - } else - ret = 1; + + switch (ca_requirement) { + case ca_or_leaf: + ret = 1; break; - case 0: - if (ret != 0) { + case must_not_be_ca: + if (X509_check_ca(x)) { ret = 0; ctx->error = X509_V_ERR_INVALID_NON_CA; } else ret = 1; break; - default: - if ((ret == 0) - || ((ctx->param->flags & X509_V_FLAG_X509_STRICT) - && (ret != 1)) - || ((ctx->param->flags & - X509_V_FLAG_REQUIRE_CA_BASIC_CONSTRAINTS) - && (ret != 1)) - ) { + case must_be_ca: + if (!X509_check_ca(x)) { ret = 0; ctx->error = X509_V_ERR_INVALID_CA; } else ret = 1; break; + default: + // impossible. + ret = 0; } + if (ret == 0) { ctx->error_depth = i; ctx->current_cert = x; @@ -666,7 +659,7 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) goto end; } if (ctx->param->purpose > 0) { - ret = X509_check_purpose(x, purpose, must_be_ca > 0); + ret = X509_check_purpose(x, purpose, ca_requirement == must_be_ca); if ((ret == 0) || ((ctx->param->flags & X509_V_FLAG_X509_STRICT) && (ret != 1))) { @@ -707,9 +700,10 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) goto end; } proxy_path_length++; - must_be_ca = 0; - } else - must_be_ca = 1; + ca_requirement = must_not_be_ca; + } else { + ca_requirement = must_be_ca; + } } ok = 1; end: diff --git a/src/crypto/x509v3/v3_purp.c b/src/crypto/x509v3/v3_purp.c index 324de85a..799f36c7 100644 --- a/src/crypto/x509v3/v3_purp.c +++ b/src/crypto/x509v3/v3_purp.c @@ -80,7 +80,6 @@ static void x509v3_cache_extensions(X509 *x); -static int check_ssl_ca(const X509 *x); static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x, int ca); static int check_purpose_ssl_server(const X509_PURPOSE *xp, const X509 *x, @@ -562,39 +561,20 @@ static void x509v3_cache_extensions(X509 *x) CRYPTO_MUTEX_unlock_write(&x->lock); } -/* - * CA checks common to all purposes return codes: 0 not a CA 1 is a CA 2 - * basicConstraints absent so "maybe" a CA 3 basicConstraints absent but self - * signed V1. 4 basicConstraints absent but keyUsage present and keyCertSign - * asserted. - */ - +/* check_ca returns one if |x| should be considered a CA certificate and zero + * otherwise. */ static int check_ca(const X509 *x) { /* keyUsage if present should allow cert signing */ if (ku_reject(x, KU_KEY_CERT_SIGN)) return 0; - if (x->ex_flags & EXFLAG_BCONS) { - if (x->ex_flags & EXFLAG_CA) - return 1; - /* If basicConstraints says not a CA then say so */ - else - return 0; - } else { - /* we support V1 roots for... uh, I don't really know why. */ - if ((x->ex_flags & V1_ROOT) == V1_ROOT) - return 3; - /* - * If key usage present it must have certSign so tolerate it - */ - else if (x->ex_flags & EXFLAG_KUSAGE) - return 4; - /* Older certificates could have Netscape-specific CA types */ - else if (x->ex_flags & EXFLAG_NSCERT && x->ex_nscert & NS_ANY_CA) - return 5; - /* can this still be regarded a CA certificate? I doubt it */ - return 0; + /* Version 1 certificates are considered CAs and don't have extensions. */ + if ((x->ex_flags & V1_ROOT) == V1_ROOT) { + return 1; } + /* Otherwise, it's only a CA if basicConstraints says so. */ + return ((x->ex_flags & EXFLAG_BCONS) && + (x->ex_flags & EXFLAG_CA)); } int X509_check_ca(X509 *x) @@ -603,27 +583,13 @@ int X509_check_ca(X509 *x) return check_ca(x); } -/* Check SSL CA: common checks for SSL client and server */ -static int check_ssl_ca(const X509 *x) -{ - int ca_ret; - ca_ret = check_ca(x); - if (!ca_ret) - return 0; - /* check nsCertType if present */ - if (ca_ret != 5 || x->ex_nscert & NS_SSL_CA) - return ca_ret; - else - return 0; -} - static int check_purpose_ssl_client(const X509_PURPOSE *xp, const X509 *x, int ca) { if (xku_reject(x, XKU_SSL_CLIENT)) return 0; if (ca) - return check_ssl_ca(x); + return check_ca(x); /* We need to do digital signatures or key agreement */ if (ku_reject(x, KU_DIGITAL_SIGNATURE | KU_KEY_AGREEMENT)) return 0; @@ -647,7 +613,7 @@ static int check_purpose_ssl_server(const X509_PURPOSE *xp, const X509 *x, if (xku_reject(x, XKU_SSL_SERVER | XKU_SGC)) return 0; if (ca) - return check_ssl_ca(x); + return check_ca(x); if (ns_reject(x, NS_SSL_SERVER)) return 0; @@ -677,15 +643,13 @@ static int purpose_smime(const X509 *x, int ca) if (xku_reject(x, XKU_SMIME)) return 0; if (ca) { - int ca_ret; - ca_ret = check_ca(x); - if (!ca_ret) - return 0; /* check nsCertType if present */ - if (ca_ret != 5 || x->ex_nscert & NS_SMIME_CA) - return ca_ret; - else - return 0; + if ((x->ex_flags & EXFLAG_NSCERT) && + (x->ex_nscert & NS_SMIME_CA) == 0) { + return 0; + } + + return check_ca(x); } if (x->ex_flags & EXFLAG_NSCERT) { if (x->ex_nscert & NS_SMIME) @@ -726,11 +690,7 @@ static int check_purpose_crl_sign(const X509_PURPOSE *xp, const X509 *x, int ca) { if (ca) { - int ca_ret; - if ((ca_ret = check_ca(x)) != 2) - return ca_ret; - else - return 0; + return check_ca(x); } if (ku_reject(x, KU_CRL_SIGN)) return 0; @@ -744,10 +704,6 @@ static int check_purpose_crl_sign(const X509_PURPOSE *xp, const X509 *x, static int ocsp_helper(const X509_PURPOSE *xp, const X509 *x, int ca) { - /* - * Must be a valid CA. Should we really support the "I don't know" value - * (2)? - */ if (ca) return check_ca(x); /* leaf certificate is checked in OCSP_verify() */ diff --git a/src/include/openssl/x509_vfy.h b/src/include/openssl/x509_vfy.h index 97375374..769d95cd 100644 --- a/src/include/openssl/x509_vfy.h +++ b/src/include/openssl/x509_vfy.h @@ -366,7 +366,8 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #define X509_V_FLAG_CRL_CHECK_ALL 0x8 /* Ignore unhandled critical extensions */ #define X509_V_FLAG_IGNORE_CRITICAL 0x10 -/* Disable workarounds for broken certificates */ +/* Enforces stricter checking on certificate purposes. + * TODO(agl): eliminate. */ #define X509_V_FLAG_X509_STRICT 0x20 /* Enable proxy certificate validation */ #define X509_V_FLAG_ALLOW_PROXY_CERTS 0x40 @@ -394,8 +395,6 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #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 -- cgit v1.2.3 From 95cbcd573db2ca89a5aa32b6b435218b49552363 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Mon, 6 Aug 2018 13:54:41 -0700 Subject: =?UTF-8?q?Don't=20accept=20=E2=80=9CSSL=20client=E2=80=9D=20as=20?= =?UTF-8?q?a=20substitute=20for=20S/MIME=20in=20the=20Netscape=20cert=20ty?= =?UTF-8?q?pe=20extension.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I believe that case was the only way that X509_check_purpose could return anything other than zero or one. Thus eliminate the last use of X509_V_FLAG_X509_STRICT. Bug: 111893041 Test: cts -m CtsLibcoreTestCases Merged-In: I47a45e6b6f46b19fcbcb6c917895867d56dcd2ca Change-Id: I0a937a284af0368dae14dcd5687af8a24db2a363 --- src/crypto/x509/x509_vfy.c | 5 ++--- src/crypto/x509v3/v3_purp.c | 10 +++------- src/include/openssl/x509_vfy.h | 5 ++--- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/crypto/x509/x509_vfy.c b/src/crypto/x509/x509_vfy.c index ef3d0f4c..f3cda0de 100644 --- a/src/crypto/x509/x509_vfy.c +++ b/src/crypto/x509/x509_vfy.c @@ -660,9 +660,8 @@ static int check_chain_extensions(X509_STORE_CTX *ctx) } if (ctx->param->purpose > 0) { ret = X509_check_purpose(x, purpose, ca_requirement == must_be_ca); - if ((ret == 0) - || ((ctx->param->flags & X509_V_FLAG_X509_STRICT) - && (ret != 1))) { + if (ret != 1) { + ret = 0; ctx->error = X509_V_ERR_INVALID_PURPOSE; ctx->error_depth = i; ctx->current_cert = x; diff --git a/src/crypto/x509v3/v3_purp.c b/src/crypto/x509v3/v3_purp.c index 799f36c7..03270377 100644 --- a/src/crypto/x509v3/v3_purp.c +++ b/src/crypto/x509v3/v3_purp.c @@ -637,7 +637,8 @@ static int check_purpose_ns_ssl_server(const X509_PURPOSE *xp, const X509 *x, return ret; } -/* common S/MIME checks */ +/* purpose_smime returns one if |x| is a valid S/MIME leaf (|ca| is zero) or CA + * (|ca| is one) certificate, and zero otherwise. */ static int purpose_smime(const X509 *x, int ca) { if (xku_reject(x, XKU_SMIME)) @@ -652,12 +653,7 @@ static int purpose_smime(const X509 *x, int ca) return check_ca(x); } if (x->ex_flags & EXFLAG_NSCERT) { - if (x->ex_nscert & NS_SMIME) - return 1; - /* Workaround for some buggy certificates */ - if (x->ex_nscert & NS_SSL_CLIENT) - return 2; - return 0; + return (x->ex_nscert & NS_SMIME) == NS_SMIME; } return 1; } diff --git a/src/include/openssl/x509_vfy.h b/src/include/openssl/x509_vfy.h index 769d95cd..9e0e0e6c 100644 --- a/src/include/openssl/x509_vfy.h +++ b/src/include/openssl/x509_vfy.h @@ -366,9 +366,8 @@ OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #define X509_V_FLAG_CRL_CHECK_ALL 0x8 /* Ignore unhandled critical extensions */ #define X509_V_FLAG_IGNORE_CRITICAL 0x10 -/* Enforces stricter checking on certificate purposes. - * TODO(agl): eliminate. */ -#define X509_V_FLAG_X509_STRICT 0x20 +/* Does nothing as its functionality has been enabled by default. */ +#define X509_V_FLAG_X509_STRICT 0x00 /* Enable proxy certificate validation */ #define X509_V_FLAG_ALLOW_PROXY_CERTS 0x40 /* Enable policy checking */ -- cgit v1.2.3