strntol: fix out-of-bounds reads when parsing numbers with leading sign#4886
Merged
ethomson merged 2 commits intolibgit2:masterfrom Nov 14, 2018
Merged
Conversation
When parsing a number, we accept a leading plus or minus sign to return a positive or negative number. When the parsed string has such a leading sign, we set up a flag indicating that the number is negative and advance the pointer to the next character in that string. This misses updating the number of bytes in the string, though, which is why the parser may later on do an out-of-bounds read. Fix the issue by correctly updating both the pointer and the number of remaining bytes. Furthermore, we need to check whether we actually have any bytes left after having advanced the pointer, as otherwise the auto-detection of the base may do an out-of-bonuds access. Add a test that detects the out-of-bound read. Note that this is not actually security critical. While there are a lot of places where the function is called, all of these places are guarded or irrelevant: - commit list: this operates on objects from the ODB, which are always NUL terminated any may thus not trigger the off-by-one OOB read. - config: the configuration is NUL terminated. - curl stream: user input is being parsed that is always NUL terminated - index: the index is read via `git_futils_readbuffer`, which always NUL terminates it. - loose objects: used to parse the length from the object's header. As we check previously that the buffer contains a NUL byte, this is safe. - rebase: this parses numbers from the rebase instruction sheet. As the rebase code uses `git_futils_readbuffer`, the buffer is always NUL terminated. - revparse: this parses a user provided buffer that is NUL terminated. - signature: this parser the header information of objects. As objects read from the ODB are always NUL terminated, this is a non-issue. The constructor `git_signature_from_buffer` does not accept a length parameter for the buffer, so the buffer needs to be NUL terminated, as well. - smart transport: the buffer that is parsed is NUL terminated - tree cache: this parses the tree cache from the index extension. The index itself is read via `git_futils_readbuffer`, which always NUL terminates it. - winhttp transport: user input is being parsed that is always NUL terminated
The function `parse_number` was replaced by `git_parse_advance_digit` which is provided by the parser interface in commit 252f2ee (parse: implement and use `git_parse_advance_digit`, 2017-07-14). As there are no remaining callers, remove it.
Member
|
Appreciate the detailed analysis. 👍 |
Merged
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.
When parsing a number, we accept a leading plus or minus sign to return
a positive or negative number. When the parsed string has such a leading
sign, we set up a flag indicating that the number is negative and
advance the pointer to the next character in that string. This misses
updating the number of bytes in the string, though, which is why the
parser may later on do an out-of-bounds read.
Fix the issue by correctly updating both the pointer and the number of
remaining bytes. Furthermore, we need to check whether we actually have
any bytes left after having advanced the pointer, as otherwise the
auto-detection of the base may do an out-of-bonuds access. Add a test
that detects the out-of-bound read.
Note that this is not actually security critical. While there are a lot
of places where the function is called, all of these places are guarded
or irrelevant:
commit list: this operates on objects from the ODB, which are always
NUL terminated any may thus not trigger the off-by-one OOB read.
config: the configuration is NUL terminated.
curl stream: user input is being parsed that is always NUL terminated
index: the index is read via
git_futils_readbuffer, which always NULterminates it.
loose objects: used to parse the length from the object's header. As
we check previously that the buffer contains a NUL byte, this is safe.
rebase: this parses numbers from the rebase instruction sheet. As the
rebase code uses
git_futils_readbuffer, the buffer is always NULterminated.
revparse: this parses a user provided buffer that is NUL terminated.
signature: this parser the header information of objects. As objects
read from the ODB are always NUL terminated, this is a non-issue. The
constructor
git_signature_from_bufferdoes not accept a lengthparameter for the buffer, so the buffer needs to be NUL terminated, as
well.
smart transport: the buffer that is parsed is NUL terminated
tree cache: this parses the tree cache from the index extension. The
index itself is read via
git_futils_readbuffer, which always NULterminates it.
winhttp transport: user input is being parsed that is always NUL
terminated