kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Require basicConstraints cA flag in intermediate certs.

Browse Source 
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>
This commit is contained in:
Adam Langley 2018-07-31 16:42:31 -07:00
parent 0224a3294a
commit 8bd1d07535
4 changed files with 50 additions and 121 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
crypto/x509/x509_test.cc
View File

@ -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));
}

58
crypto/x509/x509_vfy.c
View File

@ -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
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:

74
crypto/x509v3/v3_purp.c
View File

@ -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)
/* Version 1 certificates are considered CAs and don't have extensions. */
if ((x->ex_flags & V1_ROOT) == V1_ROOT) {
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;
}
/* 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,16 +644,14 @@ 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
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)
return 1;
@ -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() */

5
include/openssl/x509_vfy.h
View File

@ -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