Move is_sign and mac_size checking back to PSA core scope

It makes sense to do the length checking in the core rather than expect
each driver to deal with it themselves. This puts the onus on the core to
dictate which algorithm/key combinations are valid before calling a driver.

Additionally, this commit also updates the psa_mac_sign_finish function
to better deal with output buffer sanitation, as per the review comments
on #4247.

Signed-off-by: Steven Cooreman <steven.cooreman@silabs.com>

include/psa/crypto_builtin_composites.h

@@ -62,8 +62,6 @@ typedef struct
 typedef struct
 {
     psa_algorithm_t alg;
-    unsigned int is_sign : 1;
-    uint8_t mac_size;
     union
     {
         unsigned dummy; /* Make the union non-empty even with no supported algorithms. */
@@ -76,7 +74,7 @@ typedef struct
     } ctx;
 } mbedtls_psa_mac_operation_t;
 
-#define MBEDTLS_PSA_MAC_OPERATION_INIT {0, 0, 0, 0, {0}}
+#define MBEDTLS_PSA_MAC_OPERATION_INIT {0, {0}}
 
 /*
  * BEYOND THIS POINT, TEST DRIVER DECLARATIONS ONLY.

include/psa/crypto_struct.h

@@ -137,10 +137,12 @@ struct psa_mac_operation_s
      * ID value zero means the context is not valid or not assigned to
      * any driver (i.e. none of the driver contexts are active). */
     unsigned int id;
+    uint8_t mac_size;
+    unsigned int is_sign : 1;
     psa_driver_mac_context_t ctx;
 };
 
-#define PSA_MAC_OPERATION_INIT {0, {0}}
+#define PSA_MAC_OPERATION_INIT {0, 0, 0, {0}}
 static inline struct psa_mac_operation_s psa_mac_operation_init( void )
 {
     const struct psa_mac_operation_s v = PSA_MAC_OPERATION_INIT;

library/psa_crypto.c

@@ -2291,6 +2291,8 @@ psa_status_t psa_mac_abort( psa_mac_operation_t *operation )
         return( PSA_SUCCESS );
 
     psa_status_t status = psa_driver_wrapper_mac_abort( operation );
+    operation->mac_size = 0;
+    operation->is_sign = 0;
     operation->id = 0;
 
     return( status );
@@ -2304,7 +2306,6 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation,
     psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
     psa_status_t unlock_status = PSA_ERROR_CORRUPTION_DETECTED;
     psa_key_slot_t *slot;
-    size_t mac_size;
 
     /* A context must be freshly initialized before it can be set up. */
     if( operation->id != 0 )
@@ -2330,12 +2331,15 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation,
     if( status != PSA_SUCCESS )
         goto exit;
 
+    operation->is_sign = is_sign;
+
     /* Get the output length for the algorithm and key combination. None of the
      * currently supported algorithms have an output length dependent on actual
      * key size, so setting it to a bogus value is currently OK. */
-    mac_size = PSA_MAC_LENGTH( psa_get_key_type( &attributes ), 0, alg );
+    operation->mac_size = PSA_MAC_LENGTH(
+                            psa_get_key_type( &attributes ), 0, alg );
 
-    if( mac_size < 4 )
+    if( operation->mac_size < 4 )
     {
         /* A very short MAC is too short for security since it can be
          * brute-forced. Ancient protocols with 32-bit MACs do exist,
@@ -2345,8 +2349,9 @@ static psa_status_t psa_mac_setup( psa_mac_operation_t *operation,
         goto exit;
     }
 
-    if( mac_size > PSA_MAC_LENGTH( psa_get_key_type( &attributes ), 0,
+    if( operation->mac_size > PSA_MAC_LENGTH( psa_get_key_type( &attributes ),
-                                   PSA_ALG_FULL_LENGTH_MAC( alg ) ) )
+                                              0,
+                                              PSA_ALG_FULL_LENGTH_MAC( alg ) ) )
     {
         /* It's impossible to "truncate" to a larger length than the full length
          * of the algorithm. */
@@ -2423,26 +2428,45 @@ psa_status_t psa_mac_sign_finish( psa_mac_operation_t *operation,
     psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
     psa_status_t abort_status = PSA_ERROR_CORRUPTION_DETECTED;
 
+    /* Set the output length and content to a safe default, such that in
+     * case the caller misses an error check, the output would be an
+     * unachievable MAC. */
+    *mac_length = mac_size;
+
     if( operation->id == 0 )
         return( PSA_ERROR_BAD_STATE );
 
-    /* Fill the output buffer with something that isn't a valid mac
+    if( ! operation->is_sign )
-     * (barring an attack on the mac and deliberately-crafted input),
+        return( PSA_ERROR_BAD_STATE );
-     * in case the caller doesn't check the return status properly. */
+
-    *mac_length = mac_size;
+    /* Sanity checks on output buffer length. */
-    /* If mac_size is 0 then mac may be NULL and then the
+    if( mac_size == 0 || mac_size < operation->mac_size )
-     * call to memset would have undefined behavior. */
+        return( PSA_ERROR_BUFFER_TOO_SMALL );
-    if( mac_size != 0 )
-        memset( mac, '!', mac_size );
 
     status = psa_driver_wrapper_mac_sign_finish( operation,
-                                                 mac, mac_size, mac_length );
+                                                 mac, operation->mac_size,
+                                                 mac_length );
+
+    if( status == PSA_SUCCESS )
+    {
+        /* Set the excess room in the output buffer to an invalid value, to
+         * avoid potentially leaking a longer MAC. */
+        if( mac_size > operation->mac_size )
+            memset( &mac[operation->mac_size],
+                    '!',
+                    mac_size - operation->mac_size );
+    }
+    else
+    {
+        /* Set the output length and content to a safe default, such that in
+         * case the caller misses an error check, the output would be an
+         * unachievable MAC. */
+        *mac_length = mac_size;
+        memset( mac, '!', mac_size );
+    }
 
     abort_status = psa_mac_abort( operation );
 
-    if( status != PSA_SUCCESS && mac_size > 0 )
-        memset( mac, '!', mac_size );
-
     return( status == PSA_SUCCESS ? abort_status : status );
 }
 
@@ -2456,8 +2480,19 @@ psa_status_t psa_mac_verify_finish( psa_mac_operation_t *operation,
     if( operation->id == 0 )
         return( PSA_ERROR_BAD_STATE );
 
+    if( operation->is_sign )
+        return( PSA_ERROR_BAD_STATE );
+
+    if( operation->mac_size != mac_length )
+    {
+        status = PSA_ERROR_INVALID_SIGNATURE;
+        goto cleanup;
+    }
+
     status = psa_driver_wrapper_mac_verify_finish( operation,
                                                    mac, mac_length );
+
+cleanup:
     abort_status = psa_mac_abort( operation );
 
     return( status == PSA_SUCCESS ? abort_status : status );
@@ -3250,6 +3285,9 @@ static psa_status_t psa_key_derivation_start_hmac(
     psa_set_key_bits( &attributes, PSA_BYTES_TO_BITS( hmac_key_length ) );
     psa_set_key_usage_flags( &attributes, PSA_KEY_USAGE_SIGN_HASH );
 
+    operation->is_sign = 1;
+    operation->mac_size = PSA_HASH_LENGTH( hash_alg );
+
     status = psa_driver_wrapper_mac_sign_setup( operation,
                                                 &attributes,
                                                 hmac_key, hmac_key_length,

library/psa_crypto_mac.c

@@ -229,11 +229,10 @@ static psa_status_t mac_init(
 {
     psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
 
-    operation->alg = PSA_ALG_FULL_LENGTH_MAC( alg );
+    operation->alg = alg;
-    operation->is_sign = 0;
 
 #if defined(BUILTIN_ALG_CMAC)
-    if( operation->alg == PSA_ALG_CMAC )
+    if( PSA_ALG_FULL_LENGTH_MAC( operation->alg ) == PSA_ALG_CMAC )
     {
         mbedtls_cipher_init( &operation->ctx.cmac );
         status = PSA_SUCCESS;
@@ -269,7 +268,7 @@ static psa_status_t mac_abort( mbedtls_psa_mac_operation_t *operation )
     }
     else
 #if defined(BUILTIN_ALG_CMAC)
-    if( operation->alg == PSA_ALG_CMAC )
+    if( PSA_ALG_FULL_LENGTH_MAC( operation->alg ) == PSA_ALG_CMAC )
     {
         mbedtls_cipher_free( &operation->ctx.cmac );
     }
@@ -289,7 +288,6 @@ static psa_status_t mac_abort( mbedtls_psa_mac_operation_t *operation )
     }
 
     operation->alg = 0;
-    operation->is_sign = 0;
 
     return( PSA_SUCCESS );
 
@@ -306,8 +304,7 @@ static psa_status_t mac_setup( mbedtls_psa_mac_operation_t *operation,
                                const psa_key_attributes_t *attributes,
                                const uint8_t *key_buffer,
                                size_t key_buffer_size,
-                               psa_algorithm_t alg,
+                               psa_algorithm_t alg )
-                               int is_sign )
 {
     psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED;
 
@@ -318,13 +315,6 @@ static psa_status_t mac_setup( mbedtls_psa_mac_operation_t *operation,
     status = mac_init( operation, alg );
     if( status != PSA_SUCCESS )
         return( status );
-    operation->is_sign = is_sign;
-
-    /* Get the output length for the algorithm and key combination. None of the
-     * currently supported algorithms have an output length dependent on actual
-     * key size, so setting it to a bogus value is currently OK. */
-    operation->mac_size =
-        PSA_MAC_LENGTH( psa_get_key_type( attributes ), 0, alg );
 
 #if defined(BUILTIN_ALG_CMAC)
     if( PSA_ALG_FULL_LENGTH_MAC( alg ) == PSA_ALG_CMAC )
@@ -340,7 +330,8 @@ static psa_status_t mac_setup( mbedtls_psa_mac_operation_t *operation,
     if( PSA_ALG_IS_HMAC( alg ) )
     {
         /* Sanity check. This shouldn't fail on a valid configuration. */
-        if( operation->mac_size > sizeof( operation->ctx.hmac.opad ) )
+        if( PSA_MAC_LENGTH( psa_get_key_type( attributes ), 0, alg ) >
+            sizeof( operation->ctx.hmac.opad ) )
         {
             status = PSA_ERROR_NOT_SUPPORTED;
             goto exit;
@@ -363,7 +354,6 @@ static psa_status_t mac_setup( mbedtls_psa_mac_operation_t *operation,
         status = PSA_ERROR_NOT_SUPPORTED;
     }
 
-exit:
     if( status != PSA_SUCCESS )
         mac_abort( operation );
 
@@ -401,8 +391,8 @@ static psa_status_t mac_sign_setup(
     size_t key_buffer_size,
     psa_algorithm_t alg )
 {
-    return( mac_setup( operation, attributes, key_buffer, key_buffer_size, alg,
+    return( mac_setup( operation,
-                       1 ) );
+                       attributes, key_buffer, key_buffer_size, alg ) );
 }
 
 static psa_status_t mac_verify_setup(
@@ -412,8 +402,8 @@ static psa_status_t mac_verify_setup(
     size_t key_buffer_size,
     psa_algorithm_t alg )
 {
-    return( mac_setup( operation, attributes, key_buffer, key_buffer_size, alg,
+    return( mac_setup( operation,
-                        0 ) );
+                       attributes, key_buffer, key_buffer_size, alg ) );
 }
 
 static psa_status_t mac_update(
@@ -425,7 +415,7 @@ static psa_status_t mac_update(
         return( PSA_ERROR_BAD_STATE );
 
 #if defined(BUILTIN_ALG_CMAC)
-    if( operation->alg == PSA_ALG_CMAC )
+    if( PSA_ALG_FULL_LENGTH_MAC( operation->alg ) == PSA_ALG_CMAC )
     {
         return( mbedtls_to_psa_error(
                     mbedtls_cipher_cmac_update( &operation->ctx.cmac,
@@ -452,16 +442,13 @@ static psa_status_t mac_finish_internal( mbedtls_psa_mac_operation_t *operation,
                                          uint8_t *mac,
                                          size_t mac_size )
 {
-    if( mac_size < operation->mac_size )
-        return( PSA_ERROR_BUFFER_TOO_SMALL );
-
 #if defined(BUILTIN_ALG_CMAC)
-    if( operation->alg == PSA_ALG_CMAC )
+    if( PSA_ALG_FULL_LENGTH_MAC( operation->alg ) == PSA_ALG_CMAC )
     {
         uint8_t tmp[PSA_BLOCK_CIPHER_BLOCK_MAX_SIZE];
         int ret = mbedtls_cipher_cmac_finish( &operation->ctx.cmac, tmp );
         if( ret == 0 )
-            memcpy( mac, tmp, operation->mac_size );
+            memcpy( mac, tmp, mac_size );
         mbedtls_platform_zeroize( tmp, sizeof( tmp ) );
         return( mbedtls_to_psa_error( ret ) );
     }
@@ -471,13 +458,16 @@ static psa_status_t mac_finish_internal( mbedtls_psa_mac_operation_t *operation,
     if( PSA_ALG_IS_HMAC( operation->alg ) )
     {
         return( psa_hmac_finish_internal( &operation->ctx.hmac,
-                                          mac, operation->mac_size ) );
+                                          mac, mac_size ) );
     }
     else
 #endif /* BUILTIN_ALG_HMAC */
     {
         /* This shouldn't happen if `operation` was initialized by
          * a setup function. */
+        (void) operation;
+        (void) mac;
+        (void) mac_size;
         return( PSA_ERROR_BAD_STATE );
     }
 }
@@ -493,13 +483,10 @@ static psa_status_t mac_sign_finish(
     if( operation->alg == 0 )
         return( PSA_ERROR_BAD_STATE );
 
-    if( ! operation->is_sign )
-        return( PSA_ERROR_BAD_STATE );
-
     status = mac_finish_internal( operation, mac, mac_size );
 
     if( status == PSA_SUCCESS )
-        *mac_length = operation->mac_size;
+        *mac_length = mac_size;
 
     return( status );
 }
@@ -515,16 +502,11 @@ static psa_status_t mac_verify_finish(
     if( operation->alg == 0 )
         return( PSA_ERROR_BAD_STATE );
 
-    if( operation->is_sign )
+    /* Consistency check: requested MAC length fits our local buffer */
-        return( PSA_ERROR_BAD_STATE );
+    if( mac_length > sizeof( actual_mac ) )
+        return( PSA_ERROR_INVALID_ARGUMENT );
 
-    if( operation->mac_size != mac_length )
+    status = mac_finish_internal( operation, actual_mac, mac_length );
-    {
-        status = PSA_ERROR_INVALID_SIGNATURE;
-        goto cleanup;
-    }
-
-    status = mac_finish_internal( operation, actual_mac, sizeof( actual_mac ) );
     if( status != PSA_SUCCESS )
         goto cleanup;
 

library/psa_crypto_mac.h

@@ -182,13 +182,15 @@ psa_status_t mbedtls_psa_mac_update(
  *
  * \param[in,out] operation Active MAC operation.
  * \param[out] mac          Buffer where the MAC value is to be written.
- * \param mac_size          Size of the \p mac buffer in bytes.
+ * \param mac_size          Output size requested for the MAC algorithm. The PSA
- * \param[out] mac_length   On success, the number of bytes
+ *                          core guarantees this is a valid MAC length for the
- *                          that make up the MAC value. This is always
+ *                          algorithm and key combination passed to
- *                          #PSA_MAC_LENGTH(\c key_type, \c key_bits, \c alg)
+ *                          mbedtls_psa_mac_sign_setup(). It also guarantees the
- *                          where \c key_type and \c key_bits are the type and
+ *                          \p mac buffer is large enough to contain the
- *                          bit-size respectively of the key and \c alg is the
+ *                          requested output size.
- *                          MAC algorithm that is calculated.
+ * \param[out] mac_length   On success, the number of bytes output to buffer
+ *                          \p mac, which will be equal to the requested length
+ *                          \p mac_size.
  *
  * \retval #PSA_SUCCESS
  *         Success.
@@ -226,7 +228,10 @@ psa_status_t mbedtls_psa_mac_sign_finish(
  *
  * \param[in,out] operation Active MAC operation.
  * \param[in] mac           Buffer containing the expected MAC value.
- * \param mac_length        Size of the \p mac buffer in bytes.
+ * \param mac_length        Length in bytes of the expected MAC value. The PSA
+ *                          core guarantees that this length is a valid MAC
+ *                          length for the algorithm and key combination passed
+ *                          to mbedtls_psa_mac_verify_setup().
  *
  * \retval #PSA_SUCCESS
  *         The expected MAC is identical to the actual MAC of the message.