Merge remote-tracking branch 'restricted/iotssl-1398_backport-2.1' into mbedtls-2.1-restricted
* restricted/iotssl-1398_backport-2.1:
Add ChangeLog entry
Ensure application data records are not kept when fully processed
Add hard assertion to mbedtls_ssl_read_record_layer
Fix mbedtls_ssl_read
Simplify retaining of messages for future processing
diff --git a/ChangeLog b/ChangeLog
index 0a47c6f..0af0acf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,11 @@
= 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 5775f8a..906866d 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -714,7 +714,9 @@
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 6b9334f..0c93a74 100644
--- a/include/mbedtls/ssl_internal.h
+++ b/include/mbedtls/ssl_internal.h
@@ -387,6 +387,80 @@
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 67cbccc..31eb203 100644
--- a/library/ssl_cli.c
+++ b/library/ssl_cli.c
@@ -1304,6 +1304,8 @@
}
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 @@
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 @@
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 @@
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 );
}
- ssl->record_read = 0;
+
+ if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE )
+ {
+ MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) );
+ return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE );
+ }
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 22c3f99..bd2c270 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -3716,31 +3716,123 @@
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 @@
}
#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 @@
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 @@
*/
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 @@
}
#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 @@
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 @@
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 @@
#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 @@
}
#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 @@
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;