Change certificate depth limit to match OpenSSL and document
OpenSSL 1.1.0 included d9b8b89bec4480de3a10bdaf9425db371c19145b, which a
cleanup change to X509_verify_cert. This cleanup changed the semanitcs
of the depth limit. Previously, the depth limit omitted the leaf but
included the trust anchor. Now it omits both.
We forked a little before 1.0.2, so we still had the old behavior. Now
that the new behavior is well-established, switch to new one. Bump
BORINGSSL_API_VERSION so callers can detect one or the other as needed.
Document the new semantics. Also fix up some older docs where I implied
-1 was unlimited depth. Negative numbers were actually enforced as
negative numbers (which means only explicitly-trusted self-signed certs
worked).
Update-Note: The new semantics increase the limit by 1 compared to the
old ones. Thus this change should only accept more chains than
previously and be relatively safe. It also makes us more
OpenSSL-compatible. Envoy will need a tweak because they unit test the
boundary condition for the depth limit.
Bug: 426
Fixed: 459
Change-Id: Ifaa108b8135ea3d875f2ac1f2a3b2cd8a22aa323
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64707
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index b277eb0..cc40ee5 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -379,9 +379,8 @@
X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity
// The following is built up
- int valid; // if 0, rebuild chain
- int last_untrusted; // index of last untrusted cert
- STACK_OF(X509) *chain; // chain of X509s - built up and trusted
+ int last_untrusted; // index of last untrusted cert
+ STACK_OF(X509) *chain; // chain of X509s - built up and trusted
// When something goes wrong, this is why
int error_depth;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index a590e08..ab8ef7f 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -12,6 +12,8 @@
* OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
* CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+#include <limits.h>
+
#include <algorithm>
#include <functional>
#include <string>
@@ -1180,33 +1182,37 @@
// though in different ways.
for (bool trusted_first : {true, false}) {
SCOPED_TRACE(trusted_first);
- std::function<void(X509_VERIFY_PARAM *)> configure_callback;
- if (!trusted_first) {
+ bool override_depth = false;
+ int depth = -1;
+ auto configure_callback = [&](X509_VERIFY_PARAM *param) {
// Note we need the callback to clear the flag. Setting |flags| to zero
// only skips setting new flags.
- configure_callback = [&](X509_VERIFY_PARAM *param) {
+ if (!trusted_first) {
X509_VERIFY_PARAM_clear_flags(param, X509_V_FLAG_TRUSTED_FIRST);
- };
- }
+ }
+ if (override_depth) {
+ X509_VERIFY_PARAM_set_depth(param, depth);
+ }
+ };
// No trust anchors configured.
- ASSERT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+ EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
Verify(leaf.get(), /*roots=*/{}, /*intermediates=*/{},
/*crls=*/{}, /*flags=*/0, configure_callback));
- ASSERT_EQ(
+ EXPECT_EQ(
X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
Verify(leaf.get(), /*roots=*/{}, {intermediate.get()}, /*crls=*/{},
/*flags=*/0, configure_callback));
// Each chain works individually.
- ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()},
+ EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()},
/*crls=*/{}, /*flags=*/0, configure_callback));
- ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {cross_signing_root.get()},
+ EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {cross_signing_root.get()},
{intermediate.get(), root_cross_signed.get()},
/*crls=*/{}, /*flags=*/0, configure_callback));
// When both roots are available, we pick one or the other.
- ASSERT_EQ(X509_V_OK,
+ EXPECT_EQ(X509_V_OK,
Verify(leaf.get(), {cross_signing_root.get(), root.get()},
{intermediate.get(), root_cross_signed.get()}, /*crls=*/{},
/*flags=*/0, configure_callback));
@@ -1215,7 +1221,7 @@
// the cross-sign in the intermediates. With |trusted_first|, we
// preferentially stop path-building at |intermediate|. Without
// |trusted_first|, the "altchains" logic repairs it.
- ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()},
+ EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()},
{intermediate.get(), root_cross_signed.get()},
/*crls=*/{}, /*flags=*/0, configure_callback));
@@ -1226,7 +1232,7 @@
// This test exists to confirm our current behavior, but these modes are
// just workarounds for not having an actual path-building verifier. If we
// fix it, this test can be removed.
- ASSERT_EQ(trusted_first ? X509_V_OK
+ EXPECT_EQ(trusted_first ? X509_V_OK
: X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
Verify(leaf.get(), {root.get()},
{intermediate.get(), root_cross_signed.get()}, /*crls=*/{},
@@ -1234,18 +1240,45 @@
// |forgery| is signed by |leaf_no_key_usage|, but is rejected because the
// leaf is not a CA.
- ASSERT_EQ(X509_V_ERR_INVALID_CA,
+ EXPECT_EQ(X509_V_ERR_INVALID_CA,
Verify(forgery.get(), {intermediate_self_signed.get()},
{leaf_no_key_usage.get()}, /*crls=*/{}, /*flags=*/0,
configure_callback));
// Test that one cannot skip Basic Constraints checking with a contorted set
// of roots and intermediates. This is a regression test for CVE-2015-1793.
- ASSERT_EQ(X509_V_ERR_INVALID_CA,
+ EXPECT_EQ(X509_V_ERR_INVALID_CA,
Verify(forgery.get(),
{intermediate_self_signed.get(), root_cross_signed.get()},
{leaf_no_key_usage.get(), intermediate.get()}, /*crls=*/{},
/*flags=*/0, configure_callback));
+
+ // Test depth limits. |configure_callback| looks at |override_depth| and
+ // |depth|. Negative numbers have historically worked, so test those too.
+ for (int d : {-4, -3, -2, -1, 0, 1, 2, 3, 4, INT_MAX - 3, INT_MAX - 2,
+ INT_MAX - 1, INT_MAX}) {
+ SCOPED_TRACE(d);
+ override_depth = true;
+ depth = d;
+ // A chain with a leaf, two intermediates, and a root is depth two.
+ EXPECT_EQ(
+ depth >= 2 ? X509_V_OK : X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+ Verify(leaf.get(), {cross_signing_root.get()},
+ {intermediate.get(), root_cross_signed.get()},
+ /*crls=*/{}, /*flags=*/0, configure_callback));
+
+ // A chain with a leaf, a root, and no intermediates is depth zero.
+ EXPECT_EQ(
+ depth >= 0 ? X509_V_OK : X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY,
+ Verify(root_cross_signed.get(), {cross_signing_root.get()}, {},
+ /*crls=*/{}, /*flags=*/0, configure_callback));
+
+ // An explicitly trusted self-signed certificate is unaffected by depth
+ // checks.
+ EXPECT_EQ(X509_V_OK,
+ Verify(cross_signing_root.get(), {cross_signing_root.get()}, {},
+ /*crls=*/{}, /*flags=*/0, configure_callback));
+ }
}
}
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 090ed03..9b01388 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -55,6 +55,7 @@
* [including the GNU Public Licence.] */
#include <ctype.h>
+#include <limits.h>
#include <string.h>
#include <time.h>
@@ -160,11 +161,11 @@
}
int X509_verify_cert(X509_STORE_CTX *ctx) {
- X509 *x, *xtmp, *xtmp2, *chain_ss = NULL;
+ X509 *xtmp, *xtmp2, *chain_ss = NULL;
int bad_chain = 0;
X509_VERIFY_PARAM *param = ctx->param;
- int depth, i, ok = 0;
- int num, j, retry, trust;
+ int i, ok = 0;
+ int j, retry, trust;
STACK_OF(X509) *sktmp = NULL;
if (ctx->cert == NULL) {
@@ -207,17 +208,17 @@
goto end;
}
- num = (int)sk_X509_num(ctx->chain);
- x = sk_X509_value(ctx->chain, num - 1);
- depth = param->depth;
+ int num = (int)sk_X509_num(ctx->chain);
+ X509 *x = sk_X509_value(ctx->chain, num - 1);
+ // |param->depth| does not include the leaf certificate or the trust anchor,
+ // so the maximum size is 2 more.
+ int max_chain = param->depth >= INT_MAX - 2 ? INT_MAX : param->depth + 2;
for (;;) {
- // If we have enough, we break
- if (depth < num) {
- break; // FIXME: If this happens, we should take
- // note of it and, if appropriate, use the
- // X509_V_ERR_CERT_CHAIN_TOO_LONG error code
- // later.
+ if (num >= max_chain) {
+ // FIXME: If this happens, we should take note of it and, if appropriate,
+ // use the X509_V_ERR_CERT_CHAIN_TOO_LONG error code later.
+ break;
}
int is_self_signed;
@@ -321,8 +322,9 @@
}
// We now lookup certs from the certificate store
for (;;) {
- // If we have enough, we break
- if (depth < num) {
+ if (num >= max_chain) {
+ // FIXME: If this happens, we should take note of it and, if
+ // appropriate, use the X509_V_ERR_CERT_CHAIN_TOO_LONG error code later.
break;
}
if (!cert_self_signed(x, &is_self_signed)) {
diff --git a/include/openssl/base.h b/include/openssl/base.h
index f0981b8..9a47321 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -109,7 +109,7 @@
// A consumer may use this symbol in the preprocessor to temporarily build
// against multiple revisions of BoringSSL at the same time. It is not
// recommended to do so for longer than is necessary.
-#define BORINGSSL_API_VERSION 28
+#define BORINGSSL_API_VERSION 29
#if defined(BORINGSSL_SHARED_LIBRARY)
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index f1d4eb0..ab6225e 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2601,13 +2601,13 @@
OPENSSL_EXPORT void SSL_set_hostflags(SSL *ssl, unsigned flags);
// SSL_CTX_set_verify_depth sets the maximum depth of a certificate chain
-// accepted in verification. This number does not include the leaf, so a depth
-// of 1 allows the leaf and one CA certificate.
+// accepted in verification. This count excludes both the target certificate and
+// the trust anchor (root certificate).
OPENSSL_EXPORT void SSL_CTX_set_verify_depth(SSL_CTX *ctx, int depth);
// SSL_set_verify_depth sets the maximum depth of a certificate chain accepted
-// in verification. This number does not include the leaf, so a depth of 1
-// allows the leaf and one CA certificate.
+// in verification. This count excludes both the target certificate and the
+// trust anchor (root certificate).
OPENSSL_EXPORT void SSL_set_verify_depth(SSL *ssl, int depth);
// SSL_CTX_get_verify_depth returns the maximum depth of a certificate accepted
diff --git a/include/openssl/x509.h b/include/openssl/x509.h
index 852a96f..b27c97d 100644
--- a/include/openssl/x509.h
+++ b/include/openssl/x509.h
@@ -3522,8 +3522,14 @@
X509 *x);
typedef int (*X509_STORE_CTX_check_crl_fn)(X509_STORE_CTX *ctx, X509_CRL *crl);
+// X509_STORE_set_depth configures |store| to, by default, limit certificate
+// chains to |depth| intermediate certificates. This count excludes both the
+// target certificate and the trust anchor (root certificate).
OPENSSL_EXPORT int X509_STORE_set_depth(X509_STORE *store, int depth);
+// X509_STORE_CTX_set_depth configures |ctx| to, by default, limit certificate
+// chains to |depth| intermediate certificates. This count excludes both the
+// target certificate and the trust anchor (root certificate).
OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth);
#define X509_STORE_CTX_set_app_data(ctx, data) \
@@ -3734,14 +3740,12 @@
//
// As of writing these late defaults are a depth limit (see
// |X509_VERIFY_PARAM_set_depth|) and the |X509_V_FLAG_TRUSTED_FIRST| flag. This
-// warning does not apply if the parameters were set in |store|. That is,
-// callers may safely set a concrete depth limit in |store|, but unlimited depth
-// must be configured at |X509_STORE_CTX|.
+// warning does not apply if the parameters were set in |store|.
//
// TODO(crbug.com/boringssl/441): This behavior is very surprising. Can we
-// remove this notion of late defaults? A depth limit of 100 can probably be
-// applied unconditionally. |X509_V_FLAG_TRUSTED_FIRST| is mostly a workaround
-// for poor path-building.
+// remove this notion of late defaults? The unsettable value at |X509_STORE| is
+// -1, which rejects everything but explicitly-trusted self-signed certificates.
+// |X509_V_FLAG_TRUSTED_FIRST| is mostly a workaround for poor path-building.
OPENSSL_EXPORT X509_VERIFY_PARAM *X509_STORE_get0_param(X509_STORE *store);
// X509_STORE_set_verify_cb acts like |X509_STORE_CTX_set_verify_cb| but sets
@@ -3963,6 +3967,10 @@
int purpose);
OPENSSL_EXPORT int X509_VERIFY_PARAM_set_trust(X509_VERIFY_PARAM *param,
int trust);
+
+// X509_VERIFY_PARAM_set_depth configures |param| to limit certificate chains to
+// |depth| intermediate certificates. This count excludes both the target
+// certificate and the trust anchor (root certificate).
OPENSSL_EXPORT void X509_VERIFY_PARAM_set_depth(X509_VERIFY_PARAM *param,
int depth);
@@ -4043,6 +4051,8 @@
OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_ip_asc(X509_VERIFY_PARAM *param,
const char *ipasc);
+// X509_VERIFY_PARAM_get_depth returns the maximum depth configured in |param|.
+// See |X509_VERIFY_PARAM_set_depth|.
OPENSSL_EXPORT int X509_VERIFY_PARAM_get_depth(const X509_VERIFY_PARAM *param);
typedef void *(*X509V3_EXT_NEW)(void);
