From 955ef7991e41ac6c0ea5114b4b9abb98cc5fd614 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 13 Jun 2022 13:16:43 -0400 Subject: [PATCH] Rewrite SSL_add_file_cert_subjects_to_stack SSL_load_client_CA_file can just call SSL_add_file_cert_subjects_to_stack. SSL_add_file_cert_subjects_to_stack itself is rewritten to use scopers and also give subquadratic running time. Sorting after every insertion does not actually help. (It would have been faster to do a linear search.) Instead, gather the names first, then sort and deduplicate. Finally, add a SSL_add_bio_cert_subjects_to_stack. This is both to simplify testing and because Envoy code copied from SSL_add_file_cert_subjects_to_stack, complete with the quadratic behavior. It is the only external project that depends on the STACK_OF(T) comparison function. To simplify making that const-correct, just export the function they needed anyway. Bug: 498 Change-Id: I00d13c949a535c0d60873fe4ba2e5604bb585cca Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53007 Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- include/openssl/base.h | 2 +- include/openssl/ssl.h | 7 +- ssl/ssl_file.cc | 160 ++++++++++++++++------------------------- ssl/ssl_test.cc | 101 ++++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 101 deletions(-) diff --git a/include/openssl/base.h b/include/openssl/base.h index a4a91f540..7e58f0038 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -197,7 +197,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 17 +#define BORINGSSL_API_VERSION 18 #if defined(BORINGSSL_SHARED_LIBRARY) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index b990b4134..0e54ab9ef 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2718,7 +2718,7 @@ OPENSSL_EXPORT int SSL_CTX_add_client_CA(SSL_CTX *ctx, X509 *x509); // SSL_load_client_CA_file opens |file| and reads PEM-encoded certificates from // it. It returns a newly-allocated stack of the certificate subjects or NULL -// on error. +// on error. Duplicates in |file| are ignored. OPENSSL_EXPORT STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file); // SSL_dup_CA_list makes a deep copy of |list|. It returns the new list on @@ -2731,6 +2731,11 @@ OPENSSL_EXPORT STACK_OF(X509_NAME) *SSL_dup_CA_list(STACK_OF(X509_NAME) *list); OPENSSL_EXPORT int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, const char *file); +// SSL_add_bio_cert_subjects_to_stack behaves like +// |SSL_add_file_cert_subjects_to_stack| but reads from |bio|. +OPENSSL_EXPORT int SSL_add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, + BIO *bio); + // Server name indication. // diff --git a/ssl/ssl_file.cc b/ssl/ssl_file.cc index 43db37ca6..162520010 100644 --- a/ssl/ssl_file.cc +++ b/ssl/ssl_file.cc @@ -128,125 +128,87 @@ static int xname_cmp(const X509_NAME **a, const X509_NAME **b) { return X509_NAME_cmp(*a, *b); } -// TODO(davidben): Is there any reason this doesn't call -// |SSL_add_file_cert_subjects_to_stack|? STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file) { - BIO *in; - X509 *x = NULL; - X509_NAME *xn = NULL; - STACK_OF(X509_NAME) *ret = NULL, *sk; - - sk = sk_X509_NAME_new(xname_cmp); - in = BIO_new(BIO_s_file()); - - if (sk == NULL || in == NULL) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); - goto err; + bssl::UniquePtr ret(sk_X509_NAME_new_null()); + if (ret == nullptr || + !SSL_add_file_cert_subjects_to_stack(ret.get(), file)) { + return nullptr; } + return ret.release(); +} - if (!BIO_read_filename(in, file)) { - goto err; +int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, + const char *file) { + bssl::UniquePtr in(BIO_new_file(file, "r")); + if (in == nullptr) { + return 0; } + return SSL_add_bio_cert_subjects_to_stack(out, in.get()); +} +int SSL_add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, BIO *bio) { + // This function historically sorted |out| after every addition and skipped + // duplicates. This implementation preserves that behavior, but only sorts at + // the end, to avoid a quadratic running time. Existing duplicates in |out| + // are preserved, but do not introduce new duplicates. + bssl::UniquePtr to_append(sk_X509_NAME_new(xname_cmp)); + if (to_append == nullptr) { + return 0; + } + + // Temporarily switch the comparison function for |out|. + struct RestoreCmpFunc { + ~RestoreCmpFunc() { sk_X509_NAME_set_cmp_func(stack, old_cmp); } + STACK_OF(X509_NAME) *stack; + int (*old_cmp)(const X509_NAME **, const X509_NAME **); + }; + RestoreCmpFunc restore = {out, sk_X509_NAME_set_cmp_func(out, xname_cmp)}; + + sk_X509_NAME_sort(out); for (;;) { - if (PEM_read_bio_X509(in, &x, NULL, NULL) == NULL) { + bssl::UniquePtr x509( + PEM_read_bio_X509(bio, nullptr, nullptr, nullptr)); + if (x509 == nullptr) { + // TODO(davidben): This ignores PEM syntax errors. It should only succeed + // on |PEM_R_NO_START_LINE|. + ERR_clear_error(); break; } - if (ret == NULL) { - ret = sk_X509_NAME_new_null(); - if (ret == NULL) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); - goto err; - } - } - xn = X509_get_subject_name(x); - if (xn == NULL) { - goto err; - } - // Check for duplicates. - sk_X509_NAME_sort(sk); - if (sk_X509_NAME_find(sk, NULL, xn)) { + X509_NAME *subject = X509_get_subject_name(x509.get()); + // Skip if already present in |out|. Duplicates in |to_append| will be + // handled separately. + if (sk_X509_NAME_find(out, /*out_index=*/NULL, subject)) { continue; } - xn = X509_NAME_dup(xn); - if (xn == NULL || - !sk_X509_NAME_push(sk /* non-owning */, xn) || - !sk_X509_NAME_push(ret /* owning */, xn)) { - X509_NAME_free(xn); - goto err; + bssl::UniquePtr copy(X509_NAME_dup(subject)); + if (copy == nullptr || + !bssl::PushToStack(to_append.get(), std::move(copy))) { + return 0; } } - if (false) { - err: - sk_X509_NAME_pop_free(ret, X509_NAME_free); - ret = NULL; - } - - sk_X509_NAME_free(sk); - BIO_free(in); - X509_free(x); - if (ret != NULL) { - ERR_clear_error(); - } - return ret; -} - -int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *stack, - const char *file) { - BIO *in; - X509 *x = NULL; - X509_NAME *xn = NULL; - int ret = 0; - int (*oldcmp)(const X509_NAME **a, const X509_NAME **b); - - oldcmp = sk_X509_NAME_set_cmp_func(stack, xname_cmp); - in = BIO_new(BIO_s_file()); - - if (in == NULL) { - OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!BIO_read_filename(in, file)) { - goto err; - } - - for (;;) { - if (PEM_read_bio_X509(in, &x, NULL, NULL) == NULL) { - break; - } - xn = X509_get_subject_name(x); - if (xn == NULL) { - goto err; - } - - // Check for duplicates. - sk_X509_NAME_sort(stack); - if (sk_X509_NAME_find(stack, NULL, xn)) { + // Append |to_append| to |stack|, skipping any duplicates. + sk_X509_NAME_sort(to_append.get()); + size_t num = sk_X509_NAME_num(to_append.get()); + for (size_t i = 0; i < num; i++) { + bssl::UniquePtr name(sk_X509_NAME_value(to_append.get(), i)); + sk_X509_NAME_set(to_append.get(), i, nullptr); + if (i + 1 < num && + X509_NAME_cmp(name.get(), sk_X509_NAME_value(to_append.get(), i + 1)) == + 0) { continue; } - - xn = X509_NAME_dup(xn); - if (xn == NULL || - !sk_X509_NAME_push(stack, xn)) { - X509_NAME_free(xn); - goto err; + if (!bssl::PushToStack(out, std::move(name))) { + return 0; } } - ERR_clear_error(); - ret = 1; - -err: - BIO_free(in); - X509_free(x); - - (void) sk_X509_NAME_set_cmp_func(stack, oldcmp); - - return ret; + // Sort |out| one last time, to preserve the historical behavior of + // maintaining the sorted list. + sk_X509_NAME_sort(out); + return 1; } int SSL_use_certificate_file(SSL *ssl, const char *file, int type) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index e2db5a4da..a40de7914 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -8159,5 +8159,106 @@ TEST(SSLTest, NumTickets) { EXPECT_EQ(count_tickets(), 16u); } +TEST(SSLTest, CertSubjectsToStack) { + const std::string kCert1 = R"( +-----BEGIN CERTIFICATE----- +MIIBzzCCAXagAwIBAgIJANlMBNpJfb/rMAkGByqGSM49BAEwRTELMAkGA1UEBhMC +QVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoMGEludGVybmV0IFdpZGdp +dHMgUHR5IEx0ZDAeFw0xNDA0MjMyMzIxNTdaFw0xNDA1MjMyMzIxNTdaMEUxCzAJ +BgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5l +dCBXaWRnaXRzIFB0eSBMdGQwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATmK2ni +v2Wfl74vHg2UikzVl2u3qR4NRvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYa +HPUdfvGULUvPciLBo1AwTjAdBgNVHQ4EFgQUq4TSrKuV8IJOFngHVVdf5CaNgtEw +HwYDVR0jBBgwFoAUq4TSrKuV8IJOFngHVVdf5CaNgtEwDAYDVR0TBAUwAwEB/zAJ +BgcqhkjOPQQBA0gAMEUCIQDyoDVeUTo2w4J5m+4nUIWOcAZ0lVfSKXQA9L4Vh13E +BwIgfB55FGohg/B6dGh5XxSZmmi08cueFV7mHzJSYV51yRQ= +-----END CERTIFICATE----- +)"; + const std::vector kName1 = { + 0x30, 0x45, 0x31, 0x0b, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, + 0x02, 0x41, 0x55, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x08, + 0x0c, 0x0a, 0x53, 0x6f, 0x6d, 0x65, 0x2d, 0x53, 0x74, 0x61, 0x74, 0x65, + 0x31, 0x21, 0x30, 0x1f, 0x06, 0x03, 0x55, 0x04, 0x0a, 0x0c, 0x18, 0x49, + 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x65, 0x74, 0x20, 0x57, 0x69, 0x64, 0x67, + 0x69, 0x74, 0x73, 0x20, 0x50, 0x74, 0x79, 0x20, 0x4c, 0x74, 0x64}; + const std::string kCert2 = R"( +-----BEGIN CERTIFICATE----- +MIICXjCCAcegAwIBAgIIWjO48ufpunYwDQYJKoZIhvcNAQELBQAwNjEaMBgGA1UE +ChMRQm9yaW5nU1NMIFRFU1RJTkcxGDAWBgNVBAMTD0ludGVybWVkaWF0ZSBDQTAg +Fw0xNTAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowMjEaMBgGA1UEChMRQm9y +aW5nU1NMIFRFU1RJTkcxFDASBgNVBAMTC2V4YW1wbGUuY29tMIGfMA0GCSqGSIb3 +DQEBAQUAA4GNADCBiQKBgQDD0U0ZYgqShJ7oOjsyNKyVXEHqeafmk/bAoPqY/h1c +oPw2E8KmeqiUSoTPjG5IXSblOxcqpbAXgnjPzo8DI3GNMhAf8SYNYsoH7gc7Uy7j +5x8bUrisGnuTHqkqH6d4/e7ETJ7i3CpR8bvK16DggEvQTudLipz8FBHtYhFakfdh +TwIDAQABo3cwdTAOBgNVHQ8BAf8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUHAwEG +CCsGAQUFBwMCMAwGA1UdEwEB/wQCMAAwGQYDVR0OBBIEEKN5pvbur7mlXjeMEYA0 +4nUwGwYDVR0jBBQwEoAQjBpoqLV2211Xex+NFLIGozANBgkqhkiG9w0BAQsFAAOB +gQBj/p+JChp//LnXWC1k121LM/ii7hFzQzMrt70bny406SGz9jAjaPOX4S3gt38y +rhjpPukBlSzgQXFg66y6q5qp1nQTD1Cw6NkKBe9WuBlY3iYfmsf7WT8nhlT1CttU +xNCwyMX9mtdXdQicOfNjIGUCD5OLV5PgHFPRKiHHioBAhg== +-----END CERTIFICATE----- +)"; + const std::vector kName2 = { + 0x30, 0x32, 0x31, 0x1a, 0x30, 0x18, 0x06, 0x03, 0x55, 0x04, 0x0a, + 0x13, 0x11, 0x42, 0x6f, 0x72, 0x69, 0x6e, 0x67, 0x53, 0x53, 0x4c, + 0x20, 0x54, 0x45, 0x53, 0x54, 0x49, 0x4e, 0x47, 0x31, 0x14, 0x30, + 0x12, 0x06, 0x03, 0x55, 0x04, 0x03, 0x13, 0x0b, 0x65, 0x78, 0x61, + 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d}; + + const struct { + std::vector> existing; + std::string pem; + std::vector> expected; + } kTests[] = { + // Do nothing. + {{}, "", {}}, + // Append to an empty list, skipping duplicates. + {{}, kCert1 + kCert2 + kCert1, {kName1, kName2}}, + // One of the names was already present. + {{kName1}, kCert1 + kCert2, {kName1, kName2}}, + // Both names were already present. + {{kName1, kName2}, kCert1 + kCert2, {kName1, kName2}}, + // Preserve existing duplicates. + {{kName1, kName2, kName2}, kCert1 + kCert2, {kName1, kName2, kName2}}, + }; + for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kTests); i++) { + SCOPED_TRACE(i); + const auto &t = kTests[i]; + + bssl::UniquePtr stack(sk_X509_NAME_new_null()); + ASSERT_TRUE(stack); + for (const auto& name : t.existing) { + const uint8_t *inp = name.data(); + bssl::UniquePtr name_obj( + d2i_X509_NAME(nullptr, &inp, name.size())); + ASSERT_TRUE(name_obj); + EXPECT_EQ(inp, name.data() + name.size()); + ASSERT_TRUE(bssl::PushToStack(stack.get(), std::move(name_obj))); + } + + bssl::UniquePtr bio(BIO_new_mem_buf(t.pem.data(), t.pem.size())); + ASSERT_TRUE(bio); + ASSERT_TRUE(SSL_add_bio_cert_subjects_to_stack(stack.get(), bio.get())); + + // The function should have left |stack|'s comparison function alone. + EXPECT_EQ(nullptr, sk_X509_NAME_set_cmp_func(stack.get(), nullptr)); + + std::vector> expected = t.expected, result; + for (X509_NAME *name : stack.get()) { + uint8_t *der = nullptr; + int der_len = i2d_X509_NAME(name, &der); + ASSERT_GE(der_len, 0); + result.push_back(std::vector(der, der + der_len)); + OPENSSL_free(der); + } + + // |SSL_add_bio_cert_subjects_to_stack| does not return the output in a + // well-defined order. + std::sort(expected.begin(), expected.end()); + std::sort(result.begin(), result.end()); + EXPECT_EQ(result, expected); + } +} + } // namespace BSSL_NAMESPACE_END