Fix SSL_load_client_CA_file when given an empty file.
https://boringssl-review.googlesource.com/c/boringssl/+/53007
inadvertently changed the semantics of SSL_load_client_CA_file slightly.
The original implementation, by delaying allocating ret, would fail
rather than return an empty list.
Fix this and add a test. We don't have much support for testing
filesystem-related things yet, so I've just used /dev/null and gated it
to Linux + macOS for now. If we need it later, we can add temporary file
support to the test_support library.
Change-Id: If77dd493a433819a65378d76cf400cce48c0abaa
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53785
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/ssl/ssl_file.cc b/ssl/ssl_file.cc
index 1625200..d867cf1 100644
--- a/ssl/ssl_file.cc
+++ b/ssl/ssl_file.cc
@@ -128,25 +128,8 @@
return X509_NAME_cmp(*a, *b);
}
-STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file) {
- bssl::UniquePtr<STACK_OF(X509_NAME)> ret(sk_X509_NAME_new_null());
- if (ret == nullptr ||
- !SSL_add_file_cert_subjects_to_stack(ret.get(), file)) {
- return nullptr;
- }
- return ret.release();
-}
-
-int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out,
- const char *file) {
- bssl::UniquePtr<BIO> 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) {
+static int add_bio_cert_subjects_to_stack(STACK_OF(X509_NAME) *out, BIO *bio,
+ bool allow_empty) {
// 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|
@@ -165,15 +148,20 @@
RestoreCmpFunc restore = {out, sk_X509_NAME_set_cmp_func(out, xname_cmp)};
sk_X509_NAME_sort(out);
+ bool first = true;
for (;;) {
bssl::UniquePtr<X509> x509(
PEM_read_bio_X509(bio, nullptr, nullptr, nullptr));
if (x509 == nullptr) {
+ if (first && !allow_empty) {
+ return 0;
+ }
// TODO(davidben): This ignores PEM syntax errors. It should only succeed
// on |PEM_R_NO_START_LINE|.
ERR_clear_error();
break;
}
+ first = false;
X509_NAME *subject = X509_get_subject_name(x509.get());
// Skip if already present in |out|. Duplicates in |to_append| will be
@@ -211,6 +199,33 @@
return 1;
}
+STACK_OF(X509_NAME) *SSL_load_client_CA_file(const char *file) {
+ bssl::UniquePtr<BIO> in(BIO_new_file(file, "r"));
+ if (in == nullptr) {
+ return nullptr;
+ }
+ bssl::UniquePtr<STACK_OF(X509_NAME)> ret(sk_X509_NAME_new_null());
+ if (ret == nullptr || //
+ !add_bio_cert_subjects_to_stack(ret.get(), in.get(),
+ /*allow_empty=*/false)) {
+ return nullptr;
+ }
+ return ret.release();
+}
+
+int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out,
+ const char *file) {
+ bssl::UniquePtr<BIO> 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) {
+ return add_bio_cert_subjects_to_stack(out, bio, /*allow_empty=*/true);
+}
+
int SSL_use_certificate_file(SSL *ssl, const char *file, int type) {
int reason_code;
BIO *in;
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 1ea0893..295b6ef 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -8332,6 +8332,15 @@
}
}
+#if defined(OPENSSL_LINUX) || defined(OPENSSL_APPLE)
+TEST(SSLTest, EmptyClientCAList) {
+ // Use /dev/null on POSIX systems as an empty file.
+ bssl::UniquePtr<STACK_OF(X509_NAME)> names(
+ SSL_load_client_CA_file("/dev/null"));
+ EXPECT_FALSE(names);
+}
+#endif // OPENSSL_LINUX || OPENSSL_APPLE
+
TEST(SSLTest, EmptyWriteBlockedOnHandshakeData) {
bssl::UniquePtr<SSL_CTX> client_ctx(SSL_CTX_new(TLS_method()));
bssl::UniquePtr<SSL_CTX> server_ctx =
