Skip to main content

Browse the archive

https://github.com/git/git
31 May 2024, 23:06:24 UTC
sort by:
Revision Author Date Message Commit Date
3c7896e Junio C Hamano 20 January 2023, 23:36:21 UTC Merge branch 'backport/jk/curl-avoid-deprecated-api' into maint-2.30 Deal with a few deprecation warning from cURL library. * jk/curl-avoid-deprecated-api: http: support CURLOPT_PROTOCOLS_STR http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT 22 March 2023, 17:00:36 UTC
6f5ff3a Junio C Hamano 10 December 2022, 07:17:47 UTC Merge branch 'backport/jx/ci-ubuntu-fix' into maint-2.30 Adjust the GitHub CI to newer ubuntu release. * jx/ci-ubuntu-fix: github-actions: run gcc-8 on ubuntu-20.04 image ci: install python on ubuntu ci: use the same version of p4 on both Linux and macOS ci: remove the pipe after "p4 -V" to catch errors 22 March 2023, 17:00:35 UTC
0737200 Junio C Hamano 08 June 2022, 21:27:50 UTC Merge branch 'backport/jc/http-clear-finished-pointer' into maint-2.30 Meant to go with js/ci-gcc-12-fixes. source: <xmqq7d68ytj8.fsf_-_@gitster.g> * jc/http-clear-finished-pointer: http.c: clear the 'finished' member once we are done with it 22 March 2023, 17:00:34 UTC
0a1dc55 Junio C Hamano 01 June 2022, 02:10:35 UTC Merge branch 'backport/js/ci-gcc-12-fixes' Fixes real problems noticed by gcc 12 and works around false positives. * js/ci-gcc-12-fixes: nedmalloc: avoid new compile error compat/win32/syslog: fix use-after-realloc 22 March 2023, 17:00:34 UTC
5843080 Junio C Hamano 02 March 2023, 16:44:16 UTC http.c: clear the 'finished' member once we are done with it In http.c, the run_active_slot() function allows the given "slot" to make progress by calling step_active_slots() in a loop repeatedly, and the loop is not left until the request held in the slot completes. Ages ago, we used to use the slot->in_use member to get out of the loop, which misbehaved when the request in "slot" completes (at which time, the result of the request is copied away from the slot, and the in_use member is cleared, making the slot ready to be reused), and the "slot" gets reused to service a different request (at which time, the "slot" becomes in_use again, even though it is for a different request). The loop terminating condition mistakenly thought that the original request has yet to be completed. Today's code, after baa7b67d (HTTP slot reuse fixes, 2006-03-10) fixed this issue, uses a separate "slot->finished" member that is set in run_active_slot() to point to an on-stack variable, and the code that completes the request in finish_active_slot() clears the on-stack variable via the pointer to signal that the particular request held by the slot has completed. It also clears the in_use member (as before that fix), so that the slot itself can safely be reused for an unrelated request. One thing that is not quite clean in this arrangement is that, unless the slot gets reused, at which point the finished member is reset to NULL, the member keeps the value of &finished, which becomes a dangling pointer into the stack when run_active_slot() returns. Clear the finished member before the control leaves the function, which has a side effect of unconfusing compilers like recent GCC 12 that is over-eager to warn against such an assignment. Signed-off-by: Junio C Hamano <gitster@pobox.com> 22 March 2023, 16:58:29 UTC
321854a Johannes Schindelin 24 May 2022, 00:23:06 UTC clone.c: avoid "exceeds maximum object size" error with GCC v12.x Technically, the pointer difference `end - start` _could_ be negative, and when cast to an (unsigned) `size_t` that would cause problems. In this instance, the symptom is: dir.c: In function 'git_url_basename': dir.c:3087:13: error: 'memchr' specified bound [9223372036854775808, 0] exceeds maximum object size 9223372036854775807 [-Werror=stringop-overread] CC ewah/bitmap.o 3087 | if (memchr(start, '/', end - start) == NULL | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ While it is a bit far-fetched to think that `end` (which is defined as `repo + strlen(repo)`) and `start` (which starts at `repo` and never steps beyond the NUL terminator) could result in such a negative difference, GCC has no way of knowing that. See also https://gcc.gnu.org/bugzilla//show_bug.cgi?id=85783. Let's just add a safety check, primarily for GCC's benefit. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> 22 March 2023, 16:53:32 UTC
07f91e5 Jeff King 17 January 2023, 03:04:48 UTC http: support CURLOPT_PROTOCOLS_STR The CURLOPT_PROTOCOLS (and matching CURLOPT_REDIR_PROTOCOLS) flag was deprecated in curl 7.85.0, and using it generate compiler warnings as of curl 7.87.0. The path forward is to use CURLOPT_PROTOCOLS_STR, but we can't just do so unilaterally, as it was only introduced less than a year ago in 7.85.0. Until that version becomes ubiquitous, we have to either disable the deprecation warning or conditionally use the "STR" variant on newer versions of libcurl. This patch switches to the new variant, which is nice for two reasons: - we don't have to worry that silencing curl's deprecation warnings might cause us to miss other more useful ones - we'd eventually want to move to the new variant anyway, so this gets us set up (albeit with some extra ugly boilerplate for the conditional) There are a lot of ways to split up the two cases. One way would be to abstract the storage type (strbuf versus a long), how to append (strbuf_addstr vs bitwise OR), how to initialize, which CURLOPT to use, and so on. But the resulting code looks pretty magical: GIT_CURL_PROTOCOL_TYPE allowed = GIT_CURL_PROTOCOL_TYPE_INIT; if (...http is allowed...) GIT_CURL_PROTOCOL_APPEND(&allowed, "http", CURLOPT_HTTP); and you end up with more "#define GIT_CURL_PROTOCOL_TYPE" macros than actual code. On the other end of the spectrum, we could just implement two separate functions, one that handles a string list and one that handles bits. But then we end up repeating our list of protocols (http, https, ftp, ftp). This patch takes the middle ground. The run-time code is always there to handle both types, and we just choose which one to feed to curl. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:54 UTC
a69043d Jiang Xin 25 November 2022, 09:59:54 UTC ci: install python on ubuntu Python is missing from the default ubuntu-22.04 runner image, which prevents git-p4 from working. To install python on ubuntu, we need to provide the correct package names: * On Ubuntu 18.04 (bionic), "/usr/bin/python2" is provided by the "python" package, and "/usr/bin/python3" is provided by the "python3" package. * On Ubuntu 20.04 (focal) and above, "/usr/bin/python2" is provided by the "python2" package which has a different name from bionic, and "/usr/bin/python3" is provided by "python3". Since the "ubuntu-latest" runner image has a higher version, its safe to use "python2" or "python3" package name. Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:54 UTC
b0e3e2d Jeff King 17 January 2023, 03:04:44 UTC http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION The IOCTLFUNCTION option has been deprecated, and generates a compiler warning in recent versions of curl. We can switch to using SEEKFUNCTION instead. It was added in 2008 via curl 7.18.0; our INSTALL file already indicates we require at least curl 7.19.4. But there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL}, and those didn't arrive until 7.19.5. One workaround would be to use a bare 0/1 here (or define our own macros). But let's just bump the minimum required version to 7.19.5. That version is only a minor version bump from our existing requirement, and is only a 2 month time bump for versions that are almost 13 years old. So it's not likely that anybody cares about the distinction. Switching means we have to rewrite the ioctl functions into seek functions. In some ways they are simpler (seeking is the only operation), but in some ways more complex (the ioctl allowed only a full rewind, but now we can seek to arbitrary offsets). Curl will only ever use SEEK_SET (per their documentation), so I didn't bother implementing anything else, since it would naturally be completely untested. This seems unlikely to change, but I added an assertion just in case. Likewise, I doubt curl will ever try to seek outside of the buffer sizes we've told it, but I erred on the defensive side here, rather than do an out-of-bounds read. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:54 UTC
fda237c Jeff King 17 January 2023, 03:04:38 UTC http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT The two options do exactly the same thing, but the latter has been deprecated and in recent versions of curl may produce a compiler warning. Since the UPLOAD form is available everywhere (it was introduced in the year 2000 by curl 7.1), we can just switch to it. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:54 UTC
79e0626 Jiang Xin 25 November 2022, 09:59:53 UTC ci: use the same version of p4 on both Linux and macOS There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions. By checking each version from [1], p4d version 21.1 and above can work properly on ubuntu 22.04. But version 22.x will break some p4 test cases. So p4 version 21.x is exactly the version we can use. With this update, the versions of p4 for Linux and macOS happen to be the same. So we can add the version number directly into the "P4WHENCE" variable, and reuse it in p4 installation for macOS. By removing the "LINUX_P4_VERSION" variable from "ci/lib.sh", the comment left above has nothing to do with p4, but still applies to git-lfs. Since we have a fixed version of git-lfs installed on Linux, we may have a different version on macOS. [1]: https://cdist2.perforce.com/perforce/ Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:53 UTC
20854bc Jiang Xin 25 November 2022, 09:59:52 UTC ci: remove the pipe after "p4 -V" to catch errors When installing p4 as a dependency, we used to pipe output of "p4 -V" and "p4d -V" to validate the installation and output a condensed version information. But this would hide potential errors of p4 and would stop with an empty output. E.g.: p4d version 16.2 running on ubuntu 22.04 causes sigfaults, even before it produces any output. By removing the pipe after "p4 -V" and "p4d -V", we may get a verbose output, and stop immediately on errors because we have "set -e" in "ci/lib.sh". Since we won't look at these trace logs unless something fails, just including the raw output seems most sensible. Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:53 UTC
86f6f4f Johannes Schindelin 24 May 2022, 00:23:04 UTC nedmalloc: avoid new compile error GCC v12.x complains thusly: compat/nedmalloc/nedmalloc.c: In function 'DestroyCaches': compat/nedmalloc/nedmalloc.c:326:12: error: the comparison will always evaluate as 'true' for the address of 'caches' will never be NULL [-Werror=address] 326 | if(p->caches) | ^ compat/nedmalloc/nedmalloc.c:196:22: note: 'caches' declared here 196 | threadcache *caches[THREADCACHEMAXCACHES]; | ^~~~~~ ... and it is correct, of course. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:53 UTC
c03ffcf Jiang Xin 25 November 2022, 09:59:51 UTC github-actions: run gcc-8 on ubuntu-20.04 image GitHub starts to upgrade its runner image "ubuntu-latest" from version "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and install "gcc-8" package on the new runner image. Change the runner image of the `linux-gcc` job from "ubuntu-latest" to "ubuntu-20.04" in order to install "gcc-8" as a dependency. Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:53 UTC
417fb91 Johannes Schindelin 24 May 2022, 00:23:03 UTC compat/win32/syslog: fix use-after-realloc Git for Windows' SDK recently upgraded to GCC v12.x which points out that the `pos` variable might be used even after the corresponding memory was `realloc()`ed and therefore potentially no longer valid. Since a subset of this SDK is used in Git's CI/PR builds, we need to fix this to continue to be able to benefit from the CI/PR runs. Note: This bug has been with us since 2a6b149c64f6 (mingw: avoid using strbuf in syslog, 2011-10-06), and while it looks tempting to replace the hand-rolled string manipulation with a `strbuf`-based one, that commit's message explains why we cannot do that: The `syslog()` function is called as part of the function in `daemon.c` which is set as the `die()` routine, and since `strbuf_grow()` can call that function if it runs out of memory, this would cause a nasty infinite loop that we do not want to re-introduce. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:52 UTC
394a759 Junio C Hamano 03 February 2023, 22:58:10 UTC Git 2.30.8 Signed-off-by: Junio C Hamano <gitster@pobox.com> 06 February 2023, 08:14:45 UTC
a3033a6 Junio C Hamano 03 February 2023, 22:57:27 UTC Merge branch 'ps/apply-beyond-symlink' into maint-2.30 Fix a vulnerability (CVE-2023-23946) that allows crafted input to trick `git apply` into writing files outside of the working tree. * ps/apply-beyond-symlink: dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 06 February 2023, 08:12:16 UTC
2c9a4c7 Taylor Blau 25 January 2023, 19:58:38 UTC Merge branch 'tb/clone-local-symlinks' into maint-2.30 Resolve a security vulnerability (CVE-2023-22490) where `clone_local()` is used in conjunction with non-local transports, leading to arbitrary path exfiltration. * tb/clone-local-symlinks: dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS clone: delay picking a transport until after get_repo_path() t5619: demonstrate clone_local() with ambiguous transport 06 February 2023, 08:09:14 UTC
fade728 Patrick Steinhardt 02 February 2023, 10:54:34 UTC apply: fix writing behind newly created symbolic links When writing files git-apply(1) initially makes sure that none of the files it is about to create are behind a symlink: ``` $ git init repo Initialized empty Git repository in /tmp/repo/.git/ $ cd repo/ $ ln -s dir symlink $ git apply - <<EOF diff --git a/symlink/file b/symlink/file new file mode 100644 index 0000000..e69de29 EOF error: affected file 'symlink/file' is beyond a symbolic link ``` This safety mechanism is crucial to ensure that we don't write outside of the repository's working directory. It can be fooled though when the patch that is being applied creates the symbolic link in the first place, which can lead to writing files in arbitrary locations. Fix this by checking whether the path we're about to create is beyond a symlink or not. Tightening these checks like this should be fine as we already have these precautions in Git as explained above. Ideally, we should update the check we do up-front before starting to reflect the computed changes to the working tree so that we catch this case as well, but as part of embargoed security work, adding an equivalent check just before we try to write out a file should serve us well as a reasonable first step. Digging back into history shows that this vulnerability has existed since at least Git v2.9.0. As Git v2.8.0 and older don't build on my system anymore I cannot tell whether older versions are affected, as well. Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 03 February 2023, 22:41:31 UTC
bffc762 Taylor Blau 25 January 2023, 00:43:51 UTC dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS When using the dir_iterator API, we first stat(2) the base path, and then use that as a starting point to enumerate the directory's contents. If the directory contains symbolic links, we will immediately die() upon encountering them without the `FOLLOW_SYMLINKS` flag. The same is not true when resolving the top-level directory, though. As explained in a previous commit, this oversight in 6f054f9fb3 (builtin/clone.c: disallow `--local` clones with symlinks, 2022-07-28) can be used as an attack vector to include arbitrary files on a victim's filesystem from outside of the repository. Prevent resolving top-level symlinks unless the FOLLOW_SYMLINKS flag is given, which will cause clones of a repository with a symlink'd "$GIT_DIR/objects" directory to fail. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 25 January 2023, 00:52:16 UTC
cf8f6ce Taylor Blau 25 January 2023, 00:43:48 UTC clone: delay picking a transport until after get_repo_path() In the previous commit, t5619 demonstrates an issue where two calls to `get_repo_path()` could trick Git into using its local clone mechanism in conjunction with a non-local transport. That sequence is: - the starting state is that the local path https:/example.com/foo is a symlink that points to ../../../.git/modules/foo. So it's dangling. - get_repo_path() sees that no such path exists (because it's dangling), and thus we do not canonicalize it into an absolute path - because we're using --separate-git-dir, we create .git/modules/foo. Now our symlink is no longer dangling! - we pass the url to transport_get(), which sees it as an https URL. - we call get_repo_path() again, on the url. This second call was introduced by f38aa83f9a (use local cloning if insteadOf makes a local URL, 2014-07-17). The idea is that we want to pull the url fresh from the remote.c API, because it will apply any aliases. And of course now it sees that there is a local file, which is a mismatch with the transport we already selected. The issue in the above sequence is calling `transport_get()` before deciding whether or not the repository is indeed local, and not passing in an absolute path if it is local. This is reminiscent of a similar bug report in [1], where it was suggested to perform the `insteadOf` lookup earlier. Taking that approach may not be as straightforward, since the intent is to store the original URL in the config, but to actually fetch from the insteadOf one, so conflating the two early on is a non-starter. Note: we pass the path returned by `get_repo_path(remote->url[0])`, which should be the same as `repo_name` (aside from any `insteadOf` rewrites). We *could* pass `absolute_pathdup()` of the same argument, which 86521acaca (Bring local clone's origin URL in line with that of a remote clone, 2008-09-01) indicates may differ depending on the presence of ".git/" for a non-bare repo. That matters for forming relative submodule paths, but doesn't matter for the second call, since we're just feeding it to the transport code, which is fine either way. [1]: https://lore.kernel.org/git/CAMoD=Bi41mB3QRn3JdZL-FGHs4w3C2jGpnJB-CqSndO7FMtfzA@mail.gmail.com/ Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 25 January 2023, 00:52:16 UTC
58325b9 Taylor Blau 25 January 2023, 00:43:45 UTC t5619: demonstrate clone_local() with ambiguous transport When cloning a repository, Git must determine (a) what transport mechanism to use, and (b) whether or not the clone is local. Since f38aa83f9a (use local cloning if insteadOf makes a local URL, 2014-07-17), the latter check happens after the remote has been initialized, and references the remote's URL instead of the local path. This is done to make it possible for a `url.<base>.insteadOf` rule to convert a remote URL into a local one, in which case the `clone_local()` mechanism should be used. However, with a specially crafted repository, Git can be tricked into using a non-local transport while still setting `is_local` to "1" and using the `clone_local()` optimization. The below test case demonstrates such an instance, and shows that it can be used to include arbitrary (known) paths in the working copy of a cloned repository on a victim's machine[^1], even if local file clones are forbidden by `protocol.file.allow`. This happens in a few parts: 1. We first call `get_repo_path()` to see if the remote is a local path. If it is, we replace the repo name with its absolute path. 2. We then call `transport_get()` on the repo name and decide how to access it. If it was turned into an absolute path in the previous step, then we should always treat it like a file. 3. We use `get_repo_path()` again, and set `is_local` as appropriate. But it's already too late to rewrite the repo name as an absolute path, since we've already fed it to the transport code. The attack works by including a submodule whose URL corresponds to a path on disk. In the below example, the repository "sub" is reachable via the dumb HTTP protocol at (something like): http://127.0.0.1:NNNN/dumb/sub.git However, the path "http:/127.0.0.1:NNNN/dumb" (that is, a top-level directory called "http:", then nested directories "127.0.0.1:NNNN", and "dumb") exists within the repository, too. To determine this, it first picks the appropriate transport, which is dumb HTTP. It then uses the remote's URL in order to determine whether the repository exists locally on disk. However, the malicious repository also contains an embedded stub repository which is the target of a symbolic link at the local path corresponding to the "sub" repository on disk (i.e., there is a symbolic link at "http:/127.0.0.1/dumb/sub.git", pointing to the stub repository via ".git/modules/sub/../../../repo"). This stub repository fools Git into thinking that a local repository exists at that URL and thus can be cloned locally. The affected call is in `get_repo_path()`, which in turn calls `get_repo_path_1()`, which locates a valid repository at that target. This then causes Git to set the `is_local` variable to "1", and in turn instructs Git to clone the repository using its local clone optimization via the `clone_local()` function. The exploit comes into play because the stub repository's top-level "$GIT_DIR/objects" directory is a symbolic link which can point to an arbitrary path on the victim's machine. `clone_local()` resolves the top-level "objects" directory through a `stat(2)` call, meaning that we read through the symbolic link and copy or hardlink the directory contents at the destination of the link. In other words, we can get steps (1) and (3) to disagree by leveraging the dangling symlink to pick a non-local transport in the first step, and then set is_local to "1" in the third step when cloning with `--separate-git-dir`, which makes the symlink non-dangling. This can result in data-exfiltration on the victim's machine when sensitive data is at a known path (e.g., "/home/$USER/.ssh"). The appropriate fix is two-fold: - Resolve the transport later on (to avoid using the local clone optimization with a non-local transport). - Avoid reading through the top-level "objects" directory when (correctly) using the clone_local() optimization. This patch merely demonstrates the issue. The following two patches will implement each part of the above fix, respectively. [^1]: Provided that any target directory does not contain symbolic links, in which case the changes from 6f054f9fb3 (builtin/clone.c: disallow `--local` clones with symlinks, 2022-07-28) will abort the clone. Reported-by: yvvdwf <yvvdwf@gmail.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 25 January 2023, 00:52:16 UTC
0227130 Johannes Schindelin 12 January 2023, 00:05:02 UTC attr: adjust a mismatched data type On platforms where `size_t` does not have the same width as `unsigned long`, passing a pointer to the former when a pointer to the latter is expected can lead to problems. Windows and 32-bit Linux are among the affected platforms. In this instance, we want to store the size of the blob that was read in that variable. However, `read_blob_data_from_index()` passes that pointer to `read_object_file()` which expects an `unsigned long *`. Which means that on affected platforms, the variable is not fully populated and part of its value is left uninitialized. (On Big-Endian platforms, this problem would be even worse.) The consequence is that depending on the uninitialized memory's contents, we may erroneously reject perfectly fine attributes. Let's address this by passing a pointer to a variable of the expected data type. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> 19 January 2023, 21:38:06 UTC
b7b37a3 Junio C Hamano 13 December 2022, 11:56:43 UTC Git 2.30.7 Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 December 2022, 11:56:43 UTC
6662a83 Junio C Hamano 09 December 2022, 07:05:52 UTC Merge branch 'ps/attr-limits' into maint-2.30 09 December 2022, 07:05:52 UTC
3305300 Junio C Hamano 09 December 2022, 07:02:39 UTC Merge branch 'ps/format-padding-fix' into maint-2.30 09 December 2022, 07:02:39 UTC
304a50a Patrick Steinhardt 01 December 2022, 14:47:23 UTC pretty: restrict input lengths for padding and wrapping formats Both the padding and wrapping formatting directives allow the caller to specify an integer that ultimately leads to us adding this many chars to the result buffer. As a consequence, it is trivial to e.g. allocate 2GB of RAM via a single formatting directive and cause resource exhaustion on the machine executing this logic. Furthermore, it is debatable whether there are any sane usecases that require the user to pad data to 2GB boundaries or to indent wrapped data by 2GB. Restrict the input sizes to 16 kilobytes at a maximum to limit the amount of bytes that can be requested by the user. This is not meant as a fix because there are ways to trivially amplify the amount of data we generate via formatting directives; the real protection is achieved by the changes in previous steps to catch and avoid integer wraparound that causes us to under-allocate and access beyond the end of allocated memory reagions. But having such a limit significantly helps fuzzing the pretty format, because the fuzzer is otherwise quite fast to run out-of-memory as it discovers these formatters. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
f930a23 Patrick Steinhardt 01 December 2022, 14:47:15 UTC utf8: refactor `strbuf_utf8_replace` to not rely on preallocated buffer In `strbuf_utf8_replace`, we preallocate the destination buffer and then use `memcpy` to copy bytes into it at computed offsets. This feels rather fragile and is hard to understand at times. Refactor the code to instead use `strbuf_add` and `strbuf_addstr` so that we can be sure that there is no possibility to perform an out-of-bounds write. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
81c2d4c Patrick Steinhardt 01 December 2022, 14:47:10 UTC utf8: fix checking for glyph width in `strbuf_utf8_replace()` In `strbuf_utf8_replace()`, we call `utf8_width()` to compute the width of the current glyph. If the glyph is a control character though it can be that `utf8_width()` returns `-1`, but because we assign this value to a `size_t` the conversion will cause us to underflow. This bug can easily be triggered with the following command: $ git log --pretty='format:xxx%<|(1,trunc)%x10' >From all I can see though this seems to be a benign underflow that has no security-related consequences. Fix the bug by using an `int` instead. When we see a control character, we now copy it into the target buffer but don't advance the current width of the string. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
937b71c Patrick Steinhardt 01 December 2022, 14:47:04 UTC utf8: fix overflow when returning string width The return type of both `utf8_strwidth()` and `utf8_strnwidth()` is `int`, but we operate on string lengths which are typically of type `size_t`. This means that when the string is longer than `INT_MAX`, we will overflow and thus return a negative result. This can lead to an out-of-bounds write with `--pretty=format:%<1)%B` and a commit message that is 2^31+1 bytes long: ================================================================= ==26009==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000001168 at pc 0x7f95c4e5f427 bp 0x7ffd8541c900 sp 0x7ffd8541c0a8 WRITE of size 2147483649 at 0x603000001168 thread T0 #0 0x7f95c4e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 #1 0x5612bbb1068c in format_and_pad_commit pretty.c:1763 #2 0x5612bbb1087a in format_commit_item pretty.c:1801 #3 0x5612bbc33bab in strbuf_expand strbuf.c:429 #4 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869 #5 0x5612bbb12d96 in pretty_print_commit pretty.c:2161 #6 0x5612bba0a4d5 in show_log log-tree.c:781 #7 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117 #8 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508 #9 0x5612bb69235b in cmd_log_walk builtin/log.c:549 #10 0x5612bb6951a2 in cmd_log builtin/log.c:883 #11 0x5612bb56c993 in run_builtin git.c:466 #12 0x5612bb56d397 in handle_builtin git.c:721 #13 0x5612bb56db07 in run_argv git.c:788 #14 0x5612bb56e8a7 in cmd_main git.c:923 #15 0x5612bb803682 in main common-main.c:57 #16 0x7f95c4c3c28f (/usr/lib/libc.so.6+0x2328f) #17 0x7f95c4c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #18 0x5612bb5680e4 in _start ../sysdeps/x86_64/start.S:115 0x603000001168 is located 0 bytes to the right of 24-byte region [0x603000001150,0x603000001168) allocated by thread T0 here: #0 0x7f95c4ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85 #1 0x5612bbcdd556 in xrealloc wrapper.c:136 #2 0x5612bbc310a3 in strbuf_grow strbuf.c:99 #3 0x5612bbc32acd in strbuf_add strbuf.c:298 #4 0x5612bbc33aec in strbuf_expand strbuf.c:418 #5 0x5612bbb110e7 in repo_format_commit_message pretty.c:1869 #6 0x5612bbb12d96 in pretty_print_commit pretty.c:2161 #7 0x5612bba0a4d5 in show_log log-tree.c:781 #8 0x5612bba0d6c7 in log_tree_commit log-tree.c:1117 #9 0x5612bb691ed5 in cmd_log_walk_no_free builtin/log.c:508 #10 0x5612bb69235b in cmd_log_walk builtin/log.c:549 #11 0x5612bb6951a2 in cmd_log builtin/log.c:883 #12 0x5612bb56c993 in run_builtin git.c:466 #13 0x5612bb56d397 in handle_builtin git.c:721 #14 0x5612bb56db07 in run_argv git.c:788 #15 0x5612bb56e8a7 in cmd_main git.c:923 #16 0x5612bb803682 in main common-main.c:57 #17 0x7f95c4c3c28f (/usr/lib/libc.so.6+0x2328f) SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy Shadow bytes around the buggy address: 0x0c067fff81d0: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa 0x0c067fff81e0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd 0x0c067fff81f0: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa 0x0c067fff8200: fd fd fd fa fa fa fd fd fd fd fa fa 00 00 00 fa 0x0c067fff8210: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd =>0x0c067fff8220: fd fa fa fa fd fd fd fa fa fa 00 00 00[fa]fa fa 0x0c067fff8230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff8240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff8250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==26009==ABORTING Now the proper fix for this would be to convert both functions to return an `size_t` instead of an `int`. But given that this commit may be part of a security release, let's instead do the minimal viable fix and die in case we see an overflow. Add a test that would have previously caused us to crash. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
17d23e8 Patrick Steinhardt 01 December 2022, 14:47:00 UTC utf8: fix returning negative string width The `utf8_strnwidth()` function calls `utf8_width()` in a loop and adds its returned width to the end result. `utf8_width()` can return `-1` though in case it reads a control character, which means that the computed string width is going to be wrong. In the worst case where there are more control characters than non-control characters, we may even return a negative string width. Fix this bug by treating control characters as having zero width. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
522cc87 Patrick Steinhardt 01 December 2022, 14:46:53 UTC utf8: fix truncated string lengths in `utf8_strnwidth()` The `utf8_strnwidth()` function accepts an optional string length as input parameter. This parameter can either be set to `-1`, in which case we call `strlen()` on the input. Or it can be set to a positive integer that indicates a precomputed length, which callers typically compute by calling `strlen()` at some point themselves. The input parameter is an `int` though, whereas `strlen()` returns a `size_t`. This can lead to implementation-defined behaviour though when the `size_t` cannot be represented by the `int`. In the general case though this leads to wrap-around and thus to negative string sizes, which is sure enough to not lead to well-defined behaviour. Fix this by accepting a `size_t` instead of an `int` as string length. While this takes away the ability of callers to simply pass in `-1` as string length, it really is trivial enough to convert them to instead pass in `strlen()` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
48050c4 Patrick Steinhardt 01 December 2022, 14:46:49 UTC pretty: fix integer overflow in wrapping format The `%w(width,indent1,indent2)` formatting directive can be used to rewrap text to a specific width and is designed after git-shortlog(1)'s `-w` parameter. While the three parameters are all stored as `size_t` internally, `strbuf_add_wrapped_text()` accepts integers as input. As a result, the casted integers may overflow. As these now-negative integers are later on passed to `strbuf_addchars()`, we will ultimately run into implementation-defined behaviour due to casting a negative number back to `size_t` again. On my platform, this results in trying to allocate 9000 petabyte of memory. Fix this overflow by using `cast_size_t_to_int()` so that we reject inputs that cannot be represented as an integer. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
1de69c0 Patrick Steinhardt 01 December 2022, 14:46:39 UTC pretty: fix adding linefeed when placeholder is not expanded When a formatting directive has a `+` or ` ` after the `%`, then we add either a line feed or space if the placeholder expands to a non-empty string. In specific cases though this logic doesn't work as expected, and we try to add the character even in the case where the formatting directive is empty. One such pattern is `%w(1)%+d%+w(2)`. `%+d` expands to reference names pointing to a certain commit, like in `git log --decorate`. For a tagged commit this would for example expand to `\n (tag: v1.0.0)`, which has a leading newline due to the `+` modifier and a space added by `%d`. Now the second wrapping directive will cause us to rewrap the text to `\n(tag:\nv1.0.0)`, which is one byte shorter due to the missing leading space. The code that handles the `+` magic now notices that the length has changed and will thus try to insert a leading line feed at the original posititon. But as the string was shortened, the original position is past the buffer's boundary and thus we die with an error. Now there are two issues here: 1. We check whether the buffer length has changed, not whether it has been extended. This causes us to try and add the character past the string boundary. 2. The current logic does not make any sense whatsoever. When the string got expanded due to the rewrap, putting the separator into the original position is likely to put it somewhere into the middle of the rewrapped contents. It is debatable whether `%+w()` makes any sense in the first place. Strictly speaking, the placeholder never expands to a non-empty string, and consequentially we shouldn't ever accept this combination. We thus fix the bug by simply refusing `%+w()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
f6e0b9f Patrick Steinhardt 01 December 2022, 14:46:34 UTC pretty: fix out-of-bounds read when parsing invalid padding format An out-of-bounds read can be triggered when parsing an incomplete padding format string passed via `--pretty=format` or in Git archives when files are marked with the `export-subst` gitattribute. This bug exists since we have introduced support for truncating output via the `trunc` keyword a7f01c6b4d (pretty: support truncating in %>, %< and %><, 2013-04-19). Before this commit, we used to find the end of the formatting string by using strchr(3P). This function returns a `NULL` pointer in case the character in question wasn't found. The subsequent check whether any character was found thus simply checked the returned pointer. After the commit we switched to strcspn(3P) though, which only returns the offset to the first found character or to the trailing NUL byte. As the end pointer is now computed by adding the offset to the start pointer it won't be `NULL` anymore, and as a consequence the check doesn't do anything anymore. The out-of-bounds data that is being read can in fact end up in the formatted string. As a consequence, it is possible to leak memory contents either by calling git-log(1) or via git-archive(1) when any of the archived files is marked with the `export-subst` gitattribute. ==10888==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000000398 at pc 0x7f0356047cb2 bp 0x7fff3ffb95d0 sp 0x7fff3ffb8d78 READ of size 1 at 0x602000000398 thread T0 #0 0x7f0356047cb1 in __interceptor_strchrnul /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 #1 0x563b7cec9a43 in strbuf_expand strbuf.c:417 #2 0x563b7cda7060 in repo_format_commit_message pretty.c:1869 #3 0x563b7cda8d0f in pretty_print_commit pretty.c:2161 #4 0x563b7cca04c8 in show_log log-tree.c:781 #5 0x563b7cca36ba in log_tree_commit log-tree.c:1117 #6 0x563b7c927ed5 in cmd_log_walk_no_free builtin/log.c:508 #7 0x563b7c92835b in cmd_log_walk builtin/log.c:549 #8 0x563b7c92b1a2 in cmd_log builtin/log.c:883 #9 0x563b7c802993 in run_builtin git.c:466 #10 0x563b7c803397 in handle_builtin git.c:721 #11 0x563b7c803b07 in run_argv git.c:788 #12 0x563b7c8048a7 in cmd_main git.c:923 #13 0x563b7ca99682 in main common-main.c:57 #14 0x7f0355e3c28f (/usr/lib/libc.so.6+0x2328f) #15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115 0x602000000398 is located 0 bytes to the right of 8-byte region [0x602000000390,0x602000000398) allocated by thread T0 here: #0 0x7f0356072faa in __interceptor_strdup /usr/src/debug/gcc/libsanitizer/asan/asan_interceptors.cpp:439 #1 0x563b7cf7317c in xstrdup wrapper.c:39 #2 0x563b7cd9a06a in save_user_format pretty.c:40 #3 0x563b7cd9b3e5 in get_commit_format pretty.c:173 #4 0x563b7ce54ea0 in handle_revision_opt revision.c:2456 #5 0x563b7ce597c9 in setup_revisions revision.c:2850 #6 0x563b7c9269e0 in cmd_log_init_finish builtin/log.c:269 #7 0x563b7c927362 in cmd_log_init builtin/log.c:348 #8 0x563b7c92b193 in cmd_log builtin/log.c:882 #9 0x563b7c802993 in run_builtin git.c:466 #10 0x563b7c803397 in handle_builtin git.c:721 #11 0x563b7c803b07 in run_argv git.c:788 #12 0x563b7c8048a7 in cmd_main git.c:923 #13 0x563b7ca99682 in main common-main.c:57 #14 0x7f0355e3c28f (/usr/lib/libc.so.6+0x2328f) #15 0x7f0355e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #16 0x563b7c7fe0e4 in _start ../sysdeps/x86_64/start.S:115 SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:725 in __interceptor_strchrnul Shadow bytes around the buggy address: 0x0c047fff8020: fa fa fd fd fa fa 00 06 fa fa 05 fa fa fa fd fd 0x0c047fff8030: fa fa 00 02 fa fa 06 fa fa fa 05 fa fa fa fd fd 0x0c047fff8040: fa fa 00 07 fa fa 03 fa fa fa fd fd fa fa 00 00 0x0c047fff8050: fa fa 00 01 fa fa fd fd fa fa 00 00 fa fa 00 01 0x0c047fff8060: fa fa 00 06 fa fa 00 06 fa fa 05 fa fa fa 05 fa =>0x0c047fff8070: fa fa 00[fa]fa fa fd fa fa fa fd fd fa fa fd fd 0x0c047fff8080: fa fa fd fd fa fa 00 00 fa fa 00 fa fa fa fd fa 0x0c047fff8090: fa fa fd fd fa fa 00 00 fa fa fa fa fa fa fa fa 0x0c047fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff80c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==10888==ABORTING Fix this bug by checking whether `end` points at the trailing NUL byte. Add a test which catches this out-of-bounds read and which demonstrates that we used to write out-of-bounds data into the formatted message. Reported-by: Markus Vervier <markus.vervier@x41-dsec.de> Original-patch-by: Markus Vervier <markus.vervier@x41-dsec.de> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
b49f309 Patrick Steinhardt 01 December 2022, 14:46:30 UTC pretty: fix out-of-bounds read when left-flushing with stealing With the `%>>(<N>)` pretty formatter, you can ask git-log(1) et al to steal spaces. To do so we need to look ahead of the next token to see whether there are spaces there. This loop takes into account ANSI sequences that end with an `m`, and if it finds any it will skip them until it finds the first space. While doing so it does not take into account the buffer's limits though and easily does an out-of-bounds read. Add a test that hits this behaviour. While we don't have an easy way to verify this, the test causes the following failure when run with `SANITIZE=address`: ==37941==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x603000000baf at pc 0x55ba6f88e0d0 bp 0x7ffc84c50d20 sp 0x7ffc84c50d10 READ of size 1 at 0x603000000baf thread T0 #0 0x55ba6f88e0cf in format_and_pad_commit pretty.c:1712 #1 0x55ba6f88e7b4 in format_commit_item pretty.c:1801 #2 0x55ba6f9b1ae4 in strbuf_expand strbuf.c:429 #3 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869 #4 0x55ba6f890ccf in pretty_print_commit pretty.c:2161 #5 0x55ba6f7884c8 in show_log log-tree.c:781 #6 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117 #7 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508 #8 0x55ba6f41035b in cmd_log_walk builtin/log.c:549 #9 0x55ba6f4131a2 in cmd_log builtin/log.c:883 #10 0x55ba6f2ea993 in run_builtin git.c:466 #11 0x55ba6f2eb397 in handle_builtin git.c:721 #12 0x55ba6f2ebb07 in run_argv git.c:788 #13 0x55ba6f2ec8a7 in cmd_main git.c:923 #14 0x55ba6f581682 in main common-main.c:57 #15 0x7f2d08c3c28f (/usr/lib/libc.so.6+0x2328f) #16 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #17 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115 0x603000000baf is located 1 bytes to the left of 24-byte region [0x603000000bb0,0x603000000bc8) allocated by thread T0 here: #0 0x7f2d08ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85 #1 0x55ba6fa5b494 in xrealloc wrapper.c:136 #2 0x55ba6f9aefdc in strbuf_grow strbuf.c:99 #3 0x55ba6f9b0a06 in strbuf_add strbuf.c:298 #4 0x55ba6f9b1a25 in strbuf_expand strbuf.c:418 #5 0x55ba6f88f020 in repo_format_commit_message pretty.c:1869 #6 0x55ba6f890ccf in pretty_print_commit pretty.c:2161 #7 0x55ba6f7884c8 in show_log log-tree.c:781 #8 0x55ba6f78b6ba in log_tree_commit log-tree.c:1117 #9 0x55ba6f40fed5 in cmd_log_walk_no_free builtin/log.c:508 #10 0x55ba6f41035b in cmd_log_walk builtin/log.c:549 #11 0x55ba6f4131a2 in cmd_log builtin/log.c:883 #12 0x55ba6f2ea993 in run_builtin git.c:466 #13 0x55ba6f2eb397 in handle_builtin git.c:721 #14 0x55ba6f2ebb07 in run_argv git.c:788 #15 0x55ba6f2ec8a7 in cmd_main git.c:923 #16 0x55ba6f581682 in main common-main.c:57 #17 0x7f2d08c3c28f (/usr/lib/libc.so.6+0x2328f) #18 0x7f2d08c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #19 0x55ba6f2e60e4 in _start ../sysdeps/x86_64/start.S:115 SUMMARY: AddressSanitizer: heap-buffer-overflow pretty.c:1712 in format_and_pad_commit Shadow bytes around the buggy address: 0x0c067fff8120: fa fa fd fd fd fa fa fa fd fd fd fa fa fa fd fd 0x0c067fff8130: fd fd fa fa fd fd fd fd fa fa fd fd fd fa fa fa 0x0c067fff8140: fd fd fd fa fa fa fd fd fd fa fa fa fd fd fd fa 0x0c067fff8150: fa fa fd fd fd fd fa fa 00 00 00 fa fa fa fd fd 0x0c067fff8160: fd fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa =>0x0c067fff8170: fd fd fd fa fa[fa]00 00 00 fa fa fa 00 00 00 fa 0x0c067fff8180: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff8190: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff81a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff81b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c067fff81c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Luckily enough, this would only cause us to copy the out-of-bounds data into the formatted commit in case we really had an ANSI sequence preceding our buffer. So this bug likely has no security consequences. Fix it regardless by not traversing past the buffer's start. Reported-by: Patrick Steinhardt <ps@pks.im> Reported-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
81dc898 Patrick Steinhardt 01 December 2022, 14:46:25 UTC pretty: fix out-of-bounds write caused by integer overflow When using a padding specifier in the pretty format passed to git-log(1) we need to calculate the string length in several places. These string lengths are stored in `int`s though, which means that these can easily overflow when the input lengths exceeds 2GB. This can ultimately lead to an out-of-bounds write when these are used in a call to memcpy(3P): ==8340==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f1ec62f97fe at pc 0x7f2127e5f427 bp 0x7ffd3bd63de0 sp 0x7ffd3bd63588 WRITE of size 1 at 0x7f1ec62f97fe thread T0 #0 0x7f2127e5f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 #1 0x5628e96aa605 in format_and_pad_commit pretty.c:1762 #2 0x5628e96aa7f4 in format_commit_item pretty.c:1801 #3 0x5628e97cdb24 in strbuf_expand strbuf.c:429 #4 0x5628e96ab060 in repo_format_commit_message pretty.c:1869 #5 0x5628e96acd0f in pretty_print_commit pretty.c:2161 #6 0x5628e95a44c8 in show_log log-tree.c:781 #7 0x5628e95a76ba in log_tree_commit log-tree.c:1117 #8 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508 #9 0x5628e922c35b in cmd_log_walk builtin/log.c:549 #10 0x5628e922f1a2 in cmd_log builtin/log.c:883 #11 0x5628e9106993 in run_builtin git.c:466 #12 0x5628e9107397 in handle_builtin git.c:721 #13 0x5628e9107b07 in run_argv git.c:788 #14 0x5628e91088a7 in cmd_main git.c:923 #15 0x5628e939d682 in main common-main.c:57 #16 0x7f2127c3c28f (/usr/lib/libc.so.6+0x2328f) #17 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #18 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115 0x7f1ec62f97fe is located 2 bytes to the left of 4831838265-byte region [0x7f1ec62f9800,0x7f1fe62f9839) allocated by thread T0 here: #0 0x7f2127ebe7ea in __interceptor_realloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:85 #1 0x5628e98774d4 in xrealloc wrapper.c:136 #2 0x5628e97cb01c in strbuf_grow strbuf.c:99 #3 0x5628e97ccd42 in strbuf_addchars strbuf.c:327 #4 0x5628e96aa55c in format_and_pad_commit pretty.c:1761 #5 0x5628e96aa7f4 in format_commit_item pretty.c:1801 #6 0x5628e97cdb24 in strbuf_expand strbuf.c:429 #7 0x5628e96ab060 in repo_format_commit_message pretty.c:1869 #8 0x5628e96acd0f in pretty_print_commit pretty.c:2161 #9 0x5628e95a44c8 in show_log log-tree.c:781 #10 0x5628e95a76ba in log_tree_commit log-tree.c:1117 #11 0x5628e922bed5 in cmd_log_walk_no_free builtin/log.c:508 #12 0x5628e922c35b in cmd_log_walk builtin/log.c:549 #13 0x5628e922f1a2 in cmd_log builtin/log.c:883 #14 0x5628e9106993 in run_builtin git.c:466 #15 0x5628e9107397 in handle_builtin git.c:721 #16 0x5628e9107b07 in run_argv git.c:788 #17 0x5628e91088a7 in cmd_main git.c:923 #18 0x5628e939d682 in main common-main.c:57 #19 0x7f2127c3c28f (/usr/lib/libc.so.6+0x2328f) #20 0x7f2127c3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #21 0x5628e91020e4 in _start ../sysdeps/x86_64/start.S:115 SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827 in __interceptor_memcpy Shadow bytes around the buggy address: 0x0fe458c572a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0fe458c572b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0fe458c572c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0fe458c572d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0fe458c572e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0fe458c572f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa] 0x0fe458c57300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fe458c57310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fe458c57320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fe458c57330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0fe458c57340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==8340==ABORTING The pretty format can also be used in `git archive` operations via the `export-subst` attribute. So this is what in our opinion makes this a critical issue in the context of Git forges which allow to download an archive of user supplied Git repositories. Fix this vulnerability by using `size_t` instead of `int` to track the string lengths. Add tests which detect this vulnerability when Git is compiled with the address sanitizer. Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com> Original-patch-by: Joern Schneeweisz <jschneeweisz@gitlab.com> Modified-by: Taylor Blau <me@ttalorr.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:21 UTC
a244dc5 Carlo Marcelo Arenas Belón 02 November 2021, 15:46:06 UTC test-lib: add prerequisite for 64-bit platforms Allow tests that assume a 64-bit `size_t` to be skipped in 32-bit platforms and regardless of the size of `long`. This imitates the `LONG_IS_64BIT` prerequisite. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 05:26:04 UTC
3c50032 Patrick Steinhardt 01 December 2022, 14:45:53 UTC attr: ignore overly large gitattributes files Similar as with the preceding commit, start ignoring gitattributes files that are overly large to protect us against out-of-bounds reads and writes caused by integer overflows. Unfortunately, we cannot just define "overly large" in terms of any preexisting limits in the codebase. Instead, we choose a very conservative limit of 100MB. This is plenty of room for specifying gitattributes, and incidentally it is also the limit for blob sizes for GitHub. While we don't want GitHub to dictate limits here, it is still sensible to use this fact for an informed decision given that it is hosting a huge set of repositories. Furthermore, over at GitLab we scanned a subset of repositories for their root-level attribute files. We found that 80% of them have a gitattributes file smaller than 100kB, 99.99% have one smaller than 1MB, and only a single repository had one that was almost 3MB in size. So enforcing a limit of 100MB seems to give us ample of headroom. With this limit in place we can be reasonably sure that there is no easy way to exploit the gitattributes file via integer overflows anymore. Furthermore, it protects us against resource exhaustion caused by allocating the in-memory data structures required to represent the parsed attributes. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:50:03 UTC
dfa6b32 Patrick Steinhardt 01 December 2022, 14:45:48 UTC attr: ignore attribute lines exceeding 2048 bytes There are two different code paths to read gitattributes: once via a file, and once via the index. These two paths used to behave differently because when reading attributes from a file, we used fgets(3P) with a buffer size of 2kB. Consequentially, we silently truncate line lengths when lines are longer than that and will then parse the remainder of the line as a new pattern. It goes without saying that this is entirely unexpected, but it's even worse that the behaviour depends on how the gitattributes are parsed. While this is simply wrong, the silent truncation saves us with the recently discovered vulnerabilities that can cause out-of-bound writes or reads with unreasonably long lines due to integer overflows. As the common path is to read gitattributes via the worktree file instead of via the index, we can assume that any gitattributes file that had lines longer than that is already broken anyway. So instead of lifting the limit here, we can double down on it to fix the vulnerabilities. Introduce an explicit line length limit of 2kB that is shared across all paths that read attributes and ignore any line that hits this limit while printing a warning. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:33:07 UTC
d74b1fd Patrick Steinhardt 01 December 2022, 14:45:44 UTC attr: fix silently splitting up lines longer than 2048 bytes When reading attributes from a file we use fgets(3P) with a buffer size of 2048 bytes. This means that as soon as a line exceeds the buffer size we split it up into multiple parts and parse each of them as a separate pattern line. This is of course not what the user intended, and even worse the behaviour is inconsistent with how we read attributes from the index. Fix this bug by converting the code to use `strbuf_getline()` instead. This will indeed read in the whole line, which may theoretically lead to an out-of-memory situation when the gitattributes file is huge. We're about to reject any gitattributes files larger than 100MB in the next commit though, which makes this less of a concern. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:29:30 UTC
a60a66e Patrick Steinhardt 01 December 2022, 14:45:40 UTC attr: harden allocation against integer overflows When parsing an attributes line, we need to allocate an array that holds all attributes specified for the given file pattern. The calculation to determine the number of bytes that need to be allocated was prone to an overflow though when there was an unreasonable amount of attributes. Harden the allocation by instead using the `st_` helper functions that cause us to die when we hit an integer overflow. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
e1e12e9 Patrick Steinhardt 01 December 2022, 14:45:36 UTC attr: fix integer overflow with more than INT_MAX macros Attributes have a field that tracks the position in the `all_attrs` array they're stored inside. This field gets set via `hashmap_get_size` when adding the attribute to the global map of attributes. But while the field is of type `int`, the value returned by `hashmap_get_size` is an `unsigned int`. It can thus happen that the value overflows, where we would now dereference teh `all_attrs` array at an out-of-bounds value. We do have a sanity check for this overflow via an assert that verifies the index matches the new hashmap's size. But asserts are not a proper mechanism to detect against any such overflows as they may not in fact be compiled into production code. Fix this by using an `unsigned int` to track the index and convert the assert to a call `die()`. Reported-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
447ac90 Patrick Steinhardt 01 December 2022, 14:45:31 UTC attr: fix out-of-bounds read with unreasonable amount of patterns The `struct attr_stack` tracks the stack of all patterns together with their attributes. When parsing a gitattributes file that has more than 2^31 such patterns though we may trigger multiple out-of-bounds reads on 64 bit platforms. This is because while the `num_matches` variable is an unsigned integer, we always use a signed integer to iterate over them. I have not been able to reproduce this issue due to memory constraints on my systems. But despite the out-of-bounds reads, the worst thing that can seemingly happen is to call free(3P) with a garbage pointer when calling `attr_stack_free()`. Fix this bug by using unsigned integers to iterate over the array. While this makes the iteration somewhat awkward when iterating in reverse, it is at least better than knowingly running into an out-of-bounds read. While at it, convert the call to `ALLOC_GROW` to use `ALLOC_GROW_BY` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
34ace8b Patrick Steinhardt 01 December 2022, 14:45:27 UTC attr: fix out-of-bounds write when parsing huge number of attributes It is possible to trigger an integer overflow when parsing attribute names when there are more than 2^31 of them for a single pattern. This can either lead to us dying due to trying to request too many bytes: blob=$(perl -e 'print "f" . " a=" x 2147483649' | git hash-object -w --stdin) git update-index --add --cacheinfo 100644,$blob,.gitattributes git attr-check --all file ================================================================= ==1022==ERROR: AddressSanitizer: requested allocation size 0xfffffff800000032 (0xfffffff800001038 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0) #0 0x7fd3efabf411 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 #1 0x5563a0a1e3d3 in xcalloc wrapper.c:150 #2 0x5563a058d005 in parse_attr_line attr.c:384 #3 0x5563a058e661 in handle_attr_line attr.c:660 #4 0x5563a058eddb in read_attr_from_index attr.c:769 #5 0x5563a058ef12 in read_attr attr.c:797 #6 0x5563a058f24c in bootstrap_attr_stack attr.c:867 #7 0x5563a058f4a3 in prepare_attr_stack attr.c:902 #8 0x5563a05905da in collect_some_attrs attr.c:1097 #9 0x5563a059093d in git_all_attrs attr.c:1128 #10 0x5563a02f636e in check_attr builtin/check-attr.c:67 #11 0x5563a02f6c12 in cmd_check_attr builtin/check-attr.c:183 #12 0x5563a02aa993 in run_builtin git.c:466 #13 0x5563a02ab397 in handle_builtin git.c:721 #14 0x5563a02abb2b in run_argv git.c:788 #15 0x5563a02ac991 in cmd_main git.c:926 #16 0x5563a05432bd in main common-main.c:57 #17 0x7fd3ef82228f (/usr/lib/libc.so.6+0x2328f) ==1022==HINT: if you don't care about these errors you may set allocator_may_return_null=1 SUMMARY: AddressSanitizer: allocation-size-too-big /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:77 in __interceptor_calloc ==1022==ABORTING Or, much worse, it can lead to an out-of-bounds write because we underallocate and then memcpy(3P) into an array: perl -e ' print "A " . "\rh="x2000000000; print "\rh="x2000000000; print "\rh="x294967294 . "\n" ' >.gitattributes git add .gitattributes git commit -am "evil attributes" $ git clone --quiet /path/to/repo ================================================================= ==15062==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000002550 at pc 0x5555559884d5 bp 0x7fffffffbc60 sp 0x7fffffffbc58 WRITE of size 8 at 0x602000002550 thread T0 #0 0x5555559884d4 in parse_attr_line attr.c:393 #1 0x5555559884d4 in handle_attr_line attr.c:660 #2 0x555555988902 in read_attr_from_index attr.c:784 #3 0x555555988902 in read_attr_from_index attr.c:747 #4 0x555555988a1d in read_attr attr.c:800 #5 0x555555989b0c in bootstrap_attr_stack attr.c:882 #6 0x555555989b0c in prepare_attr_stack attr.c:917 #7 0x555555989b0c in collect_some_attrs attr.c:1112 #8 0x55555598b141 in git_check_attr attr.c:1126 #9 0x555555a13004 in convert_attrs convert.c:1311 #10 0x555555a95e04 in checkout_entry_ca entry.c:553 #11 0x555555d58bf6 in checkout_entry entry.h:42 #12 0x555555d58bf6 in check_updates unpack-trees.c:480 #13 0x555555d5eb55 in unpack_trees unpack-trees.c:2040 #14 0x555555785ab7 in checkout builtin/clone.c:724 #15 0x555555785ab7 in cmd_clone builtin/clone.c:1384 #16 0x55555572443c in run_builtin git.c:466 #17 0x55555572443c in handle_builtin git.c:721 #18 0x555555727872 in run_argv git.c:788 #19 0x555555727872 in cmd_main git.c:926 #20 0x555555721fa0 in main common-main.c:57 #21 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308 #22 0x555555723f39 in _start (git+0x1cff39) 0x602000002552 is located 0 bytes to the right of 2-byte region [0x602000002550,0x602000002552) allocated by thread T0 here: #0 0x7ffff768c037 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x555555d7fff7 in xcalloc wrapper.c:150 #2 0x55555598815f in parse_attr_line attr.c:384 #3 0x55555598815f in handle_attr_line attr.c:660 #4 0x555555988902 in read_attr_from_index attr.c:784 #5 0x555555988902 in read_attr_from_index attr.c:747 #6 0x555555988a1d in read_attr attr.c:800 #7 0x555555989b0c in bootstrap_attr_stack attr.c:882 #8 0x555555989b0c in prepare_attr_stack attr.c:917 #9 0x555555989b0c in collect_some_attrs attr.c:1112 #10 0x55555598b141 in git_check_attr attr.c:1126 #11 0x555555a13004 in convert_attrs convert.c:1311 #12 0x555555a95e04 in checkout_entry_ca entry.c:553 #13 0x555555d58bf6 in checkout_entry entry.h:42 #14 0x555555d58bf6 in check_updates unpack-trees.c:480 #15 0x555555d5eb55 in unpack_trees unpack-trees.c:2040 #16 0x555555785ab7 in checkout builtin/clone.c:724 #17 0x555555785ab7 in cmd_clone builtin/clone.c:1384 #18 0x55555572443c in run_builtin git.c:466 #19 0x55555572443c in handle_builtin git.c:721 #20 0x555555727872 in run_argv git.c:788 #21 0x555555727872 in cmd_main git.c:926 #22 0x555555721fa0 in main common-main.c:57 #23 0x7ffff73f1d09 in __libc_start_main ../csu/libc-start.c:308 SUMMARY: AddressSanitizer: heap-buffer-overflow attr.c:393 in parse_attr_line Shadow bytes around the buggy address: 0x0c047fff8450: fa fa 00 02 fa fa 00 07 fa fa fd fd fa fa 00 00 0x0c047fff8460: fa fa 02 fa fa fa fd fd fa fa 00 06 fa fa 05 fa 0x0c047fff8470: fa fa fd fd fa fa 00 02 fa fa 06 fa fa fa 05 fa 0x0c047fff8480: fa fa 07 fa fa fa fd fd fa fa 00 01 fa fa 00 02 0x0c047fff8490: fa fa 00 03 fa fa 00 fa fa fa 00 01 fa fa 00 03 =>0x0c047fff84a0: fa fa 00 01 fa fa 00 02 fa fa[02]fa fa fa fa fa 0x0c047fff84b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff84c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff84d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff84e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c047fff84f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==15062==ABORTING Fix this bug by using `size_t` instead to count the number of attributes so that this value cannot reasonably overflow without running out of memory before already. Reported-by: Markus Vervier <markus.vervier@x41-dsec.de> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
2455720 Patrick Steinhardt 01 December 2022, 14:45:23 UTC attr: fix integer overflow when parsing huge attribute names It is possible to trigger an integer overflow when parsing attribute names that are longer than 2^31 bytes because we assign the result of strlen(3P) to an `int` instead of to a `size_t`. This can lead to an abort in vsnprintf(3P) with the following reproducer: blob=$(perl -e 'print "A " . "B"x2147483648 . "\n"' | git hash-object -w --stdin) git update-index --add --cacheinfo 100644,$blob,.gitattributes git check-attr --all path BUG: strbuf.c:400: your vsnprintf is broken (returned -1) But furthermore, assuming that the attribute name is even longer than that, it can cause us to silently truncate the attribute and thus lead to wrong results. Fix this integer overflow by using a `size_t` instead. This fixes the silent truncation of attribute names, but it only partially fixes the BUG we hit: even though the initial BUG is fixed, we can still hit a BUG when parsing invalid attribute lines via `report_invalid_attr()`. This is due to an underlying design issue in vsnprintf(3P) which only knows to return an `int`, and thus it may always overflow with large inputs. This issue is benign though: the worst that can happen is that the error message is misreported to be either truncated or too long, but due to the buffer being NUL terminated we wouldn't ever do an out-of-bounds read here. Reported-by: Markus Vervier <markus.vervier@x41-dsec.de> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
8d0d48c Patrick Steinhardt 01 December 2022, 14:45:19 UTC attr: fix out-of-bounds read with huge attribute names There is an out-of-bounds read possible when parsing gitattributes that have an attribute that is 2^31+1 bytes long. This is caused due to an integer overflow when we assign the result of strlen(3P) to an `int`, where we use the wrapped-around value in a subsequent call to memcpy(3P). The following code reproduces the issue: blob=$(perl -e 'print "a" x 2147483649 . " attr"' | git hash-object -w --stdin) git update-index --add --cacheinfo 100644,$blob,.gitattributes git check-attr --all file AddressSanitizer:DEADLYSIGNAL ================================================================= ==8451==ERROR: AddressSanitizer: SEGV on unknown address 0x7f93efa00800 (pc 0x7f94f1f8f082 bp 0x7ffddb59b3a0 sp 0x7ffddb59ab28 T0) ==8451==The signal is caused by a READ memory access. #0 0x7f94f1f8f082 (/usr/lib/libc.so.6+0x176082) #1 0x7f94f2047d9c in __interceptor_strspn /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:752 #2 0x560e190f7f26 in parse_attr_line attr.c:375 #3 0x560e190f9663 in handle_attr_line attr.c:660 #4 0x560e190f9ddd in read_attr_from_index attr.c:769 #5 0x560e190f9f14 in read_attr attr.c:797 #6 0x560e190fa24e in bootstrap_attr_stack attr.c:867 #7 0x560e190fa4a5 in prepare_attr_stack attr.c:902 #8 0x560e190fb5dc in collect_some_attrs attr.c:1097 #9 0x560e190fb93f in git_all_attrs attr.c:1128 #10 0x560e18e6136e in check_attr builtin/check-attr.c:67 #11 0x560e18e61c12 in cmd_check_attr builtin/check-attr.c:183 #12 0x560e18e15993 in run_builtin git.c:466 #13 0x560e18e16397 in handle_builtin git.c:721 #14 0x560e18e16b2b in run_argv git.c:788 #15 0x560e18e17991 in cmd_main git.c:926 #16 0x560e190ae2bd in main common-main.c:57 #17 0x7f94f1e3c28f (/usr/lib/libc.so.6+0x2328f) #18 0x7f94f1e3c349 in __libc_start_main (/usr/lib/libc.so.6+0x23349) #19 0x560e18e110e4 in _start ../sysdeps/x86_64/start.S:115 AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV (/usr/lib/libc.so.6+0x176082) ==8451==ABORTING Fix this bug by converting the variable to a `size_t` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
eb22e7d Patrick Steinhardt 01 December 2022, 14:45:15 UTC attr: fix overflow when upserting attribute with overly long name The function `git_attr_internal()` is called to upsert attributes into the global map. And while all callers pass a `size_t`, the function itself accepts an `int` as the attribute name's length. This can lead to an integer overflow in case the attribute name is longer than `INT_MAX`. Now this overflow seems harmless as the first thing we do is to call `attr_name_valid()`, and that function only succeeds in case all chars in the range of `namelen` match a certain small set of chars. We thus can't do an out-of-bounds read as NUL is not part of that set and all strings passed to this function are NUL-terminated. And furthermore, we wouldn't ever read past the current attribute name anyway due to the same reason. And if validation fails we will return early. On the other hand it feels fragile to rely on this behaviour, even more so given that we pass `namelen` to `FLEX_ALLOC_MEM()`. So let's instead just do the correct thing here and accept a `size_t` as line length. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 05 December 2022, 06:14:16 UTC
abd4d67 Taylor Blau 30 September 2022, 20:32:10 UTC Git 2.30.6 Signed-off-by: Taylor Blau <me@ttaylorr.com> 06 October 2022, 21:38:16 UTC
0ca6ead Kevin Backhouse 28 September 2022, 22:53:32 UTC alias.c: reject too-long cmdline strings in split_cmdline() This function improperly uses an int to represent the number of entries in the resulting argument array. This allows a malicious actor to intentionally overflow the return value, leading to arbitrary heap writes. Because the resulting argv array is typically passed to execv(), it may be possible to leverage this attack to gain remote code execution on a victim machine. This was almost certainly the case for certain configurations of git-shell until the previous commit limited the size of input it would accept. Other calls to split_cmdline() are typically limited by the size of argv the OS is willing to hand us, so are similarly protected. So this is not strictly fixing a known vulnerability, but is a hardening of the function that is worth doing to protect against possible unknown vulnerabilities. One approach to fixing this would be modifying the signature of `split_cmdline()` to look something like: int split_cmdline(char *cmdline, const char ***argv, size_t *argc); Where the return value of `split_cmdline()` is negative for errors, and zero otherwise. If non-NULL, the `*argc` pointer is modified to contain the size of the `**argv` array. But this implies an absurdly large `argv` array, which more than likely larger than the system's argument limit. So even if split_cmdline() allowed this, it would fail immediately afterwards when we called execv(). So instead of converting all of `split_cmdline()`'s callers to work with `size_t` types in this patch, instead pursue the minimal fix here to prevent ever returning an array with more than INT_MAX entries in it. Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
71ad7fe Jeff King 28 September 2022, 22:52:48 UTC shell: limit size of interactive commands When git-shell is run in interactive mode (which must be enabled by creating $HOME/git-shell-commands), it reads commands from stdin, one per line, and executes them. We read the commands with git_read_line_interactively(), which uses a strbuf under the hood. That means we'll accept an input of arbitrary size (limited only by how much heap we can allocate). That creates two problems: - the rest of the code is not prepared to handle large inputs. The most serious issue here is that split_cmdline() uses "int" for most of its types, which can lead to integer overflow and out-of-bounds array reads and writes. But even with that fixed, we assume that we can feed the command name to snprintf() (via xstrfmt()), which is stuck for historical reasons using "int", and causes it to fail (and even trigger a BUG() call). - since the point of git-shell is to take input from untrusted or semi-trusted clients, it's a mild denial-of-service. We'll allocate as many bytes as the client sends us (actually twice as many, since we immediately duplicate the buffer). We can fix both by just limiting the amount of per-command input we're willing to receive. We should also fix split_cmdline(), of course, which is an accident waiting to happen, but that can come on top. Most calls to split_cmdline(), including the other one in git-shell, are OK because they are reading from an OS-provided argv, which is limited in practice. This patch should eliminate the immediate vulnerabilities. I picked 4MB as an arbitrary limit. It's big enough that nobody should ever run into it in practice (since the point is to run the commands via exec, we're subject to OS limits which are typically much lower). But it's small enough that allocating it isn't that big a deal. The code is mostly just swapping out fgets() for the strbuf call, but we have to add a few niceties like flushing and trimming line endings. We could simplify things further by putting the buffer on the stack, but 4MB is probably a bit much there. Note that we'll _always_ allocate 4MB, which for normal, non-malicious requests is more than we would before this patch. But on the other hand, other git programs are happy to use 96MB for a delta cache. And since we'd never touch most of those pages, on a lazy-allocating OS like Linux they won't even get allocated to actual RAM. The ideal would be a version of strbuf_getline() that accepted a maximum value. But for a minimal vulnerability fix, let's keep things localized and simple. We can always refactor further on top. The included test fails in an obvious way with ASan or UBSan (which notice the integer overflow and out-of-bounds reads). Without them, it fails in a less obvious way: we may segfault, or we may try to xstrfmt() a long string, leading to a BUG(). Either way, it fails reliably before this patch, and passes with it. Note that we don't need an EXPENSIVE prereq on it. It does take 10-15s to fail before this patch, but with the new limit, we fail almost immediately (and the perl process generating 2GB of data exits via SIGPIPE). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
32696a4 Jeff King 28 September 2022, 22:50:36 UTC shell: add basic tests We have no tests of even basic functionality of git-shell. Let's add a couple of obvious ones. This will serve as a framework for adding tests for new things we fix, as well as making sure we don't screw anything up too badly while doing so. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
a1d4f67 Taylor Blau 29 July 2022, 19:22:13 UTC transport: make `protocol.file.allow` be "user" by default An earlier patch discussed and fixed a scenario where Git could be used as a vector to exfiltrate sensitive data through a Docker container when a potential victim clones a suspicious repository with local submodules that contain symlinks. That security hole has since been plugged, but a similar one still exists. Instead of convincing a would-be victim to clone an embedded submodule via the "file" protocol, an attacker could convince an individual to clone a repository that has a submodule pointing to a valid path on the victim's filesystem. For example, if an individual (with username "foo") has their home directory ("/home/foo") stored as a Git repository, then an attacker could exfiltrate data by convincing a victim to clone a malicious repository containing a submodule pointing at "/home/foo/.git" with `--recurse-submodules`. Doing so would expose any sensitive contents in stored in "/home/foo" tracked in Git. For systems (such as Docker) that consider everything outside of the immediate top-level working directory containing a Dockerfile as inaccessible to the container (with the exception of volume mounts, and so on), this is a violation of trust by exposing unexpected contents in the working copy. To mitigate the likelihood of this kind of attack, adjust the "file://" protocol's default policy to be "user" to prevent commands that execute without user input (including recursive submodule initialization) from taking place by default. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
f4a32a5 Taylor Blau 29 July 2022, 19:21:53 UTC t/t9NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that interact with submodules a handful of times use `test_config_global`. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
0d3beb7 Taylor Blau 29 July 2022, 19:21:40 UTC t/t7NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
0f21b8f Taylor Blau 29 July 2022, 19:21:18 UTC t/t6NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
225d2d5 Taylor Blau 29 July 2022, 19:21:06 UTC t/t5NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
ac7e57f Taylor Blau 29 July 2022, 19:20:43 UTC t/t4NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
f8d510e Taylor Blau 29 July 2022, 19:20:28 UTC t/t3NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
99f4abb Taylor Blau 29 July 2022, 19:20:03 UTC t/2NNNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Test scripts that rely on submodules throughout use a `git config --global` during a setup test towards the beginning of the script. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
8a96dbc Taylor Blau 29 July 2022, 19:16:10 UTC t/t1NNN: allow local submodules To prepare for the default value of `protocol.file.allow` to change to "user", ensure tests that rely on local submodules can initialize them over the file protocol. Tests that only need to interact with submodules in a limited capacity have individual Git commands annotated with the appropriate configuration via `-c`. Tests that interact with submodules a handful of times use `test_config_global` instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
7de0c30 Taylor Blau 29 July 2022, 19:13:58 UTC t/lib-submodule-update.sh: allow local submodules To prepare for changing the default value of `protocol.file.allow` to "user", update the `prolog()` function in lib-submodule-update to allow submodules to be cloned over the file protocol. This is used by a handful of submodule-related test scripts, which themselves will have to tweak the value of `protocol.file.allow` in certain locations. Those will be done in subsequent commits. Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
6f054f9 Taylor Blau 28 July 2022, 21:35:17 UTC builtin/clone.c: disallow `--local` clones with symlinks When cloning a repository with `--local`, Git relies on either making a hardlink or copy to every file in the "objects" directory of the source repository. This is done through the callpath `cmd_clone()` -> `clone_local()` -> `copy_or_link_directory()`. The way this optimization works is by enumerating every file and directory recursively in the source repository's `$GIT_DIR/objects` directory, and then either making a copy or hardlink of each file. The only exception to this rule is when copying the "alternates" file, in which case paths are rewritten to be absolute before writing a new "alternates" file in the destination repo. One quirk of this implementation is that it dereferences symlinks when cloning. This behavior was most recently modified in 36596fd2df (clone: better handle symlinked files at .git/objects/, 2019-07-10), which attempted to support `--local` clones of repositories with symlinks in their objects directory in a platform-independent way. Unfortunately, this behavior of dereferencing symlinks (that is, creating a hardlink or copy of the source's link target in the destination repository) can be used as a component in attacking a victim by inadvertently exposing the contents of file stored outside of the repository. Take, for example, a repository that stores a Dockerfile and is used to build Docker images. When building an image, Docker copies the directory contents into the VM, and then instructs the VM to execute the Dockerfile at the root of the copied directory. This protects against directory traversal attacks by copying symbolic links as-is without dereferencing them. That is, if a user has a symlink pointing at their private key material (where the symlink is present in the same directory as the Dockerfile, but the key itself is present outside of that directory), the key is unreadable to a Docker image, since the link will appear broken from the container's point of view. This behavior enables an attack whereby a victim is convinced to clone a repository containing an embedded submodule (with a URL like "file:///proc/self/cwd/path/to/submodule") which has a symlink pointing at a path containing sensitive information on the victim's machine. If a user is tricked into doing this, the contents at the destination of those symbolic links are exposed to the Docker image at runtime. One approach to preventing this behavior is to recreate symlinks in the destination repository. But this is problematic, since symlinking the objects directory are not well-supported. (One potential problem is that when sharing, e.g. a "pack" directory via symlinks, different writers performing garbage collection may consider different sets of objects to be reachable, enabling a situation whereby garbage collecting one repository may remove reachable objects in another repository). Instead, prohibit the local clone optimization when any symlinks are present in the `$GIT_DIR/objects` directory of the source repository. Users may clone the repository again by prepending the "file://" scheme to their clone URL, or by adding the `--no-local` option to their `git clone` invocation. The directory iterator used by `copy_or_link_directory()` must no longer dereference symlinks (i.e., it *must* call `lstat()` instead of `stat()` in order to discover whether or not there are symlinks present). This has no bearing on the overall behavior, since we will immediately `die()` on encounter a symlink. Note that t5604.33 suggests that we do support local clones with symbolic links in the source repository's objects directory, but this was likely unintentional, or at least did not take into consideration the problem with sharing parts of the objects directory with symbolic links at the time. Update this test to reflect which options are and aren't supported. Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com> 01 October 2022, 04:23:38 UTC
88b7be6 Johannes Schindelin 27 May 2022, 21:38:36 UTC Git 2.30.5 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 23 June 2022, 10:31:05 UTC
3b0bf27 Carlo Marcelo Arenas Belón 10 May 2022, 19:35:29 UTC setup: tighten ownership checks post CVE-2022-24765 8959555cee7 (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02), adds a function to check for ownership of repositories using a directory that is representative of it, and ways to add exempt a specific repository from said check if needed, but that check didn't account for owership of the gitdir, or (when used) the gitfile that points to that gitdir. An attacker could create a git repository in a directory that they can write into but that is owned by the victim to work around the fix that was introduced with CVE-2022-24765 to potentially run code as the victim. An example that could result in privilege escalation to root in *NIX would be to set a repository in a shared tmp directory by doing (for example): $ git -C /tmp init To avoid that, extend the ensure_valid_ownership function to be able to check for all three paths. This will have the side effect of tripling the number of stat() calls when a repository is detected, but the effect is expected to be likely minimal, as it is done only once during the directory walk in which Git looks for a repository. Additionally make sure to resolve the gitfile (if one was used) to find the relevant gitdir for checking. While at it change the message printed on failure so it is clear we are referring to the repository by its worktree (or gitdir if it is bare) and not to a specific directory. Helped-by: Junio C Hamano <junio@pobox.com> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> 23 June 2022, 10:31:05 UTC
b779214 Junio C Hamano 26 May 2022, 21:51:32 UTC Merge branch 'cb/path-owner-check-with-sudo' With a recent update to refuse access to repositories of other people by default, "sudo make install" and "sudo git describe" stopped working. This series intends to loosen it while keeping the safety. * cb/path-owner-check-with-sudo: t0034: add negative tests and allow git init to mostly work under sudo git-compat-util: avoid failing dir ownership checks if running privileged t: regression git needs safe.directory when using sudo Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 23 June 2022, 10:31:04 UTC
6b11e3d Carlo Marcelo Arenas Belón 17 June 2022, 20:23:38 UTC git-compat-util: allow root to access both SUDO_UID and root owned Previous changes introduced a regression which will prevent root for accessing repositories owned by thyself if using sudo because SUDO_UID takes precedence. Loosen that restriction by allowing root to access repositories owned by both uid by default and without having to add a safe.directory exception. A previous workaround that was documented in the tests is no longer needed so it has been removed together with its specially crafted prerequisite. Helped-by: Johanness Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 17 June 2022, 21:03:08 UTC
b9063af Carlo Marcelo Arenas Belón 13 May 2022, 01:00:19 UTC t0034: add negative tests and allow git init to mostly work under sudo Add a support library that provides one function that can be used to run a "scriplet" of commands through sudo and that helps invoking sudo in the slightly awkward way that is required to ensure it doesn't block the call (if shell was allowed as tested in the prerequisite) and it doesn't run the command through a different shell than the one we intended. Add additional negative tests as suggested by Junio and that use a new workspace that is owned by root. Document a regression that was introduced by previous commits where root won't be able anymore to access directories they own unless SUDO_UID is removed from their environment. The tests document additional ways that this new restriction could be worked around and the documentation explains why it might be instead considered a feature, but a "fix" is planned for a future change. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 May 2022, 01:12:23 UTC
ae9abbb Carlo Marcelo Arenas Belón 13 May 2022, 01:00:18 UTC git-compat-util: avoid failing dir ownership checks if running privileged bdc77d1d685 (Add a function to determine whether a path is owned by the current user, 2022-03-02) checks for the effective uid of the running process using geteuid() but didn't account for cases where that user was root (because git was invoked through sudo or a compatible tool) and the original uid that repository trusted for its config was no longer known, therefore failing the following otherwise safe call: guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty [sudo] password for guy: fatal: unsafe repository ('/home/guy/Software/uncrustify' is owned by someone else) Attempt to detect those cases by using the environment variables that those tools create to keep track of the original user id, and do the ownership check using that instead. This assumes the environment the user is running on after going privileged can't be tampered with, and also adds code to restrict that the new behavior only applies if running as root, therefore keeping the most common case, which runs unprivileged, from changing, but because of that, it will miss cases where sudo (or an equivalent) was used to change to another unprivileged user or where the equivalent tool used to raise privileges didn't track the original id in a sudo compatible way. Because of compatibility with sudo, the code assumes that uid_t is an unsigned integer type (which is not required by the standard) but is used that way in their codebase to generate SUDO_UID. In systems where uid_t is signed, sudo might be also patched to NOT be unsigned and that might be able to trigger an edge case and a bug (as described in the code), but it is considered unlikely to happen and even if it does, the code would just mostly fail safely, so there was no attempt either to detect it or prevent it by the code, which is something that might change in the future, based on expected user feedback. Reported-by: Guy Maurel <guy.j@maurel.de> Helped-by: SZEDER Gábor <szeder.dev@gmail.com> Helped-by: Randall Becker <rsbecker@nexbridge.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 May 2022, 01:12:23 UTC
5f1a3fe Carlo Marcelo Arenas Belón 13 May 2022, 01:00:17 UTC t: regression git needs safe.directory when using sudo Originally reported after release of v2.35.2 (and other maint branches) for CVE-2022-24765 and blocking otherwise harmless commands that were done using sudo in a repository that was owned by the user. Add a new test script with very basic support to allow running git commands through sudo, so a reproduction could be implemented and that uses only `git status` as a proxy of the issue reported. Note that because of the way sudo interacts with the system, a much more complete integration with the test framework will require a lot more work and that was therefore intentionally punted for now. The current implementation requires the execution of a special cleanup function which should always be kept as the last "test" or otherwise the standard cleanup functions will fail because they can't remove the root owned directories that are used. This also means that if failures are found while running, the specifics of the failure might not be kept for further debugging and if the test was interrupted, it will be necessary to clean the working directory manually before restarting by running: $ sudo rm -rf trash\ directory.t0034-root-safe-directory/ The test file also uses at least one initial "setup" test that creates a parallel execution directory under the "root" sub directory, which should be used as top level directory for all repositories that are used in this test file. Unlike all other tests the repository provided by the test framework should go unused. Special care should be taken when invoking commands through sudo, since the environment is otherwise independent from what the test framework setup and might have changed the values for HOME, SHELL and dropped several relevant environment variables for your test. Indeed `git status` was used as a proxy because it doesn't even require commits in the repository to work and usually doesn't require much from the environment to run, but a future patch will add calls to `git init` and that will fail to honor the default branch name, unless that setting is NOT provided through an environment variable (which means even a CI run could fail that test if enabled incorrectly). A new SUDO prerequisite is provided that does some sanity checking to make sure the sudo command that will be used allows for passwordless execution as root without restrictions and doesn't mess with git's execution path. This matches what is provided by the macOS agents that are used as part of GitHub actions and probably nowhere else. Most of those characteristics make this test mostly only suitable for CI, but it might be executed locally if special care is taken to provide for all of them in the local configuration and maybe making use of the sudo credential cache by first invoking sudo, entering your password if needed, and then invoking the test with: $ GIT_TEST_ALLOW_SUDO=YES ./t0034-root-safe-directory.sh If it fails to run, then it means your local setup wouldn't work for the test because of the configuration sudo has or other system settings, and things that might help are to comment out sudo's secure_path config, and make sure that the account you are using has no restrictions on the commands it can run through sudo, just like is provided for the user in the CI. For example (assuming a username of marta for you) something probably similar to the following entry in your /etc/sudoers (or equivalent) file: marta ALL=(ALL:ALL) NOPASSWD: ALL Reported-by: SZEDER Gábor <szeder.dev@gmail.com> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 May 2022, 01:12:23 UTC
17083c7 Junio C Hamano 13 April 2022, 20:31:29 UTC Git 2.30.4 Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 April 2022, 20:31:29 UTC
0f85c4a Derrick Stolee 13 April 2022, 15:32:31 UTC setup: opt-out of check with safe.directory=* With the addition of the safe.directory in 8959555ce (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) released in v2.35.2, we are receiving feedback from a variety of users about the feature. Some users have a very large list of shared repositories and find it cumbersome to add this config for every one of them. In a more difficult case, certain workflows involve running Git commands within containers. The container boundary prevents any global or system config from communicating `safe.directory` values from the host into the container. Further, the container almost always runs as a different user than the owner of the directory in the host. To simplify the reactions necessary for these users, extend the definition of the safe.directory config value to include a possible '*' value. This value implies that all directories are safe, providing a single setting to opt-out of this protection. Note that an empty assignment of safe.directory clears all previous values, and this is already the case with the "if (!value || !*value)" condition. Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 April 2022, 19:42:51 UTC
bb50ec3 Matheus Valadares 13 April 2022, 15:32:30 UTC setup: fix safe.directory key not being checked It seems that nothing is ever checking to make sure the safe directories in the configs actually have the key safe.directory, so some unrelated config that has a value with a certain directory would also make it a safe directory. Signed-off-by: Matheus Valadares <me@m28.io> Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 April 2022, 19:42:51 UTC
e47363e Derrick Stolee 13 April 2022, 15:32:29 UTC t0033: add tests for safe.directory It is difficult to change the ownership on a directory in our test suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment variable to trick Git into thinking we are in a differently-owned directory. This allows us to test that the config is parsed correctly. Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 April 2022, 19:42:49 UTC
cb95038 Johannes Schindelin 17 March 2022, 09:15:15 UTC Git 2.30.3 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 23 March 2022, 23:22:17 UTC
fdcad5a Johannes Schindelin 23 March 2022, 22:00:41 UTC Fix `GIT_CEILING_DIRECTORIES` with `C:\` and the likes When determining the length of the longest ancestor of a given path with respect to to e.g. `GIT_CEILING_DIRECTORIES`, we special-case the root directory by returning 0 (i.e. we pretend that the path `/` does not end in a slash by virtually stripping it). That is the correct behavior because when normalizing paths, the root directory is special: all other directory paths have their trailing slash stripped, but not the root directory's path (because it would become the empty string, which is not a legal path). However, this special-casing of the root directory in `longest_ancestor_length()` completely forgets about Windows-style root directories, e.g. `C:\`. These _also_ get normalized with a trailing slash (because `C:` would actually refer to the current directory on that drive, not necessarily to its root directory). In fc56c7b34b (mingw: accomodate t0060-path-utils for MSYS2, 2016-01-27), we almost got it right. We noticed that `longest_ancestor_length()` expects a slash _after_ the matched prefix, and if the prefix already ends in a slash, the normalized path won't ever match and -1 is returned. But then that commit went astray: The correct fix is not to adjust the _tests_ to expect an incorrect -1 when that function is fed a prefix that ends in a slash, but instead to treat such a prefix as if the trailing slash had been removed. Likewise, that function needs to handle the case where it is fed a path that ends in a slash (not only a prefix that ends in a slash): if it matches the prefix (plus trailing slash), we still need to verify that the path does not end there, otherwise the prefix is not actually an ancestor of the path but identical to it (and we need to return -1 in that case). With these two adjustments, we no longer need to play games in t0060 where we only add `$rootoff` if the passed prefix is different from the MSYS2 pseudo root, instead we also add it for the MSYS2 pseudo root itself. We do have to be careful to skip that logic entirely for Windows paths, though, because they do are not subject to that MSYS2 pseudo root treatment. This patch fixes the scenario where a user has set `GIT_CEILING_DIRECTORIES=C:\`, which would be ignored otherwise. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 23 March 2022, 23:21:08 UTC
8959555 Johannes Schindelin 02 March 2022, 11:23:04 UTC setup_git_directory(): add an owner check for the top-level directory It poses a security risk to search for a git directory outside of the directories owned by the current user. For example, it is common e.g. in computer pools of educational institutes to have a "scratch" space: a mounted disk with plenty of space that is regularly swiped where any authenticated user can create a directory to do their work. Merely navigating to such a space with a Git-enabled `PS1` when there is a maliciously-crafted `/scratch/.git/` can lead to a compromised account. The same holds true in multi-user setups running Windows, as `C:\` is writable to every authenticated user by default. To plug this vulnerability, we stop Git from accepting top-level directories owned by someone other than the current user. We avoid looking at the ownership of each and every directories between the current and the top-level one (if there are any between) to avoid introducing a performance bottleneck. This new default behavior is obviously incompatible with the concept of shared repositories, where we expect the top-level directory to be owned by only one of its legitimate users. To re-enable that use case, we add support for adding exceptions from the new default behavior via the config setting `safe.directory`. The `safe.directory` config setting is only respected in the system and global configs, not from repository configs or via the command-line, and can have multiple values to allow for multiple shared repositories. We are particularly careful to provide a helpful message to any user trying to use a shared repository. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 21 March 2022, 12:16:26 UTC
bdc77d1 Johannes Schindelin 02 March 2022, 10:06:24 UTC Add a function to determine whether a path is owned by the current user This function will be used in the next commit to prevent `setup_git_directory()` from discovering a repository in a directory that is owned by someone other than the current user. Note: We cannot simply use `st.st_uid` on Windows just like we do on Linux and other Unix-like platforms: according to https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-functions this field is always zero on Windows (because Windows' idea of a user ID does not fit into a single numerical value). Therefore, we have to do something a little involved to replicate the same functionality there. Also note: On Windows, a user's home directory is not actually owned by said user, but by the administrator. For all practical purposes, it is under the user's control, though, therefore we pretend that it is owned by the user. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 21 March 2022, 12:16:26 UTC
2a9a586 Johannes Schindelin 17 March 2022, 11:52:12 UTC Merge branch 'cb/mingw-gmtime-r' Build fix on Windows. * cb/mingw-gmtime-r: mingw: avoid fallback for {local,gm}time_r() Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 March 2022, 11:52:12 UTC
6e7ad1e Carlo Marcelo Arenas Belón 27 November 2021, 10:15:32 UTC mingw: avoid fallback for {local,gm}time_r() mingw-w64's pthread_unistd.h had a bug that mistakenly (because there is no support for the *lockfile() functions required[1]) defined _POSIX_THREAD_SAFE_FUNCTIONS and that was being worked around since 3ecd153a3b (compat/mingw: support MSys2-based MinGW build, 2016-01-14). The bug was fixed in winphtreads, but as a side effect, leaves the reentrant functions from time.h no longer visible and therefore breaks the build. Since the intention all along was to avoid using the fallback functions, formalize the use of POSIX by setting the corresponding feature flag and compile out the implementation for the fallback functions. [1] https://unix.org/whitepapers/reentrant.html Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> 17 March 2022, 11:52:12 UTC
94f6e3e Johannes Schindelin 12 February 2021, 14:51:13 UTC Git 2.30.2 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:51:13 UTC
e4e6808 Johannes Schindelin 12 February 2021, 14:51:12 UTC Sync with 2.29.3 * maint-2.29: Git 2.29.3 Git 2.28.1 Git 2.27.1 Git 2.26.3 Git 2.25.5 Git 2.24.4 Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:51:12 UTC
0628636 Johannes Schindelin 12 February 2021, 14:50:15 UTC Git 2.29.3 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:50:15 UTC
d7bdabe Johannes Schindelin 12 February 2021, 14:50:14 UTC Sync with 2.28.1 * maint-2.28: Git 2.28.1 Git 2.27.1 Git 2.26.3 Git 2.25.5 Git 2.24.4 Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:50:14 UTC
e4f4299 Johannes Schindelin 12 February 2021, 14:50:10 UTC Git 2.28.1 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:50:10 UTC
3f01e56 Johannes Schindelin 12 February 2021, 14:50:09 UTC Sync with 2.27.1 * maint-2.27: Git 2.27.1 Git 2.26.3 Git 2.25.5 Git 2.24.4 Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:50:09 UTC
6ff7f46 Johannes Schindelin 12 February 2021, 14:50:05 UTC Git 2.27.1 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:50:05 UTC
2d1142a Johannes Schindelin 12 February 2021, 14:50:04 UTC Sync with 2.26.3 * maint-2.26: Git 2.26.3 Git 2.25.5 Git 2.24.4 Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:50:04 UTC
a79fd20 Johannes Schindelin 12 February 2021, 14:50:00 UTC Git 2.26.3 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:50:00 UTC
8f80393 Johannes Schindelin 12 February 2021, 14:49:59 UTC Sync with 2.25.5 * maint-2.25: Git 2.25.5 Git 2.24.4 Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:49:59 UTC
42ce4c7 Johannes Schindelin 12 February 2021, 14:49:55 UTC Git 2.25.5 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:49:55 UTC
97d1dcb Johannes Schindelin 12 February 2021, 14:49:55 UTC Sync with 2.24.4 * maint-2.24: Git 2.24.4 Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:49:55 UTC
06214d1 Johannes Schindelin 12 February 2021, 14:49:50 UTC Git 2.24.4 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:49:50 UTC
92ac04b Johannes Schindelin 12 February 2021, 14:49:50 UTC Sync with 2.23.4 * maint-2.23: Git 2.23.4 Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:49:50 UTC
d60b6a9 Johannes Schindelin 12 February 2021, 14:49:46 UTC Git 2.23.4 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:49:46 UTC
4bd06fd Johannes Schindelin 12 February 2021, 14:49:45 UTC Sync with 2.22.5 * maint-2.22: Git 2.22.5 Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:49:45 UTC
c753e2a Johannes Schindelin 12 February 2021, 14:49:41 UTC Git 2.22.5 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:49:41 UTC
bcf08f3 Johannes Schindelin 12 February 2021, 14:49:41 UTC Sync with 2.21.4 * maint-2.21: Git 2.21.4 Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:49:41 UTC
c735d74 Johannes Schindelin 12 February 2021, 14:49:36 UTC Git 2.21.4 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 February 2021, 14:49:36 UTC
b1726b1 Johannes Schindelin 12 February 2021, 14:49:35 UTC Sync with 2.20.5 * maint-2.20: Git 2.20.5 Git 2.19.6 Git 2.18.5 Git 2.17.6 unpack_trees(): start with a fresh lstat cache run-command: invalidate lstat cache after a command finished checkout: fix bug that makes checkout follow symlinks in leading path 12 February 2021, 14:49:35 UTC
back to top