What the developer wrote
Authored by kdmukai
Hold the parent key in each cache entry
The cache keys on id(parent_key), which is the parent's address in
memory. Python is free to hand that address to a new object once the
original is released, so an entry outliving its parent could be matched
by an unrelated key that landed on the same address, and
_derive_with_cache would return that entry's child instead of deriving
the right one. It fails silently: a recycled address is
indistinguishable from a hit.
Nothing triggers this today, because every parent -- the signing seed's
root and the cosigner xpubs from psbt.xpubs -- is held for the whole
parse. But that is a property of the call sites rather than of the
function, and the failure would be a wrong key in the middle of change
detection.
Storing the parent alongside its derived child keeps that address
occupied for as long as the entry depending on it is alive. The entry
holds a reference to a key the parser already owns, so nothing new is
retained and nothing is copied. Costs 56 bytes per entry, so the cap now
holds the cache to about 600 KB; the comment is updated to match.
Reported by newtonick in review.
What changed, and why it matters
This commit fixes a subtle caching bug in how SeedSigner derives child keys from parent keys when parsing Bitcoin transactions. The cache used the memory address of the parent key as its identifier, but Python can reuse that address after the parent is discarded. In theory, an unrelated new parent could land on the same address, causing the cache to return the wrong child key without any error. The fix stores the parent key inside each cache entry so its memory address stays occupied as long as the cached result exists. The commit itself says nothing currently triggers the bug because callers keep parents alive, but the function was unsafe on its own.
Treat this as a defensive correctness fix with latent security implications. Review whether any other components use id() as a cache key for cryptographic material. No immediate emergency response is indicated because the commit states current call sites keep parents alive, but the patch should be included in the next release.
Security signals we found
Use of id() as a cache key for security-critical objects
Potential use-after-free / object identity reuse in Python cache
Silent wrong-key return in key derivation path
Change-detection key derivation correctness
Memory cap adjustment to account for parent reference storage
Evidence from the diff
In psbt_parser.py, _derive_with_cache previously keyed its LRU-style cache on (id(parent_key), derivation_path_so_far), where id() returns the object’s memory address. Because Python may recycle object addresses after garbage collection, a cache entry could outlive its parent_key and later be matched by a different HDKey instance whose id() happened to collide. The result would be a silent wrong-child return from _derive_with_cache, corrupting change detection or key derivation. The patch stores (parent_key, derived_child) tuples in the cache, keeping a reference to the original parent and preventing address reuse for the lifetime of the entry. Memory cost is ~56 bytes per entry; the cap comment is updated from ~500 KB to ~600 KB.
Changed components
src/seedsigner/models/psbt_parser.py_derive_with_cache methodPSBT change detection / child key derivation cacheInspect captured patch +12 / −4
### src/seedsigner/models/psbt_parser.py
@@ -35,7 +35,7 @@ class PSBTParser():
# Upper bound on how many levels of derivation a single parse will cache in
# _child_key_derivation_cache. 1000 is just slightly under a 3-of-5 multisig
- # consolidating 200 inputs and holds the cache to a max of about half a megabyte. A
+ # consolidating 200 inputs and holds the cache to a max of about 600 kilobytes. A
# psbt that requires more levels will still parse correctly, but may have to derive
# some levels more than once. Capping the cache at a realistic upper bound protects
# against a maliciously crafted psbt that would otherwise consume unbounded memory
@@ -391,6 +391,11 @@ def _derive_with_cache(parent_key: bip32.HDKey, derivation_path: List[int], cach
object's identity; the parent belongs in the key because a multisig parse runs
these same derivations below each cosigner's xpub in turn.
+ Each entry also holds on to the parent it was derived from. id() is only the
+ object's address, which Python is free to hand to a new object once the original
+ is released. Keeping the parent means its address cannot be reused for as long as
+ the entry it belongs to is alive.
+
Keying on the parent's fingerprint was rejected: four bytes is small enough for a
malicious coordinator to grind a deliberate collision, and the cosigner xpubs come
from the psbt.
@@ -407,13 +412,16 @@ def _derive_with_cache(parent_key: bip32.HDKey, derivation_path: List[int], cach
for index in derivation_path:
derivation_path_so_far += (index,)
cache_key = (id(parent_key), derivation_path_so_far)
- already_derived = cache.get(cache_key)
- if already_derived is None:
+ cached_entry = cache.get(cache_key)
+ if cached_entry is None:
# First time deriving this level. Do the work to derive this level's child
# and store it in the cache.
already_derived = derived_key.child(index)
if len(cache) < PSBTParser.MAX_CACHED_DERIVATIONS:
- cache[cache_key] = already_derived
+ # Parent must also be stored to keep its id() from being reused
+ cache[cache_key] = (parent_key, already_derived)
+ else:
+ cached_parent, already_derived = cached_entry
derived_key = already_derived
return derived_key
Why this scored 59/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.