Move some fields from tmp to hs.
This releases memory associated with them after the handshake. Note this
changes the behavior of |SSL_get0_certificate_types| and
|SSL_get_client_CA_list| slightly. Both functions now return NULL
outside of the handshake. But they were already documented to return
something undefined when not called at the CertificateRequest.
A survey of callers finds none that would care. (Note
SSL_get_client_CA_list is used both as a getter for the corresponding
server config setter and to report client handshake properties. Only the
latter is affected.) It's also pretty difficult to imagine why a caller
would wish to query this stuff at any other time, and there are clear
benefits to dropping the CA list after the handshake (some servers send
ABSURDLY large lists).
Change-Id: I3ac3b601ff0cfa601881ce77ae33d99bb5327004
Reviewed-on: https://boringssl-review.googlesource.com/11521
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 938bcb5..f920ce6 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -4358,19 +4358,6 @@
uint16_t received;
} custom_extensions;
- /* should_ack_sni is used by a server and indicates that the SNI extension
- * should be echoed in the ServerHello. */
- unsigned should_ack_sni:1;
-
- /* Client-only: ca_names contains the list of CAs received in a
- * CertificateRequest message. */
- STACK_OF(X509_NAME) *ca_names;
-
- /* Client-only: certificate_types contains the set of certificate types
- * received in a CertificateRequest message. */
- uint8_t *certificate_types;
- size_t num_certificate_types;
-
uint8_t *key_block;
uint8_t key_block_length;
@@ -4378,18 +4365,6 @@
uint8_t new_key_len;
uint8_t new_fixed_iv_len;
- /* cert_request is true if a client certificate was requested and false
- * otherwise. */
- unsigned cert_request:1;
-
- /* certificate_status_expected is true if OCSP stapling was negotiated and
- * the server is expected to send a CertificateStatus message. (This is
- * used on both the client and server sides.) */
- unsigned certificate_status_expected:1;
-
- /* ocsp_stapling_requested is true if a client requested OCSP stapling. */
- unsigned ocsp_stapling_requested:1;
-
/* Server-only: peer_supported_group_list contains the supported group IDs
* advertised by the peer. This is only set on the server's end. The server
* does not advertise this extension to the client. */
@@ -4404,10 +4379,6 @@
* didn't use it to create the master secret initially. */
char extended_master_secret;
- /* Client-only: in_false_start is one if there is a pending handshake in
- * False Start. The client may write data at this point. */
- char in_false_start;
-
/* peer_signature_algorithm is the signature algorithm used to authenticate
* the peer, or zero if not applicable. */
uint16_t peer_signature_algorithm;
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index 31394c0..e2b5a71 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -276,7 +276,7 @@
break;
case SSL3_ST_CR_CERT_STATUS_A:
- if (ssl->s3->tmp.certificate_status_expected) {
+ if (ssl->s3->hs->certificate_status_expected) {
ret = ssl3_get_cert_status(ssl);
if (ret <= 0) {
goto end;
@@ -331,7 +331,7 @@
case SSL3_ST_CW_CERT_A:
case SSL3_ST_CW_CERT_B:
case SSL3_ST_CW_CERT_C:
- if (ssl->s3->tmp.cert_request) {
+ if (ssl->s3->hs->cert_request) {
ret = ssl3_send_client_certificate(ssl);
if (ret <= 0) {
goto end;
@@ -354,7 +354,7 @@
case SSL3_ST_CW_CERT_VRFY_A:
case SSL3_ST_CW_CERT_VRFY_B:
case SSL3_ST_CW_CERT_VRFY_C:
- if (ssl->s3->tmp.cert_request) {
+ if (ssl->s3->hs->cert_request) {
ret = ssl3_send_cert_verify(ssl);
if (ret <= 0) {
goto end;
@@ -440,7 +440,7 @@
case SSL3_ST_FALSE_START:
ssl->state = SSL3_ST_CR_SESSION_TICKET_A;
- ssl->s3->tmp.in_false_start = 1;
+ ssl->s3->hs->in_false_start = 1;
ssl_free_wbio_buffer(ssl);
ret = 1;
@@ -541,10 +541,7 @@
ssl->s3->hs = NULL;
const int is_initial_handshake = !ssl->s3->initial_handshake_complete;
-
- ssl->s3->tmp.in_false_start = 0;
ssl->s3->initial_handshake_complete = 1;
-
if (is_initial_handshake) {
/* Renegotiations do not participate in session resumption. */
ssl_update_cache(ssl, SSL_SESS_CACHE_CLIENT);
@@ -1385,8 +1382,6 @@
return msg_ret;
}
- ssl->s3->tmp.cert_request = 0;
-
if (ssl->s3->tmp.message_type == SSL3_MT_SERVER_HELLO_DONE) {
ssl->s3->tmp.reuse_message = 1;
/* If we get here we don't need the handshake buffer as we won't be doing
@@ -1412,8 +1407,8 @@
return -1;
}
- if (!CBS_stow(&certificate_types, &ssl->s3->tmp.certificate_types,
- &ssl->s3->tmp.num_certificate_types)) {
+ if (!CBS_stow(&certificate_types, &ssl->s3->hs->certificate_types,
+ &ssl->s3->hs->num_certificate_types)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return -1;
}
@@ -1442,9 +1437,9 @@
return -1;
}
- ssl->s3->tmp.cert_request = 1;
- sk_X509_NAME_pop_free(ssl->s3->tmp.ca_names, X509_NAME_free);
- ssl->s3->tmp.ca_names = ca_sk;
+ ssl->s3->hs->cert_request = 1;
+ sk_X509_NAME_pop_free(ssl->s3->hs->ca_names, X509_NAME_free);
+ ssl->s3->hs->ca_names = ca_sk;
return 1;
}
@@ -1494,7 +1489,7 @@
}
if (!ssl_has_certificate(ssl)) {
- ssl->s3->tmp.cert_request = 0;
+ ssl->s3->hs->cert_request = 0;
/* Without a client certificate, the handshake buffer may be released. */
ssl3_free_handshake_buffer(ssl);
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index fd0223f..935c40e 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -270,7 +270,7 @@
case SSL3_ST_SW_CERT_STATUS_A:
case SSL3_ST_SW_CERT_STATUS_B:
- if (ssl->s3->tmp.certificate_status_expected) {
+ if (ssl->s3->hs->certificate_status_expected) {
ret = ssl3_send_certificate_status(ssl);
if (ret <= 0) {
goto end;
@@ -302,7 +302,7 @@
case SSL3_ST_SW_CERT_REQ_A:
case SSL3_ST_SW_CERT_REQ_B:
- if (ssl->s3->tmp.cert_request) {
+ if (ssl->s3->hs->cert_request) {
ret = ssl3_send_certificate_request(ssl);
if (ret <= 0) {
goto end;
@@ -324,7 +324,7 @@
break;
case SSL3_ST_SR_CERT_A:
- if (ssl->s3->tmp.cert_request) {
+ if (ssl->s3->hs->cert_request) {
ret = ssl3_get_client_certificate(ssl);
if (ret <= 0) {
goto end;
@@ -836,7 +836,6 @@
}
ssl->s3->tmp.new_cipher = ssl->session->cipher;
- ssl->s3->tmp.cert_request = 0;
} else {
/* Call |cert_cb| to update server certificates if required. */
if (ssl->cert->cert_cb != NULL) {
@@ -864,18 +863,18 @@
ssl->s3->tmp.new_cipher = c;
/* Determine whether to request a client certificate. */
- ssl->s3->tmp.cert_request = !!(ssl->verify_mode & SSL_VERIFY_PEER);
+ ssl->s3->hs->cert_request = !!(ssl->verify_mode & SSL_VERIFY_PEER);
/* Only request a certificate if Channel ID isn't negotiated. */
if ((ssl->verify_mode & SSL_VERIFY_PEER_IF_NO_OBC) &&
ssl->s3->tlsext_channel_id_valid) {
- ssl->s3->tmp.cert_request = 0;
+ ssl->s3->hs->cert_request = 0;
}
/* CertificateRequest may only be sent in certificate-based ciphers. */
if (!ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
- ssl->s3->tmp.cert_request = 0;
+ ssl->s3->hs->cert_request = 0;
}
- if (!ssl->s3->tmp.cert_request) {
+ if (!ssl->s3->hs->cert_request) {
/* OpenSSL returns X509_V_OK when no certificates are requested. This is
* classed by them as a bug, but it's assumed by at least NGINX. */
ssl->s3->new_session->verify_result = X509_V_OK;
@@ -888,7 +887,7 @@
}
/* Release the handshake buffer if client authentication isn't required. */
- if (!ssl->s3->tmp.cert_request) {
+ if (!ssl->s3->hs->cert_request) {
ssl3_free_handshake_buffer(ssl);
}
@@ -1291,7 +1290,7 @@
}
static int ssl3_get_client_certificate(SSL *ssl) {
- assert(ssl->s3->tmp.cert_request);
+ assert(ssl->s3->hs->cert_request);
int msg_ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
if (msg_ret <= 0) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 720b822..e3c2668 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -922,11 +922,42 @@
/* num_peer_sigalgs is the number of entries in |peer_sigalgs|. */
size_t num_peer_sigalgs;
+ /* session_tickets_sent, in TLS 1.3, is the number of tickets the server has
+ * sent. */
uint8_t session_tickets_sent;
+ /* cert_request is one if a client certificate was requested and zero
+ * otherwise. */
+ unsigned cert_request:1;
+
+ /* certificate_status_expected is one if OCSP stapling was negotiated and the
+ * server is expected to send a CertificateStatus message. (This is used on
+ * both the client and server sides.) */
+ unsigned certificate_status_expected:1;
+
+ /* ocsp_stapling_requested is one if a client requested OCSP stapling. */
+ unsigned ocsp_stapling_requested:1;
+
+ /* should_ack_sni is used by a server and indicates that the SNI extension
+ * should be echoed in the ServerHello. */
+ unsigned should_ack_sni:1;
+
+ /* in_false_start is one if there is a pending client handshake in False
+ * Start. The client may write data at this point. */
+ unsigned in_false_start:1;
+
/* peer_psk_identity_hint, on the client, is the psk_identity_hint sent by the
* server when using a TLS 1.2 PSK key exchange. */
char *peer_psk_identity_hint;
+
+ /* ca_names, on the client, contains the list of CAs received in a
+ * CertificateRequest message. */
+ STACK_OF(X509_NAME) *ca_names;
+
+ /* certificate_types, on the client, contains the set of certificate types
+ * received in a CertificateRequest message. */
+ uint8_t *certificate_types;
+ size_t num_certificate_types;
} /* SSL_HANDSHAKE */;
SSL_HANDSHAKE *ssl_handshake_new(enum ssl_hs_wait_t (*do_handshake)(SSL *ssl));
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 52c93aa..5092a3b 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -154,6 +154,8 @@
OPENSSL_free(hs->public_key);
OPENSSL_free(hs->peer_sigalgs);
OPENSSL_free(hs->peer_psk_identity_hint);
+ sk_X509_NAME_pop_free(hs->ca_names, X509_NAME_free);
+ OPENSSL_free(hs->certificate_types);
OPENSSL_free(hs);
}
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 2a7bbae..472c9cb 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -207,8 +207,6 @@
OPENSSL_free(ssl->s3->tmp.peer_key);
OPENSSL_free(ssl->s3->tmp.server_params);
- sk_X509_NAME_pop_free(ssl->s3->tmp.ca_names, X509_NAME_free);
- OPENSSL_free(ssl->s3->tmp.certificate_types);
OPENSSL_free(ssl->s3->tmp.peer_supported_group_list);
SSL_SESSION_free(ssl->s3->new_session);
SSL_SESSION_free(ssl->s3->established_session);
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 0c5a7af..9c6d08e 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -395,7 +395,11 @@
* |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->s3->hs != NULL) {
+ return ssl->s3->hs->ca_names;
+ }
+
+ return NULL;
}
if (ssl->client_CA != NULL) {
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index f17dc0a..5235b4d 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2021,12 +2021,12 @@
}
size_t SSL_get0_certificate_types(SSL *ssl, const uint8_t **out_types) {
- if (ssl->server) {
+ if (ssl->server || ssl->s3->hs == NULL) {
*out_types = NULL;
return 0;
}
- *out_types = ssl->s3->tmp.certificate_types;
- return ssl->s3->tmp.num_certificate_types;
+ *out_types = ssl->s3->hs->certificate_types;
+ return ssl->s3->hs->num_certificate_types;
}
void ssl_get_compatible_server_ciphers(SSL *ssl, uint32_t *out_mask_k,
@@ -2674,7 +2674,10 @@
}
int SSL_in_false_start(const SSL *ssl) {
- return ssl->s3->tmp.in_false_start;
+ if (ssl->s3->hs == NULL) {
+ return 0;
+ }
+ return ssl->s3->hs->in_false_start;
}
int SSL_cutthrough_complete(const SSL *ssl) {
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 2aca268..3dbfb5e 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -706,10 +706,6 @@
*
* https://tools.ietf.org/html/rfc6066#section-3. */
-static void ext_sni_init(SSL *ssl) {
- ssl->s3->tmp.should_ack_sni = 0;
-}
-
static int ext_sni_add_clienthello(SSL *ssl, CBB *out) {
if (ssl->tlsext_hostname == NULL) {
return 1;
@@ -797,7 +793,7 @@
return 0;
}
- ssl->s3->tmp.should_ack_sni = 1;
+ ssl->s3->hs->should_ack_sni = 1;
}
return 1;
@@ -805,7 +801,7 @@
static int ext_sni_add_serverhello(SSL *ssl, CBB *out) {
if (ssl->session != NULL ||
- !ssl->s3->tmp.should_ack_sni ||
+ !ssl->s3->hs->should_ack_sni ||
ssl->s3->new_session->tlsext_hostname == NULL) {
return 1;
}
@@ -1211,7 +1207,6 @@
* https://tools.ietf.org/html/rfc6066#section-8 */
static void ext_ocsp_init(SSL *ssl) {
- ssl->s3->tmp.certificate_status_expected = 0;
ssl->tlsext_status_type = -1;
}
@@ -1251,7 +1246,7 @@
* status_request here does not make sense, but OpenSSL does so and the
* specification does not say anything. Tolerate it but ignore it. */
- ssl->s3->tmp.certificate_status_expected = 1;
+ ssl->s3->hs->certificate_status_expected = 1;
return 1;
}
@@ -1292,13 +1287,13 @@
/* We cannot decide whether OCSP stapling will occur yet because the correct
* SSL_CTX might not have been selected. */
- ssl->s3->tmp.ocsp_stapling_requested = status_type == TLSEXT_STATUSTYPE_ocsp;
+ ssl->s3->hs->ocsp_stapling_requested = status_type == TLSEXT_STATUSTYPE_ocsp;
return 1;
}
static int ext_ocsp_add_serverhello(SSL *ssl, CBB *out) {
- if (!ssl->s3->tmp.ocsp_stapling_requested ||
+ if (!ssl->s3->hs->ocsp_stapling_requested ||
ssl->ctx->ocsp_response_length == 0 ||
ssl->s3->session_reused ||
(ssl3_protocol_version(ssl) < TLS1_3_VERSION &&
@@ -1312,7 +1307,7 @@
return 1;
}
- ssl->s3->tmp.certificate_status_expected = 1;
+ ssl->s3->hs->certificate_status_expected = 1;
return CBB_add_u16(out, TLSEXT_TYPE_status_request) &&
CBB_add_u16(out, 0 /* length */);
@@ -2482,7 +2477,7 @@
},
{
TLSEXT_TYPE_server_name,
- ext_sni_init,
+ NULL,
ext_sni_add_clienthello,
ext_sni_parse_serverhello,
ext_sni_parse_clienthello,
@@ -2979,7 +2974,7 @@
return 1;
case SSL_TLSEXT_ERR_NOACK:
- ssl->s3->tmp.should_ack_sni = 0;
+ ssl->s3->hs->should_ack_sni = 0;
return 1;
default:
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index c4407da..1527173 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -603,6 +603,31 @@
}
static int CertCallback(SSL *ssl, void *arg) {
+ const TestConfig *config = GetTestConfig(ssl);
+
+ // Check the CertificateRequest metadata is as expected.
+ //
+ // TODO(davidben): Test |SSL_get_client_CA_list|.
+ if (!SSL_is_server(ssl) &&
+ !config->expected_certificate_types.empty()) {
+ const uint8_t *certificate_types;
+ size_t certificate_types_len =
+ SSL_get0_certificate_types(ssl, &certificate_types);
+ if (certificate_types_len != config->expected_certificate_types.size() ||
+ memcmp(certificate_types,
+ config->expected_certificate_types.data(),
+ certificate_types_len) != 0) {
+ fprintf(stderr, "certificate types mismatch\n");
+ return 0;
+ }
+ }
+
+ // The certificate will be installed via other means.
+ if (!config->async || config->use_early_callback ||
+ config->use_old_client_cert_callback) {
+ return 1;
+ }
+
if (!GetTestState(ssl)->cert_ready) {
return -1;
}
@@ -1153,19 +1178,6 @@
}
}
- if (!config->expected_certificate_types.empty()) {
- const uint8_t *certificate_types;
- size_t certificate_types_len =
- SSL_get0_certificate_types(ssl, &certificate_types);
- if (certificate_types_len != config->expected_certificate_types.size() ||
- memcmp(certificate_types,
- config->expected_certificate_types.data(),
- certificate_types_len) != 0) {
- fprintf(stderr, "certificate types mismatch\n");
- return false;
- }
- }
-
if (!config->expected_next_proto.empty()) {
const uint8_t *next_proto;
unsigned next_proto_len;
@@ -1309,13 +1321,14 @@
!SSL_set_mode(ssl.get(), SSL_MODE_SEND_FALLBACK_SCSV)) {
return false;
}
- if (!config->use_early_callback && !config->use_old_client_cert_callback) {
- if (config->async) {
- SSL_set_cert_cb(ssl.get(), CertCallback, NULL);
- } else if (!InstallCertificate(ssl.get())) {
- return false;
- }
+ // Install the certificate synchronously if nothing else will handle it.
+ if (!config->use_early_callback &&
+ !config->use_old_client_cert_callback &&
+ !config->async &&
+ !InstallCertificate(ssl.get())) {
+ return false;
}
+ SSL_set_cert_cb(ssl.get(), CertCallback, nullptr);
if (config->require_any_client_certificate) {
SSL_set_verify(ssl.get(), SSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERT,
NULL);
diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 8f1b516..49ba096 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -359,8 +359,6 @@
static enum ssl_hs_wait_t do_process_certificate_request(SSL *ssl,
SSL_HANDSHAKE *hs) {
- ssl->s3->tmp.cert_request = 0;
-
/* CertificateRequest may only be sent in non-resumption handshakes. */
if (ssl->s3->session_reused) {
hs->state = state_process_server_finished;
@@ -403,9 +401,9 @@
return ssl_hs_error;
}
- ssl->s3->tmp.cert_request = 1;
- sk_X509_NAME_pop_free(ssl->s3->tmp.ca_names, X509_NAME_free);
- ssl->s3->tmp.ca_names = ca_sk;
+ ssl->s3->hs->cert_request = 1;
+ sk_X509_NAME_pop_free(ssl->s3->hs->ca_names, X509_NAME_free);
+ ssl->s3->hs->ca_names = ca_sk;
if (!ssl->method->hash_current_message(ssl)) {
return ssl_hs_error;
@@ -458,7 +456,7 @@
static enum ssl_hs_wait_t do_certificate_callback(SSL *ssl, SSL_HANDSHAKE *hs) {
/* The peer didn't request a certificate. */
- if (!ssl->s3->tmp.cert_request) {
+ if (!ssl->s3->hs->cert_request) {
hs->state = state_send_client_finished;
return ssl_hs_ok;
}
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 7332347..be485a1 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -360,13 +360,13 @@
static enum ssl_hs_wait_t do_send_certificate_request(SSL *ssl,
SSL_HANDSHAKE *hs) {
/* Determine whether to request a client certificate. */
- ssl->s3->tmp.cert_request = !!(ssl->verify_mode & SSL_VERIFY_PEER);
+ ssl->s3->hs->cert_request = !!(ssl->verify_mode & SSL_VERIFY_PEER);
/* CertificateRequest may only be sent in non-resumption handshakes. */
if (ssl->s3->session_reused) {
- ssl->s3->tmp.cert_request = 0;
+ ssl->s3->hs->cert_request = 0;
}
- if (!ssl->s3->tmp.cert_request) {
+ if (!ssl->s3->hs->cert_request) {
/* Skip this state. */
hs->state = state_send_server_certificate;
return ssl_hs_ok;
@@ -469,7 +469,7 @@
static enum ssl_hs_wait_t do_process_client_certificate(SSL *ssl,
SSL_HANDSHAKE *hs) {
- if (!ssl->s3->tmp.cert_request) {
+ if (!ssl->s3->hs->cert_request) {
/* OpenSSL returns X509_V_OK when no certificates are requested. This is
* classed by them as a bug, but it's assumed by at least NGINX. */
ssl->s3->new_session->verify_result = X509_V_OK;
