psbt: overflow checks when computing Taproot BIP32 derivation min size#2374
Conversation
|
An alternative safeguard would be to explicitly cap Given that, I think it's more appropriate to rely on the natural limit imposed by |
| // 1 (leaf hash count) + (N × 32) + 4 (fingerprint) + 1 (empty path count) | ||
| // | ||
| // First, we calculate the total number of bytes for the leaf hashes. | ||
| totalHashesBytes, mulCarry := bits.Mul64(numHashes, 32) |
There was a problem hiding this comment.
Q: when will the mulCarry or the addCarry be non-zero? Why is this method preferred than, say, just calculating 32 * numHashes + 6?
There was a problem hiding this comment.
Since numHashes is uint64, numHashes * 32 fits in 64 bits unless numHashes > 2^64 / 32. mulCarry will be non-zero only in that overflow case. Using bits.Mul64 is safer and explicit about overflow handling.
guggero
left a comment
guggero
left a comment
There was a problem hiding this comment.
Thanks for the fix. Though I think this can be simplified quite a bit.
ffranr
force-pushed
the
2372-fix-bip32-derivation-overflow-bug
branch
3 times, most recently
from
b4ef41a to
16450e3
Compare
Pull Request Test Coverage Report for Build 15211625047Details
💛 - Coveralls |
Protect against overflows when parsing malformed Taproot BIP32 derivation fields. This ensures that deserialization fails safely if the declared number of leaf hashes would otherwise cause an integer overflow.
ffranr
force-pushed
the
2372-fix-bip32-derivation-overflow-bug
branch
from
16450e3 to
16c2b92
Compare
Cap the `value` slice to `MaxPsbtValueLength` to prevent potential out-of-memory conditions during parsing. This ensures that total allocation remains bounded and consistent with other PSBT fields.
| func ReadTaprootBip32Derivation(xOnlyPubKey, | ||
| value []byte) (*TaprootBip32Derivation, error) { | ||
|
|
||
| // This function allocates additional memory while parsing the serialized |
…n-overflow-bug psbt: overflow checks when computing Taproot BIP32 derivation min size
4 participants
Closes #2372
Thank you to @brunoerg for opening the issue.
Protect against overflows when parsing malformed Taproot BIP32 derivation fields. This ensures that deserialization fails safely if the declared number of leaf hashes would otherwise cause an integer overflow.