diff options
author | Matt Caswell <matt@openssl.org> | 2017-03-07 10:21:58 +0000 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2017-03-07 16:41:25 +0000 |
commit | 524420d8459fa07a8e4900bc9dfb558b215edbbd (patch) | |
tree | 6792e3cd7069ee3d658d6e1e3fd3fa16fc080454 /ssl | |
parent | b8c49611bc26c8f9a980b814496a3069cd524b79 (diff) |
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)
Diffstat (limited to 'ssl')
-rw-r--r-- | ssl/ssl_err.c | 1 | ||||
-rw-r--r-- | ssl/statem/statem_clnt.c | 10 | ||||
-rw-r--r-- | ssl/statem/statem_lib.c | 20 |
3 files changed, 31 insertions, 0 deletions
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; |