Driver-only hashes: PKCS5 #6244

AndrzejKurek · 2022-08-30T10:14:48Z

This PR adds all of the required changes to have the PKCS5 module working without MD.
Fixes #6145.

Test coverage comparison:

*** Comparing before-default -> after-default ***
          pk: total 168; skipped  92 ->  92
     pkparse: total 276; skipped  20 ->  20
     pkwrite: total  12; skipped   0 ->   0
       pkcs5: total  54; skipped   0 ->   0

*** Comparing before-full -> after-full ***
          pk: total 168; skipped   8 ->   8
     pkparse: total 276; skipped  20 ->  20
     pkwrite: total  12; skipped   0 ->   0
       pkcs5: total  54; skipped   0 ->   0

*** Comparing reference -> drivers ***
          pk: total 168; skipped  30 ->   8
     pkparse: total 276; skipped  20 ->  20
     pkwrite: total  12; skipped   0 ->   0
       pkcs5: total  54; skipped   0 ->   0

library/pkcs5.c

mpg

Looks good to me! Just left one suggestion for improvement. Let's hope the CI agrees :)

mpg · 2022-09-01T10:45:43Z

tests/suites/test_suite_pkcs5.function

@@ -14,12 +14,14 @@ void pbkdf2_hmac( int hash, data_t * pw_str, data_t * salt_str,
 {
    unsigned char key[100];

+    PSA_INIT();


This is OK but I think we could be even more precise: define a new macro PSA_INIT_IF_NO_MD() that only calls psa_crypto_init() if MD_C is not defined. That way, we can check that things still work without psa_crypto_init() when MD_C is defined, as the documentation say. Wdyt?

Right, that's a good thing to test. Do you think that it will be useful in other .function files, or just for pkcs5?

It seems likely to be used in other files. Actually, even in this PR, it's already used in pkparse too IIRC.

Code that only uses hashes should not require psa_crypto_init, which among other things requires entropy.

Code that only uses hashes should not require psa_crypto_init, which among other things requires entropy.

PBKDF2 uses HMAC, which in PSA requires the key store to be initialized unless I missed something.

Even it was just hashing, I don't think there's any point in the documentation right now that says you can use psa_hash_xxx() functions without calling psa_crypto_init() first, and I'd rather not rely on undocumented properties. (Also, isn't there some kind of driver initialization step that needs to happen when hashes are provided by drivers?)

So, if you're talking about the future, with split init and the parts that can be done transparently happening automatically under the hood, then yes, sure, but as far as this PR is concerned, I think right now we do need to call psa_crypto_init().

mprse

I did the first round of review and found few places for minor improvements.

mprse · 2022-09-01T09:31:31Z

include/mbedtls/config_psa.h

 #define MBEDTLS_PSA_BUILTIN_ALG_HMAC 1
 #define PSA_WANT_ALG_HMAC 1
 #define PSA_WANT_KEY_TYPE_HMAC
+
+#if defined(MBEDTLS_MD_C)


Is this ok that MBEDTLS_PSA_BUILTIN_ALG_HMAC, PSA_WANT_ALG_HMAC, PSA_WANT_KEY_TYPE_HMAC are always defined?

mprse · 2022-09-01T10:35:40Z

library/pkcs5.c

+{
+#if defined(MBEDTLS_MD_C)
+    mbedtls_md_context_t md_ctx;
+    const mbedtls_md_info_t *md_info;


For consistency I suggest to init md_info to NULL and ret to MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED.

mprse · 2022-09-01T10:37:40Z

library/pkcs5.c

+
+    md_info = mbedtls_md_info_from_type( md_alg );
+    if( md_info == NULL )
+        return( MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE );


md_ctx is already initalized, so we need to set ret to MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE and go to exit in order to free md_ctx or move this block before mbedtls_md_init().

Technically, we only need to call md_free() after md_setup() has been called, but we might still want to apply your suggestion for the sake of uniformity.

mprse · 2022-09-01T10:39:51Z

library/pkcs5.c

+    psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
+
+
+    memset( counter, 0, 4 );


Suggest to use sizeof( counter ) instead 4.

mprse · 2022-09-01T10:44:10Z

library/pkcs5.c

+    size_t use_len, out_len, out_size;
+    unsigned char *out_p = output;
+    unsigned char counter[4];
+    mbedtls_svc_key_id_t psa_hmac_key;


Suggest to init key to MBEDTLS_SVC_KEY_ID_INIT.

mprse · 2022-09-01T10:46:48Z

library/pkcs5.c

+        if( ( status = psa_mac_update( &operation, salt, slen ) ) != PSA_SUCCESS )
+            goto cleanup;
+
+        if( ( status = psa_mac_update( &operation, counter, 4 ) ) != PSA_SUCCESS )


Suggest to use sizeof( counter ) instead 4.

mprse · 2022-09-01T10:50:21Z

library/pkcs5.c

+    memset( counter, 0, 4 );
+    counter[3] = 1;
+    psa_algorithm_t alg = PSA_ALG_HMAC( mbedtls_hash_info_psa_from_md( md_alg ) );
+    out_size = PSA_MAC_LENGTH( PSA_KEY_TYPE_HMAC, 0, alg );


Suggest to declare out_size as const size_t.

mprse · 2022-09-01T10:51:29Z

library/pkcs5.c

+    unsigned int i;
+    unsigned char md1[PSA_HASH_MAX_SIZE];
+    unsigned char work[PSA_HASH_MAX_SIZE];
+    unsigned char md_size = mbedtls_hash_info_get_size( md_alg );


Suggest to declare md_size as const unsigned char.

mprse · 2022-09-01T10:56:19Z

library/pkcs5.c

+            != PSA_SUCCESS )
+            goto cleanup;
+
+        memcpy( md1, work, md_size );


I understand that md_size should be equal to out_len, but shouldn't we use out_len as this is the real size of work (returned by psa_mac_sign_finish)?

mprse · 2022-09-01T11:02:14Z

library/pkcs5.c

+    /* Zeroise buffers to clear sensitive data from memory. */
+    mbedtls_platform_zeroize( work, PSA_HASH_MAX_SIZE );
+    mbedtls_platform_zeroize( md1, PSA_HASH_MAX_SIZE );
+    psa_destroy_key( psa_hmac_key );


Status of key destruction is skipped.

mpg · 2022-09-02T08:01:00Z

(Also, this needs rebasing to resolve trivial conflicts now that the PKCS12 work has been merged.)

Use the new implementation locally Signed-off-by: Andrzej Kurek <andrzej.kurek@arm.com>