-
Notifications
You must be signed in to change notification settings - Fork 177
Subtle and unnecessary Variable Length Array #30
Description
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_keystatically sized, to the worst-case (= the usual case) size ofTC_SHA256_BLOCK_SIZEas per the patch below. - Nice to have: Limit scoping of
dummy_keyanddummy_stateto 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:
- 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.
- Exists only in C99. VLAs were made optional in C11.
- 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.