Always enable X509_V_FLAG_TRUSTED_FIRST No one seems to ever disable it. This means we can unwind the alt-chains logic, as it's never used as a result. As discussed in https://boringssl-review.googlesource.com/c/boringssl/+/49746, neither mode is really correct. An ideal implementation would be a backtracking path builder. But the particular implementation of backtracking in alt-chains is both insufficient and particularly messy. Simplify this for now and go from there. Update-Note: X509_V_FLAG_TRUSTED_FIRST is now always enabled. (It was previously a default.) Passing it to X509_VERIFY_PARAM_clear_flags now does nothing. Running tests suggests no one is relying on this. This means X509_V_FLAG_NO_ALT_CHAINS is now also a no-op because it does nothing when X509_V_FLAG_TRUSTED_FIRST is enabled. Bug: 544895602 Change-Id: I0b6519f705de2abacea2b6041ae855d59286b631 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/100787 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Rudolf Polzer <rpolzer@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 06a4a16..694b6dd 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc
@@ -1445,108 +1445,84 @@ ASSERT_TRUE(forgery); ASSERT_TRUE(leaf_no_key_usage); - // Most of these tests work with or without `X509_V_FLAG_TRUSTED_FIRST`, - // though in different ways. - for (bool trusted_first : {true, false}) { - SCOPED_TRACE(trusted_first); - bool override_depth = false; - int depth = -1; - auto configure_callback = [&](X509_STORE_CTX *ctx) { - X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx); - // Note we need the callback to clear the flag. Setting `flags` to zero - // only skips setting new flags. - 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. - EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, - Verify(leaf.get(), /*roots=*/{}, /*intermediates=*/{}, - /*crls=*/{}, /*flags=*/0, configure_callback)); - 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. - EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()}, - /*crls=*/{}, /*flags=*/0, configure_callback)); - 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. - 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)); - - // This is the “altchains” test – we remove the cross-signing CA but include - // the cross-sign in the intermediates. With `trusted_first`, we - // preferentially stop path-building at `intermediate`. Without - // `trusted_first`, the "altchains" logic repairs it. - EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, - {intermediate.get(), root_cross_signed.get()}, - /*crls=*/{}, /*flags=*/0, configure_callback)); - - // If `X509_V_FLAG_NO_ALT_CHAINS` is set and `trusted_first` is disabled, we - // get stuck on `root_cross_signed`. If either feature is enabled, we can - // build the path. - // - // 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. - 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=*/{}, - /*flags=*/X509_V_FLAG_NO_ALT_CHAINS, configure_callback)); - - // `forgery` is signed by `leaf_no_key_usage`, but is rejected because the - // leaf is not a 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. - 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)); + bool override_depth = false; + int depth = -1; + auto configure_callback = [&](X509_STORE_CTX *ctx) { + X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx); + if (override_depth) { + X509_VERIFY_PARAM_set_depth(param, depth); } + }; + + // No trust anchors configured. + EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, + Verify(leaf.get(), /*roots=*/{}, /*intermediates=*/{}, + /*crls=*/{}, /*flags=*/0, configure_callback)); + 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. + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()}, + /*crls=*/{}, /*flags=*/0, configure_callback)); + 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. + 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)); + + // This is the “altchains” test, which has now been superceded by the + // “trusted first” behavior – we remove the cross-signing CA but include the + // cross-sign in the intermediates. We preferentially stop path-building at + // `intermediate`. + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, + {intermediate.get(), root_cross_signed.get()}, + /*crls=*/{}, /*flags=*/0, configure_callback)); + + // `forgery` is signed by `leaf_no_key_usage`, but is rejected because the + // leaf is not a 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. + 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)); } } @@ -5092,9 +5068,9 @@ check_attribute(attr.get(), 0); } -// Test that, by default, `X509_V_FLAG_TRUSTED_FIRST` is set, which means we'll -// skip over server-sent expired intermediates when there is a local trust -// anchor that works better. +// Test that we'll skip over server-sent expired intermediates when there is a +// local trust anchor that works better. This was once controlled by an +// on-by-default flag, `X509_V_FLAG_TRUSTED_FIRST`, but is now always enabled. TEST(X509Test, TrustedFirst) { // Generate the following certificates: // @@ -5144,36 +5120,12 @@ Verify(leaf.get(), {root2.get()}, {intermediate.get(), root1_cross.get()}, {})); - // By default, we should find the `leaf` -> `intermediate` -> `root2` chain, - // skipping `root1_cross`. + // We should find the `leaf` -> `intermediate` -> `root1` chain, skipping + // `root1_cross`, whether or not `root2` is trusted. EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root1.get(), root2.get()}, {intermediate.get(), root1_cross.get()}, {})); - - // When `X509_V_FLAG_TRUSTED_FIRST` is disabled, we get stuck on the expired - // intermediate. Note we need the callback to clear the flag. Setting `flags` - // to zero only skips setting new flags. - // - // 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. - EXPECT_EQ(X509_V_ERR_CERT_HAS_EXPIRED, - Verify(leaf.get(), {root1.get(), root2.get()}, - {intermediate.get(), root1_cross.get()}, {}, /*flags=*/0, - [&](X509_STORE_CTX *ctx) { - X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx); - X509_VERIFY_PARAM_clear_flags(param, - X509_V_FLAG_TRUSTED_FIRST); - })); - - // Even when `X509_V_FLAG_TRUSTED_FIRST` is disabled, if `root2` is not - // trusted, the alt chains logic recovers the path. - EXPECT_EQ( - X509_V_OK, - Verify(leaf.get(), {root1.get()}, {intermediate.get(), root1_cross.get()}, - {}, /*flags=*/0, [&](X509_STORE_CTX *ctx) { - X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx); - X509_VERIFY_PARAM_clear_flags(param, X509_V_FLAG_TRUSTED_FIRST); - })); + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root1.get()}, + {intermediate.get(), root1_cross.get()}, {})); } // Test that notBefore and notAfter checks work correctly.
diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc index ce54dd6..fedc4a0 100644 --- a/crypto/x509/x509_vfy.cc +++ b/crypto/x509/x509_vfy.cc
@@ -138,7 +138,7 @@ int bad_chain = 0; X509_VERIFY_PARAM *param = ctx->param; int i, ok = 0; - int j, retry, trust; + int trust; STACK_OF(X509) *sktmp = nullptr; { @@ -206,19 +206,17 @@ if (is_self_signed) { break; } - // If asked see if we can find issuer in trusted store first - if (ctx->param->flags & X509_V_FLAG_TRUSTED_FIRST) { - X509 *issuer = get_trusted_issuer(ctx, x); - if (issuer != nullptr) { - // Free the certificate. It will be picked up again later. - X509_free(issuer); - break; - } + // See if we can find issuer in trusted store first + X509 *issuer = get_trusted_issuer(ctx, x); + if (issuer != nullptr) { + // Free the certificate. It will be picked up again later. + X509_free(issuer); + break; } // If we were passed a cert chain, use it first if (sktmp != nullptr) { - X509 *issuer = find_issuer(ctx, sktmp, x); + issuer = find_issuer(ctx, sktmp, x); if (issuer != nullptr) { if (!sk_X509_push(ctx->chain, issuer)) { ctx->error = X509_V_ERR_OUT_OF_MEM; @@ -236,121 +234,88 @@ break; } - // Remember how many untrusted certs we have - j = num; - // at this point, chain should contain a list of untrusted certificates. + // At this point, chain should contain a list of untrusted certificates. // We now need to add at least one trusted one, if possible, otherwise we // complain. - do { - // Examine last certificate in chain and see if it is self signed. - i = (int)sk_X509_num(ctx->chain); - x = sk_X509_value(ctx->chain, i - 1); + // Examine last certificate in chain and see if it is self signed. + i = (int)sk_X509_num(ctx->chain); + x = sk_X509_value(ctx->chain, i - 1); - int is_self_signed; + int is_self_signed; + if (!cert_self_signed(x, &is_self_signed)) { + ctx->error = X509_V_ERR_INVALID_EXTENSION; + goto end; + } + + if (is_self_signed) { + // we have a self signed certificate + if (sk_X509_num(ctx->chain) == 1) { + // We have a single self signed certificate: see if we can + // find it in the store. We must have an exact match to avoid + // possible impersonation. + X509 *issuer = get_trusted_issuer(ctx, x); + if (issuer == nullptr || X509_cmp(x, issuer) != 0) { + X509_free(issuer); + ctx->error = X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT; + ctx->current_cert = x; + ctx->error_depth = i - 1; + bad_chain = 1; + if (!call_verify_cb(0, ctx)) { + goto end; + } + } else { + // We have a match: replace certificate with store + // version so we get any trust settings. + X509_free(x); + x = issuer; + (void)sk_X509_set(ctx->chain, i - 1, x); + ctx->last_untrusted = 0; + } + } else { + // extract and save self signed certificate for later use + chain_ss = sk_X509_pop(ctx->chain); + ctx->last_untrusted--; + num--; + x = sk_X509_value(ctx->chain, num - 1); + } + } + // We now lookup certs from the certificate store + for (;;) { + 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)) { ctx->error = X509_V_ERR_INVALID_EXTENSION; goto end; } - + // If we are self signed, we break if (is_self_signed) { - // we have a self signed certificate - if (sk_X509_num(ctx->chain) == 1) { - // We have a single self signed certificate: see if we can - // find it in the store. We must have an exact match to avoid - // possible impersonation. - X509 *issuer = get_trusted_issuer(ctx, x); - if (issuer == nullptr || X509_cmp(x, issuer) != 0) { - X509_free(issuer); - ctx->error = X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT; - ctx->current_cert = x; - ctx->error_depth = i - 1; - bad_chain = 1; - if (!call_verify_cb(0, ctx)) { - goto end; - } - } else { - // We have a match: replace certificate with store - // version so we get any trust settings. - X509_free(x); - x = issuer; - (void)sk_X509_set(ctx->chain, i - 1, x); - ctx->last_untrusted = 0; - } - } else { - // extract and save self signed certificate for later use - chain_ss = sk_X509_pop(ctx->chain); - ctx->last_untrusted--; - num--; - j--; - x = sk_X509_value(ctx->chain, num - 1); - } + break; } - // We now lookup certs from the certificate store - for (;;) { - 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)) { - ctx->error = X509_V_ERR_INVALID_EXTENSION; - goto end; - } - // If we are self signed, we break - if (is_self_signed) { - break; - } - X509 *issuer = get_trusted_issuer(ctx, x); - if (issuer == nullptr) { - break; - } - x = issuer; - if (!sk_X509_push(ctx->chain, x)) { - X509_free(issuer); - ctx->error = X509_V_ERR_OUT_OF_MEM; - goto end; - } - num++; + X509 *issuer = get_trusted_issuer(ctx, x); + if (issuer == nullptr) { + break; } - - // we now have our chain, lets check it... - trust = check_trust(ctx); - - // If explicitly rejected error - if (trust == X509_TRUST_REJECTED) { + x = issuer; + if (!sk_X509_push(ctx->chain, x)) { + X509_free(issuer); + ctx->error = X509_V_ERR_OUT_OF_MEM; goto end; } - // If it's not explicitly trusted then check if there is an alternative - // chain that could be used. We only do this if we haven't already - // checked via TRUSTED_FIRST and the user hasn't switched off alternate - // chain checking - retry = 0; - if (trust != X509_TRUST_TRUSTED && - !(ctx->param->flags & X509_V_FLAG_TRUSTED_FIRST) && - !(ctx->param->flags & X509_V_FLAG_NO_ALT_CHAINS)) { - while (j-- > 1) { - X509 *issuer = - get_trusted_issuer(ctx, sk_X509_value(ctx->chain, j - 1)); - // Check if we found an alternate chain - if (issuer != nullptr) { - // Free up the found cert we'll add it again later - X509_free(issuer); + num++; + } - // Dump all the certs above this point - we've found an - // alternate chain - while (num > j) { - X509_free(sk_X509_pop(ctx->chain)); - num--; - } - ctx->last_untrusted = (int)sk_X509_num(ctx->chain); - retry = 1; - break; - } - } - } - } while (retry); + // we now have our chain, lets check it... + trust = check_trust(ctx); + + // If explicitly rejected error + if (trust == X509_TRUST_REJECTED) { + goto end; + } // If not explicitly trusted then indicate error unless it's a single // self signed certificate in which case we've indicated an error already
diff --git a/crypto/x509/x509_vpm.cc b/crypto/x509/x509_vpm.cc index e5b9963..9841ad7 100644 --- a/crypto/x509/x509_vpm.cc +++ b/crypto/x509/x509_vpm.cc
@@ -366,7 +366,7 @@ static const X509_VERIFY_PARAM kDefaultParam = { /*check_time=*/0, - /*flags=*/X509_V_FLAG_TRUSTED_FIRST, + /*flags=*/0, /*purpose=*/0, /*trust=*/0, /*depth=*/100,
diff --git a/include/openssl/x509.h b/include/openssl/x509.h index d08e888..f1b21e2 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h
@@ -2847,13 +2847,12 @@ // explicitly unset after creating the `X509_STORE_CTX`. // // 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`. +// `X509_VERIFY_PARAM_set_depth`). This 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? 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_set1_param copies verification parameters from `param` as in @@ -3276,16 +3275,14 @@ // X509_V_FLAG_CHECK_SS_SIGNATURE checks the redundant signature on self-signed // trust anchors. This check provides no security benefit and only wastes CPU. #define X509_V_FLAG_CHECK_SS_SIGNATURE 0x4000 -// X509_V_FLAG_TRUSTED_FIRST, during path-building, checks for a match in the -// trust store before considering an untrusted intermediate. This flag is -// enabled by default. -#define X509_V_FLAG_TRUSTED_FIRST 0x8000 +// X509_V_FLAG_TRUSTED_FIRST does nothing. The behavior it controls is always +// enabled. +#define X509_V_FLAG_TRUSTED_FIRST 0x0 // X509_V_FLAG_PARTIAL_CHAIN treats all trusted certificates as trust anchors, // independent of the `X509_VERIFY_PARAM_set_trust` setting. #define X509_V_FLAG_PARTIAL_CHAIN 0x80000 -// X509_V_FLAG_NO_ALT_CHAINS disables building alternative chains if the initial -// one was rejected. -#define X509_V_FLAG_NO_ALT_CHAINS 0x100000 +// X509_V_FLAG_NO_ALT_CHAINS does nothing. +#define X509_V_FLAG_NO_ALT_CHAINS 0x0 // X509_V_FLAG_NO_CHECK_TIME disables all time checks in certificate // verification. #define X509_V_FLAG_NO_CHECK_TIME 0x200000
diff --git a/rust/bssl-x509/src/params.rs b/rust/bssl-x509/src/params.rs index 2af6aef..afbc2e4 100644 --- a/rust/bssl-x509/src/params.rs +++ b/rust/bssl-x509/src/params.rs
@@ -98,8 +98,6 @@ /// Treat all trusted certificates as trust anchors regardless of the /// [`CertificateVerificationParams::set_trust`] setting. const PARTIAL_CHAIN = bssl_sys::X509_V_FLAG_PARTIAL_CHAIN as c_ulong; - /// Disable building of alternative chains, when the first built chain was rejected. - const NO_ALT_CHAINS = bssl_sys::X509_V_FLAG_NO_ALT_CHAINS as c_ulong; /// Disable all time checks during certificate verification. const NO_CHECK_TIME = bssl_sys::X509_V_FLAG_NO_CHECK_TIME as c_ulong; }