From af0665d8b0f39d14ba88209dfdb169a0b99d3734 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 May 2017 09:16:26 +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/mbedtls/ssl.h | 4 ++- library/ssl_cli.c | 70 ++++++++++++++++++++----------------------- library/ssl_tls.c | 37 ++++++++++++++--------- 3 files changed, 59 insertions(+), 52 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 49b1aca8c..51c1c60d7 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -845,7 +845,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/library/ssl_cli.c b/library/ssl_cli.c index 616cadc08..a2b9f8cfe 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1471,6 +1471,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 */ @@ -2316,13 +2318,17 @@ 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" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); + return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } @@ -2640,38 +2646,32 @@ 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" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_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" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_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; + } /* * struct { @@ -2766,21 +2766,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 208500a66..0a29970e8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3720,27 +3720,35 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> read record" ) ); - do { + if( ssl->keep_current_message == 0 ) + { + do { - if( ( ret = mbedtls_ssl_read_record_layer( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record_layer( ssl ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); + return( ret ); + } + + ret = mbedtls_ssl_handle_message_type( ssl ); + + } while( MBEDTLS_ERR_SSL_NON_FATAL == ret ); + + if( 0 != ret ) { MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); return( ret ); } - ret = mbedtls_ssl_handle_message_type( ssl ); - - } while( MBEDTLS_ERR_SSL_NON_FATAL == ret ); - - if( 0 != ret ) - { - MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_handle_message_type" ), ret ); - return( ret ); + if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) + { + mbedtls_ssl_update_handshake_status( ssl ); + } } - - if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) + else { - mbedtls_ssl_update_handshake_status( ssl ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= reuse previously read message" ) ); + ssl->keep_current_message = 0; } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read record" ) ); @@ -5603,7 +5611,8 @@ 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; From 4a810fba69fa4736c2d870b446bcea92be8be334 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 May 2017 16:27:30 +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. --- include/mbedtls/ssl_internal.h | 73 ++++++++++++++ library/ssl_tls.c | 179 ++++++++++++++++++++++++++------- 2 files changed, 213 insertions(+), 39 deletions(-) diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index 766b4abaf..756360b18 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -403,6 +403,79 @@ int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ); int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ); void mbedtls_ssl_update_handshake_status( 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_tls.c b/library/ssl_tls.c index 0a29970e8..67cbdf423 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3760,31 +3760,103 @@ int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ) { int ret; - if( ssl->in_hslen != 0 && ssl->in_hslen < ssl->in_msglen ) + /* + * 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. + * + * (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 ) { /* * 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 ); + /* 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 ); + MBEDTLS_SSL_DEBUG_BUF( 4, "remaining content in record", + ssl->in_msg, ssl->in_msglen ); + } + 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; + } + + /* + * 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 ); } - ssl->in_hslen = 0; + /* Need to fetch a new record */ - /* - * Read the record header and parse it - */ #if defined(MBEDTLS_SSL_PROTO_DTLS) read_record_header: #endif + /* 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 ); @@ -3876,6 +3948,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; } @@ -6643,7 +6721,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 ) @@ -6666,8 +6744,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 ); @@ -6677,11 +6769,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 ); @@ -6697,16 +6786,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 && @@ -6730,10 +6816,16 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "received handshake message" ) ); + /* + * - For client-side, expect SERVER_HELLO_REQUEST. + * - For server-side, expect CLIENT_HELLO. + * - Fail (TLS) or silently drop record (DTLS) in other cases. + */ + #if defined(MBEDTLS_SSL_CLI_C) if( ssl->conf->endpoint == MBEDTLS_SSL_IS_CLIENT && ( ssl->in_msg[0] != MBEDTLS_SSL_HS_HELLO_REQUEST || - ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) ) ) + ssl->in_hslen != mbedtls_ssl_hs_hdr_len( ssl ) ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "handshake received (not HelloRequest)" ) ); @@ -6744,7 +6836,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 ) { @@ -6757,13 +6851,19 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) #endif return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } -#endif +#endif /* MBEDTLS_SSL_SRV_C */ + + /* Determine whether renegotiation attempt should be accepted */ if( ssl->conf->disable_renegotiation == MBEDTLS_SSL_RENEGOTIATION_DISABLED || ( ssl->secure_renegotiation == MBEDTLS_SSL_LEGACY_RENEGOTIATION && ssl->conf->allow_legacy_renegotiation == MBEDTLS_SSL_LEGACY_NO_RENEGOTIATION ) ) { + /* + * Refuse renegotiation + */ + MBEDTLS_SSL_DEBUG_MSG( 3, ( "refusing renegotiation, sending alert" ) ); #if defined(MBEDTLS_SSL_PROTO_SSL3) @@ -6798,6 +6898,10 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) } else { + /* + * Accept renegotiation request + */ + /* DTLS clients need to know renego is server-initiated */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && @@ -6807,25 +6911,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 ) @@ -6873,7 +6970,7 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) } } #endif /* MBEDTLS_SSL_SRV_C && MBEDTLS_SSL_RENEGOTIATION */ -#endif +#endif /* MBEDTLS_SSL_PROTO_DTLS */ } n = ( len < ssl->in_msglen ) @@ -6883,11 +6980,15 @@ 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 */ + { + /* all bytes consumed */ ssl->in_offt = NULL; + } else + { /* more data available */ ssl->in_offt += n; + } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read" ) ); From bb9dd0c044c3239aa9f82a6876b940ddc48a19ca Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 11:55:34 +0100 Subject: [PATCH 3/5] Add hard assertion to mbedtls_ssl_read_record_layer This commit adds a hard assertion to mbedtls_ssl_read_record_layer 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 | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 67cbdf423..911664eb9 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3777,6 +3777,7 @@ int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ) * 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. @@ -3791,6 +3792,15 @@ int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ) /* Case (1): Handshake messages */ if( ssl->in_hslen != 0 ) { + /* Hard assertion to be sure that no application data + * is in flight, as corrupting ssl->in_msglen during + * ssl->in_offt != NULL is fatal. */ + 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 */ @@ -3808,6 +3818,9 @@ int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ) * Again, it's wrong for DTLS handshake fragmentation. * The following check is therefore mandatory, and * should not be treated as a silently corrected assertion. + * Additionally, ssl->in_hslen might be arbitrarily out of + * bounds after handling a DTLS message with an unexpected + * sequence number, see mbedtls_ssl_prepare_handshake_record. */ if( ssl->in_hslen < ssl->in_msglen ) { From bdf3905fff532500f1f4f836e30baa664ef9fd93 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 9 Jun 2017 10:42:03 +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 mbedtls_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. It also documents and suggests how the problem might be solved in a more structural way on the long run. --- library/ssl_tls.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 911664eb9..661ae7065 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6790,6 +6790,41 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) } } + /* + * TODO + * + * The logic should be streamlined here: + * + * Instead of + * + * - Manually checking whether ssl->in_offt is NULL + * - Fetching a new record if yes + * - Setting ssl->in_offt if one finds an application record + * - Resetting keep_current_message after handling the application data + * + * one should + * + * - Adapt read_record to set ssl->in_offt automatically + * when a new application data record is processed. + * - Always call mbedtls_ssl_read_record here. + * + * This way, the logic of ssl_read would be much clearer: + * + * (1) Always call record layer and see what kind of record is on + * and have it ready for consumption (in particular, in_offt + * properly set for application data records). + * (2) If it's application data (either freshly fetched + * or something already being partially processed), + * serve the read request from it. + * (3) If it's something different from application data, + * handle it accordingly, e.g. potentially start a + * renegotiation. + * + * This will also remove the need to manually reset + * ssl->keep_current_message = 0 below. + * + */ + if( ssl->in_offt == NULL ) { /* Start timer if not already running */ @@ -6996,6 +7031,7 @@ int mbedtls_ssl_read( mbedtls_ssl_context *ssl, unsigned char *buf, size_t len ) { /* all bytes consumed */ ssl->in_offt = NULL; + ssl->keep_current_message = 0; } else { From bf4c2e3f79176800704e5f27cb549790b6c736ec Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 9 Jun 2017 11:28:45 +0100 Subject: [PATCH 5/5] Add ChangeLog entry --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 08edd7796..9dcc3f2b2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.x.x branch 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. * Removed SHA-1 and RIPEMD-160 from the default hash algorithms for certificate verification. SHA-1 can be turned back on with a compile-time option if needed.