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