From 10699cc96c2d1cf6e4d70731bb2167851c344c9a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 15:41:02 +0100 Subject: [PATCH 1/5] Simplify retaining of messages for future processing There are situations in which it is not clear what message to expect next. For example, the message following the ServerHello might be either a Certificate, a ServerKeyExchange or a CertificateRequest. We deal with this situation in the following way: Initially, the message processing function for one of the allowed message types is called, which fetches and decodes a new message. If that message is not the expected one, the function returns successfully (instead of throwing an error as usual for unexpected messages), and the handshake continues to the processing function for the next possible message. To not have this function fetch a new message, a flag in the SSL context structure is used to indicate that the last message was retained for further processing, and if that's set, the following processing function will not fetch a new record. This commit simplifies the usage of this message-retaining parameter by doing the check within the record-fetching routine instead of the specific message-processing routines. The code gets cleaner this way and allows retaining messages to be used in other situations as well without much effort. This will be used in the next commits. --- include/polarssl/ssl.h | 4 ++- library/ssl_cli.c | 61 +++++++++++++++++++----------------------- library/ssl_tls.c | 11 +++++++- 3 files changed, 40 insertions(+), 36 deletions(-) diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 74b131702..4a01bbf4c 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -846,7 +846,9 @@ struct _ssl_context size_t in_hslen; /*!< current handshake message length */ int nb_zero; /*!< # of 0-length encrypted messages */ - int record_read; /*!< record is already present */ + + int keep_current_message; /*!< drop or reuse current message + on next call to record layer? */ /* * Record layer (outgoing data) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 34ab7e06d..c12543f20 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1943,7 +1943,9 @@ static int ssl_parse_server_key_exchange( ssl_context *ssl ) if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_PSK || ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_RSA_PSK ) { - ssl->record_read = 1; + /* Current message is probably either + * CertificateRequest or ServerHelloDone */ + ssl->keep_current_message = 1; goto exit; } @@ -2260,36 +2262,31 @@ static int ssl_parse_certificate_request( ssl_context *ssl ) * n+4 .. ... Distinguished Name #1 * ... .. ... length of DN 2, etc. */ - if( ssl->record_read == 0 ) + + if( ( ret = ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = ssl_read_record( ssl ) ) != 0 ) - { - SSL_DEBUG_RET( 1, "ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != SSL_MSG_HANDSHAKE ) - { - SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); - return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE ); - } - - ssl->record_read = 1; + SSL_DEBUG_RET( 1, "ssl_read_record", ret ); + return( ret ); } - ssl->client_auth = 0; - ssl->state++; + if( ssl->in_msgtype != SSL_MSG_HANDSHAKE ) + { + SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); + return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE ); + } - if( ssl->in_msg[0] == SSL_HS_CERTIFICATE_REQUEST ) - ssl->client_auth++; + ssl->state++; + ssl->client_auth = ( ssl->in_msg[0] == SSL_HS_CERTIFICATE_REQUEST ); SSL_DEBUG_MSG( 3, ( "got %s certificate request", ssl->client_auth ? "a" : "no" ) ); if( ssl->client_auth == 0 ) + { + /* Current message is probably the ServerHelloDone */ + ssl->keep_current_message = 1; goto exit; - - ssl->record_read = 0; + } // TODO: handshake_failure alert for an anonymous server to request // client authentication @@ -2386,21 +2383,17 @@ static int ssl_parse_server_hello_done( ssl_context *ssl ) SSL_DEBUG_MSG( 2, ( "=> parse server hello done" ) ); - if( ssl->record_read == 0 ) + if( ( ret = ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = ssl_read_record( ssl ) ) != 0 ) - { - SSL_DEBUG_RET( 1, "ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != SSL_MSG_HANDSHAKE ) - { - SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) ); - return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE ); - } + SSL_DEBUG_RET( 1, "ssl_read_record", ret ); + return( ret ); + } + + if( ssl->in_msgtype != SSL_MSG_HANDSHAKE ) + { + SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) ); + return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE ); } - ssl->record_read = 0; if( ssl->in_hslen != 4 || ssl->in_msg[0] != SSL_HS_SERVER_HELLO_DONE ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 577922978..08b0470c8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2169,6 +2169,15 @@ int ssl_read_record( ssl_context *ssl ) SSL_DEBUG_MSG( 2, ( "=> read record" ) ); + if( ssl->keep_current_message == 1 ) + { + SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) ); + SSL_DEBUG_MSG( 2, ( "<= read record" ) ); + ssl->keep_current_message = 0; + + return( 0 ); + } + if( ssl->in_hslen != 0 && ssl->in_hslen < ssl->in_msglen ) { @@ -3750,7 +3759,7 @@ int ssl_session_reset( ssl_context *ssl ) ssl->in_hslen = 0; ssl->nb_zero = 0; - ssl->record_read = 0; + ssl->keep_current_message = 0; ssl->out_msg = ssl->out_ctr + 13; ssl->out_msgtype = 0; From d37839e3fabccb62dbd3e45ba5e6f7fef5d43e5b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 15:56:50 +0100 Subject: [PATCH 2/5] Fix mbedtls_ssl_read Don't fetch a new record in mbedtls_ssl_read_record_layer as long as an application data record is being processed. --- library/ssl_cli.c | 2 + library/ssl_tls.c | 112 +++++++++++++++++++++++++--------------------- 2 files changed, 63 insertions(+), 51 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index c12543f20..5f5beecb5 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1195,6 +1195,8 @@ static int ssl_parse_server_hello( ssl_context *ssl ) } SSL_DEBUG_MSG( 1, ( "non-handshake message during renego" ) ); + + ssl->keep_current_message = 1; return( POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ); } #endif /* POLARSSL_SSL_RENEGOTIATION */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 08b0470c8..8d49be4f6 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2178,47 +2178,67 @@ int ssl_read_record( ssl_context *ssl ) return( 0 ); } - if( ssl->in_hslen != 0 && - ssl->in_hslen < ssl->in_msglen ) + if( ssl->in_hslen != 0 ) { /* * Get next Handshake message in the current record */ - ssl->in_msglen -= ssl->in_hslen; - memmove( ssl->in_msg, ssl->in_msg + ssl->in_hslen, - ssl->in_msglen ); - - ssl->in_hslen = 4; - ssl->in_hslen += ( ssl->in_msg[2] << 8 ) | ssl->in_msg[3]; - - SSL_DEBUG_MSG( 3, ( "handshake message: msglen =" - " %d, type = %d, hslen = %d", - ssl->in_msglen, ssl->in_msg[0], ssl->in_hslen ) ); - - if( ssl->in_msglen < 4 || ssl->in_msg[1] != 0 ) + if( ssl->in_hslen < ssl->in_msglen ) { - SSL_DEBUG_MSG( 1, ( "bad handshake length" ) ); - return( POLARSSL_ERR_SSL_INVALID_RECORD ); + ssl->in_msglen -= ssl->in_hslen; + memmove( ssl->in_msg, ssl->in_msg + ssl->in_hslen, + ssl->in_msglen ); + + ssl->in_hslen = 4; + ssl->in_hslen += ( ssl->in_msg[2] << 8 ) | ssl->in_msg[3]; + + SSL_DEBUG_MSG( 3, ( "handshake message: msglen =" + " %d, type = %d, hslen = %d", + ssl->in_msglen, ssl->in_msg[0], ssl->in_hslen ) ); + + if( ssl->in_msglen < 4 || ssl->in_msg[1] != 0 ) + { + SSL_DEBUG_MSG( 1, ( "bad handshake length" ) ); + return( POLARSSL_ERR_SSL_INVALID_RECORD ); + } + + if( ssl->in_msglen < ssl->in_hslen ) + { + SSL_DEBUG_MSG( 1, ( "bad handshake length" ) ); + return( POLARSSL_ERR_SSL_INVALID_RECORD ); + } + + if( ssl->state != SSL_HANDSHAKE_OVER ) + ssl->handshake->update_checksum( ssl, ssl->in_msg, ssl->in_hslen ); + + return( 0 ); } - if( ssl->in_msglen < ssl->in_hslen ) - { - SSL_DEBUG_MSG( 1, ( "bad handshake length" ) ); - return( POLARSSL_ERR_SSL_INVALID_RECORD ); - } + ssl->in_msglen = 0; + ssl->in_hslen = 0; + } + else if( ssl->in_offt != NULL ) + { + return( 0 ); + } + else + { + ssl->in_msglen = 0; + } - if( ssl->state != SSL_HANDSHAKE_OVER ) - ssl->handshake->update_checksum( ssl, ssl->in_msg, ssl->in_hslen ); + /* + * Fetch and decode new record if current one is fully consumed. + */ + if( ssl->in_msglen > 0 ) + { + /* There's something left to be processed in the current record. */ return( 0 ); } - ssl->in_hslen = 0; + /* Need to fetch a new record */ - /* - * Read the record header and validate it - */ read_record_header: if( ( ret = ssl_fetch_input( ssl, 5 ) ) != 0 ) { @@ -4651,13 +4671,15 @@ static int ssl_check_ctr_renegotiate( ssl_context *ssl ) */ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) { - int ret, record_read = 0; + int ret; size_t n; SSL_DEBUG_MSG( 2, ( "=> read" ) ); #if defined(POLARSSL_SSL_RENEGOTIATION) - if( ( ret = ssl_check_ctr_renegotiate( ssl ) ) != 0 ) + ret = ssl_check_ctr_renegotiate( ssl ); + if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) { SSL_DEBUG_RET( 1, "ssl_check_ctr_renegotiate", ret ); return( ret ); @@ -4667,11 +4689,8 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) if( ssl->state != SSL_HANDSHAKE_OVER ) { ret = ssl_handshake( ssl ); - if( ret == POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ) - { - record_read = 1; - } - else if( ret != 0 ) + if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) { SSL_DEBUG_RET( 1, "ssl_handshake", ret ); return( ret ); @@ -4680,16 +4699,13 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) if( ssl->in_offt == NULL ) { - if( ! record_read ) + if( ( ret = ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = ssl_read_record( ssl ) ) != 0 ) - { - if( ret == POLARSSL_ERR_SSL_CONN_EOF ) - return( 0 ); + if( ret == POLARSSL_ERR_SSL_CONN_EOF ) + return( 0 ); - SSL_DEBUG_RET( 1, "ssl_read_record", ret ); - return( ret ); - } + SSL_DEBUG_RET( 1, "ssl_read_record", ret ); + return( ret ); } if( ssl->in_msglen == 0 && @@ -4763,21 +4779,15 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) else { ret = ssl_start_renegotiation( ssl ); - if( ret == POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ) - { - record_read = 1; - } - else if( ret != 0 ) + if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) { SSL_DEBUG_RET( 1, "ssl_start_renegotiation", ret ); return( ret ); } } - /* If a non-handshake record was read during renego, fallthrough, - * else tell the user they should call ssl_read() again */ - if( ! record_read ) - return( POLARSSL_ERR_NET_WANT_READ ); + return( POLARSSL_ERR_NET_WANT_READ ); } else if( ssl->renegotiation == SSL_RENEGOTIATION_PENDING ) { From 1bf86b7e32b12c961c8078ed2b3c63c7b5e1927a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 15:58:02 +0100 Subject: [PATCH 3/5] Add hard assertion to ssl_read_record This commit adds a hard assertion to mbedtls_ssl_read_record triggering if both ssl->in_hslen and ssl->in_offt are not 0. This should never happen, and if it does, there's no sensible way of telling whether the previous message was a handshake or an application data message. --- library/ssl_tls.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8d49be4f6..f81a5e2d0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2180,6 +2180,12 @@ int ssl_read_record( ssl_context *ssl ) if( ssl->in_hslen != 0 ) { + if( ssl->in_offt != NULL ) + { + SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); + } + /* * Get next Handshake message in the current record */ From 0401a3d888e49a9cdc8255feabd1f92993041eb2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 9 Jun 2017 10:52:45 +0100 Subject: [PATCH 4/5] Ensure application data records are not kept when fully processed This commit fixes the following case: If a client is both expecting a SERVER_HELLO and has an application data record that's partially processed in flight (that's the situation the client gets into after receiving a ServerHelloRequest followed by ApplicationData), a subsequent call to ssl_read will set keep_current_message = 1 when seeing the unexpected application data, but not reset it to 0 after the application data has been processed. This commit fixes this. --- library/ssl_tls.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index f81a5e2d0..bae8433fe 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4832,8 +4832,11 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) ssl->in_msglen -= n; if( ssl->in_msglen == 0 ) + { /* all bytes consumed */ ssl->in_offt = NULL; + ssl->keep_current_message = 0; + } else /* more data available */ ssl->in_offt += n; From b9c09af596a6bcae60eb4c43284f06c8420530c5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 9 Jun 2017 11:31:43 +0100 Subject: [PATCH 5/5] Add ChangeLog entry --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 3220df9a2..2579c3c3c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 1.3.x branch released xxxx-xx-xx Security + * Fixed unlimited overread of heap-based buffer in ssl_read(). + The issue could only happen client-side with renegotiation enabled. + Could result in DoS (application crash) or information leak + (if the application layer sent data read from ssl_read() + back to the server or to a third party). Can be triggered remotely. * Add exponent blinding to RSA private operations as a countermeasure against side-channel attacks like the cache attack described in https://arxiv.org/abs/1702.08719v2.