commit 1d128f369c797b8bae996d5e973fc0b8a099a1b8
author David Benjamin <davidben@chromium.org> Tue Sep 08 17:41:40 2015 -0400
committer Adam Langley <agl@google.com> Fri Sep 11 22:30:55 2015 +0000
tree c6742edbf730a98d4119beb7e77358e60d0c9c8b
parent af096751e8d1cae9a570990b49b12527317c0003

Make SSL_get_client_CA_list slightly more OpenSSL-compatible.

SSL_get_client_CA_list is one of those dreaded functions which may query either
configuration state or handshake state. Moreover, it does so based on
|ssl->server|, which may not be configured until later. Also check
|ssl->handshake_func| to make sure |ssl| is not in an indeterminate state.

This also fixes a bug where SSL_get_client_CA_list wouldn't work in DTLS due to
the incorrect |ssl->version| check.

Change-Id: Ie564dbfeecd2c8257fd6bcb148bc5db827390c77
Reviewed-on: https://boringssl-review.googlesource.com/5827
Reviewed-by: Adam Langley <agl@google.com>

ssl/ssl_cert.c

diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 296004f..46cb4c0 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -368,20 +368,20 @@
   return ctx->client_CA;
 }
 
-STACK_OF(X509_NAME) *SSL_get_client_CA_list(const SSL *s) {
-  if (s->server) {
-    if (s->client_CA != NULL) {
-      return s->client_CA;
-    } else {
-      return s->ctx->client_CA;
-    }
-  } else {
-    if ((s->version >> 8) == SSL3_VERSION_MAJOR && s->s3 != NULL) {
-      return s->s3->tmp.ca_names;
-    } else {
-      return NULL;
-    }
+STACK_OF(X509_NAME) *SSL_get_client_CA_list(const SSL *ssl) {
+  /* For historical reasons, this function is used both to query configuration
+   * state on a server as well as handshake state on a client. However, whether
+   * |ssl| is a client or server is not known until explicitly configured with
+   * |SSL_set_connect_state|. If |handshake_func| is NULL, |ssl| is in an
+   * indeterminate mode and |ssl->server| is unset. */
+  if (ssl->handshake_func != NULL && !ssl->server) {
+    return ssl->s3->tmp.ca_names;
   }
+
+  if (ssl->client_CA != NULL) {
+    return ssl->client_CA;
+  }
+  return ssl->ctx->client_CA;
 }
 
 static int add_client_CA(STACK_OF(X509_NAME) **sk, X509 *x) {

ssl/ssl_test.cc

diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 2944748..810d3fa 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -796,7 +796,29 @@
   return true;
 }
 
-int main(void) {
+// Test that |SSL_get_client_CA_list| echoes back the configured parameter even
+// before configuring as a server.
+static bool TestClientCAList() {
+  ScopedSSL_CTX ctx(SSL_CTX_new(TLS_method()));
+  if (!ctx) {
+    return false;
+  }
+  ScopedSSL ssl(SSL_new(ctx.get()));
+  if (!ssl) {
+    return false;
+  }
+
+  STACK_OF(X509_NAME) *stack = sk_X509_NAME_new_null();
+  if (stack == nullptr) {
+    return false;
+  }
+  // |SSL_set_client_CA_list| takes ownership.
+  SSL_set_client_CA_list(ssl.get(), stack);
+
+  return SSL_get_client_CA_list(ssl.get()) == stack;
+}
+
+int main() {
   SSL_library_init();
 
   if (!TestCipherRules() ||
@@ -815,7 +837,8 @@
       !TestDefaultVersion(DTLS1_VERSION, &DTLSv1_method) ||
       !TestDefaultVersion(DTLS1_2_VERSION, &DTLSv1_2_method) ||
       !TestCipherGetRFCName() ||
-      !TestPaddingExtension()) {
+      !TestPaddingExtension() ||
+      !TestClientCAList()) {
     ERR_print_errors_fp(stderr);
     return 1;
   }