diff --git a/src/crypto/x509/internal.h b/src/crypto/x509/internal.h index b277eb0db..cc40ee538 100644 --- a/src/crypto/x509/internal.h +++ b/src/crypto/x509/internal.h @@ -379,9 +379,8 @@ struct x509_store_ctx_st { 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/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc index a590e0876..ab8ef7f65 100644 --- a/src/crypto/x509/x509_test.cc +++ b/src/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 + #include #include #include @@ -1180,33 +1182,37 @@ TEST(X509Test, TestVerify) { // though in different ways. for (bool trusted_first : {true, false}) { SCOPED_TRACE(trusted_first); - std::function 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 @@ TEST(X509Test, TestVerify) { // 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 @@ TEST(X509Test, TestVerify) { // 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 @@ TEST(X509Test, TestVerify) { // |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/src/crypto/x509/x509_vfy.c b/src/crypto/x509/x509_vfy.c index 090ed033a..9b013883c 100644 --- a/src/crypto/x509/x509_vfy.c +++ b/src/crypto/x509/x509_vfy.c @@ -55,6 +55,7 @@ * [including the GNU Public Licence.] */ #include +#include #include #include @@ -160,11 +161,11 @@ static X509 *lookup_cert_match(X509_STORE_CTX *ctx, X509 *x) { } 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 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { 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 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { } // 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/src/include/openssl/base.h b/src/include/openssl/base.h index f0981b8b7..9a47321bd 100644 --- a/src/include/openssl/base.h +++ b/src/include/openssl/base.h @@ -109,7 +109,7 @@ extern "C" { // 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/src/include/openssl/ssl.h b/src/include/openssl/ssl.h index f1d4eb066..ab6225e9e 100644 --- a/src/include/openssl/ssl.h +++ b/src/include/openssl/ssl.h @@ -2601,13 +2601,13 @@ OPENSSL_EXPORT int SSL_set1_host(SSL *ssl, const char *hostname); 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/src/include/openssl/x509.h b/src/include/openssl/x509.h index 852a96f5e..b27c97d33 100644 --- a/src/include/openssl/x509.h +++ b/src/include/openssl/x509.h @@ -3522,8 +3522,14 @@ typedef int (*X509_STORE_CTX_get_crl_fn)(X509_STORE_CTX *ctx, X509_CRL **crl, 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 @@ OPENSSL_EXPORT int X509_STORE_set1_param(X509_STORE *store, // // 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 @@ OPENSSL_EXPORT int X509_VERIFY_PARAM_set_purpose(X509_VERIFY_PARAM *param, 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(X509_VERIFY_PARAM *param, 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);