UdfIn: fix OOB read in CFileId::Parse alignment padding loop - #263
Open
tonghuaroot wants to merge 1 commit into
Open
UdfIn: fix OOB read in CFileId::Parse alignment padding loop#263tonghuaroot wants to merge 1 commit into
tonghuaroot wants to merge 1 commit into
Conversation
Upstream 7-Zip 26.01 added a bounds check to the trailing alignment-padding loop in CFileId::Parse(). After consuming the 38-byte fixed header plus the variable-length impLen and idLen fields, the loop advances `processed` up to 3 bytes to reach the next 4-byte boundary while requiring those bytes to be zero. In p7zip's pre-fix code, that loop reads p[processed] without verifying that `processed < size`. Pre-loop the only guarantee is `size >= 38 + idLen + impLen == processed` (note: `<` not `<=`), so when the crafted UDF File Identifier descriptor fills the buffer exactly to the fixed-plus-variable size, the loop reads up to 3 bytes past the buffer end on attacker-controlled input. This patch matches upstream 7-Zip 26.01: short-circuit the loop condition with `processed >= size` so the OOB index can never be dereferenced. The post-loop `(processed <= size) ? S_OK : S_FALSE` check is now redundant since the loop guards `processed < size`. Signed-off-by: tonghuaroot <tonghuaroot@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backports the bounds-check addition landed in upstream 7-Zip 26.01 (released 2026-04-27).
What changed upstream
After consuming the 38-byte fixed File Identifier descriptor header plus the variable-length
impLenandidLenfields,CFileId::Parseadvancesprocessedup to 3 bytes to reach the next 4-byte boundary while requiring those bytes to be zero. In p7zip the loop readsp[processed]without verifyingprocessed < size:```cpp
for (;(processed & 3) != 0; processed++)
if (p[processed] != 0)
return S_FALSE;
```
26.01 short-circuits the loop with a bounds check:
```cpp
for (; processed & 3; processed++)
if (processed >= size || p[processed])
return S_FALSE;
```
Impact
Pre-loop, the only available bound is
size >= 38 + idLen + impLen == processed(note:<not<=in the pre-check). When the crafted UDF File Identifier descriptor fills the buffer exactly to the fixed-plus-variable size, the alignment loop reads up to 3 bytes past the buffer end on attacker-controlled input.Patch
Byte-for-byte matches upstream 26.01 for the loop hunk.
Disclosure
I am not a native English speaker. AI tooling was used to polish the prose in this PR description. The fix itself is a direct backport from upstream 7-Zip 26.01 source; no design choices were made beyond matching upstream verbatim.