Bound a w or W blank run at the end of the string buffer - #3589
Open
LouisDeconinck wants to merge 1 commit into
Open
LouisDeconinck wants to merge 1 commit into
LouisDeconinck wants to merge 1 commit into
Conversation
A blank in a w or W value skipped a whitespace run of any length, so a test still matched when the run carried the next byte past the end of the buffer libmagic compares against. libmagic copies the candidate bytes into a MAXstring-byte union and forces its last byte to NUL (file/src/softmagic.c:1238), and file_strncmp stops the run at the end of that buffer (file/src/softmagic.c:2102-2121), so the byte after the run compares against the NUL and a run of 126 or more blanks fails. StringMatch.matches now runs the pattern over a reproduction of that buffer: at most MAX_STRING_BYTES - 1 bytes of the candidate, plus the terminating NUL. A search keeps its own bound, the end of the buffer it reads in place (file/src/softmagic.c:2363-2364), so it is untouched. Fixes trailofbits#3571
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
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.
libmagic behavior
A blank in a
worWvalue skips a whitespace run in the candidate bytes, but the run is not unbounded:mcopycopies the candidate into aMAXstring-byte union (file/src/file.h:179,file/src/softmagic.c:1478-1511),mconvertforces that buffer's last byte to NUL before the comparison (file/src/softmagic.c:1238), andfile_strncmpstops the run at the end of the buffer viaeb = b + maxlen(file/src/softmagic.c:2070-2072,2102-2121). A run long enough to reach the buffer's last byte therefore ends there, and the next byte of the value compares against the terminating NUL and fails.Divergence
PolyFile compiled each blank to an unbounded
[ \t\n\v\f\r]+(or*), so astring/Worstring/wtest matched a run of any length:PolyFile reported
foundfor that file and for a run of any length.Fix
StringMatch.matchesnow runs the compiled pattern over a reproduction of libmagic's buffer: at mostMAX_STRING_BYTES - 1bytes of the candidate plus the terminating NUL (data[:limit].ljust(MAX_STRING_BYTES, b"\x00")). Astr_range(string/N) shortens the copy the same waymcopyshortens it (softmagic.c:1480-1481).searchis untouched: libmagic reads that buffer in place and bounds the run at the end of the remaining buffer instead (file/src/softmagic.c:2363-2364), which the search path already does.Test
test_a_blank_run_stops_at_the_end_of_the_string_bufferpins the boundary for both spellings: 125 blanks match, 126 or more fall back to the text result, and a padded file shows the bound is the buffer rather than the file's length. Asearch/200/Wcase pins that a search still matches a longer run.Verified against
file5.46 and the bundled 5.48 source: both flags, negated tests (string/W !A\ Breportsfoundon a 126-blank file, as libmagic does), and multi-blank values agree at every boundary probed.pytest tests/test_magic.py: 220 passed.flake8on the touched files reports only the warnings already present on master.Fixes #3571