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 <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

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)
 

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.
 //

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;
+  bssl::UniquePtr<STACK_OF(X509_NAME)> ret(sk_X509_NAME_new_null());
-  X509 *x = NULL;
+  if (ret == nullptr ||
-  X509_NAME *xn = NULL;
+      !SSL_add_file_cert_subjects_to_stack(ret.get(), file)) {
-  STACK_OF(X509_NAME) *ret = NULL, *sk;
+    return nullptr;
-
-  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;
   }
+  return ret.release();
+}
 
-  if (!BIO_read_filename(in, file)) {
+int SSL_add_file_cert_subjects_to_stack(STACK_OF(X509_NAME) *out,
-    goto err;
+                                        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) {
+  // 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<STACK_OF(X509_NAME)> 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> 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.
+    X509_NAME *subject = X509_get_subject_name(x509.get());
-    sk_X509_NAME_sort(sk);
+    // Skip if already present in |out|. Duplicates in |to_append| will be
-    if (sk_X509_NAME_find(sk, NULL, xn)) {
+    // handled separately.
+    if (sk_X509_NAME_find(out, /*out_index=*/NULL, subject)) {
       continue;
     }
 
-    xn = X509_NAME_dup(xn);
+    bssl::UniquePtr<X509_NAME> copy(X509_NAME_dup(subject));
-    if (xn == NULL ||
+    if (copy == nullptr ||
-        !sk_X509_NAME_push(sk /* non-owning */, xn) ||
+        !bssl::PushToStack(to_append.get(), std::move(copy))) {
-        !sk_X509_NAME_push(ret /* owning */, xn)) {
+      return 0;
-      X509_NAME_free(xn);
-      goto err;
-    }
-  }
-
-  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.
+  // Append |to_append| to |stack|, skipping any duplicates.
-    sk_X509_NAME_sort(stack);
+  sk_X509_NAME_sort(to_append.get());
-    if (sk_X509_NAME_find(stack, NULL, xn)) {
+  size_t num = sk_X509_NAME_num(to_append.get());
+  for (size_t i = 0; i < num; i++) {
+    bssl::UniquePtr<X509_NAME> 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;
     }
-
+    if (!bssl::PushToStack(out, std::move(name))) {
-    xn = X509_NAME_dup(xn);
+      return 0;
-    if (xn == NULL ||
-        !sk_X509_NAME_push(stack, xn)) {
-      X509_NAME_free(xn);
-      goto err;
     }
   }
 
-  ERR_clear_error();
+  // Sort |out| one last time, to preserve the historical behavior of
-  ret = 1;
+  // maintaining the sorted list.
-
+  sk_X509_NAME_sort(out);
-err:
+  return 1;
-  BIO_free(in);
-  X509_free(x);
-
-  (void) sk_X509_NAME_set_cmp_func(stack, oldcmp);
-
-  return ret;
 }
 
 int SSL_use_certificate_file(SSL *ssl, const char *file, int type) {

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<uint8_t> 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<uint8_t> 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<std::vector<uint8_t>> existing;
+    std::string pem;
+    std::vector<std::vector<uint8_t>> 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_OF(X509_NAME)> stack(sk_X509_NAME_new_null());
+    ASSERT_TRUE(stack);
+    for (const auto& name : t.existing) {
+      const uint8_t *inp = name.data();
+      bssl::UniquePtr<X509_NAME> 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(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<std::vector<uint8_t>> 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<uint8_t>(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