From 704f493730b12db10a156c9e3b5d2a20d9fbaece Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 13:08:45 +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 | 63 +++++++++++++++++++------------------------ library/ssl_tls.c | 11 +++++++- 3 files changed, 41 insertions(+), 37 deletions(-) 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/library/ssl_cli.c b/library/ssl_cli.c index 67cbccc90..8d0374f3b 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2102,11 +2102,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 +2392,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 +2514,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..b5c166ecc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3716,6 +3716,15 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> read record" ) ); + if( ssl->keep_current_message == 1 ) + { + MBEDTLS_SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read record" ) ); + ssl->keep_current_message = 0; + + return( 0 ); + } + if( ssl->in_hslen != 0 && ssl->in_hslen < ssl->in_msglen ) { /* @@ -5452,7 +5461,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; From 6a582e80f27bf8d72b392b2501e94ee7326b8444 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 13:38:05 +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 | 74 +++++++++++++++ library/ssl_cli.c | 2 + library/ssl_tls.c | 159 +++++++++++++++++++++++++-------- 3 files changed, 198 insertions(+), 37 deletions(-) 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 8d0374f3b..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 */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index b5c166ecc..acb3bad52 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3725,31 +3725,107 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) return( 0 ); } - 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 ); - if( ( ret = ssl_prepare_handshake_record( ssl ) ) != 0 ) - return( ret ); + 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; + } + + /* + * 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 - */ 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 ); @@ -3841,6 +3917,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; } @@ -6448,7 +6530,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 ) @@ -6471,8 +6553,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 ); @@ -6482,11 +6578,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 ); @@ -6502,16 +6595,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 && @@ -6549,7 +6639,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 ) { @@ -6612,25 +6704,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 ) From bfbc494114074da58d555cbf01bd54b450f631e1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 13:39:23 +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 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index acb3bad52..dafef1f5b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3742,6 +3742,7 @@ int mbedtls_ssl_read_record( 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. @@ -3757,6 +3758,12 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) 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 */ From cc019084b86bb1a225dba437d95c4f170a6625b5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 9 Jun 2017 10:51:37 +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. --- library/ssl_tls.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index dafef1f5b..bd2c27057 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6780,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; From 88647ace2bb60f97ace1ab7659b706faef144290 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 9 Jun 2017 11:30:33 +0100 Subject: [PATCH 5/5] Add ChangeLog entry --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 7c04ce0f3..b6067c6ca 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.1.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. * 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.