diff --git a/ChangeLog b/ChangeLog index 0a47c6f1d..0af0acf3c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.1.8 released xxxx-xx-xx Security + * Fixed unlimited overread of heap-based buffer in mbedtls_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 mbedtls_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/mbedtls/ssl.h b/include/mbedtls/ssl.h index 5775f8a49..906866ded 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -714,7 +714,9 @@ struct mbedtls_ssl_context size_t in_hslen; /*!< current handshake message length, including the handshake header */ 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/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 6b9334f7b..0c93a748e 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -387,6 +387,80 @@ int mbedtls_ssl_send_fatal_handshake_failure( mbedtls_ssl_context *ssl ); void mbedtls_ssl_reset_checksum( mbedtls_ssl_context *ssl ); int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ); +/** + * \brief Update record layer + * + * This function roughly separates the implementation + * of the logic of (D)TLS from the implementation + * of the secure transport. + * + * \param ssl SSL context to use + * + * \return 0 or non-zero error code. + * + * \note A clarification on what is called 'record layer' here + * is in order, as many sensible definitions are possible: + * + * The record layer takes as input an untrusted underlying + * transport (stream or datagram) and transforms it into + * a serially multiplexed, secure transport, which + * conceptually provides the following: + * + * (1) Three datagram based, content-agnostic transports + * for handshake, alert and CCS messages. + * (2) One stream- or datagram-based transport + * for application data. + * (3) Functionality for changing the underlying transform + * securing the contents. + * + * The interface to this functionality is given as follows: + * + * a Updating + * [Currently implemented by mbedtls_ssl_read_record] + * + * Check if and on which of the four 'ports' data is pending: + * Nothing, a controlling datagram of type (1), or application + * data (2). In any case data is present, internal buffers + * provide access to the data for the user to process it. + * Consumption of type (1) datagrams is done automatically + * on the next update, invalidating that the internal buffers + * for previous datagrams, while consumption of application + * data (2) is user-controlled. + * + * b Reading of application data + * [Currently manual adaption of ssl->in_offt pointer] + * + * As mentioned in the last paragraph, consumption of data + * is different from the automatic consumption of control + * datagrams (1) because application data is treated as a stream. + * + * c Tracking availability of application data + * [Currently manually through decreasing ssl->in_msglen] + * + * For efficiency and to retain datagram semantics for + * application data in case of DTLS, the record layer + * provides functionality for checking how much application + * data is still available in the internal buffer. + * + * d Changing the transformation securing the communication. + * + * Given an opaque implementation of the record layer in the + * above sense, it should be possible to implement the logic + * of (D)TLS on top of it without the need to know anything + * about the record layer's internals. This is done e.g. + * in all the handshake handling functions, and in the + * application data reading function mbedtls_ssl_read. + * + * \note The above tries to give a conceptual picture of the + * record layer, but the current implementation deviates + * from it in some places. For example, our implementation of + * the update functionality through mbedtls_ssl_read_record + * discards datagrams depending on the current state, which + * wouldn't fall under the record layer's responsibility + * following the above definition. + * + */ + int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ); int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ); diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 67cbccc90..31eb203d8 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1304,6 +1304,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) } MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-handshake message during renego" ) ); + + ssl->keep_current_message = 1; return( MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ); } #endif /* MBEDTLS_SSL_RENEGOTIATION */ @@ -2102,11 +2104,14 @@ static int ssl_parse_server_key_exchange( mbedtls_ssl_context *ssl ) if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_PSK || ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_RSA_PSK ) { - ssl->record_read = 1; + /* Current message is probably either + * CertificateRequest or ServerHelloDone */ + ssl->keep_current_message = 1; goto exit; } - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server key exchange message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "server key exchange message must " + "not be skipped" ) ); return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } @@ -2389,36 +2394,30 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) return( 0 ); } - if( ssl->record_read == 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } - - ssl->record_read = 1; + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + return( ret ); } - ssl->client_auth = 0; - ssl->state++; + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); + } - if( ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE_REQUEST ) - ssl->client_auth++; + ssl->state++; + ssl->client_auth = ( ssl->in_msg[0] == MBEDTLS_SSL_HS_CERTIFICATE_REQUEST ); MBEDTLS_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 @@ -2517,21 +2516,17 @@ static int ssl_parse_server_hello_done( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse server hello done" ) ); - if( ssl->record_read == 0 ) + if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) - { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } - - if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) ); - return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); - } + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + return( ret ); + } + + if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } - ssl->record_read = 0; if( ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) || ssl->in_msg[0] != MBEDTLS_SSL_HS_SERVER_HELLO_DONE ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 22c3f9936..bd2c27057 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3716,31 +3716,123 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) MBEDTLS_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 ); - - MBEDTLS_SSL_DEBUG_BUF( 4, "remaining content in record", - ssl->in_msg, ssl->in_msglen ); - - if( ( ret = ssl_prepare_handshake_record( ssl ) ) != 0 ) - return( ret ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read record" ) ); + ssl->keep_current_message = 0; return( 0 ); } - ssl->in_hslen = 0; + /* + * Step A + * + * Consume last content-layer message and potentially + * update in_msglen which keeps track of the contents' + * consumption state. + * + * (1) Handshake messages: + * Remove last handshake message, move content + * and adapt in_msglen. + * + * (2) Alert messages: + * Consume whole record content, in_msglen = 0. + * + * NOTE: This needs to be fixed, since like for + * handshake messages it is allowed to have + * multiple alerts witin a single record. + * Internal reference IOTSSL-1321. + * + * (3) Change cipher spec: + * Consume whole record content, in_msglen = 0. + * + * (4) Application data: + * Don't do anything - the record layer provides + * the application data as a stream transport + * and consumes through mbedtls_ssl_read only. + * + */ + + /* Case (1): Handshake messages */ + + if( ssl->in_hslen != 0 ) + { + if( ssl->in_offt != NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } + + /* + * Get next Handshake message in the current record + */ + + /* Notes: + * (1) in_hslen is *NOT* necessarily the size of the + * current handshake content: If DTLS handshake + * fragmentation is used, that's the fragment + * size instead. Using the total handshake message + * size here is FAULTY and should be changed at + * some point. Internal reference IOTSSL-1414. + * (2) While it doesn't seem to cause problems, one + * has to be very careful not to assume that in_hslen + * is always <= in_msglen in a sensible communication. + * Again, it's wrong for DTLS handshake fragmentation. + * The following check is therefore mandatory, and + * should not be treated as a silently corrected assertion. + */ + 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 ); + + MBEDTLS_SSL_DEBUG_BUF( 4, "remaining content in record", + ssl->in_msg, ssl->in_msglen ); + + if( ( ret = ssl_prepare_handshake_record( ssl ) ) != 0 ) + return( ret ); + + return( 0 ); + } + else + { + ssl->in_msglen = 0; + } + + ssl->in_hslen = 0; + } + /* Case (4): Application data */ + else if( ssl->in_offt != NULL ) + { + return( 0 ); + } + /* Everything else (CCS & Alerts) */ + else + { + ssl->in_msglen = 0; + } /* - * Read the record header and parse it + * Step B + * + * 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: + + /* Current record either fully processed or to be discarded. */ + if( ( ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_hdr_len( ssl ) ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); @@ -3832,6 +3924,12 @@ read_record_header: } #endif + /* As above, invalid records cause + * dismissal of the whole datagram. */ + + ssl->next_record_offset = 0; + ssl->in_left = 0; + MBEDTLS_SSL_DEBUG_MSG( 1, ( "discarding invalid record (mac)" ) ); goto read_record_header; } @@ -5452,7 +5550,7 @@ static int ssl_session_reset_int( mbedtls_ssl_context *ssl, int partial ) ssl->in_hslen = 0; ssl->nb_zero = 0; - ssl->record_read = 0; + ssl->keep_current_message = 0; ssl->out_msg = ssl->out_buf + 13; ssl->out_msgtype = 0; @@ -6439,7 +6537,7 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) */ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) { - int ret, record_read = 0; + int ret; size_t n; if( ssl == NULL || ssl->conf == NULL ) @@ -6462,8 +6560,22 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) } #endif + /* + * Check if renegotiation is necessary and/or handshake is + * in process. If yes, perform/continue, and fall through + * if an unexpected packet is received while the client + * is waiting for the ServerHello. + * + * (There is no equivalent to the last condition on + * the server-side as it is not treated as within + * a handshake while waiting for the ClientHello + * after a renegotiation request.) + */ + #if defined(MBEDTLS_SSL_RENEGOTIATION) - if( ( ret = ssl_check_ctr_renegotiate( ssl ) ) != 0 ) + ret = ssl_check_ctr_renegotiate( ssl ); + if( ret != MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "ssl_check_ctr_renegotiate", ret ); return( ret ); @@ -6473,11 +6585,8 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER ) { ret = mbedtls_ssl_handshake( ssl ); - if( ret == MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ) - { - record_read = 1; - } - else if( ret != 0 ) + if( ret != MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_handshake", ret ); return( ret ); @@ -6493,16 +6602,13 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) ssl_set_timer( ssl, ssl->conf->read_timeout ); } - if( ! record_read ) + if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) { - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) - { - if( ret == MBEDTLS_ERR_SSL_CONN_EOF ) - return( 0 ); + if( ret == MBEDTLS_ERR_SSL_CONN_EOF ) + return( 0 ); - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); - return( ret ); - } + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + return( ret ); } if( ssl->in_msglen == 0 && @@ -6540,7 +6646,9 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) #endif return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } +#endif /* MBEDTLS_SSL_CLI_C */ +#if defined(MBEDTLS_SSL_SRV_C) if( ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER && ssl->in_msg[0] != MBEDTLS_SSL_HS_CLIENT_HELLO ) { @@ -6603,25 +6711,18 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) } #endif ret = ssl_start_renegotiation( ssl ); - if( ret == MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO ) - { - record_read = 1; - } - else if( ret != 0 ) + if( ret != MBEDTLS_ERR_SSL_WAITING_SERVER_HELLO_RENEGO && + ret != 0 ) { MBEDTLS_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 mbedtls_ssl_read() again */ - if( ! record_read ) - return( MBEDTLS_ERR_SSL_WANT_READ ); + return( MBEDTLS_ERR_SSL_WANT_READ ); } else if( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_PENDING ) { - if( ssl->conf->renego_max_records >= 0 ) { if( ++ssl->renego_records_seen > ssl->conf->renego_max_records ) @@ -6679,8 +6780,11 @@ int mbedtls_ssl_read( mbedtls_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;