src/transports/smart_pkt.c


Log

Author Commit Date CI Message
Edward Thomson f0e693b1 2021-09-07T17:53:49 str: introduce `git_str` for internal, `git_buf` is external libgit2 has two distinct requirements that were previously solved by `git_buf`. We require: 1. A general purpose string class that provides a number of utility APIs for manipulating data (eg, concatenating, truncating, etc). 2. A structure that we can use to return strings to callers that they can take ownership of. By using a single class (`git_buf`) for both of these purposes, we have confused the API to the point that refactorings are difficult and reasoning about correctness is also difficult. Move the utility class `git_buf` to be called `git_str`: this represents its general purpose, as an internal string buffer class. The name also is an homage to Junio Hamano ("gitstr"). The public API remains `git_buf`, and has a much smaller footprint. It is generally only used as an "out" param with strict requirements that follow the documentation. (Exceptions exist for some legacy APIs to avoid breaking callers unnecessarily.) Utility functions exist to convert a user-specified `git_buf` to a `git_str` so that we can call internal functions, then converting it back again.
Patrick Steinhardt 86852613 2019-12-13T12:13:05 smart_pkt: fix overflow resulting in OOB read/write of one byte 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.
Edward Thomson f673e232 2018-12-27T13:47:34 git_error: use new names in internal APIs and usage Move to the `git_error` name in the internal API for error-related functions.
Patrick Steinhardt 2613fbb2 2018-10-18T11:58:14 global: replace remaining use of `git__strtol32` Replace remaining uses of the `git__strtol32` function. While these uses are all safe as the strings were either sanitized or from a trusted source, we want to remove `git__strtol32` altogether to avoid future misuse.
Patrick Steinhardt 1bc5b05c 2018-10-03T16:17:21 smart_pkt: do not accept callers passing in no line length 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 c05790a8 2018-08-09T11:16:15 smart_pkt: return parsed length via out-parameter 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 0b3dfbf4 2018-08-09T11:13:59 smart_pkt: reorder and rename parameters of `git_pkt_parse_line` 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 5fabaca8 2018-08-09T11:04:42 smart_pkt: fix buffer overflow when parsing "unpack" packets 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 b5ba7af2 2018-08-09T11:03:37 smart_pkt: fix "ng" parser accepting non-space character 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 a9f1ca09 2018-08-09T11:01:00 smart_pkt: fix buffer overflow when parsing "ok" packets 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 bc349045 2018-08-09T10:38:10 smart_pkt: fix buffer overflow when parsing "ACK" packets 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 5edcf5d1 2018-08-09T10:57:06 smart_pkt: adjust style of "ref" packet parsing function 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 786426ea 2018-08-09T10:46:58 smart_pkt: check whether error packets are prefixed with "ERR " 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 40fd84cc 2018-08-09T10:46:26 smart_pkt: explicitly avoid integer overflows when parsing packets 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 4a5804c9 2018-08-09T10:36:44 smart_pkt: honor line length when determining packet type 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.
Christian Schlack 50dd7fea 2018-08-11T13:06:14 Fix 'invalid packet line' for ng packets containing errors
Patrick Steinhardt 19bed3e2 2018-07-19T13:00:42 smart_pkt: fix potential OOB-read when processing ng packet 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
Nelson Elhage b8408557 2018-06-29T16:53:23 Merge remote-tracking branch 'origin/master' into no-pkt-pack
Nelson Elhage 895a668e 2018-06-28T05:27:36 Small style tweak, and set an error
Edward Thomson 12232a5e 2018-06-27T17:19:37 Merge pull request #4698 from nelhage/fix-leaks Fix assorted leaks found via fuzzing
Nelson Elhage 90cf8607 2018-06-26T02:32:50 Remove GIT_PKT_PACK entirely
Nelson Elhage 3a547417 2018-06-25T15:38:29 git_pkt_free: Allow freeing NULL
Nelson Elhage 437ee5a7 2018-06-24T19:47:08 Verify ref_pkt's are long enough If the remote sends a too-short packet, we'll allow `len` to go negative and eventually issue a malloc for <= 0 bytes on ``` pkt->head.name = git__malloc(alloclen); ```
Patrick Steinhardt ecf4f33a 2018-02-08T11:14:48 Convert usage of `git_buf_free` to new `git_buf_dispose`
Carlos Martín Nieto a6d833a2 2017-01-13T17:05:58 Merge pull request #4049 from libgit2/ethomson/error_msgs giterr_set: consistent error messages
Patrick Steinhardt 2fdef641 2016-11-15T11:44:51 smart_pkt: treat empty packet lines as error 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 66e3774d 2016-11-15T11:36:27 smart_pkt: verify packet length exceeds PKT_LEN_SIZE 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`.
Edward Thomson 909d5494 2016-12-29T12:25:15 giterr_set: consistent error messages Error messages should be sentence fragments, and therefore: 1. Should not begin with a capital letter, 2. Should not conclude with punctuation, and 3. Should not end a sentence and begin a new one
Patrick Steinhardt 7d02019a 2016-06-06T12:59:17 transports: smart: fix potential invalid memory dereferences 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 003c5e46 2016-02-22T15:52:49 transports: smart_pkt: fix memory leaks on error paths
Patrick Steinhardt 6e2a3755 2016-02-23T11:45:43 smart_pkt: check buffer with GITERR_CHECK_ALLOC_BUF
Patrick Steinhardt b0f7512f 2016-02-15T11:46:10 transports: smart_pkt: fix memory leaks
Anders Borum 2d1d2bb5 2015-08-05T18:50:25 Include the 4 characters not recognised as hex-number when setting error in parse_len
Matthew Plough 768f8be3 2015-06-30T19:00:41 Fix #3094 - improve use of portable size_t/ssize_t format specifiers. The header src/cc-compat.h defines portable format specifiers PRIuZ, PRIdZ, and PRIxZ. The original report highlighted the need to use these specifiers in examples/network/fetch.c. For this commit, I checked all C source and header files not in deps/ and transitioned to the appropriate format specifier where appropriate.
Edward Thomson f1453c59 2015-02-12T12:19:37 Make our overflow check look more like gcc/clang's Make our overflow checking look more like gcc and clang's, so that we can substitute it out with the compiler instrinsics on platforms that support it. This means dropping the ability to pass `NULL` as an out parameter. As a result, the macros also get updated to reflect this as well.
Edward Thomson ec3b4d35 2015-02-11T11:20:05 Use `size_t` to hold size of arrays Use `size_t` to hold the size of arrays to ease overflow checking, lest we check for overflow of a `size_t` then promptly truncate by packing the length into a smaller type.
Edward Thomson 4aa664ae 2015-02-10T23:55:07 git_buf_grow_by: increase buf asize incrementally Introduce `git_buf_grow_by` to incrementally increase the size of a `git_buf`, performing an overflow calculation on the growth.
Edward Thomson 392702ee 2015-02-09T23:41:13 allocations: test for overflow of requested size Introduce some helper macros to test integer overflow from arithmetic and set error message appropriately.
Carlos Martín Nieto 306475eb 2014-05-20T09:55:26 remote: expose the remote's symref mappings Add a symref_target field to git_remote_head to expose the symref mappings to the user.
Jacques Germishuys 98020d3a 2014-04-21T10:55:37 Rename progress callback to sideband_progress
Carlos Martín Nieto 2f8c481c 2013-10-08T16:22:21 protocol: basic support for multi_ack_detailed 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 b4342b11 2013-10-04T10:27:45 net: advertise our support for fixing thin packs
Philip Kelley b8c32580 2013-03-12T15:19:32 Advertise and support side-band-64k when calling receive-pack
Edward Thomson 359fc2d2 2013-01-08T17:07:25 update copyrights
Philip Kelley 6762fe08 2012-11-29T08:29:26 Remove casts of return values of type void *
Philip Kelley 613d5eb9 2012-11-28T11:42:37 Push! By schu, phkelley, and congyiwu, et al
Philip Kelley 41fb1ca0 2012-10-29T13:41:14 Reorganize transport architecture (squashed 3)