reject hex jump and repeat lengths that overflow an int - #2237
Open
munzzyy wants to merge 1 commit into
Open
Conversation
The hex string and regexp lexers parse jump lengths and repeat intervals
with atoi, which is undefined for numbers that don't fit in an int. glibc
hands back the low 32 bits, so a value that wraps to a small positive
number passes every bounds check and silently changes the pattern:
rule r1 { strings: $a = /a{4294967296}/ condition: $a }
rule r2 { strings: $b = { 61 [0-4294967297] 62 } condition: $b }
Before this change r1 compiled as a{0} and matched at every offset, and
r2 compiled as { 61 [0-1] 62 }. MSVC saturates atoi at INT_MAX instead,
so the same rule behaves differently there, which is what VirusTotal#1791 reports.
Parse with strtoll instead. Numbers too large for a long long are clamped
to LLONG_MAX, which still fails the range check, so "repeat interval too
large" now fires for everything above RE_MAX_RANGE on any platform. Hex
jumps get an explicit INT_MAX check because RE_NODE stores start and end
as ints, and the grammar already casts to int.
The hi_bound < 0 and lo_bound < 0 checks are gone. They were there to
catch atoi wrapping negative and are unreachable now that the only thing
the pattern matches is digits.
Fixes VirusTotal#1791.
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.
Fixes #1791.
Both pattern lexers parse numbers with
atoi, which is undefined for input thatdoesn't fit in an int. On glibc you get the low 32 bits back. A value that wraps
to a small positive number then passes every bounds check, and the rule compiles
into something the author did not write.
On master at 604822d:
a{4294967296}becamea{0}and hit at every offset. The hex jump became[0-1]and matchedab. Rule b is the quiet one. It doesn't appear abovebecause
b{1,4294967297}becameb{1,1}, so it missesabbbbcand matchesabcinstead:Nothing warned in any of these cases. Values one order of magnitude smaller are
rejected properly, which is what makes this easy to miss:
The rule from #1791 lands on the other side of the wrap. It does error out, but
with a message describing the truncation rather than the input, because
atoicame back negative:
MSVC saturates
atoiat INT_MAX rather than wrapping, so the same rule filecompiles differently there. That is the platform split reported in the issue.
The fix parses with
strtollin both lexers. Numbers past LLONG_MAX get clampedthere and still fail the range check, so
repeat interval too largenow firesfor anything above RE_MAX_RANGE no matter what the platform does. Hex jumps get
an explicit INT_MAX check, since RE_NODE holds start and end as ints and the
grammar already casts with
$$->start = (int) $2. I dropped thehi_bound < 0and
lo_bound < 0checks because they only existed to catchatoiwrappingnegative, and the lexer patterns match digits only.
After:
Valid patterns are untouched.
{ 61 [2-4] 62 },{ 61 [0-] 62 },{ 61 [0-2147483647] 62 },/a{1,5}/and/ab{32767}c/all still compile, and/ab{5,2}c/still givesbad repeat interval. The only inputs whose behaviourchanges are the ones that used to wrap.
Tests go next to the existing
a{2977952116}overflow test and the hex stringerror tests. Both halves are load bearing, confirmed by reverting one lexer at a
time and rebuilding:
Full suite on Linux, gcc 16.1.1, configured with
--enable-dotnet --enable-magic --enable-cuckoo --disable-shared:The generated
hex_lexer.candre_lexer.care checked in, so they areregenerated here with flex 2.6.4, the same version that produced the current
ones. Both
.lfiles set%option outfile, so regeneration has to go throughthe build instead of a direct
flex -ocall:The checked-in files come back byte for byte identical.
This does not add the INT16_MAX cap on hex jumps that the issue floats as an
option. That would reject rules people are running today, and it is a separate
call from fixing the overflow.