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.
diff --git a/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc
index b66f365..ecb6e7f 100644
--- a/src/crypto/x509/x509_test.cc
+++ b/src/crypto/x509/x509_test.cc
@@ -1179,19 +1179,9 @@
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) {
@@ -1205,17 +1195,7 @@
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/src/crypto/x509/x509_vfy.c b/src/crypto/x509/x509_vfy.c
index 8fd8f9a..ef3d0f4 100644
--- a/src/crypto/x509/x509_vfy.c
+++ b/src/crypto/x509/x509_vfy.c
@@ -578,7 +578,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 324de85..799f36c 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 @@
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 @@
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 @@
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 @@
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 @@
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 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 9737537..769d95c 100644
--- a/src/include/openssl/x509_vfy.h
+++ b/src/include/openssl/x509_vfy.h
@@ -366,7 +366,8 @@
#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 @@
#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
