1. 13 Dec, 2019 1 commit
    • smart_pkt: fix overflow resulting in OOB read/write of one byte · 86852613
      When parsing OK packets, we copy any information after the initial "ok "
      prefix into the resulting packet. As newlines act as packet boundaries,
      we also strip the trailing newline if there is any. We do not check
      whether there is any data left after the initial "ok " prefix though,
      which leads to a pointer overflow in that case as `len == 0`:
      
      	if (line[len - 1] == '\n')
      		--len;
      
      This out-of-bounds read is a rather useless gadget, as we can only
      deduce whether at some offset there is a newline character. In case
      there accidentally is one, we overflow `len` to `SIZE_MAX` and then
      write a NUL byte into an array indexed by it:
      
      	pkt->ref[len] = '\0';
      
      Again, this doesn't seem like something that's possible to be exploited
      in any meaningful way, but it may surely lead to inconsistencies or DoS.
      
      Fix the issue by checking whether there is any trailing data after the
      packet prefix.
      Patrick Steinhardt committed
  2. 22 Jan, 2019 1 commit
  3. 18 Oct, 2018 1 commit
  4. 03 Oct, 2018 11 commits
    • smart_pkt: do not accept callers passing in no line length · 1bc5b05c
      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.
      Patrick Steinhardt committed
    • smart_pkt: return parsed length via out-parameter · c05790a8
      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.
      Patrick Steinhardt committed
    • smart_pkt: reorder and rename parameters of `git_pkt_parse_line` · 0b3dfbf4
      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.
      Patrick Steinhardt committed
    • smart_pkt: fix buffer overflow when parsing "unpack" packets · 5fabaca8
      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.
      Patrick Steinhardt committed
    • smart_pkt: fix "ng" parser accepting non-space character · b5ba7af2
      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 ".
      Patrick Steinhardt committed
    • smart_pkt: fix buffer overflow when parsing "ok" packets · a9f1ca09
      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)
      Patrick Steinhardt committed
    • smart_pkt: fix buffer overflow when parsing "ACK" packets · bc349045
      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.
      Patrick Steinhardt committed
    • smart_pkt: adjust style of "ref" packet parsing function · 5edcf5d1
      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.
      Patrick Steinhardt committed
    • smart_pkt: check whether error packets are prefixed with "ERR " · 786426ea
      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.
      Patrick Steinhardt committed
    • smart_pkt: explicitly avoid integer overflows when parsing packets · 40fd84cc
      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.
      Patrick Steinhardt committed
    • smart_pkt: honor line length when determining packet type · 4a5804c9
      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.
      Patrick Steinhardt committed
  5. 17 Aug, 2018 1 commit
  6. 19 Jul, 2018 1 commit
    • smart_pkt: fix potential OOB-read when processing ng packet · 19bed3e2
      OSS-fuzz has reported a potential out-of-bounds read when processing a
      "ng" smart packet:
      
      ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6310000249c0 at pc 0x000000493a92 bp 0x7ffddc882cd0 sp 0x7ffddc882480
      	READ of size 65529 at 0x6310000249c0 thread T0
      	SCARINESS: 26 (multi-byte-read-heap-buffer-overflow)
      	#0 0x493a91 in __interceptor_strchr.part.35 /src/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:673
      	#1 0x813960 in ng_pkt libgit2/src/transports/smart_pkt.c:320:14
      	#2 0x810f79 in git_pkt_parse_line libgit2/src/transports/smart_pkt.c:478:9
      	#3 0x82c3c9 in git_smart__store_refs libgit2/src/transports/smart_protocol.c:47:12
      	#4 0x6373a2 in git_smart__connect libgit2/src/transports/smart.c:251:15
      	#5 0x57688f in git_remote_connect libgit2/src/remote.c:708:15
      	#6 0x52e59b in LLVMFuzzerTestOneInput /src/download_refs_fuzzer.cc:145:9
      	#7 0x52ef3f in ExecuteFilesOnyByOne(int, char**) /src/libfuzzer/afl/afl_driver.cpp:301:5
      	#8 0x52f4ee in main /src/libfuzzer/afl/afl_driver.cpp:339:12
      	#9 0x7f6c910db82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/libc-start.c:291
      	#10 0x41d518 in _start
      
      When parsing an "ng" packet, we keep track of both the current position
      as well as the remaining length of the packet itself. But instead of
      taking care not to exceed the length, we pass the current pointer's
      position to `strchr`, which will search for a certain character until
      hitting NUL. It is thus possible to create a crafted packet which
      doesn't contain a NUL byte to trigger an out-of-bounds read.
      
      Fix the issue by instead using `memchr`, passing the remaining length as
      restriction. Furthermore, verify that we actually have enough bytes left
      to produce a match at all.
      
      OSS-Fuzz-Issue: 9406
      Patrick Steinhardt committed
  7. 28 Jun, 2018 1 commit
  8. 26 Jun, 2018 1 commit
  9. 25 Jun, 2018 1 commit
  10. 24 Jun, 2018 1 commit
  11. 10 Jun, 2018 1 commit
  12. 06 Jan, 2017 2 commits
    • smart_pkt: treat empty packet lines as error · 2fdef641
      The Git protocol does not specify what should happen in the case
      of an empty packet line (that is a packet line "0004"). We
      currently indicate success, but do not return a packet in the
      case where we hit an empty line. The smart protocol was not
      prepared to handle such packets in all cases, though, resulting
      in a `NULL` pointer dereference.
      
      Fix the issue by returning an error instead. As such kind of
      packets is not even specified by upstream, this is the right
      thing to do.
      Patrick Steinhardt committed
    • smart_pkt: verify packet length exceeds PKT_LEN_SIZE · 66e3774d
      Each packet line in the Git protocol is prefixed by a four-byte
      length of how much data will follow, which we parse in
      `git_pkt_parse_line`. The transmitted length can either be equal
      to zero in case of a flush packet or has to be at least of length
      four, as it also includes the encoded length itself. Not
      checking this may result in a buffer overflow as we directly pass
      the length to functions which accept a `size_t` length as
      parameter.
      
      Fix the issue by verifying that non-flush packets have at least a
      length of `PKT_LEN_SIZE`.
      Patrick Steinhardt committed
  13. 29 Dec, 2016 1 commit
  14. 07 Jun, 2016 1 commit
    • transports: smart: fix potential invalid memory dereferences · 7d02019a
      When we receive a packet of exactly four bytes encoding its
      length as those four bytes it can be treated as an empty line.
      While it is not really specified how those empty lines should be
      treated, we currently ignore them and do not return an error when
      trying to parse it but simply advance the data pointer.
      
      Callers invoking `git_pkt_parse_line` are currently not prepared
      to handle this case as they do not explicitly check this case.
      While they could always reset the passed out-pointer to `NULL`
      before calling `git_pkt_parse_line` and determine if the pointer
      has been set afterwards, it makes more sense to update
      `git_pkt_parse_line` to set the out-pointer to `NULL` itself when
      it encounters such an empty packet. Like this it is guaranteed
      that there will be no invalid memory references to free'd
      pointers.
      
      As such, the issue has been fixed such that `git_pkt_parse_line`
      always sets the packet out pointer to `NULL` when an empty packet
      has been received and callers check for this condition, skipping
      such packets.
      Patrick Steinhardt committed
  15. 23 Feb, 2016 2 commits
  16. 18 Feb, 2016 1 commit
  17. 17 Aug, 2015 1 commit
  18. 12 Jul, 2015 1 commit
  19. 13 Feb, 2015 4 commits
  20. 21 May, 2014 1 commit
  21. 21 Apr, 2014 1 commit
  22. 30 Oct, 2013 1 commit
    • protocol: basic support for multi_ack_detailed · 2f8c481c
      This tells the server that we speak it, but we don't make use of its
      extra information to determine if there's a better place to stop
      negotiating.
      
      In a somewhat-related change, reorder the capabilities so we ask for
      them in the same order as git does.
      
      Also take this opportunity to factor out a fairly-indented portion of
      the negotiation logic.
      Carlos Martín Nieto committed
  23. 04 Oct, 2013 1 commit
  24. 12 Mar, 2013 1 commit
  25. 08 Jan, 2013 1 commit