Skip to content

commit: fix out-of-bound reads when parsing truncated author fields#4894

Merged
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/commit-author-oob
Nov 21, 2018
Merged

commit: fix out-of-bound reads when parsing truncated author fields#4894
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/commit-author-oob

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Nov 21, 2018

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants