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>

ssl/ssl_file.cc

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;

ssl/ssl_test.cc

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 =