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>
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index 2791abc..7794ddd 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -1478,7 +1478,8 @@
}
}
- 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;
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index 909e921..6ce49f5 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -916,6 +916,10 @@
}
}
+ 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 =
diff --git a/ssl/internal.h b/ssl/internal.h
index 7a10936..97ae6a0 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -779,6 +779,11 @@
* 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
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 3eaf499..397fbf0 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -557,56 +557,55 @@
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;
-
- if (!X509_STORE_CTX_init(&xs_ctx, ssl->ctx->cert_store, x, NULL)) {
- OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB);
- 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);
}
+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;
+ }
+
+ 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_STORE_CTX_cleanup(&ctx);
+ return 1;
+}
+
/* ssl_cert_skip_to_spki parses a DER-encoded, X.509 certificate from |in| and
* positions |*out_tbs_cert| to cover the TBSCertificate, starting at the
* subjectPublicKeyInfo. */
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 0c376b8..45461cd 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -2885,11 +2885,6 @@
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();
diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 67807e8..20b80ed 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -487,7 +487,8 @@
}
}
- if (!tls13_prepare_certificate(hs)) {
+ if (!ssl_auto_chain_if_needed(ssl) ||
+ !tls13_prepare_certificate(hs)) {
return ssl_hs_error;
}
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index a1bfe44..cdf78e6 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -199,6 +199,10 @@
}
}
+ 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)) {
