Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the CBOR decoder against integer overflow when computing total lengths for bytes/string items, reducing the risk of wraparound and incorrect bounds checks on malformed inputs.
Changes:
- Added explicit overflow detection in
Decode::getTotalLen()for bytes/string total lengths. - Updated
Decode::getBytes()to compute required length in 64-bit and reject values exceedingUINT32_MAX. - Adjusted
Decode::isValid()to use 64-bit length calculation for bytes/string validity checks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sergei-boiko-trustwallet
left a comment
sergei-boiko-trustwallet
left a comment
There was a problem hiding this comment.
LGTM, just code style comments
Binary size comparison➡️ aarch64-apple-ios: 14.34 MB ➡️ aarch64-apple-ios-sim: 14.34 MB ➡️ aarch64-linux-android: 18.77 MB ➡️ armv7-linux-androideabi: 16.20 MB ➡️ wasm32-unknown-emscripten: 13.68 MB |
sergei-boiko-trustwallet
left a comment
sergei-boiko-trustwallet
left a comment
There was a problem hiding this comment.
LGTM, just a couple questions
| case MT_string: | ||
| return (uint32_t)typeDesc.byteCount + (uint32_t)typeDesc.value; | ||
| { | ||
| uint64_t totalLen = static_cast<uint64_t>(typeDesc.byteCount) + typeDesc.value; |
There was a problem hiding this comment.
Can this sum overflow?
| } | ||
| auto len = (uint32_t)typeDesc.value; | ||
| if (length() < (uint32_t)typeDesc.byteCount + (uint32_t)len) { | ||
| uint64_t requiredLen = static_cast<uint64_t>(typeDesc.byteCount) + typeDesc.value; |
There was a problem hiding this comment.
Same question
| case MT_string: | ||
| { | ||
| auto len = (uint32_t)(typeDesc.byteCount + typeDesc.value); | ||
| uint64_t len = static_cast<uint64_t>(typeDesc.byteCount) + typeDesc.value; |
There was a problem hiding this comment.
Same here
This pull request improves the robustness of CBOR decoding by adding overflow checks when handling bytes and string lengths, preventing potential security or stability issues due to excessively large or malformed data. The main changes focus on ensuring that length calculations do not exceed the maximum value representable by a 32-bit unsigned integer.
CBOR decoding overflow protection:
Decode::getTotalLen()to throw an exception if the total length of bytes or string data exceedsUINT32_MAX, preventing silent wrapping or undefined behavior.Decode::getBytes()to check for overflow when calculating the required length, throwing an exception if the total exceedsUINT32_MAXor if the available data is insufficient.Decode::isValid()to use a 64-bit variable for length calculation, ensuring that validity checks are accurate even for large data lengths.