Check CA names during the handshake.

Rather than store CA names and only find out that they're unparsable
when we're asked for a |STACK_OF(X509_NAME)|, check that we can parse
them all during the handshake. This avoids changing the semantics with
the previous change that kept CA names as |CRYPTO_BUFFER|s.

Change-Id: I0fc7a4e6ab01685347e7a5be0d0579f45b8a4818
Reviewed-on: https://boringssl-review.googlesource.com/13969
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index db526a8..519ed79 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1446,6 +1446,11 @@
 };
 
 struct ssl_x509_method_st {
+  /* check_client_CA_list returns one if |names| is a good list of X.509
+   * distinguished names and zero otherwise. This is used to ensure that we can
+   * reject unparsable values at handshake time when using crypto/x509. */
+  int (*check_client_CA_list)(STACK_OF(CRYPTO_BUFFER) *names);
+
   /* cert_clear frees and NULLs all X509-related state. */
   void (*cert_clear)(CERT *cert);
   /* cert_flush_cached_chain drops any cached |X509|-based certificate chain

ssl/ssl_cert.c

diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index c334ea6..0498cd2 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -726,6 +726,12 @@
     }
   }
 
+  if (!ssl->ctx->x509_method->check_client_CA_list(ret)) {
+    *out_alert = SSL_AD_INTERNAL_ERROR;
+    OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+    goto err;
+  }
+
   return ret;
 
 err:

ssl/ssl_x509.c

diff --git a/ssl/ssl_x509.c b/ssl/ssl_x509.c
index 8a132ff..81d2642 100644
--- a/ssl/ssl_x509.c
+++ b/ssl/ssl_x509.c
@@ -331,6 +331,23 @@
   cert->x509_chain = NULL;
 }
 
+static int ssl_crypto_x509_check_client_CA_list(
+    STACK_OF(CRYPTO_BUFFER) *names) {
+  for (size_t i = 0; i < sk_CRYPTO_BUFFER_num(names); i++) {
+    const CRYPTO_BUFFER *buffer = sk_CRYPTO_BUFFER_value(names, i);
+    const uint8_t *inp = CRYPTO_BUFFER_data(buffer);
+    X509_NAME *name = d2i_X509_NAME(NULL, &inp, CRYPTO_BUFFER_len(buffer));
+    const int ok = name != NULL && inp == CRYPTO_BUFFER_data(buffer) +
+                                              CRYPTO_BUFFER_len(buffer);
+    X509_NAME_free(name);
+    if (!ok) {
+      return 0;
+    }
+  }
+
+  return 1;
+}
+
 static void ssl_crypto_x509_clear(CERT *cert) {
   ssl_crypto_x509_flush_cached_leaf(cert);
   ssl_crypto_x509_flush_cached_chain(cert);
@@ -427,6 +444,7 @@
 }
 
 const SSL_X509_METHOD ssl_crypto_x509_method = {
+  ssl_crypto_x509_check_client_CA_list,
   ssl_crypto_x509_clear,
   ssl_crypto_x509_flush_cached_chain,
   ssl_crypto_x509_flush_cached_leaf,

ssl/tls_method.c

diff --git a/ssl/tls_method.c b/ssl/tls_method.c
index d7e4701..dbb8183 100644
--- a/ssl/tls_method.c
+++ b/ssl/tls_method.c
@@ -258,6 +258,11 @@
   return TLS_method();
 }
 
+static int ssl_noop_x509_check_client_CA_names(
+    STACK_OF(CRYPTO_BUFFER) *names) {
+  return 1;
+}
+
 static void ssl_noop_x509_clear(CERT *cert) {}
 static void ssl_noop_x509_flush_cached_leaf(CERT *cert) {}
 static void ssl_noop_x509_flush_cached_chain(CERT *cert) {}
@@ -275,6 +280,7 @@
 static void ssl_noop_x509_ssl_ctx_flush_cached_client_CA(SSL_CTX *ctx) {}
 
 const SSL_X509_METHOD ssl_noop_x509_method = {
+  ssl_noop_x509_check_client_CA_names,
   ssl_noop_x509_clear,
   ssl_noop_x509_flush_cached_chain,
   ssl_noop_x509_flush_cached_leaf,