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 |
btcutil/psbt/taproot.go
Outdated
| // 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.
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.
16450e3 to
16c2b92
Compare
| } | ||
|
|
||
| // Ensure that value is at least the minimum size. | ||
| if uint64(len(value)) < minByteSize { |
There was a problem hiding this comment.
Okay, I guess this check prevents us from OOMing if the encoded length is larger from the number of bytes we actually have available.
There was a problem hiding this comment.
Thank you for your attention/review. I think you reviewed before I managed to push my latest change. I've added:
// This function allocates additional memory while parsing the serialized
// data. To prevent potential out-of-memory (OOM) issues, we must validate
// the length of the value slice before proceeding.
if len(value) > MaxPsbtValueLength {
return nil, ErrInvalidPsbtFormat
}
let me know if that works for you
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.
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.