Skip to main content

Browse the archive

https://github.com/git/git
05 April 2024, 19:32:26 UTC
sort by:
Revision Author Date Message Commit Date
3a19048 Johannes Schindelin 11 March 2023, 19:29:33 UTC Git 2.33.8 Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:54 UTC
bcd874d Johannes Schindelin 11 March 2023, 19:29:32 UTC Sync with 2.32.7 * maint-2.32: (26 commits) Git 2.32.7 Git 2.31.8 tests: avoid using `test_i18ncmp` Git 2.30.9 gettext: avoid using gettext if the locale dir is not present apply --reject: overwrite existing `.rej` symlink if it exists http.c: clear the 'finished' member once we are done with it clone.c: avoid "exceeds maximum object size" error with GCC v12.x range-diff: use ssize_t for parsed "len" in read_patches() range-diff: handle unterminated lines in read_patches() range-diff: drop useless "offset" variable from read_patches() t5604: GETTEXT_POISON fix, conclusion t5604: GETTEXT_POISON fix, part 1 t5619: GETTEXT_POISON fix t0003: GETTEXT_POISON fix, conclusion t0003: GETTEXT_POISON fix, part 1 t0033: GETTEXT_POISON fix http: support CURLOPT_PROTOCOLS_STR http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT ci: install python on ubuntu ... 17 April 2023, 19:15:52 UTC
b8787a9 Johannes Schindelin 11 March 2023, 18:24:36 UTC Git 2.32.7 Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:51 UTC
31f7fe5 Johannes Schindelin 11 March 2023, 18:24:34 UTC Sync with 2.31.8 * maint-2.31: (25 commits) Git 2.31.8 tests: avoid using `test_i18ncmp` Git 2.30.9 gettext: avoid using gettext if the locale dir is not present apply --reject: overwrite existing `.rej` symlink if it exists http.c: clear the 'finished' member once we are done with it clone.c: avoid "exceeds maximum object size" error with GCC v12.x range-diff: use ssize_t for parsed "len" in read_patches() range-diff: handle unterminated lines in read_patches() range-diff: drop useless "offset" variable from read_patches() t5604: GETTEXT_POISON fix, conclusion t5604: GETTEXT_POISON fix, part 1 t5619: GETTEXT_POISON fix t0003: GETTEXT_POISON fix, conclusion t0003: GETTEXT_POISON fix, part 1 t0033: GETTEXT_POISON fix http: support CURLOPT_PROTOCOLS_STR http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT ci: install python on ubuntu ci: use the same version of p4 on both Linux and macOS ... 17 April 2023, 19:15:49 UTC
ea56f91 Johannes Schindelin 11 March 2023, 16:54:15 UTC Git 2.31.8 Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:47 UTC
92957d8 Johannes Schindelin 11 March 2023, 17:02:04 UTC tests: avoid using `test_i18ncmp` Since `test_i18ncmp` was deprecated in v2.31.*, the instances added in v2.30.9 needed to be converted to `test_cmp` calls. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:45 UTC
b524e89 Johannes Schindelin 11 March 2023, 16:54:13 UTC Sync with 2.30.9 * maint-2.30: (23 commits) Git 2.30.9 gettext: avoid using gettext if the locale dir is not present apply --reject: overwrite existing `.rej` symlink if it exists http.c: clear the 'finished' member once we are done with it clone.c: avoid "exceeds maximum object size" error with GCC v12.x range-diff: use ssize_t for parsed "len" in read_patches() range-diff: handle unterminated lines in read_patches() range-diff: drop useless "offset" variable from read_patches() t5604: GETTEXT_POISON fix, conclusion t5604: GETTEXT_POISON fix, part 1 t5619: GETTEXT_POISON fix t0003: GETTEXT_POISON fix, conclusion t0003: GETTEXT_POISON fix, part 1 t0033: GETTEXT_POISON fix http: support CURLOPT_PROTOCOLS_STR http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT 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 github-actions: run gcc-8 on ubuntu-20.04 image ... 17 April 2023, 19:15:44 UTC
668f2d5 Taylor Blau 14 April 2023, 15:54:08 UTC Git 2.30.9 Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:43 UTC
528290f Taylor Blau 14 April 2023, 15:46:59 UTC Merge branch 'tb/config-copy-or-rename-in-file-injection' Avoids issues with renaming or deleting sections with long lines, where configuration values may be interpreted as sections, leading to configuration injection. Addresses CVE-2023-29007. * tb/config-copy-or-rename-in-file-injection: config.c: disallow overly-long lines in `copy_or_rename_section_in_file()` config.c: avoid integer truncation in `copy_or_rename_section_in_file()` config: avoid fixed-sized buffer when renaming/deleting a section t1300: demonstrate failure when renaming sections with long lines Signed-off-by: Taylor Blau <me@ttaylorr.com> 17 April 2023, 19:15:42 UTC
4fe5d0b Johannes Schindelin 14 March 2023, 20:32:42 UTC Merge branch 'avoid-using-uninitialized-gettext' Avoids the overhead of calling `gettext` when initialization of the translated messages was skipped. Addresses CVE-2023-25815. * avoid-using-uninitialized-gettext: (1 commit) gettext: avoid using gettext if the locale dir is not present 17 April 2023, 19:15:42 UTC
18e2b1c Junio C Hamano 02 March 2023, 23:13:30 UTC Merge branch 'js/apply-overwrite-rej-symlink-if-exists' into maint-2.30 Address CVE-2023-25652 by deleting any existing `.rej` symbolic links instead of following them. * js/apply-overwrite-rej-symlink-if-exists: apply --reject: overwrite existing `.rej` symlink if it exists Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> 17 April 2023, 19:15:41 UTC
3bb3d6b Taylor Blau 12 April 2023, 23:18:28 UTC config.c: disallow overly-long lines in `copy_or_rename_section_in_file()` As a defense-in-depth measure to guard against any potentially-unknown buffer overflows in `copy_or_rename_section_in_file()`, refuse to work with overly-long lines in a gitconfig. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> 17 April 2023, 19:15:40 UTC
e91cfe6 Taylor Blau 06 April 2023, 18:28:53 UTC config.c: avoid integer truncation in `copy_or_rename_section_in_file()` There are a couple of spots within `copy_or_rename_section_in_file()` that incorrectly use an `int` to track an offset within a string, which may truncate or wrap around to a negative value. Historically it was impossible to have a line longer than 1024 bytes anyway, since we used fgets() with a fixed-size buffer of exactly that length. But the recent change to use a strbuf permits us to read lines of arbitrary length, so it's possible for a malicious input to cause us to overflow past INT_MAX and do an out-of-bounds array read. Practically speaking, however, this should never happen, since it requires 2GB section names or values, which are unrealistic in non-malicious circumstances. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> 17 April 2023, 19:15:40 UTC
a5bb10f Taylor Blau 06 April 2023, 18:07:58 UTC config: avoid fixed-sized buffer when renaming/deleting a section When renaming (or deleting) a section of configuration, Git uses the function `git_config_copy_or_rename_section_in_file()` to rewrite the configuration file after applying the rename or deletion to the given section. To do this, Git repeatedly calls `fgets()` to read the existing configuration data into a fixed size buffer. When the configuration value under `old_name` exceeds the size of the buffer, we will call `fgets()` an additional time even if there is no newline in the configuration file, since our read length is capped at `sizeof(buf)`. If the first character of the buffer (after zero or more characters satisfying `isspace()`) is a '[', Git will incorrectly treat it as beginning a new section when the original section is being removed. In other words, a configuration value satisfying this criteria can incorrectly be considered as a new secftion instead of a variable in the original section. Avoid this issue by using a variable-width buffer in the form of a strbuf rather than a fixed-with region on the stack. A couple of small points worth noting: - Using a strbuf will cause us to allocate arbitrary sizes to match the length of each line. In practice, we don't expect any reasonable configuration files to have lines that long, and a bandaid will be introduced in a later patch to ensure that this is the case. - We are using strbuf_getwholeline() here instead of strbuf_getline() in order to match `fgets()`'s behavior of leaving the trailing LF character on the buffer (as well as a trailing NUL). This could be changed later, but using strbuf_getwholeline() changes the least about this function's implementation, so it is picked as the safest path. - It is temping to want to replace the loop to skip over characters matching isspace() at the beginning of the buffer with a convenience function like `strbuf_ltrim()`. But this is the wrong approach for a couple of reasons: First, it involves a potentially large and expensive `memmove()` which we would like to avoid. Second, and more importantly, we also *do* want to preserve those spaces to avoid changing the output of other sections. In all, this patch is a minimal replacement of the fixed-width buffer in `git_config_copy_or_rename_section_in_file()` to instead use a `struct strbuf`. Reported-by: André Baptista <andre@ethiack.com> Reported-by: Vítor Pinho <vitor@ethiack.com> Helped-by: Patrick Steinhardt <ps@pks.im> Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com> 17 April 2023, 19:15:40 UTC
2919821 Taylor Blau 06 April 2023, 15:42:03 UTC t1300: demonstrate failure when renaming sections with long lines When renaming a configuration section which has an entry whose length exceeds the size of our buffer in config.c's implementation of `git_config_copy_or_rename_section_in_file()`, Git will incorrectly form a new configuration section with part of the data in the section being removed. In this instance, our first configuration file looks something like: [b] c = d <spaces> [a] e = f [a] g = h Here, we have two configuration values, "b.c", and "a.g". The value "[a] e = f" belongs to the configuration value "b.c", and does not form its own section. However, when renaming the section 'a' to 'xyz', Git will write back "[xyz]\ne = f", but "[xyz]" is still attached to the value of "b.c", which is why "e = f" on its own line becomes a new entry called "b.e". A slightly different example embeds the section being renamed within another section. Demonstrate this failure in a test in t1300, which we will fix in the following commit. Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Taylor Blau <me@ttaylorr.com> 17 April 2023, 19:15:39 UTC
c4137be Johannes Schindelin 22 February 2023, 11:40:55 UTC gettext: avoid using gettext if the locale dir is not present In cc5e1bf99247 (gettext: avoid initialization if the locale dir is not present, 2018-04-21) Git was taught to avoid a costly gettext start-up when there are not even any localized messages to work with. But we still called `gettext()` and `ngettext()` functions. Which caused a problem in Git for Windows when the libgettext that is consumed from the MSYS2 project stopped using a runtime prefix in https://github.com/msys2/MINGW-packages/pull/10461 Due to that change, we now use an unintialized gettext machinery that might get auto-initialized _using an unintended locale directory_: `C:\mingw64\share\locale`. Let's record the fact when the gettext initialization was skipped, and skip calling the gettext functions accordingly. This addresses CVE-2023-25815. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:39 UTC
9db0571 Johannes Schindelin 09 March 2023, 15:02:54 UTC apply --reject: overwrite existing `.rej` symlink if it exists The `git apply --reject` is expected to write out `.rej` files in case one or more hunks fail to apply cleanly. Historically, the command overwrites any existing `.rej` files. The idea being that apply/reject/edit cycles are relatively common, and the generated `.rej` files are not considered precious. But the command does not overwrite existing `.rej` symbolic links, and instead follows them. This is unsafe because the same patch could potentially create such a symbolic link and point at arbitrary paths outside the current worktree, and `git apply` would write the contents of the `.rej` file into that location. Therefore, let's make sure that any existing `.rej` file or symbolic link is removed before writing it. Reported-by: RyotaK <ryotak.mail@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Linus Torvalds <torvalds@linuxfoundation.org> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 17 April 2023, 19:15:38 UTC
2f3b28f Johannes Schindelin 09 March 2023, 22:08:47 UTC Merge branch 'js/gettext-poison-fixes' The `maint-2.30` branch accumulated quite a few fixes over the past two years. Most of those fixes were originally based on newer versions, and while the patches cherry-picked cleanly, we weren't diligent enough to pay attention to the CI builds and the GETTEXT_POISON job regressed. This topic branch fixes that. * js/gettext-poison-fixes t0033: GETTEXT_POISON fix t0003: GETTEXT_POISON fix, part 1 t0003: GETTEXT_POISON fix, conclusion t5619: GETTEXT_POISON fix t5604: GETTEXT_POISON fix, part 1 t5604: GETTEXT_POISON fix, conclusion 17 April 2023, 19:15:37 UTC
4989c35 Junio C Hamano 13 September 2022, 19:21:10 UTC Merge branch 'ds/github-actions-use-newer-ubuntu' Update the version of Ubuntu used for GitHub Actions CI from 18.04 to 22.04. * ds/github-actions-use-newer-ubuntu: ci: update 'static-analysis' to Ubuntu 22.04 17 April 2023, 19:15:36 UTC
fef08dd Derrick Stolee 23 August 2022, 17:28:11 UTC ci: update 'static-analysis' to Ubuntu 22.04 GitHub Actions scheduled a brownout of Ubuntu 18.04, which canceled all runs of the 'static-analysis' job in our CI runs. Update to 22.04 to avoid this as the brownout later turns into a complete deprecation. The use of 18.04 was set in d051ed77ee6 (.github/workflows/main.yml: run static-analysis on bionic, 2021-02-08) due to the lack of Coccinelle being available on 20.04 (which continues today). Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 17 April 2023, 16:17:53 UTC
e4cb369 Junio C Hamano 30 August 2021, 23:06:05 UTC Merge branch 'backport/jk/range-diff-fixes' "git range-diff" code clean-up. Needed to pacify modern GCC versions. * jk/range-diff-fixes: range-diff: use ssize_t for parsed "len" in read_patches() range-diff: handle unterminated lines in read_patches() range-diff: drop useless "offset" variable from read_patches() 22 March 2023, 17:00:36 UTC
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
0c8d22a Johannes Schindelin 09 March 2023, 21:54:32 UTC t5604: GETTEXT_POISON fix, conclusion In fade728df122 (apply: fix writing behind newly created symbolic links, 2023-02-02), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:56 UTC
7c811ed Johannes Schindelin 09 March 2023, 21:55:40 UTC t5604: GETTEXT_POISON fix, part 1 In bffc762f87ae (dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS, 2023-01-24), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:56 UTC
a2b2173 Johannes Schindelin 09 March 2023, 21:56:06 UTC t5619: GETTEXT_POISON fix In cf8f6ce02a13 (clone: delay picking a transport until after get_repo_path(), 2023-01-24), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:56 UTC
c025b4b Jeff King 09 August 2021, 22:48:48 UTC range-diff: use ssize_t for parsed "len" in read_patches() As we iterate through the buffer containing git-log output, parsing lines, we use an "int" to store the size of an individual line. This should be a size_t, as we have no guarantee that there is not a malicious 2GB+ commit-message line in the output. Overflowing this integer probably doesn't do anything _too_ terrible. We are not using the value to size a buffer, so the worst case is probably an out-of-bounds read from before the array. But it's easy enough to fix. Note that we have to use ssize_t here, since we also store the length result from parse_git_diff_header(), which may return a negative value for error. That function actually returns an int itself, which has a similar overflow problem, but I'll leave that for another day. Much of the apply.c code uses ints and should be converted as a whole; in the meantime, a negative return from parse_git_diff_header() will be interpreted as an error, and we'll bail (so we can't handle such a case, but given that it's likely to be malicious anyway, the important thing is we don't have any memory errors). Signed-off-by: Jeff King <peff@peff.net> Acked-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:55 UTC
a36df79 Jeff King 09 August 2021, 22:48:39 UTC range-diff: handle unterminated lines in read_patches() When parsing our buffer of output from git-log, we have a find_end_of_line() helper that finds the next newline, and gives us the number of bytes to move past it, or the size of the whole remaining buffer if there is no newline. But trying to handle both those cases leads to some oddities: - we try to overwrite the newline with NUL in the caller, by writing over line[len-1]. This is at best redundant, since the helper will already have done so if it saw a newline. But if it didn't see a newline, it's actively wrong; we'll overwrite the byte at the end of the (unterminated) line. We could solve this just dropping the extra NUL assignment in the caller and just letting the helper do the right thing. But... - if we see a "diff --git" line, we'll restore the newline on top of the NUL byte, so we can pass the string to parse_git_diff_header(). But if there was no newline in the first place, we can't do this. There's no place to put it (the current code writes a newline over whatever byte we obliterated earlier). The best we can do is feed the complete remainder of the buffer to the function (which is, in fact, a string, by virtue of being a strbuf). To solve this, the caller needs to know whether we actually found a newline or not. We could modify find_end_of_line() to return that information, but we can further observe that it has only one caller. So let's just inline it in that caller. Nobody seems to have noticed this case, probably because git-log would never produce input that doesn't end with a newline. Arguably we could just return an error as soon as we see that the output does not end in a newline. But the code to do so actually ends up _longer_, mostly because of the cleanup we have to do in handling the error. Signed-off-by: Jeff King <peff@peff.net> Acked-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 12 March 2023, 19:31:55 UTC
d99728b Johannes Schindelin 09 March 2023, 21:52:41 UTC t0003: GETTEXT_POISON fix, conclusion In 3c50032ff528 (attr: ignore overly large gitattributes files, 2022-12-01), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:55 UTC
e4298cc Johannes Schindelin 09 March 2023, 21:51:43 UTC t0003: GETTEXT_POISON fix, part 1 In dfa6b32b5e59 (attr: ignore attribute lines exceeding 2048 bytes, 2022-12-01), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:55 UTC
8516dac Johannes Schindelin 09 March 2023, 21:54:06 UTC t0033: GETTEXT_POISON fix In e47363e5a8bd (t0033: add tests for safe.directory, 2022-04-13), we backported a patch onto v2.30.* that was originally based on a much newer version. The v2.30.* release train still has the GETTEXT_POISON CI job, though, and hence needs `test_i18n*` in its tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 12 March 2023, 19:31:55 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
18bc8eb Jeff King 09 August 2021, 22:47:42 UTC range-diff: drop useless "offset" variable from read_patches() The "offset" variable was was introduced in 44b67cb62b (range-diff: split lines manually, 2019-07-11), but it has never done anything useful. We use it to count up the number of bytes we've consumed, but we never look at the result. It was probably copied accidentally from an almost-identical loop in apply.c:find_header() (and the point of that commit was to make use of the parse_git_diff_header() function which underlies both). Because the variable was set but not used, most compilers didn't seem to notice, but the upcoming clang-14 does complain about it, via its -Wunused-but-set-variable warning. Signed-off-by: Jeff King <peff@peff.net> Acked-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> 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
ed4404a Johannes Schindelin 06 February 2023, 08:25:58 UTC Git 2.33.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 06 February 2023, 08:25:58 UTC
87248c5 Johannes Schindelin 06 February 2023, 08:25:56 UTC Sync with 2.32.6 * maint-2.32: Git 2.32.6 Git 2.31.7 Git 2.30.8 apply: fix writing behind newly created symbolic links 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:25:56 UTC
2aedeff Johannes Schindelin 06 February 2023, 08:25:09 UTC Git 2.32.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 06 February 2023, 08:25:09 UTC
aeb93d7 Johannes Schindelin 06 February 2023, 08:25:08 UTC Sync with 2.31.7 * maint-2.31: Git 2.31.7 Git 2.30.8 apply: fix writing behind newly created symbolic links 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:25:08 UTC
0bbcf95 Johannes Schindelin 06 February 2023, 08:24:07 UTC Git 2.31.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> 06 February 2023, 08:24:07 UTC
e14d6b8 Johannes Schindelin 06 February 2023, 08:24:06 UTC Sync with 2.30.8 * maint-2.30: Git 2.30.8 apply: fix writing behind newly created symbolic links 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:24:06 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
25d7cb6 Junio C Hamano 19 January 2023, 21:46:04 UTC Sync with maint-2.32 * maint-2.32: attr: adjust a mismatched data type 19 January 2023, 21:46:04 UTC
012e0d7 Junio C Hamano 19 January 2023, 21:45:37 UTC Sync with maint-2.31 * maint-2.31: attr: adjust a mismatched data type 19 January 2023, 21:45:37 UTC
f8bf6b8 Junio C Hamano 19 January 2023, 21:45:23 UTC Sync with maint-2.30 * maint-2.30: attr: adjust a mismatched data type 19 January 2023, 21:45:23 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
7fe9bf5 Junio C Hamano 13 December 2022, 12:13:48 UTC Git 2.33.6 Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 December 2022, 12:13:48 UTC
5f22dcc Junio C Hamano 13 December 2022, 12:13:11 UTC Sync with Git 2.32.5 13 December 2022, 12:13:11 UTC
d96ea53 Junio C Hamano 13 December 2022, 12:10:27 UTC Git 2.32.5 Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 December 2022, 12:10:27 UTC
32e357b Junio C Hamano 13 December 2022, 12:09:56 UTC Merge branch 'ps/attr-limits-with-fsck' into maint-2.32 13 December 2022, 12:09:56 UTC
8a755ed Junio C Hamano 13 December 2022, 12:09:40 UTC Sync with Git 2.31.6 13 December 2022, 12:09:40 UTC
82689d5 Junio C Hamano 13 December 2022, 12:04:03 UTC Git 2.31.6 Signed-off-by: Junio C Hamano <gitster@pobox.com> 13 December 2022, 12:04:03 UTC
1612876 Junio C Hamano 13 December 2022, 12:02:20 UTC Sync with Git 2.30.7 13 December 2022, 12:02:20 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
27ab478 Patrick Steinhardt 01 December 2022, 14:46:09 UTC fsck: implement checks for gitattributes Recently, a vulnerability was reported that can lead to an out-of-bounds write when reading an unreasonably large gitattributes file. The root cause of this error are multiple integer overflows in different parts of the code when there are either too many lines, when paths are too long, when attribute names are too long, or when there are too many attributes declared for a pattern. As all of these are related to size, it seems reasonable to restrict the size of the gitattributes file via git-fsck(1). This allows us to both stop distributing known-vulnerable objects via common hosting platforms that have fsck enabled, and users to protect themselves by enabling the `fetch.fsckObjects` config. There are basically two checks: 1. We verify that size of the gitattributes file is smaller than 100MB. 2. We verify that the maximum line length does not exceed 2048 bytes. With the preceding commits, both of these conditions would cause us to either ignore the complete gitattributes file or blob in the first case, or the specific line in the second case. Now with these consistency checks added, we also grow the ability to stop distributing such files in the first place when `receive.fsckObjects` is enabled. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 08:07:04 UTC
f8587c3 Patrick Steinhardt 01 December 2022, 14:46:05 UTC fsck: move checks for gitattributes Move the checks for gitattributes so that they can be extended more readily. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 08:05:00 UTC
a59a8c6 Patrick Steinhardt 01 December 2022, 14:46:01 UTC fsck: pull out function to check a set of blobs In `fsck_finish()` we check all blobs for consistency that we have found during the tree walk, but that haven't yet been checked. This is only required for gitmodules right now, but will also be required for a new check for gitattributes. Pull out a function `fsck_blobs()` that allows the caller to check a set of blobs for consistency. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 08:05:00 UTC
bb3a926 Patrick Steinhardt 01 December 2022, 14:45:57 UTC fsck: refactor `fsck_blob()` to allow for more checks In general, we don't need to validate blob contents as they are opaque blobs about whose content Git doesn't need to care about. There are some exceptions though when blobs are linked into trees so that they would be interpreted by Git. We only have a single such check right now though, which is the one for gitmodules that has been added in the context of CVE-2018-11235. Now we have found another vulnerability with gitattributes that can lead to out-of-bounds writes and reads. So let's refactor `fsck_blob()` so that it is more extensible and can check different types of blobs. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com> 09 December 2022, 08:05:00 UTC
e0bfc0b Junio C Hamano 09 December 2022, 08:03:49 UTC Merge branch 'ps/attr-limits' into maint-2.32 09 December 2022, 08:03:49 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
7800e1d Taylor Blau 30 September 2022, 21:04:26 UTC Git 2.33.5 Signed-off-by: Taylor Blau <me@ttaylorr.com> 06 October 2022, 21:42:27 UTC
back to top