fix(crypto): Clean up stack in hdnode_deserialize().
What the developer wrote
Authored by Andrew Kozlik
fix(crypto): Clean up stack in hdnode_deserialize().
What changed, and why it matters
This commit fixes a bug in the Trezor firmware's code that reads BIP32 extended keys (the xpub/xprv strings used by wallets). Previously, if the function encountered an invalid key string, it returned an error immediately without wiping a temporary 78-byte buffer that held decoded key material. That leftover data could remain on the device stack and potentially leak small pieces of secret key information to later code. The fix ensures the buffer is always cleared before the function returns, even on error paths.
Treat as a low-to-moderate security hardening fix. Review whether any other functions in the crypto module leave decoded key material on the stack on error paths, and consider static analysis or stack-clearing conventions for sensitive buffers.
Security signals we found
Sensitive stack buffer not cleared on error paths
Potential information disclosure of decoded key material
Use of goto cleanup for centralized secure cleanup
memzero added to wipe node_data before return
Evidence from the diff
In crypto/bip32.c, hdnode_deserialize() used a stack-allocated uint8_t node_data[78] buffer to hold the base58-decoded extended key. On success, the function already populated the HDNode and returned 0. However, on the three early error paths (base58 decode failure, wrong version, invalid private-key padding), it returned directly without calling memzero() on node_data. The patch introduces a single cleanup label, replaces those direct returns with ret = …; goto cleanup, and adds memzero(node_data, sizeof(node_data)) at cleanup. This is a classic secure-coding fix to prevent sensitive material from persisting on the stack across function returns.
Changed components
crypto/bip32.chdnode_deserialize()hdnode_deserialize_public() / hdnode_deserialize_private() callersInspect captured patch +11 / −4
diff --git a/crypto/bip32.c b/crypto/bip32.c
index e966f038..3ebd1aaa 100644
--- a/crypto/bip32.c
+++ b/crypto/bip32.c
@@ -712,21 +712,25 @@ int hdnode_serialize_private(const HDNode *node, uint32_t fingerprint,
static int hdnode_deserialize(const char *str, uint32_t version,
bool use_private, const char *curve, HDNode *node,
uint32_t *fingerprint) {
+ int ret = 0;
uint8_t node_data[78] = {0};
memzero(node, sizeof(HDNode));
node->curve = get_curve_by_name(curve);
if (base58_decode_check(str, node->curve->hasher_base58, node_data,
sizeof(node_data)) != sizeof(node_data)) {
- return -1;
+ ret = -1;
+ goto cleanup;
}
uint32_t ver = read_be(node_data);
if (ver != version) {
- return -3; // invalid version
+ ret = -3; // invalid version
+ goto cleanup;
}
if (use_private) {
// invalid data
if (node_data[45]) {
- return -2;
+ ret = -2;
+ goto cleanup;
}
memcpy(node->private_key, node_data + 46, 32);
memzero(node->public_key, sizeof(node->public_key));
@@ -742,7 +746,10 @@ static int hdnode_deserialize(const char *str, uint32_t version,
}
node->child_num = read_be(node_data + 9);
memcpy(node->chain_code, node_data + 13, 32);
- return 0;
+
+cleanup:
+ memzero(node_data, sizeof(node_data));
+ return ret;
}
int hdnode_deserialize_public(const char *str, uint32_t version,
Why this scored 44/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.