From 309b8279db7430b9ded5c4d8b9b6373ec5bb8789 Mon Sep 17 00:00:00 2001 From: Bohdan Date: Tue, 25 Aug 2026 17:35:55 +0300 Subject: [PATCH] fix(hotp): bound Non-Secure key size before copy into K[] register_shared_key() copied params[0].memref.size bytes into the fixed 64-byte secure buffer K[] without bounding the length. That size is supplied verbatim by the Non-Secure caller, so any NS caller could overflow a secure .bss buffer with attacker-controlled data. This is not covered by the pointer-attribution hardening in 27e314c: nsec_check() proves the source range is Non-Secure resident, which is a different property from "the source fits in the destination". A legitimate 4 KB Non-Secure buffer passes that check exactly as designed and is then copied into 64 bytes. The correct bounds check already existed in this file (MIN_KEY_SIZE / MAX_KEY_SIZE in hmac_sha1()) but ran on the GET_HOTP path, a later and separate invocation, long after the corruption. Move the identical rejection ahead of the copy. Reject rather than truncate, matching apps/aes/ta/aes_ta.c: a silently shortened HMAC key would be a cryptographic defect of its own. Not a compatibility break, since such a key already failed later at hmac_sha1(). Two further defects in the same code: - DMSG traced K with "%s": it printed the HMAC shared secret to the console, and a key of exactly sizeof(K) leaves no NUL for %s to stop at, so it also over-read past the array. Trace only the length. - get_hotp() called truncate_() without checking res from hmac_sha1(), truncating uninitialised stack when the HMAC failed. Verified under QEMU (mps2-an505, cortex-m33): sizes 4096, 65 and 4 are now rejected with TEE_ERROR_BAD_PARAMETERS, the 20-byte RFC4226 key still registers and GET_HOTP still yields the specified value 755224. With this patch reverted the 4096-byte request returns TEE_SUCCESS and the secure world then stops responding. Co-Authored-By: Claude Opus 5 (1M context) --- apps/hotp/ta/hotp_ta.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/apps/hotp/ta/hotp_ta.c b/apps/hotp/ta/hotp_ta.c index 9ef01ba2..3c0bcf6f 100644 --- a/apps/hotp/ta/hotp_ta.c +++ b/apps/hotp/ta/hotp_ta.c @@ -144,12 +144,36 @@ static TEE_Result register_shared_key(uint32_t param_types, TEE_Param params[4]) return TEE_ERROR_BAD_PARAMETERS; } + /* + * params[0] comes from the Non-Secure world: both the buffer pointer and + * the size are attacker-controlled. The secure-world boundary only + * guarantees the source range is Non-Secure resident -- it says nothing + * about how much of it fits in K, so the size must be bounded here + * before it is used as a memcpy() length. Reject rather than truncate: a + * silently shortened HMAC key would be a cryptographic bug of its own. + */ + if (!params[0].memref.buffer) + return TEE_ERROR_BAD_PARAMETERS; + + if (params[0].memref.size < MIN_KEY_SIZE || + params[0].memref.size > sizeof(K)) { + EMSG("Bad key size %u, expect %u..%u bytes", + (unsigned int)params[0].memref.size, + (unsigned int)MIN_KEY_SIZE, (unsigned int)sizeof(K)); + return TEE_ERROR_BAD_PARAMETERS; + } + memset(counter, 0, sizeof(counter)); memset(K, 0, sizeof(K)); memcpy(K, params[0].memref.buffer, params[0].memref.size); K_len = params[0].memref.size; - DMSG("Got shared key %s (%u bytes).", K, params[0].memref.size); + /* + * Do not trace K itself: it is the HMAC shared secret, and a key of + * exactly sizeof(K) bytes leaves no NUL for the "%s" to stop at. + */ + DMSG("Registered shared key (%u bytes).", + (unsigned int)params[0].memref.size); return res; } @@ -173,6 +197,10 @@ static TEE_Result get_hotp(uint32_t param_types, TEE_Param params[4]) } res = hmac_sha1(K, K_len, counter, sizeof(counter), mac, &mac_len); + if (res != TEE_SUCCESS) { + EMSG("hmac_sha1 failed: 0x%x", res); + return res; + } /* Increment the counter. */ for (i = sizeof(counter) - 1; i >= 0; i--) {