commit: fix out-of-bound reads when parsing truncated author fields #4894
Conversation
This file contains 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
While commit objects usually should have only one author field, our commit parser actually handles the case where a commit has multiple author fields because some tools that exist in the wild actually write them. Detection of those additional author fields is done by using a simple `git__prefixcmp`, checking whether the current line starts with the string "author ". In case where we are handed a non-NUL-terminated string that ends directly after the space, though, we may have an out-of-bounds read of one byte when trying to compare the expected final NUL byte. Fix the issue by using `git__prefixncmp` instead of `git_prefixcmp`. Unfortunately, a test cannot be easily written to catch this case. While we could test the last error message and verify that it didn't in fact fail parsing a signature (because that would indicate that it has in fact tried to parse the additional "author " field, which it shouldn't be able to detect in the first place), this doesn't work as the next line needs to be the "committer" field, which would error out with the same error message even if we hadn't done an out-of-bounds read. As objects read from the object database are always NUL terminated, this issue cannot be triggered in normal code and thus it's not security critical.
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.
While commit objects usually should have only one author field, our commit
parser actually handles the case where a commit has multiple author fields
because some tools that exist in the wild actually write them. Detection of
those additional author fields is done by using a simple
git__prefixcmp,checking whether the current line starts with the string "author ". In case
where we are handed a non-NUL-terminated string that ends directly after the
space, though, we may have an out-of-bounds read of one byte when trying to
compare the expected final NUL byte.
Fix the issue by using
git__prefixncmpinstead ofgit_prefixcmp.Unfortunately, a test cannot be easily written to catch this case. While we
could test the last error message and verify that it didn't in fact fail parsing
a signature (because that would indicate that it has in fact tried to parse the
additional "author " field, which it shouldn't be able to detect in the first
place), this doesn't work as the next line needs to be the "committer" field,
which would error out with the same error message even if we hadn't done an
out-of-bounds read.
As objects read from the object database are always NUL terminated, this issue
cannot be triggered in normal code and thus it's not security critical.