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.
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.
Change-Id: I9e41f4c567084cf30ed08f015a744959982940af
Reviewed-on: https://boringssl-review.googlesource.com/30185
Reviewed-by: Matt Braithwaite <mab@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index df236ae..551bd8c 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1484,19 +1484,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) {
@@ -1510,17 +1500,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/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index fdce251..a76eda4 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -579,7 +579,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;
@@ -587,15 +587,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) {
@@ -607,6 +605,8 @@
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 @@
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 @@
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 @@
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 f70a804..0275b1b 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 @@
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 @@
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 @@
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 @@
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 @@
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 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 6a7c554..1d4ea5a 100644
--- a/include/openssl/x509_vfy.h
+++ b/include/openssl/x509_vfy.h
@@ -382,7 +382,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
@@ -410,8 +411,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
