commit 6a0a44e16704b59040f8b330e57e52cf1cfa645d
author Gilles Peskine <Gilles.Peskine@arm.com> Mon Jun 11 17:42:48 2018 +0200
committer itayzafrir <itay.zafrir@arm.com> Wed Sep 12 16:13:49 2018 +0300
tree 9e93f8836802205b3af4acbe40d8570658cc5e94
parent c102e3ce4b6652b76bf0ca6520e04e3095a0cf41

HMAC: clean up local variables containing key material

In psa_mac_start, the hash of the key and ipad contain material that
can be used to make HMAC calculations with the key, therefore they
must be wiped.

In psa_mac_finish_internal, tmp contains an intermediate value which
could reveal the HMAC. This is definitely sensitive in the verify case,
and marginally sensitive in the finish case (it isn't if the hash
function is ideal, but it could make things worse if the hash function
is partially broken).

library/psa_crypto.c

diff --git a/library/psa_crypto.c b/library/psa_crypto.c
index 9fb80bb..b5b1a96 100755
--- a/library/psa_crypto.c
+++ b/library/psa_crypto.c
@@ -1116,10 +1116,19 @@
     status = psa_hash_start( &operation->ctx.hmac.hash_ctx,
                              PSA_ALG_HMAC_HASH( alg ) );
     if( status != PSA_SUCCESS )
-        return( status );
+        goto cleanup;
 
     status = psa_hash_update( &operation->ctx.hmac.hash_ctx, ipad,
                               block_size );
+
+cleanup:
+    if( key_bits / 8 > (size_t) block_size )
+        mbedtls_zeroize( sum, key_length );
+    mbedtls_zeroize( ipad, key_length );
+    /* opad is in the context. It needs to stay in memory if this function
+     * succeeds, and it will be wiped by psa_mac_abort() called from
+     * psa_mac_start in the error case. */
+
     return( status );
 }
 
@@ -1188,7 +1197,7 @@
      * context may contain data that needs to be wiped on error. */
     if( status != PSA_SUCCESS )
     {
-        psa_mac_abort(operation);
+        psa_mac_abort( operation );
     }
 
     else
@@ -1288,26 +1297,27 @@
                                           sizeof( tmp ), &hash_size );
                 if( status != PSA_SUCCESS )
                     goto cleanup;
+                /* From here on, tmp needs to be wiped. */
 
                 status = psa_hash_start( &operation->ctx.hmac.hash_ctx,
                                          PSA_ALG_HMAC_HASH( operation->alg ) );
                 if( status != PSA_SUCCESS )
-                    goto cleanup;
+                    goto hmac_cleanup;
 
                 status = psa_hash_update( &operation->ctx.hmac.hash_ctx, opad,
                                           block_size );
                 if( status != PSA_SUCCESS )
-                    goto cleanup;
+                    goto hmac_cleanup;
 
                 status = psa_hash_update( &operation->ctx.hmac.hash_ctx, tmp,
                                           hash_size);
                 if( status != PSA_SUCCESS )
-                    goto cleanup;
+                    goto hmac_cleanup;
 
                 status = psa_hash_finish( &operation->ctx.hmac.hash_ctx, mac,
                                           mac_size, mac_length );
-                if( status != PSA_SUCCESS )
-                    goto cleanup;
+            hmac_cleanup:
+                mbedtls_zeroize( tmp, hash_size );
             }
             else
 #endif /* MBEDTLS_MD_C */