Skip to content

Tree parsing fixes - #4871

Merged
pks-t merged 5 commits into
libgit2:masterfrom
pks-t:pks/tree-parsing-fixes
Nov 7, 2018
Merged

Tree parsing fixes#4871
pks-t merged 5 commits into
libgit2:masterfrom
pks-t:pks/tree-parsing-fixes

Conversation

@pks-t

@pks-t pks-t commented Oct 29, 2018

Copy link
Copy Markdown
Member

Various fixes to our parser of tree objects. Many of them have been pointed out by oss-fuzz and are of out-of-bounds nature. But in fact, they aren't really harmful: The object parsing fuzzer intentionally uses a buffer that may not be NUL terminated, while objects from the ODB will always have a trailing NUL byte and thus the OOB reads will never happen in real-world code.

@pks-t
pks-t force-pushed the pks/tree-parsing-fixes branch from f1eae88 to 7fafec0 Compare November 2, 2018 12:31
pks-t added 5 commits November 2, 2018 13:31
The `git__strntol` family of functions accepts leading spaces and will
simply skip them. The skipping will not honor the provided buffer's
length, though, which may lead it to read outside of the provided
buffer's bounds if it is not a simple NUL-terminated string.
Furthermore, if leading space is trimmed, the function will further
advance the pointer but not update the number of remaining bytes, which
may also lead to out-of-bounds reads.

Fix the issue by properly paying attention to the buffer length and
updating it when stripping leading whitespace characters. Add a test
that verifies that we won't read past the provided buffer length.
The `git__strntol` family of functions has the ability to auto-detect
a number's base if the string has either the common '0x' prefix for
hexadecimal numbers or '0' prefix for octal numbers. The detection of
such prefixes and following handling has two major issues though that are
being fixed in one go now.

- We do not do any bounds checking previous to verifying the '0x' base.
  While we do verify that there is at least one digit available
  previously, we fail to verify that there are two digits available and
  thus may do an out-of-bounds read when parsing this
  two-character-prefix.

- When skipping the prefix of such numbers, we only update the pointer
  length without also updating the number of remaining bytes. Thus if we
  try to parse a number '0x1' of total length 3, we will first skip the
  first two bytes and then try to read 3 bytes starting at '1'.

Fix both issues by disentangling the logic. Instead of doing the
detection and skipping of such prefixes in one go, we will now first try
to detect the base while also honoring how many bytes are left. Only if
we have a valid base that is either 8 or 16 and have one of the known
prefixes, we will now advance the pointer and update the remaining bytes
in one step.

Add some tests that verify that no out-of-bounds parsing happens and
that autodetection works as advertised.
We currently don't have any tests that directly exercise the tree
parser. This is due to the fact that the parsers for raw object data has
only been recently introduce with commit ca4db5f (object: implement
function to parse raw data, 2017-10-13), and previous to that the setup
simply was too cumbersome as it always required going through the ODB.

Now that we have the infrastructure, add a suite of tests that directly
exercise the tree parser and various edge cases.
When parsing a tree entry's mode, we will eagerly parse until we hit a
character that is not in the accepted set of octal digits '0' - '7'. If
the provided buffer is not a NUL terminated one, we may thus read
out-of-bounds.

Fix the issue by passing the buffer length to `parse_mode` and paying
attention to it. Note that this is not a vulnerability in our usual code
paths, as all object data read from the ODB is NUL terminated.
The `parse_mode` option uses an open-coded octal number parser. The
parser is quite naive in that it simply parses until hitting a character
that is not in the accepted range of '0' - '7', completely ignoring the
fact that we can at most accept a 16 bit unsigned integer as filemode.
If the filemode is bigger than UINT16_MAX, it will thus overflow and
provide an invalid filemode for the object entry.

Fix the issue by using `git__strntol32` instead and doing a bounds
check. As this function already handles overflows, it neatly solves the
problem.

Note that previously, `parse_mode` was also skipping the character
immediately after the filemode. In proper trees, this should be a simple
space, but in fact the parser accepted any character and simply skipped
over it. As a consequence of using `git__strntol32`, we now need to an
explicit check for a trailing whitespace after having parsed the
filemode. Because of the newly introduced error message, the test
object::tree::parse::mode_doesnt_cause_oob_read needs adjustment to its
error message check, which in fact is a good thing as it demonstrates
that we now fail looking for the whitespace immediately following the
filemode.

Add a test that shows that we will fail to parse such invalid filemodes
now.
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.

1 participant