legacy/v0.1: fix truncating U32 cast in offset check; drop bogus pointer-as-int check#4680
legacy/v0.1: fix truncating U32 cast in offset check; drop bogus pointer-as-int check#4680evilgensec wants to merge 1 commit into
Conversation
|
Hi @evilgensec! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes offset validation in legacy Zstandard v0.1 decompression to avoid truncation and meaningless pointer-address comparisons, improving correctness for large buffers and 64-bit builds.
Changes:
- Replace a truncating
(U32)cast with full-width arithmetic forsequence.offsetvalidation. - Remove a logically incorrect
sequence.offset > (size_t)basecheck and document why it was invalid.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Use full-precision size_t arithmetic; the previous `(U32)` cast on the | ||
| * `ptrdiff_t` truncated silently when `oLitEnd - base` exceeded | ||
| * `UINT32_MAX`, letting larger `sequence.offset` values bypass the | ||
| * check on 64-bit builds with multi-GiB destination buffers. */ | ||
| if (sequence.offset > (size_t)(oLitEnd - base)) return ERROR(corruption_detected); |
There was a problem hiding this comment.
Good catch. Added an explicit if (oLitEnd < base) return ERROR(corruption_detected); before the unsigned cast so a negative ptrdiff_t cannot wrap. Kept the comparison in size_t for consistency with the rest of the file (the project does not use ptrdiff_t arithmetic in legacy decoders). Force-pushed.
| /* check */ | ||
| if (match < base) return ERROR(corruption_detected); | ||
| if (sequence.offset > (size_t)base) return ERROR(corruption_detected); | ||
| /* The previous `sequence.offset > (size_t)base` check compared the | ||
| * offset against the numeric address of the `base` pointer, which | ||
| * is meaningless: on 64-bit systems `base` is a high virtual | ||
| * address that no 32-bit offset can exceed; on 32-bit systems the | ||
| * check fires only on accidental low-address allocations rather | ||
| * than on any underflow condition. The real underflow detection | ||
| * is the `match < base` line above, which uses the actual | ||
| * post-subtraction pointer. */ |
There was a problem hiding this comment.
The pointer-arithmetic UB concern is closed by the upstream check at line 1738 (sequence.offset > (size_t)(oLitEnd - base)): op == oLitEnd at line 1756 because op += litLength already ran at line 1747, so the upstream check provably guarantees op - sequence.offset >= base before match = op - sequence.offset forms. I made this explicit in the comment block above the kept if (match < base) defense-in-depth check. Force-pushed.
…pointer-as-int check
`ZSTD_execSequence` in `lib/legacy/zstd_v01.c` had two related
correctness issues in its sequence-offset validation:
1. The check at line 1735
if (sequence.offset > (U32)(oLitEnd - base)) return ERROR(...);
casts the `ptrdiff_t` operand to `U32` before comparing. On 64-bit
builds with destination buffers larger than `UINT32_MAX`, the cast
truncates silently and the comparison is taken against the
low-32-bit remainder of the actual distance. An attacker that
crafts a v0.1 legacy frame whose `sequence.offset` exceeds the
truncated value but is still within `oLitEnd - base` passes the
check and reaches the match-copy step with an oversized offset.
2. The check at line 1759
if (sequence.offset > (size_t)base) return ERROR(...);
compares the offset against the numeric address of the `base`
pointer. On any 64-bit system the address is far higher than any
32-bit offset can reach, so the check never fires; on 32-bit
systems it fires only when `base` happens to be allocated at a
low address, which is unrelated to whether `op - sequence.offset`
underflows `base`.
The real underflow detection is the `if (match < base)` line that
runs in between -- which is correct, and is left untouched.
Replace the U32 cast with a `(size_t)` cast so the comparison uses
full pointer precision; remove the no-op pointer-as-int check.
v0.1 legacy frames remain decodable; the legitimate parser path
through `ZSTDv01_decompressDCtx` does not regress.
Built locally: `make -C lib libzstd.a` clean.
* lib/legacy/zstd_v01.c (ZSTD_execSequence): fix offset bound and
remove bogus pointer comparison.
evilgensec
force-pushed
the
fix-zstd-v01-offset-check
branch
from
9c16d8f to
82b4fb7
Compare
Summary
ZSTD_execSequenceinlib/legacy/zstd_v01.chas two related correctness issues in its sequence-offset validation.1. Truncating
U32cast atlib/legacy/zstd_v01.c:1735The cast forces the
ptrdiff_toperand into 32-bit before the comparison. On 64-bit builds with a destination buffer larger thanUINT32_MAX, the cast truncates silently and the bound becomes the low-32-bit remainder of the actual distance. A v0.1 legacy frame whosesequence.offsetexceeds the truncated value but is still ≤oLitEnd - basepasses the check and reaches the match-copy step with an oversized offset.The runtime catch at line 1758 (
if (match < base) return ERROR(corruption_detected);) still fires for the resulting underflow, so this is not a directly exploitable arbitrary read; but the design intent of the line-1735 check -- catching the corruption before pointer arithmetic -- is defeated whenever the destination buffer is multi-GiB.2. Bogus pointer-as-integer comparison at
lib/legacy/zstd_v01.c:1759Compares the offset against the numeric address of the
basepointer. On any 64-bit system,baseis a virtual address far higher than any 32-bit offset can reach, so the comparison never returns true. On 32-bit systems it fires only whenbaseis allocated at a low address, which is unrelated to whetherop - sequence.offsetunderflows.The actual underflow detection is the
if (match < base)line just above, which uses the post-subtraction pointer and is correct.Fix
(U32)cast with(size_t)so the comparison uses full pointer precision.if (match < base)line is the real guard.Diff
Verification
make -C lib libzstd.aclean, no new warnings.(size_t)comparison is mathematically a strict superset of the previous(U32)comparison for valid inputs; it can only refuse more malformed inputs, never reject a previously-accepted valid frame).ZSTD_decompressvia the auto-detection inZSTD_decompressLegacy;ZSTD_LEGACY_SUPPORTis 8 by default (lib/legacy/zstd_legacy.h:25).Refs
This is in the
legacy/tree, which is opt-in but enabled by default and reachable fromZSTD_decompressvia magic auto-detection.