Merge remote-tracking branch 'restricted/iotssl-1398_backport-1.3' into mbedtls-1.3-restricted
* restricted/iotssl-1398_backport-1.3:
Add ChangeLog entry
Ensure application data records are not kept when fully processed
Add hard assertion to ssl_read_record
Fix mbedtls_ssl_read
Simplify retaining of messages for future processing
diff --git a/ChangeLog b/ChangeLog
index cea282a..9778fbe 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,11 @@
= mbed TLS 1.3.20 released xxxx-xx-xx
Security
+ * Fixed unlimited overread of heap-based buffer in 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 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/polarssl/ssl.h b/include/polarssl/ssl.h
index 74b1317..4a01bbf 100644
--- a/include/polarssl/ssl.h
+++ b/include/polarssl/ssl.h
@@ -846,7 +846,9 @@
size_t in_hslen; /*!< current handshake message length */
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 34ab7e0..5f5beec 100644
--- a/library/ssl_cli.c
+++ b/library/ssl_cli.c
@@ -1195,6 +1195,8 @@
}
SSL_DEBUG_MSG( 1, ( "non-handshake message during renego" ) );
+
+ ssl->keep_current_message = 1;
return( POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO );
}
#endif /* POLARSSL_SSL_RENEGOTIATION */
@@ -1943,7 +1945,9 @@
if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_PSK ||
ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_RSA_PSK )
{
- ssl->record_read = 1;
+ /* Current message is probably either
+ * CertificateRequest or ServerHelloDone */
+ ssl->keep_current_message = 1;
goto exit;
}
@@ -2260,36 +2264,31 @@
* n+4 .. ... Distinguished Name #1
* ... .. ... length of DN 2, etc.
*/
- if( ssl->record_read == 0 )
+
+ if( ( ret = ssl_read_record( ssl ) ) != 0 )
{
- if( ( ret = ssl_read_record( ssl ) ) != 0 )
- {
- SSL_DEBUG_RET( 1, "ssl_read_record", ret );
- return( ret );
- }
-
- if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
- {
- SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) );
- return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
- }
-
- ssl->record_read = 1;
+ SSL_DEBUG_RET( 1, "ssl_read_record", ret );
+ return( ret );
}
- ssl->client_auth = 0;
- ssl->state++;
+ if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
+ {
+ SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) );
+ return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
+ }
- if( ssl->in_msg[0] == SSL_HS_CERTIFICATE_REQUEST )
- ssl->client_auth++;
+ ssl->state++;
+ ssl->client_auth = ( ssl->in_msg[0] == SSL_HS_CERTIFICATE_REQUEST );
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
@@ -2386,21 +2385,17 @@
SSL_DEBUG_MSG( 2, ( "=> parse server hello done" ) );
- if( ssl->record_read == 0 )
+ if( ( ret = ssl_read_record( ssl ) ) != 0 )
{
- if( ( ret = ssl_read_record( ssl ) ) != 0 )
- {
- SSL_DEBUG_RET( 1, "ssl_read_record", ret );
- return( ret );
- }
-
- if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
- {
- SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) );
- return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
- }
+ SSL_DEBUG_RET( 1, "ssl_read_record", ret );
+ return( ret );
}
- ssl->record_read = 0;
+
+ if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
+ {
+ SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) );
+ return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
+ }
if( ssl->in_hslen != 4 ||
ssl->in_msg[0] != SSL_HS_SERVER_HELLO_DONE )
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 5779229..bae8433 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -2169,47 +2169,82 @@
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 );
-
- ssl->in_hslen = 4;
- ssl->in_hslen += ( ssl->in_msg[2] << 8 ) | ssl->in_msg[3];
-
- SSL_DEBUG_MSG( 3, ( "handshake message: msglen ="
- " %d, type = %d, hslen = %d",
- ssl->in_msglen, ssl->in_msg[0], ssl->in_hslen ) );
-
- if( ssl->in_msglen < 4 || ssl->in_msg[1] != 0 )
- {
- SSL_DEBUG_MSG( 1, ( "bad handshake length" ) );
- return( POLARSSL_ERR_SSL_INVALID_RECORD );
- }
-
- if( ssl->in_msglen < ssl->in_hslen )
- {
- SSL_DEBUG_MSG( 1, ( "bad handshake length" ) );
- return( POLARSSL_ERR_SSL_INVALID_RECORD );
- }
-
- if( ssl->state != SSL_HANDSHAKE_OVER )
- ssl->handshake->update_checksum( ssl, ssl->in_msg, ssl->in_hslen );
+ SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) );
+ SSL_DEBUG_MSG( 2, ( "<= read record" ) );
+ ssl->keep_current_message = 0;
return( 0 );
}
- ssl->in_hslen = 0;
+ if( ssl->in_hslen != 0 )
+ {
+ if( ssl->in_offt != NULL )
+ {
+ SSL_DEBUG_MSG( 1, ( "should never happen" ) );
+ return( POLARSSL_ERR_SSL_INTERNAL_ERROR );
+ }
+
+ /*
+ * Get next Handshake message in the current record
+ */
+
+ 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 );
+
+ ssl->in_hslen = 4;
+ ssl->in_hslen += ( ssl->in_msg[2] << 8 ) | ssl->in_msg[3];
+
+ SSL_DEBUG_MSG( 3, ( "handshake message: msglen ="
+ " %d, type = %d, hslen = %d",
+ ssl->in_msglen, ssl->in_msg[0], ssl->in_hslen ) );
+
+ if( ssl->in_msglen < 4 || ssl->in_msg[1] != 0 )
+ {
+ SSL_DEBUG_MSG( 1, ( "bad handshake length" ) );
+ return( POLARSSL_ERR_SSL_INVALID_RECORD );
+ }
+
+ if( ssl->in_msglen < ssl->in_hslen )
+ {
+ SSL_DEBUG_MSG( 1, ( "bad handshake length" ) );
+ return( POLARSSL_ERR_SSL_INVALID_RECORD );
+ }
+
+ if( ssl->state != SSL_HANDSHAKE_OVER )
+ ssl->handshake->update_checksum( ssl, ssl->in_msg, ssl->in_hslen );
+
+ return( 0 );
+ }
+
+ ssl->in_msglen = 0;
+ ssl->in_hslen = 0;
+ }
+ else if( ssl->in_offt != NULL )
+ {
+ return( 0 );
+ }
+ else
+ {
+ ssl->in_msglen = 0;
+ }
/*
- * Read the record header and validate it
+ * 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:
if( ( ret = ssl_fetch_input( ssl, 5 ) ) != 0 )
{
@@ -3750,7 +3785,7 @@
ssl->in_hslen = 0;
ssl->nb_zero = 0;
- ssl->record_read = 0;
+ ssl->keep_current_message = 0;
ssl->out_msg = ssl->out_ctr + 13;
ssl->out_msgtype = 0;
@@ -4642,13 +4677,15 @@
*/
int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len )
{
- int ret, record_read = 0;
+ int ret;
size_t n;
SSL_DEBUG_MSG( 2, ( "=> read" ) );
#if defined(POLARSSL_SSL_RENEGOTIATION)
- if( ( ret = ssl_check_ctr_renegotiate( ssl ) ) != 0 )
+ ret = ssl_check_ctr_renegotiate( ssl );
+ if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO &&
+ ret != 0 )
{
SSL_DEBUG_RET( 1, "ssl_check_ctr_renegotiate", ret );
return( ret );
@@ -4658,11 +4695,8 @@
if( ssl->state != SSL_HANDSHAKE_OVER )
{
ret = ssl_handshake( ssl );
- if( ret == POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO )
- {
- record_read = 1;
- }
- else if( ret != 0 )
+ if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO &&
+ ret != 0 )
{
SSL_DEBUG_RET( 1, "ssl_handshake", ret );
return( ret );
@@ -4671,16 +4705,13 @@
if( ssl->in_offt == NULL )
{
- if( ! record_read )
+ if( ( ret = ssl_read_record( ssl ) ) != 0 )
{
- if( ( ret = ssl_read_record( ssl ) ) != 0 )
- {
- if( ret == POLARSSL_ERR_SSL_CONN_EOF )
- return( 0 );
+ if( ret == POLARSSL_ERR_SSL_CONN_EOF )
+ return( 0 );
- SSL_DEBUG_RET( 1, "ssl_read_record", ret );
- return( ret );
- }
+ SSL_DEBUG_RET( 1, "ssl_read_record", ret );
+ return( ret );
}
if( ssl->in_msglen == 0 &&
@@ -4754,21 +4785,15 @@
else
{
ret = ssl_start_renegotiation( ssl );
- if( ret == POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO )
- {
- record_read = 1;
- }
- else if( ret != 0 )
+ if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO &&
+ ret != 0 )
{
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 ssl_read() again */
- if( ! record_read )
- return( POLARSSL_ERR_NET_WANT_READ );
+ return( POLARSSL_ERR_NET_WANT_READ );
}
else if( ssl->renegotiation == SSL_RENEGOTIATION_PENDING )
{
@@ -4807,8 +4832,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;