From 524420d8459fa07a8e4900bc9dfb558b215edbbd Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 7 Mar 2017 10:21:58 +0000 Subject: Check TLSv1.3 ServerHello, Finished and KeyUpdates are on record boundary In TLSv1.3 the above messages signal a key change. The spec requires that the end of these messages must align with a record boundary. We can detect this by checking for decrypted but as yet unread record data sitting in OpenSSL buffers at the point where we process the messages. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/2875) --- include/openssl/ssl.h | 1 + ssl/ssl_err.c | 1 + ssl/statem/statem_clnt.c | 10 ++++++++++ ssl/statem/statem_lib.c | 20 ++++++++++++++++++++ 4 files changed, 32 insertions(+) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index c569407701..9fbf3d1b11 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2575,6 +2575,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_MISSING_SRP_PARAM 358 # define SSL_R_MISSING_TMP_DH_KEY 171 # define SSL_R_MISSING_TMP_ECDH_KEY 311 +# define SSL_R_NOT_ON_RECORD_BOUNDARY 182 # define SSL_R_NO_CERTIFICATES_RETURNED 176 # define SSL_R_NO_CERTIFICATE_ASSIGNED 177 # define SSL_R_NO_CERTIFICATE_SET 179 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index ee1ca6293c..23987e64a4 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -625,6 +625,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = { {ERR_REASON(SSL_R_MISSING_SRP_PARAM), "can't find SRP server param"}, {ERR_REASON(SSL_R_MISSING_TMP_DH_KEY), "missing tmp dh key"}, {ERR_REASON(SSL_R_MISSING_TMP_ECDH_KEY), "missing tmp ecdh key"}, + {ERR_REASON(SSL_R_NOT_ON_RECORD_BOUNDARY), "not on record boundary"}, {ERR_REASON(SSL_R_NO_CERTIFICATES_RETURNED), "no certificates returned"}, {ERR_REASON(SSL_R_NO_CERTIFICATE_ASSIGNED), "no certificate assigned"}, {ERR_REASON(SSL_R_NO_CERTIFICATE_SET), "no certificate set"}, diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index b11cd19ffa..9f4a719fa1 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1237,6 +1237,16 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) goto f_err; } + /* + * In TLSv1.3 a ServerHello message signals a key change so the end of the + * message must be on a record boundary. + */ + if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) { + al = SSL_AD_UNEXPECTED_MESSAGE; + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_NOT_ON_RECORD_BOUNDARY); + goto f_err; + } + /* load the server hello data */ /* load the server random */ if (!PACKET_copy_bytes(pkt, s->s3->server_random, SSL3_RANDOM_SIZE)) { diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 32bcad4e2f..36c96e5697 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -539,6 +539,16 @@ MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt) goto err; } + /* + * A KeyUpdate message signals a key change so the end of the message must + * be on a record boundary. + */ + if (RECORD_LAYER_processed_read_pending(&s->rlayer)) { + al = SSL_AD_UNEXPECTED_MESSAGE; + SSLerr(SSL_F_TLS_PROCESS_KEY_UPDATE, SSL_R_NOT_ON_RECORD_BOUNDARY); + goto err; + } + if (!PACKET_get_1(pkt, &updatetype) || PACKET_remaining(pkt) != 0 || (updatetype != SSL_KEY_UPDATE_NOT_REQUESTED @@ -676,6 +686,16 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) if (s->server) s->statem.cleanuphand = 1; + /* + * In TLSv1.3 a Finished message signals a key change so the end of the + * message must be on a record boundary. + */ + if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) { + al = SSL_AD_UNEXPECTED_MESSAGE; + SSLerr(SSL_F_TLS_PROCESS_FINISHED, SSL_R_NOT_ON_RECORD_BOUNDARY); + goto f_err; + } + /* If this occurs, we have missed a message */ if (!SSL_IS_TLS13(s) && !s->s3->change_cipher_spec) { al = SSL_AD_UNEXPECTED_MESSAGE; -- cgit v1.2.3