signature: fix out-of-bounds read when parsing timezone offset#4883
Merged
pks-t merged 1 commit intolibgit2:masterfrom Nov 13, 2018
Merged
signature: fix out-of-bounds read when parsing timezone offset#4883pks-t merged 1 commit intolibgit2:masterfrom
pks-t merged 1 commit intolibgit2:masterfrom
Conversation
When parsing a signature's timezone offset, we first check whether there is a timezone at all by verifying that there are still bytes left to read following the time itself. The check thus looks like `time_end + 1 < buffer_end`, which is actually correct in this case. After setting the timezone's start pointer to that location, we compute the remaining bytes by using the formula `buffer_end - tz_start + 1`, re-using the previous `time_end + 1`. But this is in fact missing the braces around `(tz_start + 1)`, thus leading to an overestimation of the remaining bytes by a length of two. In case of a non-NUL terminated buffer, this will result in an overflow. The function `git_signature__parse` is only used in two locations. First is `git_signature_from_buffer`, which only accepts a string without a length. The string thus necessarily has to be NUL terminated and cannot trigger the issue. The other function is `git_commit__parse_raw`, which can in fact trigger the error as it may receive non-NUL terminated commit data. But as objects read from the ODB are always NUL-terminated by us as a cautionary measure, it cannot trigger the issue either. In other words, this error does not have any impact on security.
Member
Author
|
/rebuild |
|
Okay, @pks-t, I started to rebuild this pull request. |
Member
Author
|
/rebuild |
|
Okay, @pks-t, I started to rebuild this pull request. |
Member
Author
|
online::badssl is hitting again :/ |
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.
When parsing a signature's timezone offset, we first check whether there
is a timezone at all by verifying that there are still bytes left to
read following the time itself. The check thus looks like
time_end + 1 < buffer_end, which is actually correct in this case. After setting thetimezone's start pointer to that location, we compute the remaining
bytes by using the formula
buffer_end - tz_start + 1, re-using theprevious
time_end + 1. But this is in fact missing the braces around(tz_start + 1), thus leading to an overestimation of the remainingbytes by a length of two. In case of a non-NUL terminated buffer, this
will result in an overflow.
The function
git_signature__parseis only used in two locations. Firstis
git_signature_from_buffer, which only accepts a string without alength. The string thus necessarily has to be NUL terminated and cannot
trigger the issue.
The other function is
git_commit__parse_raw, which can in fact triggerthe error as it may receive non-NUL terminated commit data. But as
objects read from the ODB are always NUL-terminated by us as a
cautionary measure, it cannot trigger the issue either.
In other words, this error does not have any impact on security.