fix(crypto): Avoid caching uncacheable nodes in bip32.c
What the developer wrote
Authored by Andrew Kozlik
fix(crypto): Avoid caching uncacheable nodes in bip32.c
[no changelog]
(cherry picked from commit 88441bdb4fbcc1ba2b7539f9f8d1c02255185fa3)
What changed, and why it matters
This commit fixes a memory corruption bug in the code that caches Bitcoin-style key derivation paths inside Trezor's cryptographic library. When a derivation path was too deep to be cached, the code could store the derived key under the wrong path label. Later, a wallet or app asking for a different deep path could receive the wrong private key, which could lead to sending funds to an attacker-controlled address or signing with the wrong key. The fix stops the cache from storing these too-deep paths, and a new test confirms the wrong-key behavior no longer happens.
Treat this as a security-relevant fix. Review whether the fixed firmware version has been shipped to users, and consider whether any affected devices need guidance about avoiding deep derivation paths on unfixed firmware. The regression test should be run in CI to prevent reintroduction.
Security signals we found
Memory corruption / cache key mismatch in BIP32 derivation cache
Possible return of incorrect private key for a requested derivation path
Regression test explicitly labels the issue as a 'memory corruption bug'
Fix prevents caching of nodes whose depth exceeds cache capacity
No changelog entry and cherry-picked from another commit
Evidence from the diff
In crypto/bip32.c, hdnode_private_ckd_cached() derives a sequence of child HD nodes and uses an LRU cache keyed by the parent fingerprint and child index. The function had a fast path for i_count == 1 that bypassed the cache, but for any i_count > 1 it cached intermediate nodes without checking whether the remaining depth exceeded BIP32_CACHE_MAXDEPTH. If the path was deeper than the cache supports, the final node could be cached under an incorrect key (the last cached index plus the final index), causing later lookups for a different deep path to return the wrong derived node. The patch adds an explicit uncacheable path: when i_count - 1 > BIP32_CACHE_MAXDEPTH, it derives all non-final nodes directly, skips caching, and only derives the final child. A regression test (test_bip32_cache_3) demonstrates that without the fix two different deep paths could produce identical private keys.
Changed components
crypto/bip32.chdnode_private_ckd_cached()BIP32 derivation cacheInspect captured patch +33 / −2
diff --git a/crypto/bip32.c b/crypto/bip32.c
index 7bbaa7d5..f5ccfcde 100644
--- a/crypto/bip32.c
+++ b/crypto/bip32.c
@@ -361,11 +361,15 @@ int hdnode_private_ckd_cached(HDNode *inout, const uint32_t *i, size_t i_count,
// no way how to compute parent fingerprint
return 1;
}
- if (i_count == 1) {
+ if (i_count == 1 || i_count - 1 > BIP32_CACHE_MAXDEPTH) {
+ // when parent is uncacheable just derive the node and return
+ for (size_t k = 0; k < i_count - 1; k++) {
+ if (hdnode_private_ckd(inout, i[k]) == 0) return 0;
+ }
if (fingerprint) {
*fingerprint = hdnode_fingerprint(inout);
}
- if (hdnode_private_ckd(inout, i[0]) == 0) return 0;
+ if (hdnode_private_ckd(inout, i[i_count - 1]) == 0) return 0;
return 1;
}
diff --git a/crypto/tests/test_check.c b/crypto/tests/test_check.c
index db36d15d..0fa1d86b 100644
--- a/crypto/tests/test_check.c
+++ b/crypto/tests/test_check.c
@@ -2128,6 +2128,32 @@ START_TEST(test_bip32_cache_2) {
}
END_TEST
+START_TEST(test_bip32_cache_3) {
+ // Tests for a fixed memory corruption bug in the BIP32 cache.
+ HDNode node1 = {0};
+ HDNode node2 = {0};
+
+ const uint8_t *seed = fromhex(
+ "301133282ad079cbeb59bc446ad39d333928f74c46997d3609cd3e2801ca69d62788f9f1"
+ "74429946ff4e9be89f67c22fae28cb296a9b37734f75e73d1477af19");
+ hdnode_from_seed(seed, 64, SECP256K1_NAME, &node1);
+ hdnode_from_seed(seed, 64, SECP256K1_NAME, &node2);
+
+ uint32_t path[BIP32_CACHE_MAXDEPTH + 2] = {0};
+ size_t depth = sizeof(path) / sizeof(path[0]);
+ ck_assert_int_eq(hdnode_private_ckd_cached(&node1, path, depth, NULL), 1);
+
+ // In the presence of the memory corruption bug we cached the node under an
+ // incorrect path. Now we look up the corrupted path to make sure that the
+ // node we get is different.
+ path[BIP32_CACHE_MAXDEPTH] = BIP32_CACHE_MAXDEPTH + 1;
+ ck_assert_int_eq(hdnode_private_ckd_cached(&node2, path, depth, NULL), 1);
+
+ ck_assert_mem_ne(node1.private_key, node2.private_key,
+ sizeof(node1.private_key));
+}
+END_TEST
+
START_TEST(test_bip32_nist_seed) {
HDNode node;
@@ -11686,6 +11712,7 @@ Suite *test_suite(void) {
tcase_add_test(tc, test_bip32_compare);
tcase_add_test(tc, test_bip32_cache_1);
tcase_add_test(tc, test_bip32_cache_2);
+ tcase_add_test(tc, test_bip32_cache_3);
suite_add_tcase(s, tc);
tc = tcase_create("bip32-nist");
Why this scored 68/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.