From 523d6c74c0c325139614b34619be592ad897a732 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 1 Oct 2021 16:56:12 -0400 Subject: [PATCH] Remove X509_STORE_set0_additional_untrusted. This was added in https://boringssl-review.googlesource.com/c/boringssl/+/12980/, but does not appear to be used anymore. The corresponding function does not exist in OpenSSL. This simplifies the tests slightly, some of which were inadvertently specifying the boolean and some weren't. Change-Id: I9b956dcd9f7151910f93f377d207c88273bd9ccf Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49747 Reviewed-by: Adam Langley Commit-Queue: Adam Langley --- crypto/x509/internal.h | 1 - crypto/x509/x509_lu.c | 5 --- crypto/x509/x509_test.cc | 74 ++++++++++++-------------------------- crypto/x509/x509_vfy.c | 36 +------------------ include/openssl/x509_vfy.h | 5 --- 5 files changed, 23 insertions(+), 98 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index ac687550a..a1c938fc4 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -268,7 +268,6 @@ struct x509_store_st { int cache; // if true, stash any hits STACK_OF(X509_OBJECT) *objs; // Cache of all objects CRYPTO_MUTEX objs_lock; - STACK_OF(X509) *additional_untrusted; // These are external lookup methods STACK_OF(X509_LOOKUP) *get_cert_methods; diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 6d51ffdef..041a5fd5a 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -404,11 +404,6 @@ int X509_STORE_add_crl(X509_STORE *ctx, X509_CRL *x) return ret; } -void X509_STORE_set0_additional_untrusted(X509_STORE *ctx, - STACK_OF(X509) *untrusted) { - ctx->additional_untrusted = untrusted; -} - int X509_OBJECT_up_ref_count(X509_OBJECT *a) { switch (a->type) { diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 8878030cf..32b9af656 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1111,9 +1111,8 @@ static const time_t kReferenceTime = 1474934400 /* Sep 27th, 2016 */; static int Verify( X509 *leaf, const std::vector &roots, const std::vector &intermediates, - const std::vector &crls, unsigned long flags, - bool use_additional_untrusted, - std::function configure_callback, + const std::vector &crls, unsigned long flags = 0, + std::function configure_callback = nullptr, int (*verify_callback)(int, X509_STORE_CTX *) = nullptr) { bssl::UniquePtr roots_stack(CertsToStack(roots)); bssl::UniquePtr intermediates_stack( @@ -1133,14 +1132,8 @@ static int Verify( return X509_V_ERR_UNSPECIFIED; } - if (use_additional_untrusted) { - X509_STORE_set0_additional_untrusted(store.get(), - intermediates_stack.get()); - } - - if (!X509_STORE_CTX_init( - ctx.get(), store.get(), leaf, - use_additional_untrusted ? nullptr : intermediates_stack.get())) { + if (!X509_STORE_CTX_init(ctx.get(), store.get(), leaf, + intermediates_stack.get())) { return X509_V_ERR_UNSPECIFIED; } @@ -1164,27 +1157,6 @@ static int Verify( return X509_V_OK; } -static int Verify( - X509 *leaf, const std::vector &roots, - const std::vector &intermediates, - const std::vector &crls, unsigned long flags = 0, - std::function configure_callback = nullptr) { - const int r1 = Verify(leaf, roots, intermediates, crls, flags, false, - configure_callback); - const int r2 = - Verify(leaf, roots, intermediates, crls, flags, true, configure_callback); - - if (r1 != r2) { - fprintf(stderr, - "Verify with, and without, use_additional_untrusted gave different " - "results: %d vs %d.\n", - r1, r2); - return false; - } - - return r1; -} - TEST(X509Test, TestVerify) { // cross_signing_root // | @@ -1325,7 +1297,7 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // The correct value should work. ASSERT_EQ(X509_V_OK, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [&test](X509_VERIFY_PARAM *param) { ASSERT_TRUE(test.func(param, test.correct_value, test.correct_value_len)); @@ -1333,7 +1305,7 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // The wrong value should trigger a verification error. ASSERT_EQ(test.mismatch_error, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [&test](X509_VERIFY_PARAM *param) { ASSERT_TRUE(test.func(param, test.incorrect_value, test.incorrect_value_len)); @@ -1342,7 +1314,7 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // Passing zero as the length, unlike OpenSSL, should trigger an error and // should cause verification to fail. ASSERT_EQ(X509_V_ERR_INVALID_CALL, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [&test](X509_VERIFY_PARAM *param) { ASSERT_FALSE(test.func(param, test.correct_value, 0)); })); @@ -1350,7 +1322,7 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // Passing an empty value should be an error when setting and should cause // verification to fail. ASSERT_EQ(X509_V_ERR_INVALID_CALL, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [&test](X509_VERIFY_PARAM *param) { ASSERT_FALSE(test.func(param, nullptr, 0)); })); @@ -1358,7 +1330,7 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // Passing a value with embedded NULs should also be an error and should // also cause verification to fail. ASSERT_EQ(X509_V_ERR_INVALID_CALL, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [&test](X509_VERIFY_PARAM *param) { ASSERT_FALSE(test.func(param, "a", 2)); })); @@ -1368,14 +1340,14 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // The correct value should still work. ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, - false, [](X509_VERIFY_PARAM *param) { + [](X509_VERIFY_PARAM *param) { ASSERT_TRUE(X509_VERIFY_PARAM_set1_ip( param, kIP, sizeof(kIP))); })); // Incorrect values should still fail. ASSERT_EQ(X509_V_ERR_IP_ADDRESS_MISMATCH, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [](X509_VERIFY_PARAM *param) { ASSERT_TRUE(X509_VERIFY_PARAM_set1_ip(param, kWrongIP, sizeof(kWrongIP))); @@ -1384,14 +1356,14 @@ TEST(X509Test, ZeroLengthsWithX509PARAM) { // Zero length values should trigger an error when setting and cause // verification to always fail. ASSERT_EQ(X509_V_ERR_INVALID_CALL, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [](X509_VERIFY_PARAM *param) { ASSERT_FALSE(X509_VERIFY_PARAM_set1_ip(param, kIP, 0)); })); // ... and so should NULL values. ASSERT_EQ(X509_V_ERR_INVALID_CALL, - Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, false, + Verify(leaf.get(), {root.get()}, {}, empty_crls, 0, [](X509_VERIFY_PARAM *param) { ASSERT_FALSE(X509_VERIFY_PARAM_set1_ip(param, nullptr, 0)); })); @@ -2500,11 +2472,10 @@ TEST(X509Test, CommonNameFallback) { ASSERT_TRUE(with_ip); auto verify_cert = [&](X509 *leaf, unsigned flags, const char *host) { - return Verify( - leaf, {root.get()}, {}, {}, 0, false, [&](X509_VERIFY_PARAM *param) { - ASSERT_TRUE(X509_VERIFY_PARAM_set1_host(param, host, strlen(host))); - X509_VERIFY_PARAM_set_hostflags(param, flags); - }); + return Verify(leaf, {root.get()}, {}, {}, 0, [&](X509_VERIFY_PARAM *param) { + ASSERT_TRUE(X509_VERIFY_PARAM_set1_host(param, host, strlen(host))); + X509_VERIFY_PARAM_set_hostflags(param, flags); + }); }; // By default, the common name is ignored if the SAN list is present but @@ -2617,7 +2588,7 @@ TEST(X509Test, CommonNameAndNameConstraints) { auto verify_cert = [&](X509 *leaf, unsigned flags, const char *host) { return Verify( - leaf, {root.get()}, {intermediate.get()}, {}, 0, false, + leaf, {root.get()}, {intermediate.get()}, {}, 0, [&](X509_VERIFY_PARAM *param) { ASSERT_TRUE(X509_VERIFY_PARAM_set1_host(param, host, strlen(host))); X509_VERIFY_PARAM_set_hostflags(param, flags); @@ -2638,12 +2609,12 @@ TEST(X509Test, CommonNameAndNameConstraints) { // separately call |X509_check_host|. EXPECT_EQ(X509_V_ERR_NAME_CONSTRAINTS_WITHOUT_SANS, Verify(not_permitted.get(), {root.get()}, {intermediate.get()}, {}, - 0 /* no flags */, false, nullptr)); + 0 /* no flags */, nullptr)); // If the leaf certificate has SANs, the common name fallback is always // disabled, so the name constraints do not apply. EXPECT_EQ(X509_V_OK, Verify(not_permitted_with_sans.get(), {root.get()}, - {intermediate.get()}, {}, 0, false, nullptr)); + {intermediate.get()}, {}, 0, nullptr)); EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH, verify_cert(not_permitted_with_sans.get(), 0 /* no flags */, kCommonNameNotPermittedWithSANs)); @@ -2651,7 +2622,7 @@ TEST(X509Test, CommonNameAndNameConstraints) { // If the common name does not look like a DNS name, we apply neither name // constraints nor common name fallback. EXPECT_EQ(X509_V_OK, Verify(not_dns.get(), {root.get()}, {intermediate.get()}, - {}, 0, false, nullptr)); + {}, 0, nullptr)); EXPECT_EQ(X509_V_ERR_HOSTNAME_MISMATCH, verify_cert(not_dns.get(), 0 /* no flags */, kCommonNameNotDNS)); } @@ -2675,8 +2646,7 @@ TEST(X509Test, ServerGatedCryptoEKUs) { auto verify_cert = [&root](X509 *leaf) { return Verify(leaf, {root.get()}, /*intermediates=*/{}, /*crls=*/{}, - /*flags=*/0, /*use_additional_untrusted=*/false, - [&](X509_VERIFY_PARAM *param) { + /*flags=*/0, [&](X509_VERIFY_PARAM *param) { ASSERT_TRUE(X509_VERIFY_PARAM_set_purpose( param, X509_PURPOSE_SSL_SERVER)); }); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 818459ca0..2dfdce20f 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -222,8 +222,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx) X509_up_ref(ctx->cert); ctx->last_untrusted = 1; - /* We use a temporary STACK so we can chop and hack at it. - * sktmp = ctx->untrusted ++ ctx->ctx->additional_untrusted */ + /* We use a temporary STACK so we can chop and hack at it. */ if (ctx->untrusted != NULL && (sktmp = sk_X509_dup(ctx->untrusted)) == NULL) { OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); @@ -231,28 +230,6 @@ int X509_verify_cert(X509_STORE_CTX *ctx) goto end; } - if (ctx->ctx->additional_untrusted != NULL) { - if (sktmp == NULL) { - sktmp = sk_X509_new_null(); - if (sktmp == NULL) { - OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); - ctx->error = X509_V_ERR_OUT_OF_MEM; - goto end; - } - } - - for (size_t k = 0; k < sk_X509_num(ctx->ctx->additional_untrusted); - k++) { - if (!sk_X509_push(sktmp, - sk_X509_value(ctx->ctx->additional_untrusted, - k))) { - OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE); - ctx->error = X509_V_ERR_OUT_OF_MEM; - goto end; - } - } - } - num = sk_X509_num(ctx->chain); x = sk_X509_value(ctx->chain, num - 1); depth = param->depth; @@ -1351,17 +1328,6 @@ static void crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, return; } } - - for (i = 0; i < sk_X509_num(ctx->ctx->additional_untrusted); i++) { - crl_issuer = sk_X509_value(ctx->ctx->additional_untrusted, i); - if (X509_NAME_cmp(X509_get_subject_name(crl_issuer), cnm)) - continue; - if (X509_check_akid(crl_issuer, crl->akid) == X509_V_OK) { - *pissuer = crl_issuer; - *pcrl_score |= CRL_SCORE_AKID; - return; - } - } } /* diff --git a/include/openssl/x509_vfy.h b/include/openssl/x509_vfy.h index d8781afd8..719807eda 100644 --- a/include/openssl/x509_vfy.h +++ b/include/openssl/x509_vfy.h @@ -306,11 +306,6 @@ OPENSSL_EXPORT int X509_STORE_set_trust(X509_STORE *ctx, int trust); OPENSSL_EXPORT int X509_STORE_set1_param(X509_STORE *ctx, X509_VERIFY_PARAM *pm); OPENSSL_EXPORT X509_VERIFY_PARAM *X509_STORE_get0_param(X509_STORE *ctx); -// X509_STORE_set0_additional_untrusted sets a stack of additional, untrusted -// certificates that are available for chain building. This function does not -// take ownership of the stack. -OPENSSL_EXPORT void X509_STORE_set0_additional_untrusted( - X509_STORE *ctx, STACK_OF(X509) *untrusted); OPENSSL_EXPORT void X509_STORE_set_verify(X509_STORE *ctx, X509_STORE_CTX_verify_fn verify);