Subtle and unnecessary Variable Length Array

[mped-oticon]

This issue is born out of real-world usage, from the Zephyr project where work is ongoing porting to new platform and compliance cleanup.

A VLA is used in tinycrypt/source/hmac.c :: tc_hmac_set_key(), dummy_key. This is unfortunate for a number of reasons, listed later below.

My understanding: It appears that dummy_key is written when key_size <= TC_SHA256_BLOCK_SIZE and timing between the two branches should be nearly the same to cover up the sidechannel mentioned therein. Normally, key_size == TC_SHA256_BLOCK_SIZE and dummy_key will be written and thrown away. That makes sense, as we only want the timing behavior from tc_sha256_* in the then-branch. Fine. Call to rekey is what matters.

Request:

• Have dummy_key statically sized, to the worst-case (= the usual case) size of TC_SHA256_BLOCK_SIZE as per the patch below.
• Nice to have: Limit scoping of dummy_key and dummy_state to the then-branch.

This patch should improve safety too, as 1) the total stack frame usage can now better be analyzed, 2) some compilers may ignore that dummy_key is only written guarded by key_size <= TC_SHA256_BLOCK_SIZE since it is declared in the scope outside. So 2 means that stack overflow could happen if key_size was big, even if guarded.

Variable length arrays have multiple issues:

1. Insecure: No way to check for stack overflow. Different platforms have different stack sizes. tinycrypt is very appealing for embedded platforms, yet many of these have very limited stack sizes.
2. Exists only in C99. VLAs were made optional in C11.
3. Limits portability.

Patch:

diff --git a/ext/lib/crypto/tinycrypt/source/hmac.c b/ext/lib/crypto/tinycrypt/source/hmac.c
index 89878cec7..6e65ae717 100644
--- a/ext/lib/crypto/tinycrypt/source/hmac.c
+++ b/ext/lib/crypto/tinycrypt/source/hmac.c
@@ -63 +63 @@ int tc_hmac_set_key(TCHmacState_t ctx, const uint8_t *key,
-       const uint8_t dummy_key[key_size];
+       const uint8_t dummy_key[TC_SHA256_BLOCK_SIZE];

I'll also be happy to submit a PR.

[mped-oticon]

Should I submit a pull request?

[mczraf]

@mped-oticon, I agree with your suggestion. Would appreciate if you submit a pull request replacing the variable length array to a fixed length array. Thanks

[mped-oticon]

@mped-oticon, I agree with your suggestion. Would appreciate if you submit a pull request replacing the variable length array to a fixed length array. Thanks

Done :) Sorry it took so long.