fix(crypto): make aes functions reject negative length
What changed, and why it matters
This commit fixes a bug in Trezor's AES encryption/decryption code where a negative length value could be passed to low-level AES functions. Previously, those functions treated the signed length as a large unsigned number, which could cause them to read or write far beyond the intended memory buffer. The patch makes every AES mode reject negative lengths and adds error handling so callers fail safely instead of silently producing bad results or touching memory they shouldn't.
Treat this as a security hardening fix with possible memory-safety implications. Review whether any Trezor message handler or Python API could previously supply a negative length (e.g., from malformed protobuf fields or MicroPython buffer objects) and confirm the new error paths are reachable. Run the added `test_aes_negative_length` tests and consider fuzzing the AES bindings with edge-case lengths.
Security signals we found
Signed/unsigned confusion on length parameter in cryptographic primitives
Missing bounds check allowing negative length to bypass alignment validation
Potential out-of-bounds read/write in AES mode loops
Return-value propagation added to callers (defense-in-depth)
New unit tests specifically targeting negative length rejection
Evidence from the diff
The AES mode routines in crypto/aes/aes_modes.c take an int len parameter. Before the patch, only non-alignment was checked (len & (AES_BLOCK_SIZE - 1)); a negative value passed that check because two’s-complement bitwise AND does not detect sign. The code then computed nb = len >> AES_BLOCK_SIZE_P2 and looped while(nb--); for negative len, right-shifting a negative signed integer is implementation-defined but typically produces a negative nb, causing the loop condition to be true for a very large number of iterations and reading/writing past buffer bounds. The patch adds len < 0 checks to aes_ecb_encrypt, aes_ecb_decrypt, aes_cbc_encrypt, aes_cbc_decrypt, aes_cfb_encrypt, aes_cfb_decrypt, aes_ofb_crypt, and aes_ctr_crypt, returning EXIT_FAILURE. It also updates MicroPython bindings and the legacy firmware CipherKeyValue handler to propagate these failures, and adds unit tests for negative lengths including INT_MIN.
Changed components
crypto/aes/aes_modes.ccore/embed/upymod/modtrezorcrypto/modtrezorcrypto-aes.hlegacy/firmware/fsm_msg_crypto.hcrypto/tests/test_check.cInspect captured patch +143 / −50
### core/embed/upymod/modtrezorcrypto/modtrezorcrypto-aes.h
@@ -112,6 +112,7 @@ static mp_obj_t aes_update(mp_obj_t self, mp_obj_t data, bool encrypt) {
return mp_const_empty_bytes;
}
vstr_t vstr = {0};
+ int ret = 0;
vstr_init_len(&vstr, buf.len);
mp_obj_AES_t *o = MP_OBJ_TO_PTR(self);
switch (o->mode) {
@@ -120,43 +121,46 @@ static mp_obj_t aes_update(mp_obj_t self, mp_obj_t data, bool encrypt) {
mp_raise_ValueError(MP_ERROR_TEXT("Invalid data length"));
}
if (encrypt) {
- aes_ecb_encrypt(buf.buf, (uint8_t *)vstr.buf, buf.len,
- &(o->encrypt_ctx));
+ ret = aes_ecb_encrypt(buf.buf, (uint8_t *)vstr.buf, buf.len,
+ &(o->encrypt_ctx));
} else {
- aes_ecb_decrypt(buf.buf, (uint8_t *)vstr.buf, buf.len,
- &(o->decrypt_ctx));
+ ret = aes_ecb_decrypt(buf.buf, (uint8_t *)vstr.buf, buf.len,
+ &(o->decrypt_ctx));
}
break;
case CBC:
if (buf.len & (AES_BLOCK_SIZE - 1)) {
mp_raise_ValueError(MP_ERROR_TEXT("Invalid data length"));
}
if (encrypt) {
- aes_cbc_encrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
- &(o->encrypt_ctx));
+ ret = aes_cbc_encrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
+ &(o->encrypt_ctx));
} else {
- aes_cbc_decrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
- &(o->decrypt_ctx));
+ ret = aes_cbc_decrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
+ &(o->decrypt_ctx));
}
break;
case CFB:
if (encrypt) {
- aes_cfb_encrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
- &(o->encrypt_ctx));
+ ret = aes_cfb_encrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
+ &(o->encrypt_ctx));
} else {
- aes_cfb_decrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
- &(o->encrypt_ctx)); // decrypt uses encrypt_ctx
+ ret = aes_cfb_decrypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
+ &(o->encrypt_ctx)); // decrypt uses encrypt_ctx
}
break;
case OFB: // (encrypt == decrypt)
- aes_ofb_crypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
- &(o->encrypt_ctx));
+ ret = aes_ofb_crypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
+ &(o->encrypt_ctx));
break;
case CTR: // (encrypt == decrypt)
- aes_ctr_crypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
- aes_ctr_cbuf_inc, &(o->encrypt_ctx));
+ ret = aes_ctr_crypt(buf.buf, (uint8_t *)vstr.buf, buf.len, o->iv,
+ aes_ctr_cbuf_inc, &(o->encrypt_ctx));
break;
}
+ if (ret != 0) {
+ mp_raise_type(&mp_type_RuntimeError);
+ }
return mp_obj_new_bytes_from_vstr(&vstr);
}
### crypto/aes/aes_modes.c
@@ -136,11 +136,13 @@ AES_RETURN aes_mode_reset(aes_encrypt_ctx ctx[1])
AES_RETURN aes_ecb_encrypt(const unsigned char *ibuf, unsigned char *obuf,
int len, const aes_encrypt_ctx ctx[1])
-{ int nb = len >> AES_BLOCK_SIZE_P2;
+{ int nb = 0;
- if(len & (AES_BLOCK_SIZE - 1))
+ if(len < 0 || (len & (AES_BLOCK_SIZE - 1)))
return EXIT_FAILURE;
+ nb = len >> AES_BLOCK_SIZE_P2;
+
#if defined( USE_VIA_ACE_IF_PRESENT )
if(ctx->inf.b[1] == 0xff)
@@ -198,11 +200,13 @@ AES_RETURN aes_ecb_encrypt(const unsigned char *ibuf, unsigned char *obuf,
AES_RETURN aes_ecb_decrypt(const unsigned char *ibuf, unsigned char *obuf,
int len, const aes_decrypt_ctx ctx[1])
-{ int nb = len >> AES_BLOCK_SIZE_P2;
+{ int nb = 0;
- if(len & (AES_BLOCK_SIZE - 1))
+ if(len < 0 || (len & (AES_BLOCK_SIZE - 1)))
return EXIT_FAILURE;
+ nb = len >> AES_BLOCK_SIZE_P2;
+
#if defined( USE_VIA_ACE_IF_PRESENT )
if(ctx->inf.b[1] == 0xff)
@@ -260,11 +264,13 @@ AES_RETURN aes_ecb_decrypt(const unsigned char *ibuf, unsigned char *obuf,
AES_RETURN aes_cbc_encrypt(const unsigned char *ibuf, unsigned char *obuf,
int len, unsigned char *iv, const aes_encrypt_ctx ctx[1])
-{ int nb = len >> AES_BLOCK_SIZE_P2;
+{ int nb = 0;
- if(len & (AES_BLOCK_SIZE - 1))
+ if(len < 0 || (len & (AES_BLOCK_SIZE - 1)))
return EXIT_FAILURE;
+ nb = len >> AES_BLOCK_SIZE_P2;
+
#if defined( USE_VIA_ACE_IF_PRESENT )
if(ctx->inf.b[1] == 0xff)
@@ -358,11 +364,13 @@ AES_RETURN aes_cbc_encrypt(const unsigned char *ibuf, unsigned char *obuf,
AES_RETURN aes_cbc_decrypt(const unsigned char *ibuf, unsigned char *obuf,
int len, unsigned char *iv, const aes_decrypt_ctx ctx[1])
{ unsigned char tmp[AES_BLOCK_SIZE] = {0};
- int nb = len >> AES_BLOCK_SIZE_P2;
+ int nb = 0;
- if(len & (AES_BLOCK_SIZE - 1))
+ if(len < 0 || (len & (AES_BLOCK_SIZE - 1)))
return EXIT_FAILURE;
+ nb = len >> AES_BLOCK_SIZE_P2;
+
#if defined( USE_VIA_ACE_IF_PRESENT )
if(ctx->inf.b[1] == 0xff)
@@ -458,6 +466,9 @@ AES_RETURN aes_cfb_encrypt(const unsigned char *ibuf, unsigned char *obuf,
int len, unsigned char *iv, aes_encrypt_ctx ctx[1])
{ int cnt = 0, b_pos = (int)ctx->inf.b[2], nb = 0;
+ if(len < 0)
+ return EXIT_FAILURE;
+
if(b_pos) /* complete any partial block */
{
while(b_pos < AES_BLOCK_SIZE && cnt < len)
@@ -583,6 +594,9 @@ AES_RETURN aes_cfb_decrypt(const unsigned char *ibuf, unsigned char *obuf,
int len, unsigned char *iv, aes_encrypt_ctx ctx[1])
{ int cnt = 0, b_pos = (int)ctx->inf.b[2], nb = 0;
+ if(len < 0)
+ return EXIT_FAILURE;
+
if(b_pos) /* complete any partial block */
{ uint8_t t = 0;
@@ -724,6 +738,9 @@ AES_RETURN aes_ofb_crypt(const unsigned char *ibuf, unsigned char *obuf,
int len, unsigned char *iv, aes_encrypt_ctx ctx[1])
{ int cnt = 0, b_pos = (int)ctx->inf.b[2], nb = 0;
+ if(len < 0)
+ return EXIT_FAILURE;
+
if(b_pos) /* complete any partial block */
{
while(b_pos < AES_BLOCK_SIZE && cnt < len)
@@ -860,6 +877,9 @@ AES_RETURN aes_ctr_crypt(const unsigned char *ibuf, unsigned char *obuf,
uint8_t buf[BFR_LENGTH] = {0};
#endif
+ if(len < 0)
+ return EXIT_FAILURE;
+
if(b_pos)
{
memcpy(buf, cbuf, AES_BLOCK_SIZE);
### crypto/tests/test_check.c
@@ -24,6 +24,7 @@
#include <assert.h>
#include <check.h>
#include <inttypes.h>
+#include <limits.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
@@ -4518,6 +4519,7 @@ START_TEST(test_aes) {
aes_decrypt_ctx ctxd;
uint8_t ibuf[16], obuf[16], iv[16], cbuf[16];
const char **ivp, **plainp, **cipherp;
+ int res = 0;
// ECB
static const char *ecb_vector[] = {
@@ -4537,20 +4539,24 @@ START_TEST(test_aes) {
cipherp = ecb_vector + 1;
while (*plainp && *cipherp) {
// encrypt
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(ibuf, fromhex(*plainp), 16);
- aes_ecb_encrypt(ibuf, obuf, 16, &ctxe);
+ res = aes_ecb_encrypt(ibuf, obuf, 16, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*cipherp), 16);
// decrypt
- aes_decrypt_key256(
+ res = aes_decrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxd);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(ibuf, fromhex(*cipherp), 16);
- aes_ecb_decrypt(ibuf, obuf, 16, &ctxd);
+ res = aes_ecb_decrypt(ibuf, obuf, 16, &ctxd);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*plainp), 16);
plainp += 2;
cipherp += 2;
@@ -4580,22 +4586,26 @@ START_TEST(test_aes) {
cipherp = cbc_vector + 2;
while (*plainp && *cipherp) {
// encrypt
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(iv, fromhex(*ivp), 16);
memcpy(ibuf, fromhex(*plainp), 16);
- aes_cbc_encrypt(ibuf, obuf, 16, iv, &ctxe);
+ res = aes_cbc_encrypt(ibuf, obuf, 16, iv, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*cipherp), 16);
// decrypt
- aes_decrypt_key256(
+ res = aes_decrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxd);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(iv, fromhex(*ivp), 16);
memcpy(ibuf, fromhex(*cipherp), 16);
- aes_cbc_decrypt(ibuf, obuf, 16, iv, &ctxd);
+ res = aes_cbc_decrypt(ibuf, obuf, 16, iv, &ctxd);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*plainp), 16);
ivp += 3;
plainp += 3;
@@ -4625,22 +4635,26 @@ START_TEST(test_aes) {
cipherp = cfb_vector + 2;
while (*plainp && *cipherp) {
// encrypt
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(iv, fromhex(*ivp), 16);
memcpy(ibuf, fromhex(*plainp), 16);
- aes_cfb_encrypt(ibuf, obuf, 16, iv, &ctxe);
+ res = aes_cfb_encrypt(ibuf, obuf, 16, iv, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*cipherp), 16);
// decrypt (uses encryption)
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(iv, fromhex(*ivp), 16);
memcpy(ibuf, fromhex(*cipherp), 16);
- aes_cfb_decrypt(ibuf, obuf, 16, iv, &ctxe);
+ res = aes_cfb_decrypt(ibuf, obuf, 16, iv, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*plainp), 16);
ivp += 3;
plainp += 3;
@@ -4670,22 +4684,26 @@ START_TEST(test_aes) {
cipherp = ofb_vector + 2;
while (*plainp && *cipherp) {
// encrypt
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(iv, fromhex(*ivp), 16);
memcpy(ibuf, fromhex(*plainp), 16);
- aes_ofb_encrypt(ibuf, obuf, 16, iv, &ctxe);
+ res = aes_ofb_encrypt(ibuf, obuf, 16, iv, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*cipherp), 16);
// decrypt (uses encryption)
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
memcpy(iv, fromhex(*ivp), 16);
memcpy(ibuf, fromhex(*cipherp), 16);
- aes_ofb_decrypt(ibuf, obuf, 16, iv, &ctxe);
+ res = aes_ofb_decrypt(ibuf, obuf, 16, iv, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*plainp), 16);
ivp += 3;
plainp += 3;
@@ -4710,13 +4728,15 @@ START_TEST(test_aes) {
plainp = ctr_vector;
cipherp = ctr_vector + 1;
memcpy(cbuf, fromhex("f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff"), 16);
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
while (*plainp && *cipherp) {
memcpy(ibuf, fromhex(*plainp), 16);
- aes_ctr_encrypt(ibuf, obuf, 16, cbuf, aes_ctr_cbuf_inc, &ctxe);
+ res = aes_ctr_encrypt(ibuf, obuf, 16, cbuf, aes_ctr_cbuf_inc, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*cipherp), 16);
plainp += 2;
cipherp += 2;
@@ -4725,20 +4745,61 @@ START_TEST(test_aes) {
plainp = ctr_vector;
cipherp = ctr_vector + 1;
memcpy(cbuf, fromhex("f0f1f2f3f4f5f6f7f8f9fafbfcfdfeff"), 16);
- aes_encrypt_key256(
+ res = aes_encrypt_key256(
fromhex(
"603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"),
&ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
while (*plainp && *cipherp) {
memcpy(ibuf, fromhex(*cipherp), 16);
- aes_ctr_decrypt(ibuf, obuf, 16, cbuf, aes_ctr_cbuf_inc, &ctxe);
+ res = aes_ctr_decrypt(ibuf, obuf, 16, cbuf, aes_ctr_cbuf_inc, &ctxe);
+ ck_assert_int_eq(res, EXIT_SUCCESS);
ck_assert_mem_eq(obuf, fromhex(*plainp), 16);
plainp += 2;
cipherp += 2;
}
}
END_TEST
+// the AES mode functions take a signed length parameter,
+// negative values have to be rejected
+START_TEST(test_aes_negative_length) {
+ aes_encrypt_ctx ctxe;
+ aes_decrypt_ctx ctxd;
+ uint8_t ibuf[16] = {0};
+ uint8_t obuf[16] = {0};
+ uint8_t iv[16] = {0};
+ uint8_t cbuf[16] = {0};
+
+ // -16 is a negative multiple of AES_BLOCK_SIZE
+ static const int lengths[] = {-1, -16, INT_MIN};
+
+ const uint8_t *key = fromhex(
+ "603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4");
+ ck_assert_int_eq(aes_encrypt_key256(key, &ctxe), EXIT_SUCCESS);
+ ck_assert_int_eq(aes_decrypt_key256(key, &ctxd), EXIT_SUCCESS);
+
+ for (size_t i = 0; i < sizeof(lengths) / sizeof(lengths[0]); i++) {
+ int len = lengths[i];
+
+ ck_assert_int_eq(aes_ecb_encrypt(ibuf, obuf, len, &ctxe), EXIT_FAILURE);
+ ck_assert_int_eq(aes_ecb_decrypt(ibuf, obuf, len, &ctxd), EXIT_FAILURE);
+ ck_assert_int_eq(aes_cbc_encrypt(ibuf, obuf, len, iv, &ctxe), EXIT_FAILURE);
+ ck_assert_int_eq(aes_cbc_decrypt(ibuf, obuf, len, iv, &ctxd), EXIT_FAILURE);
+ ck_assert_int_eq(aes_cfb_encrypt(ibuf, obuf, len, iv, &ctxe), EXIT_FAILURE);
+ ck_assert_int_eq(aes_cfb_decrypt(ibuf, obuf, len, iv, &ctxe), EXIT_FAILURE);
+ ck_assert_int_eq(aes_ofb_encrypt(ibuf, obuf, len, iv, &ctxe), EXIT_FAILURE);
+ ck_assert_int_eq(aes_ofb_decrypt(ibuf, obuf, len, iv, &ctxe), EXIT_FAILURE);
+ ck_assert_int_eq(
+ aes_ctr_encrypt(ibuf, obuf, len, cbuf, aes_ctr_cbuf_inc, &ctxe),
+ EXIT_FAILURE);
+ ck_assert_int_eq(
+ aes_ctr_decrypt(ibuf, obuf, len, cbuf, aes_ctr_cbuf_inc, &ctxe),
+ EXIT_FAILURE);
+ }
+}
+END_TEST
+
static void test_ecdh_multiply_helper(
int (*ecdh_multiply_fn)(const ecdsa_curve *curve, const uint8_t *priv_key,
const uint8_t *pub_key, uint8_t *session_key)) {
@@ -12520,6 +12581,7 @@ Suite *test_suite(void) {
tc = tcase_create("aes");
tcase_add_test(tc, test_aes);
+ tcase_add_test(tc, test_aes_negative_length);
suite_add_tcase(s, tc);
tc = tcase_create("aes_ccm");
### legacy/firmware/fsm_msg_crypto.h
@@ -54,16 +54,23 @@ void fsm_msgCipherKeyValue(const CipherKeyValue *msg) {
}
RESP_INIT(CipheredKeyValue);
+ bool ok = false;
if (encrypt) {
aes_encrypt_ctx ctx;
- aes_encrypt_key256(data, &ctx);
- aes_cbc_encrypt(msg->value.bytes, resp->value.bytes, msg->value.size,
- data + 32, &ctx);
+ ok = aes_encrypt_key256(data, &ctx) == EXIT_SUCCESS &&
+ aes_cbc_encrypt(msg->value.bytes, resp->value.bytes, msg->value.size,
+ data + 32, &ctx) == EXIT_SUCCESS;
} else {
aes_decrypt_ctx ctx;
- aes_decrypt_key256(data, &ctx);
- aes_cbc_decrypt(msg->value.bytes, resp->value.bytes, msg->value.size,
- data + 32, &ctx);
+ ok = aes_decrypt_key256(data, &ctx) == EXIT_SUCCESS &&
+ aes_cbc_decrypt(msg->value.bytes, resp->value.bytes, msg->value.size,
+ data + 32, &ctx) == EXIT_SUCCESS;
+ }
+ if (!ok) {
+ fsm_sendFailure(FailureType_Failure_ProcessError,
+ _("Failed to cipher key value"));
+ layoutHome();
+ return;
}
resp->value.size = msg->value.size;
msg_write(MessageType_MessageType_CipheredKeyValue, resp);Why this scored 47/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.