fix(legacy): Avoid creation of zero-length VLA.
What the developer wrote
Authored by Andrew Kozlik
fix(legacy): Avoid creation of zero-length VLA.
[no changelog]
What changed, and why it matters
This commit fixes a coding issue in the older Trezor firmware where a temporary memory buffer could be created with zero size when handling multi-signature Bitcoin wallets. The fix replaces a variable-length buffer with a fixed maximum-size buffer. A zero-length variable-length array is undefined behavior in C and could, in theory, be exploited to destabilize the device, though the commit itself does not describe a practical attack.
Treat as a hardening/defensive fix. Review whether any caller can pass a MultisigRedeemScriptType with zero pubkeys and confirm the existing guard in cryptoMultisigPubkeys() prevents zero-length VLA usage in practice. Consider adding an explicit n == 0 check in cryptoMultisigPubkeyIndex() for defense in depth. No urgent security patch is indicated by the diff alone.
Security signals we found
Zero-length variable-length array (VLA) removed
Magic number 15 replaced with named constant MAX_MULTISIG_PUBKEY_COUNT
Buffer sizing moved from runtime variable to compile-time maximum
Potential undefined behavior in C mitigated
Evidence from the diff
In legacy/firmware/crypto.c, cryptoMultisigPubkeyIndex() previously declared uint8_t pubkeys[33 * n] where n came from cryptoMultisigPubkeyCount(). If n were 0, this would create a zero-length VLA, which is undefined behavior in C and could lead to stack corruption or unexpected behavior. The patch replaces the VLA with a fixed-size buffer of 33 * MAX_MULTISIG_PUBKEY_COUNT (where MAX_MULTISIG_PUBKEY_COUNT is newly defined as 15), and moves the n lookup after the cryptoMultisigPubkeys() call. The patch also replaces magic number 15 with the new constant in related checks. The change is defensive and partial: it does not add runtime bounds checks beyond the existing n > 15 guard, and it does not address how a zero n would be handled in cryptoMultisigPubkeyIndex (it would still return -1 via cryptoMultisigPubkeys).
Changed components
legacy/firmware/crypto.ccryptoMultisigPubkeyIndex()cryptoMultisigPubkeys()cryptoMultisigFingerprint()Inspect captured patch +8 / −7
diff --git a/legacy/firmware/crypto.c b/legacy/firmware/crypto.c
index 19b5a9fd..60dca466 100644
--- a/legacy/firmware/crypto.c
+++ b/legacy/firmware/crypto.c
@@ -37,6 +37,8 @@
#include "cash_addr.h"
#endif
+#define MAX_MULTISIG_PUBKEY_COUNT 15
+
uint32_t ser_length(uint32_t len, uint8_t *out) {
if (len < 253) {
out[0] = len & 0xFF;
@@ -378,7 +380,7 @@ uint32_t cryptoMultisigPubkeys(const CoinInfo *coin,
const MultisigRedeemScriptType *multisig,
uint8_t *pubkeys) {
const uint32_t n = cryptoMultisigPubkeyCount(multisig);
- if (n < 1 || n > 15) {
+ if (n < 1 || n > MAX_MULTISIG_PUBKEY_COUNT) {
return 0;
}
@@ -401,13 +403,12 @@ uint32_t cryptoMultisigPubkeys(const CoinInfo *coin,
int cryptoMultisigPubkeyIndex(const CoinInfo *coin,
const MultisigRedeemScriptType *multisig,
const uint8_t *pubkey) {
- uint32_t n = cryptoMultisigPubkeyCount(multisig);
-
- uint8_t pubkeys[33 * n];
+ uint8_t pubkeys[33 * MAX_MULTISIG_PUBKEY_COUNT];
if (!cryptoMultisigPubkeys(coin, multisig, pubkeys)) {
return -1;
}
+ uint32_t n = cryptoMultisigPubkeyCount(multisig);
for (size_t i = 0; i < n; i++) {
if (memcmp(pubkeys + i * 33, pubkey, 33) == 0) {
return i;
@@ -436,12 +437,12 @@ static int comparePubnodesLexicographically(const void *first,
int cryptoMultisigFingerprint(const MultisigRedeemScriptType *multisig,
uint8_t *hash) {
- static const HDNodeType *pubnodes[15];
+ static const HDNodeType *pubnodes[MAX_MULTISIG_PUBKEY_COUNT];
const uint32_t n = cryptoMultisigPubkeyCount(multisig);
- if (n < 1 || n > 15) {
+ if (n < 1 || n > MAX_MULTISIG_PUBKEY_COUNT) {
return 0;
}
- if (multisig->m < 1 || multisig->m > 15) {
+ if (multisig->m < 1 || multisig->m > MAX_MULTISIG_PUBKEY_COUNT) {
return 0;
}
for (uint32_t i = 0; i < n; i++) {
Why this scored 42/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.