diff --git a/ChangeLog b/ChangeLog index cea282ae8..9778fbe63 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 1.3.20 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. 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..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 */ @@ -1943,7 +1945,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 +2264,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 +2385,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..bae8433fe 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2169,47 +2169,82 @@ int ssl_read_record( ssl_context *ssl ) SSL_DEBUG_MSG( 2, ( "=> read record" ) ); - if( ssl->in_hslen != 0 && - ssl->in_hslen < ssl->in_msglen ) + if( ssl->keep_current_message == 1 ) { - /* - * 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 ) - { - 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 ); + SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) ); + SSL_DEBUG_MSG( 2, ( "<= read record" ) ); + ssl->keep_current_message = 0; return( 0 ); } - ssl->in_hslen = 0; + 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 + */ + + if( ssl->in_hslen < ssl->in_msglen ) + { + 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 ); + } + + ssl->in_msglen = 0; + ssl->in_hslen = 0; + } + else if( ssl->in_offt != NULL ) + { + return( 0 ); + } + else + { + ssl->in_msglen = 0; + } /* - * Read the record header and validate it + * 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 ); + } + + /* Need to fetch a new record */ + read_record_header: if( ( ret = ssl_fetch_input( ssl, 5 ) ) != 0 ) { @@ -3750,7 +3785,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; @@ -4642,13 +4677,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 ); @@ -4658,11 +4695,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 ); @@ -4671,16 +4705,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 && @@ -4754,21 +4785,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 ) { @@ -4807,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;