src


Log

Author Commit Date CI Message
Carlos Martín Nieto a221f58e 2018-10-05T11:47:39 submodule: ignore path and url attributes if they look like options These can be used to inject options in an implementation which performs a recursive clone by executing an external command via crafted url and path attributes such that it triggers a local executable to be run. The library is not vulnerable as we do not rely on external executables but a user of the library might be relying on that so we add this protection. This matches this aspect of git's fix for CVE-2018-17456.
Patrick Steinhardt 614c266d 2018-10-05T10:56:02 config_file: properly ignore includes without "path" value In case a configuration includes a key "include.path=" without any value, the generated configuration entry will have its value set to `NULL`. This is unexpected by the logic handling includes, and as soon as we try to calculate the included path we will unconditionally dereference that `NULL` pointer and thus segfault. Fix the issue by returning early in both `parse_include` and `parse_conditional_include` in case where the `file` argument is `NULL`. Add a test to avoid future regression. The issue has been found by the oss-fuzz project, issue 10810. (cherry picked from commit d06d4220eec035466d1a837972a40546b8904330)
Patrick Steinhardt f5c3442b 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. (cherry picked from commit 1bc5b05c614c7b10de021fa392943e8e6bd12c77)
Patrick Steinhardt f7c3f6cc 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. (cherry picked from commit c05790a8a8dd4351e61fc06c0a06c6a6fb6134dc)
Patrick Steinhardt 7e3cd611 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. (cherry picked from commit 0b3dfbf425d689101663341beb94237614f1b5c2)
Patrick Steinhardt 356f60f4 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. (cherry picked from commit 5fabaca801e1f5e7a1054be612e8fabec7cd6a7f)
Patrick Steinhardt b5b7c303 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 ". (cherry picked from commit b5ba7af2d30c958b090dcf135749d9afe89ec703)
Patrick Steinhardt 319f0c03 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) (cherry picked from commit a9f1ca09178af0640963e069a2142d5ced53f0b4)
Patrick Steinhardt 0599c267 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. (cherry picked from commit bc349045b1be8fb3af2b02d8554483869e54d5b8)
Patrick Steinhardt 0fe87761 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. (cherry picked from commit 5edcf5d190f3b379740b223ff6a649d08fa49581)
Patrick Steinhardt 97156614 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. (cherry picked from commit 786426ea6ec2a76ffe2515dc5182705fb3d44603)
Patrick Steinhardt 5c0d1100 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. (cherry picked from commit 40fd84cca68db24f325e460a40dabe805e7a5d35)
Patrick Steinhardt 20e58aac 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. (cherry picked from commit 4a5804c983317100eed509537edc32d69c8d7aa2)
Nelson Elhage 003cbc3f 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); ``` (cherry picked from commit 437ee5a70711ac2e027877d71ee4ae17e5ec3d6c)
Etienne Samson 4385aef3 2017-08-22T16:29:07 smart: typedef git_pkt_type and clarify recv_pkt return type (cherry picked from commit 08961c9d0d6927bfcc725bd64c9a87dbcca0c52c)
Nelson Elhage 21ffc57d 2018-06-28T05:27:36 Small style tweak, and set an error (cherry picked from commit 895a668e19dc596e7b12ea27724ceb7b68556106)
Nelson Elhage be98c9e9 2018-06-26T02:32:50 Remove GIT_PKT_PACK entirely (cherry picked from commit 90cf86070046fcffd5306915b57786da054d8964)
Christian Schlack bf4342f7 2018-08-11T13:06:14 Fix 'invalid packet line' for ng packets containing errors (cherry picked from commit 50dd7fea5ad1bf6c013b72ad0aa803a9c84cdede)
bisho 15e92284 2018-09-05T11:49:13 Prevent heap-buffer-overflow When running repack while doing repo writes, `packfile_load__cb()` can see some temporary files in the directory that are bigger than the usual, and makes `memcmp` overflow on the `p->pack_name` string. ASAN detected this. This just uses `strncmp`, that should not have any performance impact and is safe for comparing strings of different sizes. ``` ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61200001a3f3 at pc 0x7f4a9e1976ec bp 0x7ffc1f80e100 sp 0x7ffc1f80d8b0 READ of size 89 at 0x61200001a3f3 thread T0 SCARINESS: 26 (multi-byte-read-heap-buffer-overflow) #0 0x7f4a9e1976eb in __interceptor_memcmp.part.78 (/build/cfgr-admin#link-tree/libtools_build_sanitizers_asan-ubsan-py.so+0xcf6eb) #1 0x7f4a518c5431 in packfile_load__cb /build/libgit2/0.27.0/src/libgit2-0.27.0/src/odb_pack.c:213 #2 0x7f4a518d9582 in git_path_direach /build/libgit2/0.27.0/src/libgit2-0.27.0/src/path.c:1134 #3 0x7f4a518c58ad in pack_backend__refresh /build/libgit2/0.27.0/src/libgit2-0.27.0/src/odb_pack.c:347 #4 0x7f4a518c1b12 in git_odb_refresh /build/libgit2/0.27.0/src/libgit2-0.27.0/src/odb.c:1511 #5 0x7f4a518bff5f in git_odb__freshen /build/libgit2/0.27.0/src/libgit2-0.27.0/src/odb.c:752 #6 0x7f4a518c17d4 in git_odb_stream_finalize_write /build/libgit2/0.27.0/src/libgit2-0.27.0/src/odb.c:1415 #7 0x7f4a51b9d015 in Repository_write /build/pygit2/0.27.0/src/pygit2-0.27.0/src/repository.c:509 ``` (cherry picked from commit d22cd1f4a4c10ff47b04c57560e6765d77e5a8fd)
Patrick Steinhardt 39706ded 2018-09-03T10:49:46 config_parse: refactor error handling when parsing multiline variables The current error handling for the multiline variable parser is a bit fragile, as each error condition has its own code to clear memory. Instead, unify error handling as far as possible to avoid this repetitive code. While at it, make use of `GITERR_CHECK_ALLOC` to correctly handle OOM situations and verify that the buffer we print into does not run out of memory either. (cherry picked from commit bc63e1ef521ab5900dc0b0dcd578b8bf18627fb1)
Nelson Elhage 68823395 2018-09-01T03:50:26 config: Fix a leak parsing multi-line config entries (cherry picked from commit 38b852558eb518f96c313cdcd9ce5a7af6ded194)
Nelson Elhage 24c7b23d 2018-08-25T17:04:39 config: convert unbounded recursion into a loop (cherry picked from commit a03113e80332fba6c77f43b21d398caad50b4b89)
Patrick Steinhardt 1f9a8510 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
Patrick Steinhardt c1577110 2018-07-05T13:30:46 delta: fix overflow when computing limit When checking whether a delta base offset and length fit into the base we have in memory already, we can trigger an overflow which breaks the check. This would subsequently result in us reading memory from out of bounds of the base. The issue is easily fixed by checking for overflow when adding `off` and `len`, thus guaranteeting that we are never indexing beyond `base_len`. This corresponds to the git patch 8960844a7 (check patch_delta bounds more carefully, 2006-04-07), which adds these overflow checks. Reported-by: Riccardo Schirone <rschiron@redhat.com>
Patrick Steinhardt 9844d38b 2018-06-29T09:11:02 delta: fix out-of-bounds read of delta When computing the offset and length of the delta base, we repeatedly increment the `delta` pointer without checking whether we have advanced past its end already, which can thus result in an out-of-bounds read. Fix this by repeatedly checking whether we have reached the end. Add a test which would cause Valgrind to produce an error. Reported-by: Riccardo Schirone <rschiron@redhat.com> Test-provided-by: Riccardo Schirone <rschiron@redhat.com>
Patrick Steinhardt 3f461902 2018-06-29T07:45:18 delta: fix sign-extension of big left-shift Our delta code was originally adapted from JGit, which itself adapted it from git itself. Due to this heritage, we inherited a bug from git.git in how we compute the delta offset, which was fixed upstream in 48fb7deb5 (Fix big left-shifts of unsigned char, 2009-06-17). As explained by Linus: Shifting 'unsigned char' or 'unsigned short' left can result in sign extension errors, since the C integer promotion rules means that the unsigned char/short will get implicitly promoted to a signed 'int' due to the shift (or due to other operations). This normally doesn't matter, but if you shift things up sufficiently, it will now set the sign bit in 'int', and a subsequent cast to a bigger type (eg 'long' or 'unsigned long') will now sign-extend the value despite the original expression being unsigned. One example of this would be something like unsigned long size; unsigned char c; size += c << 24; where despite all the variables being unsigned, 'c << 24' ends up being a signed entity, and will get sign-extended when then doing the addition in an 'unsigned long' type. Since git uses 'unsigned char' pointers extensively, we actually have this bug in a couple of places. In our delta code, we inherited such a bogus shift when computing the offset at which the delta base is to be found. Due to the sign extension we can end up with an offset where all the bits are set. This can allow an arbitrary memory read, as the addition in `base_len < off + len` can now overflow if `off` has all its bits set. Fix the issue by casting the result of `*delta++ << 24UL` to an unsigned integer again. Add a test with a crafted delta that would actually succeed with an out-of-bounds read in case where the cast wouldn't exist. Reported-by: Riccardo Schirone <rschiron@redhat.com> Test-provided-by: Riccardo Schirone <rschiron@redhat.com>
Patrick Steinhardt 7392799d 2018-05-30T08:35:06 submodule: detect duplicated submodule paths When loading submodule names, we build a map of submodule paths and their respective names. While looping over the configuration keys, we do not check though whether a submodule path was seen already. This leads to a memory leak in case we have multiple submodules with the same path, as we just overwrite the old value in the map in that case. Fix the error by verifying that the path to be added is not yet part of the string map. Git does not allow to have multiple submodules for a path anyway, so we now do the same and detect this duplication, reporting it to the user.
Patrick Steinhardt f2e5c092 2018-04-27T15:31:43 cmake: remove now-useless LIBGIT2_LIBDIRS handling With the recent change of always resolving pkg-config libraries to their full path, we do not have to manage the LIBGIT2_LIBDIRS variable anymore. The only other remaining user of LIBGIT2_LIBDIRS is winhttp, which is a CMake-style library target and can thus be resolved by CMake automatically. Remove the variable to simplify our build system a bit.
Patrick Steinhardt 0c8ff50f 2018-04-27T10:38:49 cmake: resolve libraries found by pkg-config Libraries found by CMake modules are usually handled with their full path. This makes linking against those libraries a lot more robust when it comes to libraries in non-standard locations, as otherwise we might mix up libraries from different locations when link directories are given. One excemption are libraries found by PKG_CHECK_MODULES. Instead of returning libraries with their complete path, it will return the variable names as well as a set of link directories. In case where multiple sets of the same library are installed in different locations, this can lead the compiler to link against the wrong libraries in the end, when link directories of other dependencies are added. To fix this shortcoming, we need to manually resolve library paths returned by CMake against their respective library directories. This is an easy task to do with `FIND_LIBRARY`.
Etienne Samson a137cdbd 2018-04-18T21:41:44 refspec: check for valid parameters in git_refspec__dwim_one CID:1383993, "In git_refspec__dwim_one: All paths that lead to this null pointer comparison already dereference the pointer earlier (CWE-476)"
Patrick Steinhardt 7fa6c8ce 2018-03-29T10:18:51 util: fix missing headers for MinGW environments There are multiple references to undefined functions in the Microsoft builds. Add headers to make them known.
Bernard Spil 0f09d9f5 2018-04-02T20:00:07 Fix build with LibreSSL 2.7 LibreSSL 2.7 adds OpenSSL 1.1 API Signed-off-by: Bernard Spil <brnrd@FreeBSD.org>
Erik van Zijst 16b62dd4 2018-04-04T21:28:31 diff: Add missing GIT_DELTA_TYPECHANGE -> 'T' mapping. This adds the 'T' status character to git_diff_status_char() for diff entries that change type.
Patrick Steinhardt 07011e60 2018-04-12T13:32:27 revwalk: fix uninteresting revs sometimes not limiting graphwalk When we want to limit our graphwalk, we use the heuristic of checking whether the newest limiting (uninteresting) revision is newer than the oldest interesting revision. We do so by inspecting whether the first item's commit time of the user-supplied list of revisions is newer than the last added interesting revision. This is wrong though, as the user supplied list is in no way guaranteed to be sorted by increasing commit dates. This could lead us to abort the revwalk early before applying all relevant limiting revisions, outputting revisions which should in fact have been hidden. Fix the heuristic by instead checking whether _any_ of the limiting commits was made earlier than the last interesting commit. Add a test.
Sven Strickroth 0f88adb6 2018-02-08T12:36:47 Submodule API should report .gitmodules parse errors Signed-off-by: Sven Strickroth <email@cs-ware.de>
Etienne Samson b2f3ff56 2018-04-19T01:08:18 worktree: fix calloc of the wrong object type
Etienne Samson 8fa0b34b 2018-04-19T01:05:05 local: fix a leaking reference when iterating over a symref Valgrind log : ==17702== 18 bytes in 1 blocks are indirectly lost in loss record 69 of 1,123 ==17702== at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==17702== by 0x5FDBB49: strdup (strdup.c:42) ==17702== by 0x632B3E: git__strdup (util.h:106) ==17702== by 0x632D2C: git_reference__alloc_symbolic (refs.c:64) ==17702== by 0x62E0AF: loose_lookup (refdb_fs.c:408) ==17702== by 0x62E636: refdb_fs_backend__iterator_next (refdb_fs.c:565) ==17702== by 0x62CD8E: git_refdb_iterator_next (refdb.c:147) ==17702== by 0x6347F2: git_reference_next (refs.c:838) ==17702== by 0x6345CB: git_reference_foreach (refs.c:748) ==17702== by 0x66BE62: local_download_pack (local.c:579) ==17702== by 0x5DB48F: git_fetch_download_pack (fetch.c:148) ==17702== by 0x639028: git_remote_download (remote.c:932) ==17702== by 0x63919A: git_remote_fetch (remote.c:969) ==17702== by 0x4ABEDD: test_fetchhead_nonetwork__fetch_into_repo_with_symrefs (nonetwork.c:362) ==17702== by 0x4125D9: clar_run_test (clar.c:222) ==17702== by 0x41287C: clar_run_suite (clar.c:286) ==17702== by 0x412DDE: clar_test_run (clar.c:433) ==17702== by 0x4105E1: main (main.c:24)
Etienne Samson 2fe887e6 2018-04-18T20:57:16 remote: repo is optional here As per CID:1378747, we might be called with a NULL repo, which would be deferenced in write_add_refspec
Sven Strickroth 96329606 2018-03-11T15:35:56 worktree: Read worktree specific reflog for HEAD Signed-off-by: Sven Strickroth <email@cs-ware.de>
Etienne Samson e2a80124 2018-04-10T21:16:43 refs: preserve the owning refdb when duping reference This fixes a segfault in git_reference_owner on references returned from git_reference__read_head and git_reference_dup ones.
Patrick Steinhardt b260fdc8 2018-04-06T12:24:10 attr_file: fix handling of directory patterns with trailing spaces When comparing whether a path matches a directory rule, we pass the both the path and directory name to `fnmatch` with `GIT_ATTR_FNMATCH_DIRECTORY` being set. `fnmatch` expects the pattern to contain no trailing directory '/', which is why we try to always strip patterns of trailing slashes. We do not handle that case correctly though when the pattern itself has trailing spaces, causing the match to fail. Fix the issue by stripping trailing spaces and tabs for a rule previous to checking whether the pattern is a directory pattern with a trailing '/'. This replaces the whitespace-stripping in our ignore file parsing code, which was stripping whitespaces too late. Add a test to catch future breakage.
Patrick Steinhardt a714e836 2018-04-06T10:39:16 transports: local: fix assert when fetching into repo with symrefs When fetching into a repository which has symbolic references via the "local" transport we run into an assert. The assert is being triggered while we negotiate the packfile between the two repositories. When hiding known revisions from the packbuilder revwalk, we unconditionally hide all references of the local refdb. In case one of these references is a symbolic reference, though, this means we're trying to hide a `NULL` OID, which triggers the assert. Fix the issue by only hiding OID references from the revwalk. Add a test to catch this issue in the future.
Patrick Steinhardt 59012bf4 2018-03-29T09:15:48 odb: mempack: fix leaking objects when freeing mempacks When a ODB mempack gets free'd, we take no measures at all to free its contents, most notably the objects added to the database, resulting in a memory leak. Call `git_mempack_reset` previous to freeing the ODB structures themselves, which takes care of releasing all associated data structures.
bgermann 4d4a7dbf 2018-03-28T17:37:39 sha1dc: update to fix errors with endianess This updates the version of SHA1DC to c3e1304ea3.
Patrick Steinhardt b89988c7 2018-03-27T15:03:15 transports: ssh: replace deprecated function `libssh2_session_startup` The function `libssh2_session_startup` has been deprecated since libssh2 version 1.2.8 in favor of `libssh2_session_handshake` introduced in the same version. libssh2 1.2.8 was released in April 2011, so it is already seven years old. It is available in Debian Wheezy, Ubuntu Trusty and CentOS 7.4, so the most important and conservative distros already have it available. As such, it seems safe to just use the new function.
Patrick Steinhardt b2e7d8c2 2018-03-27T14:49:21 transports: ssh: disconnect session before freeing it The function `ssh_stream_free` takes over the responsibility of closing channels and streams just before freeing their memory, but it does not do so for the session. In fact, we never disconnect the session ourselves at all, as libssh2 will not do so itself upon freeing the structure. Quoting the documentation of `libssh2_session_free`: > Frees all resources associated with a session instance. Typically > called after libssh2_session_disconnect_ex, The missing disconnect probably stems from a misunderstanding what it actually does. As we are already closing the TCP socket ourselves, the assumption was that no additional disconnect is required. But calling `libssh2_session_disconnect` will notify the server that we are cleanly closing the connection, such that the server can free his own resources. Add a call to `libssh2_session_disconnect` to fix that issue. [1]: https://www.libssh2.org/libssh2_session_free.html
Carlos Martín Nieto cfed1be8 2018-05-24T20:28:36 submodule: plug leaks from the escape detection
Carlos Martín Nieto f650153a 2018-05-24T19:05:59 submodule: replace index with strchr which exists on Windows
Carlos Martín Nieto a3df20cf 2018-05-24T19:00:13 submodule: the repostiory for _name_is_valid should not be const We might modify caches due to us trying to load the configuration to figure out what kinds of filesystem protections we should have.
Carlos Martín Nieto a9e60994 2018-05-23T08:40:17 path: check for a symlinked .gitmodules in fs-agnostic code We still compare case-insensitively to protect more thoroughly as we don't know what specifics we'll see on the system and it's the behaviour from git.
Carlos Martín Nieto aa003557 2018-05-22T16:13:47 path: reject .gitmodules as a symlink Any part of the library which asks the question can pass in the mode to have it checked against `.gitmodules` being a symlink. This is particularly relevant for adding entries to the index from the worktree and for checking out files.
Carlos Martín Nieto 08d4b459 2018-05-22T15:48:38 index: stat before creating the entry This is so we have it available for the path validity checking. In a later commit we will start rejecting `.gitmodules` files as symlinks.
Carlos Martín Nieto c7cac088 2018-05-22T15:21:08 path: accept the name length as a parameter We may take in names from the middle of a string so we want the caller to let us know how long the path component is that we should be checking.
Carlos Martín Nieto d7ee21ee 2018-05-22T13:58:24 path: expose dotgit detection functions per filesystem These will be used by the checkout code to detect them for the particular filesystem they're on.
Carlos Martín Nieto f907a6f5 2018-05-18T15:16:53 path: hide the dotgit file functions These can't go into the public API yet as we don't want to introduce API or ABI changes in a security release.
Carlos Martín Nieto 0cc14627 2018-05-16T15:56:04 path: add functions to detect .gitconfig and .gitattributes
Carlos Martín Nieto 26b3cec0 2018-05-16T15:42:08 path: add a function to detect an .gitmodules file Given a path component it knows what to pass to the filesystem-specific functions so we're protected even from trees which try to use the 8.3 naming rules to get around us matching on the filename exactly. The logic and test strings come from the equivalent git change.
Carlos Martín Nieto dd364dde 2018-05-16T14:47:04 path: provide a generic function for checking dogit files on NTFS It checks against the 8.3 shortname variants, including the one which includes the checksum as part of its name.
Carlos Martín Nieto 37dc60b6 2018-05-16T11:56:04 path: provide a generic dogit checking function for HFS This lets us check for other kinds of reserved files.
Carlos Martín Nieto 916af8ea 2018-05-14T16:03:15 submodule: also validate Windows-separated paths for validity Otherwise we would also admit `..\..\foo\bar` as a valid path and fail to protect Windows users. Ideally we would check for both separators without the need for the copied string, but this'll get us over the RCE.
Carlos Martín Nieto e6c757a7 2018-04-30T13:47:15 submodule: ignore submodules which include path traversal in their name If the we decide that the "name" of the submodule (i.e. its path inside `.git/modules/`) is trying to escape that directory or otherwise trick us, we ignore the configuration for that submodule. This leaves us with a half-configured submodule when looking it up by path, but it's the same result as if the configuration really were missing. The name check is potentially more strict than it needs to be, but it lets us re-use the check we're doing for the checkout. The function that encapsulates this logic is ready to be exported but we don't want to do that in a security release so it remains internal for now.
Patrick Steinhardt a52b4c51 2018-03-23T09:59:46 odb: fix writing to fake write streams In commit 7ec7aa4a7 (odb: assert on logic errors when writing objects, 2018-02-01), the check for whether we are trying to overflowing the fake stream buffer was changed from returning an error to raising an assert. The conversion forgot though that the logic around `assert`s are basically inverted. Previously, if the statement stream->written + len > steram->size evaluated to true, we would return a `-1`. Now we are asserting that this statement is true, and in case it is not we will raise an error. So the conversion to the `assert` in fact changed the behaviour to the complete opposite intention. Fix the assert by inverting its condition again and add a regression test.
Edward Thomson 0e4f3d9d 2018-03-03T21:47:22 gitno_extract_url_parts: decode hostnames RFC 3986 says that hostnames can be percent encoded. Percent decode hostnames in our URLs.
Edward Thomson 05551ca0 2018-03-03T20:14:54 Remove now unnecessary `gitno_unescape`
Edward Thomson 60e7848e 2018-03-03T20:13:30 gitno_extract_url_parts: use `git_buf`s Now that we can decode percent-encoded strings as part of `git_buf`s, use that decoder in `gitno_extract_url_parts`.
Edward Thomson 6f577906 2018-03-03T20:09:09 ssh urls: use `git_buf_decode_percent` Use `git_buf_decode_percent` so that we can avoid allocating a temporary buffer.
Edward Thomson 8070a357 2018-03-03T18:47:35 Introduce `git_buf_decode_percent` Introduce a function to take a percent-encoded string (URI encoded, described by RFC 1738) and decode it into a `git_buf`.
Steven King Jr 16210877 2018-02-28T12:59:47 Unescape repo before constructing ssh request
Steven King Jr 8a2cdbd3 2018-02-28T12:58:58 Rename unescape and make non-static
Patrick Steinhardt 3db1af1f 2018-03-08T12:36:46 index: error out on unreasonable prefix-compressed path lengths When computing the complete path length from the encoded prefix-compressed path, we end up just allocating the complete path without ever checking what the encoded path length actually is. This can easily lead to a denial of service by just encoding an unreasonable long path name inside of the index. Git already enforces a maximum path length of 4096 bytes. As we also have that enforcement ready in some places, just make sure that the resulting path is smaller than GIT_PATH_MAX. Reported-by: Krishna Ram Prakash R <krp@gtux.in> Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
Patrick Steinhardt 3207ddb0 2018-03-08T12:00:27 index: fix out-of-bounds read with invalid index entry prefix length The index format in version 4 has prefix-compressed entries, where every index entry can compress its path by using a path prefix of the previous entry. Since implmenting support for this index format version in commit 5625d86b9 (index: support index v4, 2016-05-17), though, we do not correctly verify that the prefix length that we want to reuse is actually smaller or equal to the amount of characters than the length of the previous index entry's path. This can lead to a an integer underflow and subsequently to an out-of-bounds read. Fix this by verifying that the prefix is actually smaller than the previous entry's path length. Reported-by: Krishna Ram Prakash R <krp@gtux.in> Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
Patrick Steinhardt 58a6fe94 2018-03-08T11:49:19 index: convert `read_entry` to return entry size via an out-param The function `read_entry` does not conform to our usual coding style of returning stuff via the out parameter and to use the return value for reporting errors. Due to most of our code conforming to that pattern, it has become quite natural for us to actually return `-1` in case there is any error, which has also slipped in with commit 5625d86b9 (index: support index v4, 2016-05-17). As the function returns an `size_t` only, though, the return value is wrapped around, causing the caller of `read_tree` to continue with an invalid index entry. Ultimately, this can lead to a double-free. Improve code and fix the bug by converting the function to return the index entry size via an out parameter and only using the return value to indicate errors. Reported-by: Krishna Ram Prakash R <krp@gtux.in> Reported-by: Vivek Parikh <viv0411.parikh@gmail.com>
Jacques Germishuys 53e692af 2018-03-02T12:49:54 worktree: rename parameter creason to reason
Jacques Germishuys 12356076 2018-03-02T12:41:04 worktree: lock reason should be const
Patrick Steinhardt 8a8ea1db 2018-02-28T18:14:52 Merge pull request #4552 from libgit2/cmn/config-header-common Cast less blindly between configuration objects
Edward Thomson e8e490b2 2018-02-28T17:01:47 Merge pull request #4554 from pks-t/pks/curl-init curl: initialize and cleanup global curl state
Carlos Martín Nieto 9cd0c6f1 2018-02-28T16:01:16 config: return an error if config_refresh is called on a snapshot Instead of treating it as a no-op, treat it as a programming error and return the same kind of error as if you called to set or delete variables on a snapshot.
Patrick Steinhardt 2022b004 2018-02-28T12:06:59 curl: explicitly initialize and cleanup global curl state Our curl-based streams make use of the easy curl interface. This interface automatically initializes and de-initializes the global curl state by calling out to `curl_global_init` and `curl_global_cleanup`. Thus, all global state will be repeatedly re-initialized when creating multiple curl streams in succession. Despite being inefficient, this is not thread-safe due to `curl_global_init` being not thread-safe itself. Thus a multi-threaded programing handling multiple curl streams at the same time is inherently racy. Fix the issue by globally initializing and cleaning up curl's state.
Edward Thomson a33deeb4 2018-02-28T12:20:23 win32: strncmp -> git__strncmp The win32 C library is compiled cdecl, however when configured with `STDCALL=ON`, our functions (and function pointers) will use the stdcall calling convention. You cannot set a `__stdcall` function pointer to a `__cdecl` function, so it's easier to just use our `git__strncmp` instead of sorting that mess out.
Carlos Martín Nieto 2424e64c 2018-02-28T12:06:02 config: harden our use of the backend objects a bit When we create an iterator we don't actually know that we have a live config object and we must instead only rely on the header. We fixed it to use this in a previous commit, but this makes it harder to misuse by converting to use the header object in the typecast. We also guard inside the `config_refresh` function against being given a snapshot (although callers right now do check).
Carlos Martín Nieto 1785de4e 2018-02-28T11:46:17 config: move the level field into the header We use it in a few places where we might have a full object or a snapshot so move it to where we can actually access it.
Carlos Martín Nieto c1524b2e 2018-02-28T11:33:11 config: move the repository to the diskfile header We pass this around and when creating a new iterator we need to read the repository pointer. Put it in a common place so we can reach it regardless of whether we got a full object or a snapshot.
Edward Thomson c9d59c61 2018-02-27T12:45:21 Merge pull request #4545 from libgit2/ethomson/checkout_filemode Respect core.filemode in checkout
Edward Thomson 5ecb6220 2018-02-25T15:46:51 winhttp: enable TLS 1.2 on Windows 7 and earlier Versions of Windows prior to Windows 8 do not enable TLS 1.2 by default, though support may exist. Try to enable TLS 1.2 support explicitly on connections. This request may fail if the operating system does not have TLS 1.2 support - the initial release of Vista lacks TLS 1.2 support (though it is available as a software update) and XP completely lacks TLS 1.2 support. If this request does fail, the HTTP context is still valid, and still maintains the original protocol support. So we ignore the failure from this operation.
Edward Thomson 934e6a3b 2018-02-27T11:24:30 winhttp: include constants for TLS 1.1/1.2 support For platforms that do not define `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1` and/or `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2`.
Edward Thomson 8c8db980 2018-02-27T10:32:29 mingw: update TLS option flags Include the constants for `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1` and `WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2` so that they can be used by mingw. This updates both the `deps/winhttp` framework (for classic mingw) and adds the defines for mingw64, which does not use that framework.
Edward Thomson c214ba19 2018-02-20T00:35:27 checkout: respect core.filemode when comparing filemodes Fixes #4504
Patrick Steinhardt 894ccf4b 2018-02-20T16:14:54 Merge pull request #4535 from libgit2/ethomson/checkout_typechange_with_index_and_wd checkout: when examining index (instead of workdir), also examine mode
Patrick Steinhardt ce7080a0 2018-02-20T10:38:27 diff_tform: fix rename detection with rewrite/delete pair A rewritten file can either be classified as a modification of its contents or of a delete of the complete file followed by an addition of the new content. This distinction becomes important when we want to detect renames for rewrites. Given a scenario where a file "a" has been deleted and another file "b" has been renamed to "a", this should be detected as a deletion of "a" followed by a rename of "a" -> "b". Thus, splitting of the original rewrite into a delete/add pair is important here. This splitting is represented by a flag we can set at the current delta. While the flag is already being set in case we want to break rewrites, we do not do so in case where the `GIT_DIFF_FIND_RENAMES_FROM_REWRITES` flag is set. This can trigger an assert when we try to match the source and target deltas. Fix the issue by setting the `GIT_DIFF_FLAG__TO_SPLIT` flag at the delta when it is a rename target and `GIT_DIFF_FIND_RENAMES_FROM_REWRITES` is set.
Edward Thomson d7fea1e1 2018-02-18T16:10:33 checkout: take mode into account when comparing index to baseline When checking out a file, we determine whether the baseline (what we expect to be in the working directory) actually matches the contents of the working directory. This is safe behavior to prevent us from overwriting changes in the working directory. We look at the index to optimize this test: if we know that the index matches the working directory, then we can simply look at the index data compared to the baseline. We have historically compared the baseline to the index entry by oid. However, we must also compare the mode of the two items to ensure that they are identical. Otherwise, we will refuse to update the working directory for a mode change.
Edward Thomson f1ad004c 2018-02-18T22:29:48 Merge pull request #4529 from libgit2/ethomson/index_add_requires_files git_index_add_frombuffer: only accept files/links
Edward Thomson 5f774dbf 2018-02-11T10:14:13 git_index_add_frombuffer: only accept files/links Ensure that the buffer given to `git_index_add_frombuffer` represents a regular blob, an executable blob, or a link. Explicitly reject commit entries (submodules) - it makes little sense to allow users to add a submodule from a string; there's no possible path to success.
Patrick Steinhardt 92324d84 2018-02-16T11:28:53 util: clean up header includes While "util.h" declares the macro `git__tolower`, which simply resorts to tolower(3P) on Unix-like systems, the <ctype.h> header is only being included in "util.c". Thus, anybody who has included "util.h" without having <ctype.h> included will fail to compile as soon as the macro is in use. Furthermore, we can clean up additional includes in "util.c" and simply replace them with an include for "common.h".
Patrick Steinhardt 06b8a40f 2018-02-16T11:29:46 Explicitly mark fallthrough cases with comments A lot of compilers nowadays generate warnings when there are cases in a switch statement which implicitly fall through to the next case. To avoid this warning, the last line in the case that is falling through can have a comment matching a regular expression, where one possible comment body would be `/* fall through */`. An alternative to the comment would be an explicit attribute like e.g. `[[clang::fallthrough]` or `__attribute__ ((fallthrough))`. But GCC only introduced support for such an attribute recently with GCC 7. Thus, and also because the fallthrough comment is supported by most compilers, we settle for using comments instead. One shortcoming of that method is that compilers are very strict about that. Most interestingly, that comment _really_ has to be the last line. In case a closing brace follows the comment, the heuristic will fail.
Patrick Steinhardt 7c6e9175 2018-02-16T11:11:11 index: shut up warning on uninitialized variable Even though the `entry` variable will always be initialized when `read_entry` returns success and even though we never dereference `entry` in case `read_entry` fails, GCC prints a warning about uninitialized use. Just initialize the pointer to `NULL` in order to shut GCC up.
Patrick Steinhardt 84f03b3a 2018-02-16T10:48:55 streams: openssl: fix use of uninitialized variable When verifying the server certificate, we do try to make sure that the hostname actually matches the certificate alternative names. In cases where the host is either an IPv4 or IPv6 address, we have to compare the binary representations of the hostname with the declared IP address of the certificate. We only do that comparison in case we were successfully able to parse the hostname as an IP, which would always result in the memory region being initialized. Still, GCC 6.4.0 was complaining about usage of non-initialized memory. Fix the issue by simply asserting that `addr` needs to be initialized. This shuts up the GCC warning.
Edward Thomson ee6be190 2018-01-31T08:36:19 http: standardize user-agent addition The winhttp and posix http each need to add the user-agent to their requests. Standardize on a single function to include this so that we do not get the version numbers we're sending out of sync. Assemble the complete user agent in `git_http__user_agent`, returning assembled strings. Co-authored-by: Patrick Steinhardt <ps@pks.im>
Patrick Steinhardt 178fda8a 2018-02-09T17:55:18 hash: win32: fix missing comma in `giterr_set`
Patrick Steinhardt 638c6b8c 2018-02-09T17:32:15 odb_loose: only close file descriptor if it was opened successfully
Patrick Steinhardt a43bcd2c 2018-02-09T17:31:50 odb: fix memory leaks due to not freeing hash context