From 6a582e80f27bf8d72b392b2501e94ee7326b8444 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 8 Jun 2017 13:38:05 +0100 Subject: [PATCH] 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 )