Add s->s3->initial_handshake_complete.
There's multiple different versions of this check, between
s->s3->have_version (only works at some points), s->new_session (really
weird and not actually right), s->renegotiate (fails on the server
because it's always 2 after ClientHello), and s->s3->tmp.finish_md_len
(super confusing). Add an explicit bit with clear meaning. We'll prune
some of the others later; notably s->renegotiate can go away when
initiating renegotiation is removed.
This also tidies up the extensions to be consistent about whether
they're allowed during renego:
- ALPN failed to condition when accepting from the server, so even
if the client didn't advertise, the server could.
- SCTs now *are* allowed during renego. I think forbidding it was a
stray copy-paste. It wasn't consistently enforced in both ClientHello
and ServerHello, so the server could still supply it. Moreover, SCTs
are part of the certificate, so we should accept it wherever we accept
certificates, otherwise that session's state becomes incomplete. This
matches OCSP stapling. (NB: Chrome will never insert a session created
on renego into the session cache and won't accept a certificate
change, so this is moot anyway.)
Change-Id: Ic9bd1ebe2a2dbe75930ed0213bf3c8ed8170e251
Reviewed-on: https://boringssl-review.googlesource.com/4730
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h
index 96f00cf..5c5fa61 100644
--- a/include/openssl/ssl3.h
+++ b/include/openssl/ssl3.h
@@ -366,6 +366,10 @@
* the version has not been negotiated yet. */
char have_version;
+ /* initial_handshake_complete is true if the initial handshake has
+ * completed. */
+ char initial_handshake_complete;
+
/* sniff_buffer is used by the server in the initial handshake to read a
* V2ClientHello before the record layer is initialized. */
BUF_MEM *sniff_buffer;
diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c
index 1827a67..a5a7b0c 100644
--- a/ssl/d1_clnt.c
+++ b/ssl/d1_clnt.c
@@ -474,6 +474,7 @@
s->init_num = 0;
s->renegotiate = 0;
s->new_session = 0;
+ s->s3->initial_handshake_complete = 1;
ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c
index e314910..d93b26f 100644
--- a/ssl/d1_srvr.c
+++ b/ssl/d1_srvr.c
@@ -475,6 +475,7 @@
/* skipped if we just sent a HelloRequest */
s->renegotiate = 0;
s->new_session = 0;
+ s->s3->initial_handshake_complete = 1;
ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index d01acae..700c938 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -457,7 +457,7 @@
ssl3_can_false_start(s) &&
/* No False Start on renegotiation (would complicate the state
* machine). */
- s->s3->previous_server_finished_len == 0) {
+ !s->s3->initial_handshake_complete) {
s->s3->tmp.next_state = SSL3_ST_FALSE_START;
} else {
/* Allow NewSessionTicket if ticket expected */
@@ -554,6 +554,7 @@
s->renegotiate = 0;
s->new_session = 0;
s->s3->tmp.in_false_start = 0;
+ s->s3->initial_handshake_complete = 1;
ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
@@ -778,6 +779,7 @@
goto f_err;
}
+ assert(s->s3->have_version == s->s3->initial_handshake_complete);
if (!s->s3->have_version) {
if (!ssl3_is_version_enabled(s, server_version)) {
OPENSSL_PUT_ERROR(SSL, ssl3_get_server_hello, SSL_R_UNSUPPORTED_PROTOCOL);
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 3cc3032..3e1e7c2 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -644,6 +644,7 @@
/* skipped if we just sent a HelloRequest */
s->renegotiate = 0;
s->new_session = 0;
+ s->s3->initial_handshake_complete = 1;
ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
@@ -1011,6 +1012,11 @@
}
}
+ /* Note: This codepath may run twice if |ssl_get_prev_session| completes
+ * asynchronously.
+ *
+ * TODO(davidben): Clean up the order of events around ClientHello
+ * processing. */
if (!s->s3->have_version) {
/* Select version to use */
uint16_t version = ssl3_get_mutual_version(s, client_version);
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 433a647..4830fd5 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -871,7 +871,7 @@
}
/* Add RI if renegotiating */
- if (s->renegotiate) {
+ if (s->s3->initial_handshake_complete) {
int el;
if (!ssl_add_clienthello_renegotiate_ext(s, 0, &el, 0)) {
@@ -954,7 +954,7 @@
s2n(0, ret);
}
- if (s->ctx->next_proto_select_cb && !s->s3->tmp.finish_md_len &&
+ if (s->ctx->next_proto_select_cb && !s->s3->initial_handshake_complete &&
!SSL_IS_DTLS(s)) {
/* The client advertises an emtpy extension to indicate its support for
* Next Protocol Negotiation */
@@ -965,7 +965,7 @@
s2n(0, ret);
}
- if (s->signed_cert_timestamps_enabled && !s->s3->tmp.finish_md_len) {
+ if (s->signed_cert_timestamps_enabled) {
/* The client advertises an empty extension to indicate its support for
* certificate timestamps. */
if (limit - ret - 4 < 0) {
@@ -975,7 +975,7 @@
s2n(0, ret);
}
- if (s->alpn_client_proto_list && !s->s3->tmp.finish_md_len) {
+ if (s->alpn_client_proto_list && !s->s3->initial_handshake_complete) {
if ((size_t)(limit - ret) < 6 + s->alpn_client_proto_list_len) {
return NULL;
}
@@ -1584,29 +1584,16 @@
return 0;
}
} else if (type == TLSEXT_TYPE_next_proto_neg &&
- s->s3->tmp.finish_md_len == 0 && s->s3->alpn_selected == NULL &&
- !SSL_IS_DTLS(s)) {
+ !s->s3->initial_handshake_complete &&
+ s->s3->alpn_selected == NULL && !SSL_IS_DTLS(s)) {
/* The extension must be empty. */
if (CBS_len(&extension) != 0) {
*out_alert = SSL_AD_DECODE_ERROR;
return 0;
}
-
- /* We shouldn't accept this extension on a renegotiation.
- *
- * s->new_session will be set on renegotiation, but we probably shouldn't
- * rely that it couldn't be set on the initial renegotation too in
- * certain cases (when there's some other reason to disallow resuming an
- * earlier session -- the current code won't be doing anything like that,
- * but this might change).
-
- * A valid sign that there's been a previous handshake in this connection
- * is if s->s3->tmp.finish_md_len > 0. (We are talking about a check
- * that will happen in the Hello protocol round, well before a new
- * Finished message could have been computed.) */
s->s3->next_proto_neg_seen = 1;
} else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation &&
- s->ctx->alpn_select_cb && s->s3->tmp.finish_md_len == 0) {
+ s->ctx->alpn_select_cb && !s->s3->initial_handshake_complete) {
if (!tls1_alpn_handle_client_hello(s, &extension, out_alert)) {
return 0;
}
@@ -1788,8 +1775,7 @@
/* Set a flag to expect a CertificateStatus message */
s->s3->tmp.certificate_status_expected = 1;
} else if (type == TLSEXT_TYPE_next_proto_neg &&
- s->s3->tmp.finish_md_len == 0 &&
- !SSL_IS_DTLS(s)) {
+ !s->s3->initial_handshake_complete && !SSL_IS_DTLS(s)) {
uint8_t *selected;
uint8_t selected_len;
@@ -1821,7 +1807,8 @@
s->next_proto_negotiated_len = selected_len;
s->s3->next_proto_neg_seen = 1;
- } else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation) {
+ } else if (type == TLSEXT_TYPE_application_layer_protocol_negotiation &&
+ !s->s3->initial_handshake_complete) {
CBS protocol_name_list, protocol_name;
/* We must have requested it. */
