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

ChangeLog

@@ -3,6 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date)
 = 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.

include/polarssl/ssl.h

@@ -846,7 +846,9 @@ struct _ssl_context
 
     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)

library/ssl_cli.c

@@ -1195,6 +1195,8 @@ static int ssl_parse_server_hello( ssl_context *ssl )
             }
 
             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 @@ static int ssl_parse_server_key_exchange( ssl_context *ssl )
         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 @@ static int ssl_parse_certificate_request( ssl_context *ssl )
      *    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 );
-            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->client_auth = 0;
+    if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
-    ssl->state++;
+    {
+        SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) );
+        return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
+    }
 
-    if( ssl->in_msg[0] == SSL_HS_CERTIFICATE_REQUEST )
+    ssl->state++;
-        ssl->client_auth++;
+    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 @@ static int ssl_parse_server_hello_done( ssl_context *ssl )
 
     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 );
-            SSL_DEBUG_RET( 1, "ssl_read_record", ret );
+    }
-            return( ret );
+
-        }
+    if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
-
+    {
-        if( ssl->in_msgtype != SSL_MSG_HANDSHAKE )
+        SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) );
-        {
+        return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
-            SSL_DEBUG_MSG( 1, ( "bad server hello done message" ) );
-            return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE );
-        }
     }
-    ssl->record_read = 0;
 
     if( ssl->in_hslen  != 4 ||
         ssl->in_msg[0] != SSL_HS_SERVER_HELLO_DONE )

library/ssl_tls.c

@@ -2169,47 +2169,82 @@ int ssl_read_record( ssl_context *ssl )
 
     SSL_DEBUG_MSG( 2, ( "=> read record" ) );
 
-    if( ssl->in_hslen != 0 &&
+    if( ssl->keep_current_message == 1 )
-        ssl->in_hslen < ssl->in_msglen )
     {
-        /*
+        SSL_DEBUG_MSG( 2, ( "reuse previously read message" ) );
-         * Get next Handshake message in the current record
+        SSL_DEBUG_MSG( 2, ( "<= read record" ) );
-         */
+        ssl->keep_current_message = 0;
-        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_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 @@ int ssl_session_reset( ssl_context *ssl )
 
     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 @@ static int ssl_check_ctr_renegotiate( ssl_context *ssl )
  */
 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 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len )
     if( ssl->state != SSL_HANDSHAKE_OVER )
     {
         ret = ssl_handshake( ssl );
-        if( ret == POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO )
+        if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO &&
-        {
+            ret != 0 )
-            record_read = 1;
-        }
-        else if( ret != 0 )
         {
             SSL_DEBUG_RET( 1, "ssl_handshake", ret );
             return( ret );
@@ -4671,16 +4705,13 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len )
 
     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 );
+            SSL_DEBUG_RET( 1, "ssl_read_record", ret );
-                return( ret );
+            return( ret );
-            }
         }
 
         if( ssl->in_msglen  == 0 &&
@@ -4754,21 +4785,15 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len )
             else
             {
                 ret = ssl_start_renegotiation( ssl );
-                if( ret == POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO )
+                if( ret != POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO &&
-                {
+                    ret != 0 )
-                    record_read = 1;
-                }
-                else if( ret != 0 )
                 {
                     SSL_DEBUG_RET( 1, "ssl_start_renegotiation", ret );
                     return( ret );
                 }
             }
 
-            /* If a non-handshake record was read during renego, fallthrough,
+            return( POLARSSL_ERR_NET_WANT_READ );
-             * else tell the user they should call ssl_read() again */
-            if( ! record_read )
-                return( POLARSSL_ERR_NET_WANT_READ );
         }
         else if( ssl->renegotiation == SSL_RENEGOTIATION_PENDING )
         {
@@ -4807,8 +4832,11 @@ int ssl_read( 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;