Skip to content

G
git2
Switch branch/tag
  1. 12 Oct, 2018 21 commits
  3. 05 Oct, 2018 7 commits
  5. 03 Oct, 2018 12 commits
    • smart_pkt: do not accept callers passing in no line length · f5c3442b 
      Right now, we simply ignore the `linelen` parameter of
`git_pkt_parse_line` in case the caller passed in zero. But in fact, we
never want to assume anything about the provided buffer length and
always want the caller to pass in the available number of bytes.
And in fact, checking all the callers, one can see that the funciton is
never being called in case where the buffer length is zero, and thus we
are safe to remove this check.

(cherry picked from commit 1bc5b05c)
      Patrick Steinhardt committed
    • smart_pkt: return parsed length via out-parameter · f7c3f6cc 
      The `parse_len` function currently directly returns the parsed length of
a packet line or an error code in case there was an error. Instead,
convert this to our usual style of using the return value as error code
only and returning the actual value via an out-parameter. Thus, we can
now convert the output parameter to an unsigned type, as the size of a
packet cannot ever be negative.

While at it, we also move the check whether the input buffer is long
enough into `parse_len` itself. We don't really want to pass around
potentially non-NUL-terminated buffers to functions without also passing
along the length, as this is dangerous in the unlikely case where other
callers for that function get added. Note that we need to make sure
though to not mess with `GIT_EBUFS` error codes, as these indicate not
an error to the caller but that he needs to fetch more data.

(cherry picked from commit c05790a8)
      Patrick Steinhardt committed
    • smart_pkt: reorder and rename parameters of `git_pkt_parse_line` · 7e3cd611 
      The parameters of the `git_pkt_parse_line` function are quite confusing.
First, there is no real indicator what the `out` parameter is actually
all about, and it's not really clear what the `bufflen` parameter refers
to. Reorder and rename the parameters to make this more obvious.

(cherry picked from commit 0b3dfbf4)
      Patrick Steinhardt committed
    • smart_pkt: fix buffer overflow when parsing "unpack" packets · 356f60f4 
      When checking whether an "unpack" packet returned the "ok" status or
not, we use a call to `git__prefixcmp`. In case where the passed line
isn't properly NUL terminated, though, this may overrun the line buffer.
Fix this by using `git__prefixncmp` instead.

(cherry picked from commit 5fabaca8)
      Patrick Steinhardt committed
    • smart_pkt: fix "ng" parser accepting non-space character · b5b7c303 
      When parsing "ng" packets, we blindly assume that the character
immediately following the "ng" prefix is a space and skip it. As the
calling function doesn't make sure that this is the case, we can thus
end up blindly accepting an invalid packet line.

Fix the issue by using `git__prefixncmp`, checking whether the line
starts with "ng ".

(cherry picked from commit b5ba7af2)
      Patrick Steinhardt committed
    • smart_pkt: fix buffer overflow when parsing "ok" packets · 319f0c03 
      There are two different buffer overflows present when parsing "ok"
packets. First, we never verify whether the line already ends after
"ok", but directly go ahead and also try to skip the expected space
after "ok". Second, we then go ahead and use `strchr` to scan for the
terminating newline character. But in case where the line isn't
terminated correctly, this can overflow the line buffer.

Fix the issues by using `git__prefixncmp` to check for the "ok " prefix
and only checking for a trailing '\n' instead of using `memchr`. This
also fixes the issue of us always requiring a trailing '\n'.

Reported by oss-fuzz, issue 9749:

Crash Type: Heap-buffer-overflow READ {*}
Crash Address: 0x6310000389c0
Crash State:
  ok_pkt
  git_pkt_parse_line
  git_smart__store_refs

Sanitizer: address (ASAN)
(cherry picked from commit a9f1ca09)
      Patrick Steinhardt committed
    • smart_pkt: fix buffer overflow when parsing "ACK" packets · 0599c267 
      We are being quite lenient when parsing "ACK" packets. First, we didn't
correctly verify that we're not overrunning the provided buffer length,
which we fix here by using `git__prefixncmp` instead of
`git__prefixcmp`. Second, we do not verify that the actual contents make
any sense at all, as we simply ignore errors when parsing the ACKs OID
and any unknown status strings. This may result in a parsed packet
structure with invalid contents, which is being silently passed to the
caller. This is being fixed by performing proper input validation and
checking of return codes.

(cherry picked from commit bc349045)
      Patrick Steinhardt committed
    • smart_pkt: adjust style of "ref" packet parsing function · 0fe87761 
      While the function parsing ref packets doesn't have any immediately
obvious buffer overflows, it's style is different to all the other
parsing functions. Instead of checking buffer length while we go, it
does a check up-front. This causes the code to seem a lot more magical
than it really is due to some magic constants. Refactor the function to
instead make use of the style of other packet parser and verify buffer
lengths as we go.

(cherry picked from commit 5edcf5d1)
      Patrick Steinhardt committed
    • smart_pkt: check whether error packets are prefixed with "ERR " · 97156614 
      In the `git_pkt_parse_line` function, we determine what kind of packet
a given packet line contains by simply checking for the prefix of that
line. Except for "ERR" packets, we always only check for the immediate
identifier without the trailing space (e.g. we check for an "ACK"
prefix, not for "ACK "). But for "ERR" packets, we do in fact include
the trailing space in our check. This is not really much of a problem at
all, but it is inconsistent with all the other packet types and thus
causes confusion when the `err_pkt` function just immediately skips the
space without checking whether it overflows the line buffer.

Adjust the check in `git_pkt_parse_line` to not include the trailing
space and instead move it into `err_pkt` for consistency.

(cherry picked from commit 786426ea)
      Patrick Steinhardt committed
    • smart_pkt: explicitly avoid integer overflows when parsing packets · 5c0d1100 
      When parsing data, progress or error packets, we need to copy the
contents of the rest of the current packet line into the flex-array of
the parsed packet. To keep track of this array's length, we then assign
the remaining length of the packet line to the structure. We do have a
mismatch of types here, as the structure's `len` field is a signed
integer, while the length that we are assigning has type `size_t`.

On nearly all platforms, this shouldn't pose any problems at all. The
line length can at most be 16^4, as the line's length is being encoded
by exactly four hex digits. But on a platforms with 16 bit integers,
this assignment could cause an overflow. While such platforms will
probably only exist in the embedded ecosystem, we still want to avoid
this potential overflow. Thus, we now simply change the structure's
`len` member to be of type `size_t` to avoid any integer promotion.

(cherry picked from commit 40fd84cc)
      Patrick Steinhardt committed
    • smart_pkt: honor line length when determining packet type · 20e58aac 
      When we parse the packet type of an incoming packet line, we do not
verify that we don't overflow the provided line buffer. Fix this by
using `git__prefixncmp` instead and passing in `len`. As we have
previously already verified that `len <= linelen`, we thus won't ever
overflow the provided buffer length.

(cherry picked from commit 4a5804c9)
      Patrick Steinhardt committed
    • tests: verify parsing logic for smart packets · bd069448 
      The commits following this commit are about to introduce quite a lot of
refactoring and tightening of the smart packet parser. Unfortunately, we
do not yet have any tests despite our online tests that verify that our
parser does not regress upon changes. This is doubly unfortunate as our
online tests aren't executed by default.

Add new tests that exercise the smart parsing logic directly by
executing `git_pkt_parse_line`.

(cherry picked from commit 365d2720)
      Patrick Steinhardt committed