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 <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2875)

4 files changed, 32 insertions, 0 deletions

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;