Commit 524420d8 authored by Matt Caswell's avatar Matt Caswell
Browse files

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

include/openssl/ssl.h

+1 −0
Original line number Diff line number Diff line
@@ -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

ssl/ssl_err.c

+1 −0
Original line number Diff line number Diff line
@@ -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"},

ssl/statem/statem_clnt.c

+10 −0
Original line number Diff line number Diff line
@@ -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)) {

ssl/statem/statem_lib.c

+20 −0
Original line number Diff line number Diff line
@@ -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;