From f686594e5af73a9365a2baa23dd0e29255a35571 Mon Sep 17 00:00:00 2001 From: Jade Philipoom Date: Mon, 6 Jan 2025 17:10:45 +0100 Subject: [PATCH] [crypto] Fix the KMAC-KDF implementation to return the output in shares. Previously, the KMAC driver always unmasked the shares of the digest returned by the KMAC block. This is OK for hash functions like SHA-3, but problematic for KMAC-KDF where the output is key material. The implementation also did not modify the second half of the keyblob buffer in KMAC-KDF, which would cause correctness issues if those values were not initialized to zero. This commit refactors the KMAC driver to allow returning the digest in masked form, and adjusts KMAC-KDF to use this new capability. Signed-off-by: Jade Philipoom --- sw/device/lib/crypto/drivers/kmac.c | 91 +++++++++++++------ sw/device/lib/crypto/drivers/kmac.h | 28 ++++-- sw/device/lib/crypto/impl/kdf.c | 16 ++-- sw/device/lib/crypto/impl/mac.c | 14 +-- sw/device/tests/crypto/BUILD | 1 + sw/device/tests/crypto/kdf_kmac_functest.c | 44 +++++---- sw/device/tests/crypto/kdf_set_testvectors.py | 4 +- sw/device/tests/crypto/kdf_testvectors.h.tpl | 16 ++-- 8 files changed, 130 insertions(+), 84 deletions(-) diff --git a/sw/device/lib/crypto/drivers/kmac.c b/sw/device/lib/crypto/drivers/kmac.c index 10260cb2f057d..283e8d99b0930 100644 --- a/sw/device/lib/crypto/drivers/kmac.c +++ b/sw/device/lib/crypto/drivers/kmac.c @@ -595,20 +595,23 @@ static status_t kmac_write_key_block(kmac_blinded_key_t *key) { * The caller must ensure that `message_len` bytes (rounded up to the next 32b * word) are allocated at the location pointed to by `message`, and similarly * that `digest_len_words` 32-bit words are allocated at the location pointed - * to by `digest`. + * to by `digest`. If `masked_digest` is set, then `digest` must contain 2x + * `digest_len_words` to fit both shares. * * @param operation The operation type. * @param message Input message string. * @param message_len Message length in bytes. * @param digest The struct to which the result will be written. * @param digest_len_words Requested digest length in 32-bit words. + * @param masked_digest Whether to return the digest in two shares. * @return Error code. */ OT_WARN_UNUSED_RESULT static status_t kmac_process_msg_blocks(kmac_operation_t operation, const uint8_t *message, size_t message_len, uint32_t *digest, - size_t digest_len_words) { + size_t digest_len_words, + hardened_bool_t masked_digest) { // Block until KMAC is idle. HARDENED_TRY(wait_status_bit(KMAC_STATUS_SHA3_IDLE_BIT, 1)); @@ -691,14 +694,38 @@ static status_t kmac_process_msg_blocks(kmac_operation_t operation, // Read words from the state registers (either `digest_len_words` or the // maximum number of words available). size_t offset = 0; - for (; launder32(idx) < digest_len_words && offset < keccak_rate_words; - offset++) { - uint32_t share0 = - abs_mmio_read32(kKmacStateShare0Addr + offset * sizeof(uint32_t)); - uint32_t share1 = - abs_mmio_read32(kKmacStateShare1Addr + offset * sizeof(uint32_t)); - digest[idx] = share0 ^ share1; - ++idx; + if (launder32(masked_digest) == kHardenedBoolTrue) { + HARDENED_CHECK_EQ(masked_digest, kHardenedBoolTrue); + // Read the digest into each share in turn. Do this in separate loops so + // corresponding shares aren't handled close together. + for (offset = 0; launder32(idx + offset) < digest_len_words && + offset < keccak_rate_words; + offset++) { + digest[idx + offset] = + abs_mmio_read32(kKmacStateShare0Addr + offset * sizeof(uint32_t)); + } + for (offset = 0; launder32(idx + offset) < digest_len_words && + offset < keccak_rate_words; + offset++) { + digest[idx + offset + digest_len_words] = + abs_mmio_read32(kKmacStateShare1Addr + offset * sizeof(uint32_t)); + } + idx += offset; + } else { + // Skip right to the hardened check here instead of returning + // `OTCRYPTO_BAD_ARGS` if the value is not `kHardenedBoolFalse`; this + // value always comes from within the cryptolib, so we expect it to be + // valid and should be suspicious if it's not. + HARDENED_CHECK_EQ(masked_digest, kHardenedBoolFalse); + // Unmask the digest as we read it. + for (; launder32(idx) < digest_len_words && offset < keccak_rate_words; + offset++) { + digest[idx] = + abs_mmio_read32(kKmacStateShare0Addr + offset * sizeof(uint32_t)); + digest[idx] ^= + abs_mmio_read32(kKmacStateShare1Addr + offset * sizeof(uint32_t)); + idx++; + } } // If we read all the remaining words and still need more digest, issue @@ -732,7 +759,8 @@ status_t kmac_sha3_224(const uint8_t *message, size_t message_len, size_t digest_len_words = kSha3_224DigestWords; return kmac_process_msg_blocks(kKmacOperationSHA3, message, message_len, - digest, digest_len_words); + digest, digest_len_words, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_sha3_256(const uint8_t *message, size_t message_len, @@ -742,7 +770,8 @@ status_t kmac_sha3_256(const uint8_t *message, size_t message_len, size_t digest_len_words = kSha3_256DigestWords; return kmac_process_msg_blocks(kKmacOperationSHA3, message, message_len, - digest, digest_len_words); + digest, digest_len_words, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_sha3_384(const uint8_t *message, size_t message_len, @@ -752,7 +781,8 @@ status_t kmac_sha3_384(const uint8_t *message, size_t message_len, size_t digest_len_words = kSha3_384DigestWords; return kmac_process_msg_blocks(kKmacOperationSHA3, message, message_len, - digest, digest_len_words); + digest, digest_len_words, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_sha3_512(const uint8_t *message, size_t message_len, @@ -762,7 +792,8 @@ status_t kmac_sha3_512(const uint8_t *message, size_t message_len, size_t digest_len_words = kSha3_512DigestWords; return kmac_process_msg_blocks(kKmacOperationSHA3, message, message_len, - digest, digest_len_words); + digest, digest_len_words, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_shake_128(const uint8_t *message, size_t message_len, @@ -771,7 +802,8 @@ status_t kmac_shake_128(const uint8_t *message, size_t message_len, /*hw_backed=*/kHardenedBoolFalse)); return kmac_process_msg_blocks(kKmacOperationSHAKE, message, message_len, - digest, digest_len); + digest, digest_len, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_shake_256(const uint8_t *message, size_t message_len, @@ -780,7 +812,8 @@ status_t kmac_shake_256(const uint8_t *message, size_t message_len, /*hw_backed=*/kHardenedBoolFalse)); return kmac_process_msg_blocks(kKmacOperationSHAKE, message, message_len, - digest, digest_len); + digest, digest_len, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_cshake_128(const uint8_t *message, size_t message_len, @@ -794,7 +827,8 @@ status_t kmac_cshake_128(const uint8_t *message, size_t message_len, func_name_len, cust_str, cust_str_len)); return kmac_process_msg_blocks(kKmacOperationCSHAKE, message, message_len, - digest, digest_len); + digest, digest_len, + /*masked_digest=*/kHardenedBoolFalse); } status_t kmac_cshake_256(const uint8_t *message, size_t message_len, @@ -808,13 +842,14 @@ status_t kmac_cshake_256(const uint8_t *message, size_t message_len, func_name_len, cust_str, cust_str_len)); return kmac_process_msg_blocks(kKmacOperationCSHAKE, message, message_len, - digest, digest_len); + digest, digest_len, + /*masked_digest=*/kHardenedBoolFalse); } -status_t kmac_kmac_128(kmac_blinded_key_t *key, const uint8_t *message, - size_t message_len, const unsigned char *cust_str, - size_t cust_str_len, uint32_t *digest, - size_t digest_len) { +status_t kmac_kmac_128(kmac_blinded_key_t *key, hardened_bool_t masked_digest, + const uint8_t *message, size_t message_len, + const unsigned char *cust_str, size_t cust_str_len, + uint32_t *digest, size_t digest_len) { HARDENED_TRY( kmac_init(kKmacOperationKMAC, kKmacSecurityStrength128, key->hw_backed)); @@ -837,13 +872,13 @@ status_t kmac_kmac_128(kmac_blinded_key_t *key, const uint8_t *message, cust_str_len)); return kmac_process_msg_blocks(kKmacOperationKMAC, message, message_len, - digest, digest_len); + digest, digest_len, masked_digest); } -status_t kmac_kmac_256(kmac_blinded_key_t *key, const uint8_t *message, - size_t message_len, const unsigned char *cust_str, - size_t cust_str_len, uint32_t *digest, - size_t digest_len) { +status_t kmac_kmac_256(kmac_blinded_key_t *key, hardened_bool_t masked_digest, + const uint8_t *message, size_t message_len, + const unsigned char *cust_str, size_t cust_str_len, + uint32_t *digest, size_t digest_len) { HARDENED_TRY( kmac_init(kKmacOperationKMAC, kKmacSecurityStrength256, key->hw_backed)); @@ -866,5 +901,5 @@ status_t kmac_kmac_256(kmac_blinded_key_t *key, const uint8_t *message, cust_str_len)); return kmac_process_msg_blocks(kKmacOperationKMAC, message, message_len, - digest, digest_len); + digest, digest_len, masked_digest); } diff --git a/sw/device/lib/crypto/drivers/kmac.h b/sw/device/lib/crypto/drivers/kmac.h index cfa421cd7c26d..898d41ec591af 100644 --- a/sw/device/lib/crypto/drivers/kmac.h +++ b/sw/device/lib/crypto/drivers/kmac.h @@ -240,8 +240,12 @@ status_t kmac_cshake_256(const uint8_t *message, size_t message_len, * pointers must be correctly configured and `len` must match the key length. * * The caller must ensure that `digest_len` words are allocated at the location - * pointed to by `digest`. `cust_str_len` must not exceed `kKmacCustStrMaxSize`. + * pointed to by `digest`. `cust_str_len` must not exceed + * `kKmacCustStrMaxSize`. If `masked_digest` is true, the `digest` buffer must + * have enough space for 2x `digest_len` words. * + * @param key The KMAC key. + * @param masked_digest Whether to return the digest in concatenated shares. * @param message The input message. * @param message_len The input message length in bytes. * @param cust_str The customization string. @@ -251,10 +255,10 @@ status_t kmac_cshake_256(const uint8_t *message, size_t message_len, * @return Error status. */ OT_WARN_UNUSED_RESULT -status_t kmac_kmac_128(kmac_blinded_key_t *key, const uint8_t *message, - size_t message_len, const unsigned char *cust_str, - size_t cust_str_len, uint32_t *digest, - size_t digest_len); +status_t kmac_kmac_128(kmac_blinded_key_t *key, hardened_bool_t masked_digest, + const uint8_t *message, size_t message_len, + const unsigned char *cust_str, size_t cust_str_len, + uint32_t *digest, size_t digest_len); /** * Compute KMAC-256 in one-shot. @@ -269,8 +273,12 @@ status_t kmac_kmac_128(kmac_blinded_key_t *key, const uint8_t *message, * pointers must be correctly configured and `len` must match the key length. * * The caller must ensure that `digest_len` words are allocated at the location - * pointed to by `digest`. `cust_str_len` must not exceed `kKmacCustStrMaxSize`. + * pointed to by `digest`. `cust_str_len` must not exceed + * `kKmacCustStrMaxSize`. If `masked_digest` is true, the `digest` buffer must + * have enough space for 2x `digest_len` words. * + * @param key The KMAC key. + * @param masked_digest Whether to return the digest in concatenated shares. * @param message The input message. * @param message_len The input message length in bytes. * @param cust_str The customization string. @@ -280,10 +288,10 @@ status_t kmac_kmac_128(kmac_blinded_key_t *key, const uint8_t *message, * @return Error status. */ OT_WARN_UNUSED_RESULT -status_t kmac_kmac_256(kmac_blinded_key_t *key, const uint8_t *message, - size_t message_len, const unsigned char *cust_str, - size_t cust_str_len, uint32_t *digest, - size_t digest_len); +status_t kmac_kmac_256(kmac_blinded_key_t *key, hardened_bool_t masked_digest, + const uint8_t *message, size_t message_len, + const unsigned char *cust_str, size_t cust_str_len, + uint32_t *digest, size_t digest_len); #ifdef __cplusplus } diff --git a/sw/device/lib/crypto/impl/kdf.c b/sw/device/lib/crypto/impl/kdf.c index 745a5e47239b6..43a77b8729de2 100644 --- a/sw/device/lib/crypto/impl/kdf.c +++ b/sw/device/lib/crypto/impl/kdf.c @@ -291,10 +291,10 @@ otcrypto_status_t otcrypto_kdf_kmac( } // No need to further check key size against security level because // `kmac_key_length_check` ensures that the key is at least 128-bit. - HARDENED_TRY(kmac_kmac_128(&kmac_key, kdf_context.data, kdf_context.len, - kdf_label.data, kdf_label.len, - keying_material->keyblob, - required_byte_len / sizeof(uint32_t))); + HARDENED_TRY(kmac_kmac_128( + &kmac_key, /*masked_digest=*/kHardenedBoolTrue, kdf_context.data, + kdf_context.len, kdf_label.data, kdf_label.len, + keying_material->keyblob, required_byte_len / sizeof(uint32_t))); } else if (kmac_mode == kOtcryptoKmacModeKmac256) { // Check if `key_mode` of the key derivation key matches `kmac_mode`. if (key_derivation_key.config.key_mode != kOtcryptoKeyModeKdfKmac256) { @@ -305,10 +305,10 @@ otcrypto_status_t otcrypto_kdf_kmac( if (key_derivation_key.config.key_length < 256 / 8) { return OTCRYPTO_BAD_ARGS; } - HARDENED_TRY(kmac_kmac_256(&kmac_key, kdf_context.data, kdf_context.len, - kdf_label.data, kdf_label.len, - keying_material->keyblob, - required_byte_len / sizeof(uint32_t))); + HARDENED_TRY(kmac_kmac_256( + &kmac_key, /*masked_digest=*/kHardenedBoolTrue, kdf_context.data, + kdf_context.len, kdf_label.data, kdf_label.len, + keying_material->keyblob, required_byte_len / sizeof(uint32_t))); } else { return OTCRYPTO_BAD_ARGS; } diff --git a/sw/device/lib/crypto/impl/mac.c b/sw/device/lib/crypto/impl/mac.c index edbd99e893e11..da74b1bfe2b63 100644 --- a/sw/device/lib/crypto/impl/mac.c +++ b/sw/device/lib/crypto/impl/mac.c @@ -279,9 +279,10 @@ otcrypto_status_t otcrypto_kmac(const otcrypto_blinded_key_t *key, if (key->config.key_mode != kOtcryptoKeyModeKmac128) { return OTCRYPTO_BAD_ARGS; } - HARDENED_TRY(kmac_kmac_128(&kmac_key, input_message.data, - input_message.len, customization_string.data, - customization_string.len, tag.data, tag.len)); + HARDENED_TRY(kmac_kmac_128( + &kmac_key, /*masked_digest=*/kHardenedBoolFalse, input_message.data, + input_message.len, customization_string.data, + customization_string.len, tag.data, tag.len)); break; case kOtcryptoKmacModeKmac256: // Check `key_mode` matches `mac_mode` @@ -289,9 +290,10 @@ otcrypto_status_t otcrypto_kmac(const otcrypto_blinded_key_t *key, return OTCRYPTO_BAD_ARGS; } - HARDENED_TRY(kmac_kmac_256(&kmac_key, input_message.data, - input_message.len, customization_string.data, - customization_string.len, tag.data, tag.len)); + HARDENED_TRY(kmac_kmac_256( + &kmac_key, /*masked_digest=*/kHardenedBoolFalse, input_message.data, + input_message.len, customization_string.data, + customization_string.len, tag.data, tag.len)); break; default: return OTCRYPTO_BAD_ARGS; diff --git a/sw/device/tests/crypto/BUILD b/sw/device/tests/crypto/BUILD index 99e1fd4b87b02..498f760d968d3 100644 --- a/sw/device/tests/crypto/BUILD +++ b/sw/device/tests/crypto/BUILD @@ -468,6 +468,7 @@ opentitan_test( "//sw/device/lib/crypto/drivers:kmac", "//sw/device/lib/crypto/impl:hash", "//sw/device/lib/crypto/impl:kdf", + "//sw/device/lib/crypto/impl:key_transport", "//sw/device/lib/crypto/impl:mac", "//sw/device/lib/testing/test_framework:check", "//sw/device/lib/testing/test_framework:ottf_main", diff --git a/sw/device/tests/crypto/kdf_kmac_functest.c b/sw/device/tests/crypto/kdf_kmac_functest.c index 4d0db2fa2c433..2549e6b4c127d 100644 --- a/sw/device/tests/crypto/kdf_kmac_functest.c +++ b/sw/device/tests/crypto/kdf_kmac_functest.c @@ -8,6 +8,7 @@ #include "sw/device/lib/crypto/include/datatypes.h" #include "sw/device/lib/crypto/include/hash.h" #include "sw/device/lib/crypto/include/kdf.h" +#include "sw/device/lib/crypto/include/key_transport.h" #include "sw/device/lib/crypto/include/mac.h" #include "sw/device/lib/runtime/log.h" #include "sw/device/lib/testing/test_framework/check.h" @@ -50,13 +51,8 @@ status_t get_kmac_mode(kdf_test_operation_t test_operation, static status_t run_test_vector(void) { // Below, `km` prefix refers to keying_material, and // `kdk` prefix refers to key derivation key - size_t km_key_length = current_test_vector->keying_material.config.key_length; - - size_t km_num_words = km_key_length / sizeof(uint32_t); - if (km_key_length % sizeof(uint32_t) != 0) { - km_num_words++; - } - uint32_t km_buffer[km_num_words]; + size_t km_num_words = current_test_vector->expected_output.len; + uint32_t km_buffer[2 * km_num_words]; otcrypto_kmac_mode_t mode; TRY(get_kmac_mode(current_test_vector->test_operation, &mode)); @@ -67,32 +63,42 @@ static status_t run_test_vector(void) { // The following key_mode is a dummy placeholder. It does not // necessarily match the `key_length`. .key_mode = kOtcryptoKeyModeKdfKmac128, - .key_length = km_key_length, + .key_length = km_num_words * sizeof(uint32_t), .hw_backed = kHardenedBoolFalse, .security_level = kOtcryptoKeySecurityLevelLow, .exportable = kHardenedBoolTrue, }, .keyblob = km_buffer, - .keyblob_length = 2 * km_key_length}; + .keyblob_length = sizeof(km_buffer), + }; // Populate `checksum` and `config.security_level` fields. current_test_vector->key_derivation_key.checksum = integrity_blinded_checksum(¤t_test_vector->key_derivation_key); - TRY(otcrypto_kdf_kmac( - current_test_vector->key_derivation_key, mode, current_test_vector->label, - current_test_vector->context, km_key_length, &keying_material)); + TRY(otcrypto_kdf_kmac(current_test_vector->key_derivation_key, mode, + current_test_vector->label, + current_test_vector->context, + keying_material.config.key_length, &keying_material)); HARDENED_CHECK_EQ(integrity_blinded_key_check(&keying_material), kHardenedBoolTrue); - // At the moment, the function `kmac_kmac_128` called under the hood does not - // return the digest in 2 shares. Therefore we can simply compare the first - // share of the result from the test vector, since the second share is always - // 0. - TRY_CHECK_ARRAYS_EQ((uint8_t *)keying_material.keyblob, - (uint8_t *)current_test_vector->keying_material.keyblob, - km_key_length); + // Export the derived blinded key. + uint32_t km_share0[km_num_words]; + uint32_t km_share1[km_num_words]; + TRY(otcrypto_export_blinded_key( + keying_material, + (otcrypto_word32_buf_t){.data = km_share0, .len = ARRAYSIZE(km_share0)}, + (otcrypto_word32_buf_t){.data = km_share1, .len = ARRAYSIZE(km_share1)})); + + // Unmask the derived key and compare to the expected value. + uint32_t actual_output[km_num_words]; + for (size_t i = 0; i < ARRAYSIZE(actual_output); i++) { + actual_output[i] = km_share0[i] ^ km_share1[i]; + } + TRY_CHECK_ARRAYS_EQ(actual_output, current_test_vector->expected_output.data, + km_num_words); return OTCRYPTO_OK; } diff --git a/sw/device/tests/crypto/kdf_set_testvectors.py b/sw/device/tests/crypto/kdf_set_testvectors.py index e00e106d2bbec..4936727d8f073 100755 --- a/sw/device/tests/crypto/kdf_set_testvectors.py +++ b/sw/device/tests/crypto/kdf_set_testvectors.py @@ -69,7 +69,6 @@ def main() -> int: t["key_derivation_key"] = t["key"] t["context"] = t["input_msg"] t["label"] = t["cust_str"] - t["keying_material"] = t["digest"] if t["security_str"] == 128: t["test_operation"] = "kKdfTestOperationKmac128" @@ -85,8 +84,7 @@ def main() -> int: t["keyblob"] += ["0x00000000"] * len(t["keyblob"]) # Derived key material also needs to have word granularity - t["km_keyblob"] = str_to_hex_array(t["keying_material"], return_byte_array = False) - t["km_keyblob"] += ["0x00000000"] * len(t["km_keyblob"]) + t["expected_output"] = str_to_hex_array(t["digest"], return_byte_array = False) args.headerfile.write(Template(args.template.read()).render(tests=testvecs)) args.headerfile.close() diff --git a/sw/device/tests/crypto/kdf_testvectors.h.tpl b/sw/device/tests/crypto/kdf_testvectors.h.tpl index c5a5ea79b0e04..3cf35b24b71a9 100644 --- a/sw/device/tests/crypto/kdf_testvectors.h.tpl +++ b/sw/device/tests/crypto/kdf_testvectors.h.tpl @@ -30,7 +30,7 @@ typedef struct kdf_test_vector { otcrypto_blinded_key_t key_derivation_key; otcrypto_const_byte_buf_t label; otcrypto_const_byte_buf_t context; - otcrypto_blinded_key_t keying_material; + otcrypto_word32_buf_t expected_output; } kdf_kmac_test_vector_t; static kdf_kmac_test_vector_t kKdfTestVectors[${len(tests)}] = { @@ -78,15 +78,11 @@ static kdf_kmac_test_vector_t kKdfTestVectors[${len(tests)}] = { .len = 0, % endif }, - .keying_material = { - .config = { - .key_length = ${2 * len(t["km_keyblob"])}, - .hw_backed = kHardenedBoolFalse, - }, - .keyblob_length = ${4 * len(t["km_keyblob"])}, - .keyblob = (uint32_t[]){ - % for i in range(0, len(t["km_keyblob"]), 4): - ${', '.join(t["km_keyblob"][i:i + 4])}, + .expected_output = { + .len = ${len(t["expected_output"])}, + .data = (uint32_t[]){ + % for i in range(0, len(t["expected_output"]), 4): + ${', '.join(t["expected_output"][i:i + 4])}, % endfor }, },