Merge pull request #5327 from gilles-peskine-arm/zeroize-tag-2.28

Backport 2.2x: Zeroize expected MAC/tag intermediate variables

7 files changed, 72 insertions, 35 deletions

diff --git a/ChangeLog.d/mac-zeroize.txt b/ChangeLog.d/mac-zeroize.txt
new file mode 100644
index 0000000..a43e34f
--- /dev/null
+++ b/ChangeLog.d/mac-zeroize.txt
@@ -0,0 +1,6 @@
+Security
+   * Zeroize several intermediate variables used to calculate the expected
+     value when verifying a MAC or AEAD tag. This hardens the library in
+     case the value leaks through a memory disclosure vulnerability. For
+     example, a memory disclosure vulnerability could have allowed a
+     man-in-the-middle to inject fake ciphertext into a DTLS connection.
diff --git a/ChangeLog.d/ssl-mac-zeroize.txt b/ChangeLog.d/ssl-mac-zeroize.txt
deleted file mode 100644
index b49c7ac..0000000
--- a/ChangeLog.d/ssl-mac-zeroize.txt
+++ /dev/null
@@ -1,5 +0,0 @@
-Security
-   * Zeroize intermediate variables used to calculate the MAC in CBC cipher
-     suites. This hardens the library in case stack memory leaks through a
-     memory disclosure vulnerabilty, which could formerly have allowed a
-     man-in-the-middle to inject fake ciphertext into a DTLS connection.
diff --git a/library/cipher.c b/library/cipher.c
index d51ccd7..4ec40d2 100644
--- a/library/cipher.c
+++ b/library/cipher.c
@@ -1125,6 +1125,12 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx,
     }
 #endif /* MBEDTLS_USE_PSA_CRYPTO */
 
+    /* Status to return on a non-authenticated algorithm. It would make sense
+     * to return MBEDTLS_ERR_CIPHER_INVALID_CONTEXT or perhaps
+     * MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA, but at the time I write this our
+     * unit tests assume 0. */
+    ret = 0;
+
 #if defined(MBEDTLS_GCM_C)
     if( MBEDTLS_MODE_GCM == ctx->cipher_info->mode )
     {
@@ -1140,9 +1146,10 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx,
 
         /* Check the tag in "constant-time" */
         if( mbedtls_ct_memcmp( tag, check_tag, tag_len ) != 0 )
-            return( MBEDTLS_ERR_CIPHER_AUTH_FAILED );
-
-        return( 0 );
+        {
+            ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED;
+            goto exit;
+        }
     }
 #endif /* MBEDTLS_GCM_C */
 
@@ -1162,13 +1169,16 @@ int mbedtls_cipher_check_tag( mbedtls_cipher_context_t *ctx,
 
         /* Check the tag in "constant-time" */
         if( mbedtls_ct_memcmp( tag, check_tag, tag_len ) != 0 )
-            return( MBEDTLS_ERR_CIPHER_AUTH_FAILED );
-
-        return( 0 );
+        {
+            ret = MBEDTLS_ERR_CIPHER_AUTH_FAILED;
+            goto exit;
+        }
     }
 #endif /* MBEDTLS_CHACHAPOLY_C */
 
-    return( 0 );
+exit:
+    mbedtls_platform_zeroize( check_tag, tag_len );
+    return( ret );
 }
 #endif /* MBEDTLS_GCM_C || MBEDTLS_CHACHAPOLY_C */
 
diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index e3db912..a89430d 100644
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -2249,6 +2249,7 @@ psa_status_t psa_hash_verify( psa_hash_operation_t *operation,
         status = PSA_ERROR_INVALID_SIGNATURE;
 
 exit:
+    mbedtls_platform_zeroize( actual_hash, sizeof( actual_hash ) );
     if( status != PSA_SUCCESS )
         psa_hash_abort(operation);
 
@@ -2283,12 +2284,18 @@ psa_status_t psa_hash_compare( psa_algorithm_t alg,
                             actual_hash, sizeof(actual_hash),
                             &actual_hash_length );
     if( status != PSA_SUCCESS )
-        return( status );
+        goto exit;
     if( actual_hash_length != hash_length )
-        return( PSA_ERROR_INVALID_SIGNATURE );
+    {
+        status = PSA_ERROR_INVALID_SIGNATURE;
+        goto exit;
+    }
     if( mbedtls_psa_safer_memcmp( hash, actual_hash, actual_hash_length ) != 0 )
-        return( PSA_ERROR_INVALID_SIGNATURE );
-    return( PSA_SUCCESS );
+        status = PSA_ERROR_INVALID_SIGNATURE;
+
+exit:
+    mbedtls_platform_zeroize( actual_hash, sizeof( actual_hash ) );
+    return( status );
 }
 
 psa_status_t psa_hash_clone( const psa_hash_operation_t *source_operation,
diff --git a/library/rsa.c b/library/rsa.c
index a395542..8a5d40f 100644
--- a/library/rsa.c
+++ b/library/rsa.c
@@ -1942,9 +1942,13 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx,
     memcpy( sig, sig_try, ctx->len );
 
 cleanup:
+    mbedtls_platform_zeroize( sig_try, ctx->len );
+    mbedtls_platform_zeroize( verif, ctx->len );
     mbedtls_free( sig_try );
     mbedtls_free( verif );
 
+    if( ret != 0 )
+        memset( sig, '!', ctx->len );
     return( ret );
 }
 #endif /* MBEDTLS_PKCS1_V15 */
diff --git a/library/ssl_cookie.c b/library/ssl_cookie.c
index faf92e7..abf29ae 100644
--- a/library/ssl_cookie.c
+++ b/library/ssl_cookie.c
@@ -218,15 +218,20 @@ int mbedtls_ssl_cookie_check( void *p_ctx,
 
 #if defined(MBEDTLS_THREADING_C)
     if( mbedtls_mutex_unlock( &ctx->mutex ) != 0 )
-        return( MBEDTLS_ERROR_ADD( MBEDTLS_ERR_SSL_INTERNAL_ERROR,
-                MBEDTLS_ERR_THREADING_MUTEX_ERROR ) );
+    {
+        ret = MBEDTLS_ERROR_ADD( MBEDTLS_ERR_SSL_INTERNAL_ERROR,
+                                 MBEDTLS_ERR_THREADING_MUTEX_ERROR );
+    }
 #endif
 
     if( ret != 0 )
-        return( ret );
+        goto exit;
 
     if( mbedtls_ct_memcmp( cookie + 4, ref_hmac, sizeof( ref_hmac ) ) != 0 )
-        return( -1 );
+    {
+        ret = -1;
+        goto exit;
+    }
 
 #if defined(MBEDTLS_HAVE_TIME)
     cur_time = (unsigned long) mbedtls_time( NULL );
@@ -240,8 +245,13 @@ int mbedtls_ssl_cookie_check( void *p_ctx,
                   ( (unsigned long) cookie[3]       );
 
     if( ctx->timeout != 0 && cur_time - cookie_time > ctx->timeout )
-        return( -1 );
+    {
+        ret = -1;
+        goto exit;
+    }
 
-    return( 0 );
+exit:
+    mbedtls_platform_zeroize( ref_hmac, sizeof( ref_hmac ) );
+    return( ret );
 }
 #endif /* MBEDTLS_SSL_COOKIE_C */
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index 537adc2..f07b189 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -3631,12 +3631,20 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl )
 
     MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> parse finished" ) );
 
+    /* There is currently no ciphersuite using another length with TLS 1.2 */
+#if defined(MBEDTLS_SSL_PROTO_SSL3)
+    if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 )
+        hash_len = 36;
+    else
+#endif
+        hash_len = 12;
+
     ssl->handshake->calc_finished( ssl, buf, ssl->conf->endpoint ^ 1 );
 
     if( ( ret = mbedtls_ssl_read_record( ssl, 1 ) ) != 0 )
     {
         MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret );
-        return( ret );
+        goto exit;
     }
 
     if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE )
@@ -3644,24 +3652,18 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl )
         MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad finished message" ) );
         mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL,
                                         MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE );
-        return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE );
+        ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE;
+        goto exit;
     }
 
-    /* There is currently no ciphersuite using another length with TLS 1.2 */
-#if defined(MBEDTLS_SSL_PROTO_SSL3)
-    if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 )
-        hash_len = 36;
-    else
-#endif
-        hash_len = 12;
-
     if( ssl->in_msg[0] != MBEDTLS_SSL_HS_FINISHED ||
         ssl->in_hslen  != mbedtls_ssl_hs_hdr_len( ssl ) + hash_len )
     {
         MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad finished message" ) );
         mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL,
                                         MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR );
-        return( MBEDTLS_ERR_SSL_BAD_HS_FINISHED );
+        ret = MBEDTLS_ERR_SSL_BAD_HS_FINISHED;
+        goto exit;
     }
 
     if( mbedtls_ct_memcmp( ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl ),
@@ -3670,7 +3672,8 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl )
         MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad finished message" ) );
         mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL,
                                         MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR );
-        return( MBEDTLS_ERR_SSL_BAD_HS_FINISHED );
+        ret = MBEDTLS_ERR_SSL_BAD_HS_FINISHED;
+        goto exit;
     }
 
 #if defined(MBEDTLS_SSL_RENEGOTIATION)
@@ -3699,7 +3702,9 @@ int mbedtls_ssl_parse_finished( mbedtls_ssl_context *ssl )
 
     MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse finished" ) );
 
-    return( 0 );
+exit:
+    mbedtls_platform_zeroize( buf, hash_len );
+    return( ret );
 }
 
 static void ssl_handshake_params_init( mbedtls_ssl_handshake_params *handshake )