Move state machine knowledge out of the record layer
The record layer was making decisions that should really be left to the
state machine around unexpected handshake messages that are received after
the initial handshake (i.e. renegotiation related messages). This commit
removes that code from the record layer and updates the state machine
accordingly. This simplifies the state machine and paves the way for
handling other messages post-handshake such as the NewSessionTicket in
TLSv1.3.
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2259)
diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index fe00749..1e2cc3f 100644
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -318,7 +318,7 @@
int tls_construct_ctos_npn(SSL *s, WPACKET *pkt, X509 *x, size_t chainidx,
int *al)
{
- if (s->ctx->ext.npn_select_cb == NULL || s->s3->tmp.finish_md_len != 0)
+ if (s->ctx->ext.npn_select_cb == NULL || !SSL_IS_FIRST_HANDSHAKE(s))
return 1;
/*
@@ -340,11 +340,7 @@
{
s->s3->alpn_sent = 0;
- /*
- * finish_md_len is non-zero during a renegotiation, so
- * this avoids sending ALPN during the renegotiation
- */
- if (s->ext.alpn == NULL || s->s3->tmp.finish_md_len != 0)
+ if (s->ext.alpn == NULL || !SSL_IS_FIRST_HANDSHAKE(s))
return 1;
if (!WPACKET_put_bytes_u16(pkt,
@@ -866,7 +862,7 @@
PACKET tmppkt;
/* Check if we are in a renegotiation. If so ignore this extension */
- if (s->s3->tmp.finish_md_len != 0)
+ if (!SSL_IS_FIRST_HANDSHAKE(s))
return 1;
/* We must have requested it. */
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index d58eedd..357b3b7 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -322,21 +322,8 @@
/*
* 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 renegotiation 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.)
*/
- if (s->s3->tmp.finish_md_len == 0)
+ if (SSL_IS_FIRST_HANDSHAKE(s))
s->s3->npn_seen = 1;
return 1;
@@ -352,7 +339,7 @@
{
PACKET protocol_list, save_protocol_list, protocol;
- if (s->s3->tmp.finish_md_len != 0)
+ if (!SSL_IS_FIRST_HANDSHAKE(s))
return 1;
if (!PACKET_as_length_prefixed_2(pkt, &protocol_list)
diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c
index ac78d2d..bd7d89a 100644
--- a/ssl/statem/statem.c
+++ b/ssl/statem/statem.c
@@ -105,7 +105,6 @@
*/
void ossl_statem_set_renegotiate(SSL *s)
{
- s->statem.state = MSG_FLOW_RENEGOTIATE;
s->statem.in_init = 1;
s->statem.request_state = TLS_ST_SW_HELLO_REQ;
}
@@ -190,10 +189,10 @@
/*
* The main message flow state machine. We start in the MSG_FLOW_UNINITED or
- * MSG_FLOW_RENEGOTIATE state and finish in MSG_FLOW_FINISHED. Valid states and
+ * MSG_FLOW_FINISHED state and finish in MSG_FLOW_FINISHED. Valid states and
* transitions are as follows:
*
- * MSG_FLOW_UNINITED MSG_FLOW_RENEGOTIATE
+ * MSG_FLOW_UNINITED MSG_FLOW_FINISHED
* | |
* +-----------------------+
* v
@@ -253,15 +252,7 @@
#endif
/* Initialise state machine */
-
- if (st->state == MSG_FLOW_RENEGOTIATE) {
- s->renegotiate = 1;
- if (!server)
- s->ctx->stats.sess_connect_renegotiate++;
- }
-
if (st->state == MSG_FLOW_UNINITED
- || st->state == MSG_FLOW_RENEGOTIATE
|| st->state == MSG_FLOW_FINISHED) {
if (st->state == MSG_FLOW_UNINITED) {
st->hand_state = TLS_ST_BEFORE;
@@ -322,53 +313,14 @@
goto end;
}
- if (!SSL_IS_TLS13(s)) {
- if (!server || st->state != MSG_FLOW_RENEGOTIATE) {
- if (!ssl3_init_finished_mac(s)) {
- ossl_statem_set_error(s);
- goto end;
- }
+ if (SSL_IS_FIRST_HANDSHAKE(s) || s->renegotiate) {
+ if (!tls_setup_handshake(s)) {
+ ossl_statem_set_error(s);
+ goto end;
}
- if (server) {
- if (st->state != MSG_FLOW_RENEGOTIATE) {
- s->ctx->stats.sess_accept++;
- } else if (!s->s3->send_connection_binding &&
- !(s->options &
- SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) {
- /*
- * Server attempting to renegotiate with client that doesn't
- * support secure renegotiation.
- */
- SSLerr(SSL_F_STATE_MACHINE,
- SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
- ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
- ossl_statem_set_error(s);
- goto end;
- } else {
- /*
- * st->state == MSG_FLOW_RENEGOTIATE, we will just send a
- * HelloRequest
- */
- s->ctx->stats.sess_accept_renegotiate++;
-
- s->s3->tmp.cert_request = 0;
- }
- } else {
- s->ctx->stats.sess_connect++;
-
- /* mark client_random uninitialized */
- memset(s->s3->client_random, 0, sizeof(s->s3->client_random));
- s->hit = 0;
-
- s->s3->tmp.cert_req = 0;
-
- if (SSL_IS_DTLS(s)) {
- st->use_timer = 1;
- }
- }
-
- st->read_state_first_init = 1;
+ if (SSL_IS_FIRST_HANDSHAKE(s))
+ st->read_state_first_init = 1;
}
st->state = MSG_FLOW_WRITING;
@@ -826,7 +778,7 @@
/*
* Called by the record layer to determine whether application data is
- * allowed to be sent in the current handshake state or not.
+ * allowed to be received in the current handshake state or not.
*
* Return values are:
* 1: Yes (application data allowed)
@@ -836,7 +788,7 @@
{
OSSL_STATEM *st = &s->statem;
- if (st->state == MSG_FLOW_UNINITED || st->state == MSG_FLOW_RENEGOTIATE)
+ if (st->state == MSG_FLOW_UNINITED)
return 0;
if (!s->s3->in_read_app_data || (s->s3->total_renegotiations == 0))
diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h
index 6765c30..021d2d0 100644
--- a/ssl/statem/statem.h
+++ b/ssl/statem/statem.h
@@ -46,8 +46,6 @@
MSG_FLOW_UNINITED,
/* A permanent error with this connection */
MSG_FLOW_ERROR,
- /* We are about to renegotiate */
- MSG_FLOW_RENEGOTIATE,
/* We are reading messages */
MSG_FLOW_READING,
/* We are writing messages */
@@ -92,6 +90,11 @@
int read_state_first_init;
/* true when we are actually in SSL_accept() or SSL_connect() */
int in_handshake;
+ /*
+ * True when are processing a "real" handshake that needs cleaning up (not
+ * just a HelloRequest or similar).
+ */
+ int cleanuphand;
/* Should we skip the CertificateVerify message? */
unsigned int no_cert_verify;
int use_timer;
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 90e4df6..8a308f8 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -351,6 +351,13 @@
return 1;
}
break;
+
+ case TLS_ST_OK:
+ if (mt == SSL3_MT_HELLO_REQUEST) {
+ st->hand_state = TLS_ST_CR_HELLO_REQ;
+ return 1;
+ }
+ break;
}
err:
@@ -428,6 +435,13 @@
return WRITE_TRAN_ERROR;
case TLS_ST_OK:
+ if (!s->renegotiate) {
+ /*
+ * We haven't requested a renegotiation ourselves so we must have
+ * received a message from the server. Better read it.
+ */
+ return WRITE_TRAN_FINISHED;
+ }
/* Renegotiation - fall through */
case TLS_ST_BEFORE:
st->hand_state = TLS_ST_CW_CLNT_HELLO;
@@ -515,6 +529,23 @@
ossl_statem_set_in_init(s, 0);
return WRITE_TRAN_CONTINUE;
}
+
+ case TLS_ST_CR_HELLO_REQ:
+ /*
+ * If we can renegotiate now then do so, otherwise wait for a more
+ * convenient time.
+ */
+ if (ssl3_renegotiate_check(s, 1)) {
+ if (!tls_setup_handshake(s)) {
+ ossl_statem_set_error(s);
+ return WRITE_TRAN_ERROR;
+ }
+ st->hand_state = TLS_ST_CW_CLNT_HELLO;
+ return WRITE_TRAN_CONTINUE;
+ }
+ st->hand_state = TLS_ST_OK;
+ ossl_statem_set_in_init(s, 0);
+ return WRITE_TRAN_CONTINUE;
}
}
@@ -819,6 +850,9 @@
case TLS_ST_CR_FINISHED:
return tls_process_finished(s, pkt);
+ case TLS_ST_CR_HELLO_REQ:
+ return tls_process_hello_req(s, pkt);
+
case TLS_ST_CR_ENCRYPTED_EXTENSIONS:
return tls_process_encrypted_extensions(s, pkt);
}
@@ -893,6 +927,9 @@
}
/* else use the pre-loaded session */
+ /* This is a real handshake so make sure we clean it up at the end */
+ s->statem.cleanuphand = 1;
+
p = s->s3->client_random;
/*
@@ -3112,6 +3149,30 @@
}
#endif
+MSG_PROCESS_RETURN tls_process_hello_req(SSL *s, PACKET *pkt)
+{
+ if (PACKET_remaining(pkt) > 0) {
+ /* should contain no data */
+ SSLerr(SSL_F_TLS_PROCESS_HELLO_REQ, SSL_R_LENGTH_MISMATCH);
+ ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+ ossl_statem_set_error(s);
+ return MSG_PROCESS_ERROR;
+ }
+
+ /*
+ * This is a historical discrepancy maintained for compatibility
+ * reasons. If a TLS client receives a HelloRequest it will attempt
+ * an abbreviated handshake. However if a DTLS client receives a
+ * HelloRequest it will do a full handshake.
+ */
+ if (SSL_IS_DTLS(s))
+ SSL_renegotiate(s);
+ else
+ SSL_renegotiate_abbreviated(s);
+
+ return MSG_PROCESS_FINISHED_READING;
+}
+
static MSG_PROCESS_RETURN tls_process_encrypted_extensions(SSL *s, PACKET *pkt)
{
int al = SSL_AD_INTERNAL_ERROR;
diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c
index 1c1758b..1bc82d1 100644
--- a/ssl/statem/statem_dtls.c
+++ b/ssl/statem/statem_dtls.c
@@ -788,8 +788,9 @@
return 0;
}
- if (!s->server && s->d1->r_msg_hdr.frag_off == 0 &&
- wire[0] == SSL3_MT_HELLO_REQUEST) {
+ if (!s->server && s->d1->r_msg_hdr.frag_off == 0
+ && s->statem.hand_state != TLS_ST_OK
+ && wire[0] == SSL3_MT_HELLO_REQUEST) {
/*
* The server may always send 'Hello Request' messages -- we are
* doing a handshake anyway now, so ignore them if their format is
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 905a2cc..c81c012 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -72,6 +72,49 @@
return 1;
}
+int tls_setup_handshake(SSL *s) {
+ if (!ssl3_init_finished_mac(s))
+ return 0;
+
+ if (s->server) {
+ if (SSL_IS_FIRST_HANDSHAKE(s)) {
+ s->ctx->stats.sess_accept++;
+ } else if (!s->s3->send_connection_binding &&
+ !(s->options &
+ SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) {
+ /*
+ * Server attempting to renegotiate with client that doesn't
+ * support secure renegotiation.
+ */
+ SSLerr(SSL_F_TLS_SETUP_HANDSHAKE,
+ SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
+ ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
+ return 0;
+ } else {
+ s->ctx->stats.sess_accept_renegotiate++;
+
+ s->s3->tmp.cert_request = 0;
+ }
+ } else {
+ if (SSL_IS_FIRST_HANDSHAKE(s))
+ s->ctx->stats.sess_connect++;
+ else
+ s->ctx->stats.sess_connect_renegotiate++;
+
+ /* mark client_random uninitialized */
+ memset(s->s3->client_random, 0, sizeof(s->s3->client_random));
+ s->hit = 0;
+
+ s->s3->tmp.cert_req = 0;
+
+ if (SSL_IS_DTLS(s)) {
+ s->statem.use_timer = 1;
+ }
+ }
+
+ return 1;
+}
+
/*
* Size of the to-be-signed TLS13 data, without the hash size itself:
* 64 bytes of value 32, 33 context bytes, 1 byte separator
@@ -807,10 +850,11 @@
s->init_num = 0;
- if (!s->server || s->renegotiate == 2) {
+ if (s->statem.cleanuphand) {
/* skipped if we just sent a HelloRequest */
s->renegotiate = 0;
s->new_session = 0;
+ s->statem.cleanuphand = 0;
if (s->server) {
ssl_update_cache(s, SSL_SESS_CACHE_SERVER);
@@ -891,7 +935,8 @@
skip_message = 0;
if (!s->server)
- if (p[0] == SSL3_MT_HELLO_REQUEST)
+ if (s->statem.hand_state != TLS_ST_OK
+ && p[0] == SSL3_MT_HELLO_REQUEST)
/*
* The server may always send 'Hello Request' messages --
* we are doing a handshake anyway now, so ignore them if
diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index b52de70..5e9f72d 100644
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -132,6 +132,7 @@
#ifndef OPENSSL_NO_NEXTPROTONEG
__owur int tls_construct_next_proto(SSL *s, WPACKET *pkt);
#endif
+__owur MSG_PROCESS_RETURN tls_process_hello_req(SSL *s, PACKET *pkt);
__owur MSG_PROCESS_RETURN dtls_process_hello_verify(SSL *s, PACKET *pkt);
/* some server-only functions */
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 0a72287..cb080aa 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -473,6 +473,11 @@
st->request_state = TLS_ST_BEFORE;
return WRITE_TRAN_CONTINUE;
}
+ /* Must be an incoming ClientHello */
+ if (!tls_setup_handshake(s)) {
+ ossl_statem_set_error(s);
+ return WRITE_TRAN_ERROR;
+ }
/* Fall through */
case TLS_ST_BEFORE:
@@ -1152,6 +1157,15 @@
static const unsigned char null_compression = 0;
CLIENTHELLO_MSG clienthello;
+ /* Check if this is actually an unexpected renegotiation ClientHello */
+ if (s->renegotiate == 0 && !SSL_IS_FIRST_HANDSHAKE(s)) {
+ s->renegotiate = 1;
+ s->new_session = 1;
+ }
+
+ /* This is a real handshake so make sure we clean it up at the end */
+ s->statem.cleanuphand = 1;
+
/*
* First, parse the raw ClientHello data into the CLIENTHELLO_MSG structure.
*/
@@ -1850,7 +1864,6 @@
}
}
#endif
- s->renegotiate = 2;
return WORK_FINISHED_STOP;
f_err:
