Parse ClientHello extensions before deciding on resumption.
This simplifies a little code around EMS and PSK KE modes, but requires
tweaking the SNI code.
The extensions that are more tightly integrated with the handshake are
still processed inline for now. It does, however, require an extra state
in 1.2 so the asynchronous session callback does not cause extensions to
be processed twice. Tweak a test enforce this.
This and a follow-up to move cert_cb before resumption are done in
preparation for resolving the cipher suite before resumption and only
resuming on match.
Note this has caller-visible effects:
- The legacy SNI callback happens before resumption.
- The ALPN callback happens before resumption.
- Custom extension ClientHello parsing callbacks also cannot depend on
resumption state.
- The DoS protection callback now runs after all the extension callbacks
as it is documented to be called after the resumption decision.
BUG=116
Change-Id: I1281a3b61789b95c370314aaed4f04c1babbc65f
Reviewed-on: https://boringssl-review.googlesource.com/11845
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>
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 4a99196..4cf51e1 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -352,6 +352,7 @@
#define SSL3_ST_SR_CLNT_HELLO_B (0x111 | SSL_ST_ACCEPT)
#define SSL3_ST_SR_CLNT_HELLO_C (0x112 | SSL_ST_ACCEPT)
#define SSL3_ST_SR_CLNT_HELLO_D (0x113 | SSL_ST_ACCEPT)
+#define SSL3_ST_SR_CLNT_HELLO_E (0x114 | SSL_ST_ACCEPT)
/* write to client */
#define SSL3_ST_SW_HELLO_REQ_A (0x120 | SSL_ST_ACCEPT)
#define SSL3_ST_SW_HELLO_REQ_B (0x121 | SSL_ST_ACCEPT)
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index 4c12d09..1c61cdf 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -231,6 +231,7 @@
case SSL3_ST_SR_CLNT_HELLO_B:
case SSL3_ST_SR_CLNT_HELLO_C:
case SSL3_ST_SR_CLNT_HELLO_D:
+ case SSL3_ST_SR_CLNT_HELLO_E:
ret = ssl3_get_client_hello(ssl);
if (ssl->state == SSL_ST_TLS13) {
break;
@@ -729,6 +730,24 @@
memcpy(ssl->s3->client_random, client_hello.random,
client_hello.random_len);
+ /* Only null compression is supported. */
+ if (memchr(client_hello.compression_methods, 0,
+ client_hello.compression_methods_len) == NULL) {
+ al = SSL_AD_ILLEGAL_PARAMETER;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_NO_COMPRESSION_SPECIFIED);
+ goto f_err;
+ }
+
+ /* TLS extensions. */
+ if (!ssl_parse_clienthello_tlsext(ssl, &client_hello)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
+ goto err;
+ }
+
+ ssl->state = SSL3_ST_SR_CLNT_HELLO_D;
+ }
+
+ if (ssl->state == SSL3_ST_SR_CLNT_HELLO_D) {
/* Determine whether we are doing session resumption. */
int tickets_supported = 0, renew_ticket = 0;
switch (ssl_get_prev_session(ssl, &session, &tickets_supported,
@@ -742,20 +761,9 @@
goto err;
}
- /* The EMS state is needed when making the resumption decision, but
- * extensions are not normally parsed until later. This detects the EMS
- * extension for the resumption decision and it's checked against the result
- * of the normal parse later in this function. */
- CBS ems;
- int have_extended_master_secret =
- ssl->version != SSL3_VERSION &&
- ssl_early_callback_get_extension(&client_hello, &ems,
- TLSEXT_TYPE_extended_master_secret) &&
- CBS_len(&ems) == 0;
-
if (session != NULL) {
if (session->extended_master_secret &&
- !have_extended_master_secret) {
+ !ssl->s3->tmp.extended_master_secret) {
/* A ClientHello without EMS that attempts to resume a session with EMS
* is fatal to the connection. */
al = SSL_AD_HANDSHAKE_FAILURE;
@@ -766,7 +774,8 @@
if (!ssl_session_is_resumable(ssl, session) ||
/* If the client offers the EMS extension, but the previous session
* didn't use it, then negotiate a new session. */
- have_extended_master_secret != session->extended_master_secret) {
+ ssl->s3->tmp.extended_master_secret !=
+ session->extended_master_secret) {
SSL_SESSION_free(session);
session = NULL;
}
@@ -799,32 +808,12 @@
goto f_err;
}
- /* Only null compression is supported. */
- if (memchr(client_hello.compression_methods, 0,
- client_hello.compression_methods_len) == NULL) {
- al = SSL_AD_ILLEGAL_PARAMETER;
- OPENSSL_PUT_ERROR(SSL, SSL_R_NO_COMPRESSION_SPECIFIED);
- goto f_err;
- }
-
- /* TLS extensions. */
- if (!ssl_parse_clienthello_tlsext(ssl, &client_hello)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
- goto err;
- }
-
- if (have_extended_master_secret != ssl->s3->tmp.extended_master_secret) {
- al = SSL_AD_INTERNAL_ERROR;
- OPENSSL_PUT_ERROR(SSL, SSL_R_EMS_STATE_INCONSISTENT);
- goto f_err;
- }
-
- ssl->state = SSL3_ST_SR_CLNT_HELLO_D;
+ ssl->state = SSL3_ST_SR_CLNT_HELLO_E;
}
/* Determine the remaining connection parameters. This is a separate state so
* |cert_cb| does not cause earlier logic to run multiple times. */
- assert(ssl->state == SSL3_ST_SR_CLNT_HELLO_D);
+ assert(ssl->state == SSL3_ST_SR_CLNT_HELLO_E);
if (ssl->session != NULL) {
/* Check that the cipher is in the list. */
@@ -862,6 +851,15 @@
ssl->s3->new_session->cipher = c;
ssl->s3->tmp.new_cipher = c;
+ /* On new sessions, stash the SNI value in the session. */
+ if (ssl->s3->hs->hostname != NULL) {
+ ssl->s3->new_session->tlsext_hostname = BUF_strdup(ssl->s3->hs->hostname);
+ if (ssl->s3->new_session->tlsext_hostname == NULL) {
+ al = SSL_AD_INTERNAL_ERROR;
+ goto f_err;
+ }
+ }
+
/* Determine whether to request a client certificate. */
ssl->s3->hs->cert_request = !!(ssl->verify_mode & SSL_VERIFY_PEER);
/* Only request a certificate if Channel ID isn't negotiated. */
diff --git a/ssl/internal.h b/ssl/internal.h
index 8f4300e..8860d63 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1019,6 +1019,9 @@
/* key_block is the record-layer key block for TLS 1.2 and earlier. */
uint8_t *key_block;
uint8_t key_block_len;
+
+ /* hostname, on the server, is the value of the SNI extension. */
+ char *hostname;
} SSL_HANDSHAKE;
SSL_HANDSHAKE *ssl_handshake_new(enum ssl_hs_wait_t (*do_handshake)(SSL *ssl));
@@ -1071,10 +1074,6 @@
uint8_t *out_alert, CBS *contents);
int ssl_ext_pre_shared_key_add_serverhello(SSL *ssl, CBB *out);
-int ssl_ext_psk_key_exchange_modes_parse_clienthello(SSL *ssl,
- uint8_t *out_alert,
- CBS *contents);
-
int ssl_write_client_hello(SSL *ssl);
/* ssl_clear_tls13_state releases client state only needed for TLS 1.3. It
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index f59022f..d872020 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -169,6 +169,7 @@
OPENSSL_free(hs->key_block);
}
+ OPENSSL_free(hs->hostname);
OPENSSL_free(hs);
}
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index ca3022c..982cb1a 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1706,6 +1706,15 @@
return ssl->tlsext_hostname;
}
+ /* During the handshake, report the handshake value. */
+ if (ssl->s3->hs != NULL) {
+ return ssl->s3->hs->hostname;
+ }
+
+ /* SSL_get_servername may also be called after the handshake to look up the
+ * SNI value.
+ *
+ * TODO(davidben): This is almost unused. Can we remove it? */
SSL_SESSION *session = SSL_get_session(ssl);
if (session == NULL) {
return NULL;
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index faf155c..4a1f88d 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -234,6 +234,13 @@
memcpy(new_session->peer_sha256, session->peer_sha256, SHA256_DIGEST_LENGTH);
new_session->peer_sha256_valid = session->peer_sha256_valid;
+ if (session->tlsext_hostname != NULL) {
+ new_session->tlsext_hostname = BUF_strdup(session->tlsext_hostname);
+ if (new_session->tlsext_hostname == NULL) {
+ goto err;
+ }
+ }
+
new_session->timeout = session->timeout;
new_session->time = session->time;
@@ -245,13 +252,6 @@
new_session->key_exchange_info = session->key_exchange_info;
- if (session->tlsext_hostname != NULL) {
- new_session->tlsext_hostname = BUF_strdup(session->tlsext_hostname);
- if (new_session->tlsext_hostname == NULL) {
- goto err;
- }
- }
-
memcpy(new_session->original_handshake_hash,
session->original_handshake_hash,
session->original_handshake_hash_len);
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 08c5db0..bf40284 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -705,28 +705,19 @@
return 0;
}
- /* TODO(davidben): SNI should be resolved before resumption. We have the
- * early callback as a replacement, but we should fix the current callback
- * and avoid the need for |SSL_CTX_set_session_id_context|. */
- if (ssl->session == NULL) {
- assert(ssl->s3->new_session->tlsext_hostname == NULL);
-
- /* Copy the hostname as a string. */
- if (!CBS_strdup(&host_name, &ssl->s3->new_session->tlsext_hostname)) {
- *out_alert = SSL_AD_INTERNAL_ERROR;
- return 0;
- }
-
- ssl->s3->hs->should_ack_sni = 1;
+ /* Copy the hostname as a string. */
+ if (!CBS_strdup(&host_name, &ssl->s3->hs->hostname)) {
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return 0;
}
+ ssl->s3->hs->should_ack_sni = 1;
return 1;
}
static int ext_sni_add_serverhello(SSL *ssl, CBB *out) {
- if (ssl->session != NULL ||
- !ssl->s3->hs->should_ack_sni ||
- ssl->s3->new_session->tlsext_hostname == NULL) {
+ if (ssl->s3->session_reused ||
+ !ssl->s3->hs->should_ack_sni) {
return 1;
}
@@ -2063,9 +2054,13 @@
return CBB_flush(out);
}
-int ssl_ext_psk_key_exchange_modes_parse_clienthello(SSL *ssl,
- uint8_t *out_alert,
- CBS *contents) {
+static int ext_psk_key_exchange_modes_parse_clienthello(SSL *ssl,
+ uint8_t *out_alert,
+ CBS *contents) {
+ if (contents == NULL) {
+ return 1;
+ }
+
CBS ke_modes;
if (!CBS_get_u8_length_prefixed(contents, &ke_modes) ||
CBS_len(&ke_modes) == 0 ||
@@ -2561,7 +2556,7 @@
NULL,
ext_psk_key_exchange_modes_add_clienthello,
forbid_parse_serverhello,
- ignore_parse_clienthello,
+ ext_psk_key_exchange_modes_parse_clienthello,
dont_add_serverhello,
},
{
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index ad71167..f55bf37 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -4852,6 +4852,8 @@
config: Config{
MaxVersion: ver.version,
NextProtos: []string{"foo", "bar", "baz"},
+ // Prior to TLS 1.3, exercise the asynchronous session callback.
+ SessionTicketsDisabled: ver.version < VersionTLS13,
},
flags: []string{
"-expect-advertised-alpn", "\x03foo\x03bar\x03baz",
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 979d8cc..ca4223e 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -109,36 +109,39 @@
}
memcpy(ssl->s3->client_random, client_hello.random, client_hello.random_len);
- uint8_t alert = SSL_AD_DECODE_ERROR;
- CBS psk_key_exchange_modes;
- if (ssl_early_callback_get_extension(&client_hello, &psk_key_exchange_modes,
- TLSEXT_TYPE_psk_key_exchange_modes) &&
- !ssl_ext_psk_key_exchange_modes_parse_clienthello(
- ssl, &alert, &psk_key_exchange_modes)) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
+ /* TLS 1.3 requires the peer only advertise the null compression. */
+ if (client_hello.compression_methods_len != 1 ||
+ client_hello.compression_methods[0] != 0) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_COMPRESSION_LIST);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
- SSL_SESSION *session = NULL;
- CBS binders;
- if (hs->accept_psk_mode) {
- CBS pre_shared_key;
- if (ssl_early_callback_get_extension(&client_hello, &pre_shared_key,
- TLSEXT_TYPE_pre_shared_key)) {
- /* Verify that the pre_shared_key extension is the last extension in
- * ClientHello. */
- if (CBS_data(&pre_shared_key) + CBS_len(&pre_shared_key) !=
- client_hello.extensions + client_hello.extensions_len) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PRE_SHARED_KEY_MUST_BE_LAST);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- return ssl_hs_error;
- }
+ /* TLS extensions. */
+ if (!ssl_parse_clienthello_tlsext(ssl, &client_hello)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
+ return ssl_hs_error;
+ }
- if (!ssl_ext_pre_shared_key_parse_clienthello(ssl, &session, &binders,
- &alert, &pre_shared_key)) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
- return ssl_hs_error;
- }
+ uint8_t alert = SSL_AD_DECODE_ERROR;
+ SSL_SESSION *session = NULL;
+ CBS pre_shared_key, binders;
+ if (hs->accept_psk_mode &&
+ ssl_early_callback_get_extension(&client_hello, &pre_shared_key,
+ TLSEXT_TYPE_pre_shared_key)) {
+ /* Verify that the pre_shared_key extension is the last extension in
+ * ClientHello. */
+ if (CBS_data(&pre_shared_key) + CBS_len(&pre_shared_key) !=
+ client_hello.extensions + client_hello.extensions_len) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_PRE_SHARED_KEY_MUST_BE_LAST);
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
+ return ssl_hs_error;
+ }
+
+ if (!ssl_ext_pre_shared_key_parse_clienthello(ssl, &session, &binders,
+ &alert, &pre_shared_key)) {
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
+ return ssl_hs_error;
}
}
@@ -179,20 +182,6 @@
return ssl_hs_error;
}
- /* TLS 1.3 requires the peer only advertise the null compression. */
- if (client_hello.compression_methods_len != 1 ||
- client_hello.compression_methods[0] != 0) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_COMPRESSION_LIST);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
- return ssl_hs_error;
- }
-
- /* TLS extensions. */
- if (!ssl_parse_clienthello_tlsext(ssl, &client_hello)) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
- return ssl_hs_error;
- }
-
hs->state = state_select_parameters;
return ssl_hs_ok;
}
@@ -282,6 +271,15 @@
}
ssl->s3->new_session->cipher = cipher;
+
+ /* On new sessions, stash the SNI value in the session. */
+ if (ssl->s3->hs->hostname != NULL) {
+ ssl->s3->new_session->tlsext_hostname = BUF_strdup(ssl->s3->hs->hostname);
+ if (ssl->s3->new_session->tlsext_hostname == NULL) {
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
+ return ssl_hs_error;
+ }
+ }
}
ssl->s3->tmp.new_cipher = ssl->s3->new_session->cipher;
