Clean up certificate auto-chaining.

Rather than doing it right before outputing, treat this as a part of the
pipeline to finalize the certificate chain, and run it right after
cert_cb to modify the certificate configuration itself. This means
nothing else in the stack needs to worry about this case existing.

It also makes it easy to support in both TLS 1.2 and TLS 1.3.

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

ssl/handshake_client.c

@@ -1478,7 +1478,8 @@ static int ssl3_send_client_certificate(SSL_HANDSHAKE *hs) {
     }
   }
 
-  if (!ssl3_output_cert_chain(ssl)) {
+  if (!ssl_auto_chain_if_needed(ssl) ||
+      !ssl3_output_cert_chain(ssl)) {
     return -1;
   }
   hs->state = SSL3_ST_CW_CERT_B;

ssl/handshake_server.c

@@ -916,6 +916,10 @@ static int ssl3_get_client_hello(SSL_HANDSHAKE *hs) {
       }
     }
 
+    if (!ssl_auto_chain_if_needed(ssl)) {
+      goto err;
+    }
+
     /* Negotiate the cipher suite. This must be done after |cert_cb| so the
      * certificate is finalized. */
     ssl->s3->tmp.new_cipher =

ssl/internal.h

@@ -779,6 +779,11 @@ int ssl_add_cert_to_cbb(CBB *cbb, X509 *x509);
  * empty certificate list. It returns one on success and zero on error. */
 int ssl_add_cert_chain(SSL *ssl, CBB *cbb);
 
+/* ssl_auto_chain_if_needed runs the deprecated auto-chaining logic if
+ * necessary. On success, it updates |ssl|'s certificate configuration as needed
+ * and returns one. Otherwise, it returns zero. */
+int ssl_auto_chain_if_needed(SSL *ssl);
+
 /* ssl_cert_check_digital_signature_key_usage parses the DER-encoded, X.509
  * certificate in |in| and returns one if doesn't specify a key usage or, if it
  * does, if it includes digitalSignature. Otherwise it pushes to the error

ssl/ssl_cert.c

@@ -557,54 +557,53 @@ int ssl_add_cert_chain(SSL *ssl, CBB *cbb) {
     return CBB_add_u24(cbb, 0);
   }
 
-  CERT *cert = ssl->cert;
-  X509 *x = cert->x509_leaf;
-
   CBB child;
-  if (!CBB_add_u24_length_prefixed(cbb, &child)) {
+  if (!CBB_add_u24_length_prefixed(cbb, &child) ||
+      !ssl_add_cert_with_length(&child, ssl->cert->x509_leaf)) {
     OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
     return 0;
   }
 
-  int no_chain = 0;
-  STACK_OF(X509) *chain = cert->x509_chain;
-  if ((ssl->mode & SSL_MODE_NO_AUTO_CHAIN) || chain != NULL) {
-    no_chain = 1;
-  }
-
-  if (no_chain) {
-    if (!ssl_add_cert_with_length(&child, x)) {
+  STACK_OF(X509) *chain = ssl->cert->x509_chain;
+  for (size_t i = 0; i < sk_X509_num(chain); i++) {
+    if (!ssl_add_cert_with_length(&child, sk_X509_value(chain, i))) {
       return 0;
     }
+  }
 
-    for (size_t i = 0; i < sk_X509_num(chain); i++) {
-      x = sk_X509_value(chain, i);
-      if (!ssl_add_cert_with_length(&child, x)) {
-        return 0;
-      }
-    }
-  } else {
-    X509_STORE_CTX xs_ctx;
+  return CBB_flush(cbb);
+}
+
+int ssl_auto_chain_if_needed(SSL *ssl) {
+  /* Only build a chain if there are no intermediates configured and the feature
+   * isn't disabled. */
+  if ((ssl->mode & SSL_MODE_NO_AUTO_CHAIN) ||
+      !ssl_has_certificate(ssl) ||
+      ssl->cert->x509_chain != NULL) {
+    return 1;
+  }
 
-    if (!X509_STORE_CTX_init(&xs_ctx, ssl->ctx->cert_store, x, NULL)) {
-      OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB);
+  X509_STORE_CTX ctx;
+  if (!X509_STORE_CTX_init(&ctx, ssl->ctx->cert_store, ssl->cert->x509_leaf,
+                           NULL)) {
+    OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB);
+    return 0;
+  }
+
+  /* Attempt to build a chain, ignoring the result. */
+  X509_verify_cert(&ctx);
+  ERR_clear_error();
+
+  /* Configure the intermediates from any partial chain we managed to build. */
+  for (size_t i = 1; i < sk_X509_num(ctx.chain); i++) {
+    if (!SSL_add1_chain_cert(ssl, sk_X509_value(ctx.chain, i))) {
+      X509_STORE_CTX_cleanup(&ctx);
       return 0;
     }
-    X509_verify_cert(&xs_ctx);
-    /* Don't leave errors in the queue */
-    ERR_clear_error();
-
-    for (size_t i = 0; i < sk_X509_num(xs_ctx.chain); i++) {
-      x = sk_X509_value(xs_ctx.chain, i);
-      if (!ssl_add_cert_with_length(&child, x)) {
-        X509_STORE_CTX_cleanup(&xs_ctx);
-        return 0;
-      }
-    }
-    X509_STORE_CTX_cleanup(&xs_ctx);
   }
 
-  return CBB_flush(cbb);
+  X509_STORE_CTX_cleanup(&ctx);
+  return 1;
 }
 
 /* ssl_cert_skip_to_spki parses a DER-encoded, X.509 certificate from |in| and

ssl/ssl_test.cc

@@ -2885,11 +2885,6 @@ static bool ChainsEqual(STACK_OF(X509) *chain,
 
 static bool TestAutoChain(bool is_dtls, const SSL_METHOD *method,
                           uint16_t version) {
-  if (version == TLS1_3_VERSION) {
-    // TODO(davidben): Auto-chaining does not currently work in TLS 1.3.
-    return true;
-  }
-
   bssl::UniquePtr<X509> cert = GetChainTestCertificate();
   bssl::UniquePtr<X509> intermediate = GetChainTestIntermediate();
   bssl::UniquePtr<EVP_PKEY> key = GetChainTestKey();

ssl/tls13_client.c

@@ -487,7 +487,8 @@ static enum ssl_hs_wait_t do_send_client_certificate(SSL_HANDSHAKE *hs) {
     }
   }
 
-  if (!tls13_prepare_certificate(hs)) {
+  if (!ssl_auto_chain_if_needed(ssl) ||
+      !tls13_prepare_certificate(hs)) {
     return ssl_hs_error;
   }
 

ssl/tls13_server.c

@@ -199,6 +199,10 @@ static enum ssl_hs_wait_t do_select_parameters(SSL_HANDSHAKE *hs) {
     }
   }
 
+  if (!ssl_auto_chain_if_needed(ssl)) {
+    return ssl_hs_error;
+  }
+
   SSL_CLIENT_HELLO client_hello;
   if (!ssl_client_hello_init(ssl, &client_hello, ssl->init_msg,
                              ssl->init_num)) {