bound option tag lookahead to the body in parse_supported_body - #4291
Open
Sahana2524 wants to merge 1 commit into
Open
Sahana2524 wants to merge 1 commit into
Sahana2524 wants to merge 1 commit into
Conversation
The leading dword read and the per-tag delimiter checks were never capped by body->len, so a Supported header body ending in a delimiter, a short element, or an option tag flush against the end of the body made the scan read outside the buffer it was given.
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.
Summary
parse_supported_body()reads past the end of theSupportedheader body while scanning for option tags. The reads are one to four bytes, but they are outside thestrthe function was handed, and the tag matching currently depends on one of them.Details
Two lookaheads are unbounded:
val = LOWER_DWORD(READ(p))loads four bytes with no check on what is left. The delimiter-skip loop above it can walkposall the way up tolen, so a body likeSupported: ,has the dword read starting at the end of the body. A short element such asSupported: xreads three bytes past it.pos + N <= lenand then readsp[N], which needspos + N < len. So a tag sitting flush against the end of the body (path,gruu,timer,100rel,eventlist) reads the byte right after it.The body is attacker supplied, and
parse_supported()runs on anything the registrar,sst,rr/path,tmorrlslogic looks at. Driving the function through the guard-page harness already inparser/test/test_oob.cfaults atparse_supported.c:55,:60,:69,:80,:91and:102, one per site.Case 2 also costs correctness today, which is the easier half to see. An option tag at the very end of the body is only matched because the byte that follows it inside
msg->bufhappens to be the CR of the header's own CRLF. Handparse_supported_body()astrthat is not backed by a message buffer andSupported: pathstops settingF_SUPPORTED_PATH: no guard page needed, 8 of the new test's plain assertions fail on master for that reason alone.Solution
The dword read is capped by the bytes that remain. An element shorter than four characters cannot be any of the tags below, so it falls through to the existing skip path, which is where the old code ended up anyway once the garbage dword failed to match.
IS_TAG_END()sits next toIS_DELIM()and treats the end of the body as a tag terminator rather than something to read across, so the five per-tag checks stop depending on a byte they do not own.parser/test/test_parse_supported.ccovers both halves: functional cases for each recognized tag (alone, flush against the end, and in a list), and atest_oob()sweep over the inputs that hit the six sites. Against master it fails 8 assertions and then dies with SIGBUS on the first guard-page case; with the patch it is 197/197.Compatibility
No behavior change for a
Supportedheader parsed out of a SIP message, since the bytes the old code reached for there were the trailing CRLF, and CR is a delimiter. The one visible difference is that a tag ending exactly at the end of the body is now recognized without looking at what follows, which is what the old code was already doing by accident.Closing issues
None.