diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index df236aeb..551bd8ca 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1484,19 +1484,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, + // basicConstraints. + EXPECT_EQ(X509_V_ERR_INVALID_CA, 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) { @@ -1510,17 +1500,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, + // marked as a CA in the basicConstraints. + EXPECT_EQ(X509_V_ERR_INVALID_CA, 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 fdce2510..a76eda4e 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -579,7 +579,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; @@ -587,15 +587,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) { @@ -607,6 +605,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; @@ -628,37 +628,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; @@ -667,7 +660,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))) { @@ -708,9 +701,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/crypto/x509v3/v3_purp.c b/crypto/x509v3/v3_purp.c index f70a804c..0275b1bb 100644 --- a/crypto/x509v3/v3_purp.c +++ b/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, @@ -563,39 +562,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) @@ -604,27 +584,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; @@ -648,7 +614,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; @@ -678,15 +644,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) @@ -727,11 +691,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; @@ -745,10 +705,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/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index 6a7c554c..1d4ea5a7 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -382,7 +382,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 @@ -410,8 +411,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