From 580097bf95fccda1ebed4b9d81635a27fb322fd9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Apr 2024 16:05:17 -0400 Subject: [PATCH 01/39] http: reset POSTFIELDSIZE when clearing curl handle In get_active_slot(), we return a CURL handle that may have been used before (reusing them is good because it lets curl reuse the same connection across many requests). We set a few curl options back to defaults that may have been modified by previous requests. We reset POSTFIELDS to NULL, but do not reset POSTFIELDSIZE (which defaults to "-1"). This usually doesn't matter because most POSTs will set both fields together anyway. But there is one exception: when handling a large request in remote-curl's post_rpc(), we don't set _either_, and instead set a READFUNCTION to stream data into libcurl. This can interact weirdly with a stale POSTFIELDSIZE setting, because curl will assume it should read only some set number of bytes from our READFUNCTION. However, it has worked in practice because we also manually set a "Transfer-Encoding: chunked" header, which libcurl uses as a clue to set the POSTFIELDSIZE to -1 itself. So everything works, but we're better off resetting the size manually for a few reasons: - there was a regression in curl 8.7.0 where the chunked header detection didn't kick in, causing any large HTTP requests made by Git to fail. This has since been fixed (but not yet released). In the issue, curl folks recommended setting it explicitly to -1: https://github.com/curl/curl/issues/13229#issuecomment-2029826058 and it indeed works around the regression. So even though it won't be strictly necessary after the fix there, this will help folks who end up using the affected libcurl versions. - it's consistent with what a new curl handle would look like. Since get_active_slot() may or may not return a used handle, this reduces the possibility of heisenbugs that only appear with certain request patterns. Note that the recommendation in the curl issue is to actually drop the manual Transfer-Encoding header. Modern libcurl will add the header itself when streaming from a READFUNCTION. However, that code wasn't added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to support back to 7.19.5, so those older versions still need the manual header. This is a backport of 3242311742 (http: reset POSTFIELDSIZE when clearing curl handle, 2024-04-02) into the `maint-2.39` branch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- http.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http.c b/http.c index 5c2fcfa840..bc15ffc23c 100644 --- a/http.c +++ b/http.c @@ -1247,6 +1247,7 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL); curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); + curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, -1L); curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); From ea61df9d1e0df224f7d9fe8278944247b07b9047 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Apr 2024 16:06:00 -0400 Subject: [PATCH 02/39] INSTALL: bump libcurl version to 7.21.3 Our documentation claims we support curl versions back to 7.19.5. But we can no longer compile with that version since adding an unconditional use of CURLOPT_RESOLVE in 511cfd3bff (http: add custom hostname to IP address resolutions, 2022-05-16). That feature wasn't added to libcurl until 7.21.3. We could add #ifdefs to make this work back to 7.19.5. But given that nobody noticed the compilation failure in the intervening two years, it makes more sense to bump the version in the documentation to 7.21.3 (which is itself over 13 years old). We could perhaps go forward even more (which would let us drop some cruft from git-curl-compat.h), but this should be an obviously safe jump, and we can move forward later. Note that user-visible syntax for CURLOPT_RESOLVE has grown new features in subsequent curl versions. Our documentation mentions "+" and "-" entries, which require more recent versions than 7.21.3. We could perhaps clarify that in our docs, but it's probably not worth cluttering them with restrictions of ancient curl versions. This is a backport of c28ee09503 (INSTALL: bump libcurl version to 7.21.3, 2024-04-02) into the `maint-2.39` branch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- INSTALL | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/INSTALL b/INSTALL index 8dd577e102..113920ff06 100644 --- a/INSTALL +++ b/INSTALL @@ -144,7 +144,7 @@ Issues of note: not need that functionality, use NO_CURL to build without it. - Git requires version "7.19.5" or later of "libcurl" to build + Git requires version "7.21.3" or later of "libcurl" to build without NO_CURL. This version requirement may be bumped in the future. From 5d312ec8a4981264b4ed3d3b77c43a0336385be2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2024 16:04:51 -0400 Subject: [PATCH 03/39] remote-curl: add Transfer-Encoding header only for older curl As of curl 7.66.0, we don't need to manually specify a "chunked" Transfer-Encoding header. Instead, modern curl deduces the need for it in a POST that has a POSTFIELDSIZE of -1 and uses READFUNCTION rather than POSTFIELDS. That version is recent enough that we can't just drop the header; we need to do so conditionally. Since it's only a single line, it seems like the simplest thing would just be to keep setting it unconditionally (after all, the #ifdefs are much longer than the actual code). But there's another wrinkle: HTTP/2. Curl may choose to use HTTP/2 under the hood if the server supports it. And in that protocol, we do not use the chunked encoding for streaming at all. Most versions of curl handle this just fine by recognizing and removing the header. But there's a regression in curl 8.7.0 and 8.7.1 where it doesn't, and large requests over HTTP/2 are broken (which t5559 notices). That regression has since been fixed upstream, but not yet released. Make the setting of this header conditional, which will let Git work even with those buggy curl versions. And as a bonus, it serves as a reminder that we can eventually clean up the code as we bump the supported curl versions. This is a backport of 92a209bf24 (remote-curl: add Transfer-Encoding header only for older curl, 2024-04-05) into the `maint-2.39` branch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- git-curl-compat.h | 9 +++++++++ remote-curl.c | 3 +++ 2 files changed, 12 insertions(+) diff --git a/git-curl-compat.h b/git-curl-compat.h index fd96b3cdff..e1d0bdd273 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h @@ -126,6 +126,15 @@ #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS #endif +/** + * Versions before curl 7.66.0 (September 2019) required manually setting the + * transfer-encoding for a streaming POST; after that this is handled + * automatically. + */ +#if LIBCURL_VERSION_NUM < 0x074200 +#define GIT_CURL_NEED_TRANSFER_ENCODING_HEADER +#endif + /** * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0, * released in August 2022. diff --git a/remote-curl.c b/remote-curl.c index 1b5d9ac1d8..d2e8d57c7e 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "git-curl-compat.h" #include "config.h" #include "remote.h" #include "connect.h" @@ -942,7 +943,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece /* The request body is large and the size cannot be predicted. * We must use chunked encoding to send it. */ +#ifdef GIT_CURL_NEED_TRANSFER_ENCODING_HEADER headers = curl_slist_append(headers, "Transfer-Encoding: chunked"); +#endif rpc->initial_buffer = 1; curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out); curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc); From 6741e917def23aff497ace35aa34debd1782330a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 12 Apr 2024 10:49:58 +0200 Subject: [PATCH 04/39] repository: avoid leaking `fsmonitor` data The `fsmonitor` repo-setting data is allocated lazily. It needs to be released in `repo_clear()` along the rest of the allocated data in `repository`. This is needed because the next commit will merge v2.39.4, which will add a `git checkout --recurse-modules` call (which would leak memory without this here fix) to an otherwise leak-free test script. Signed-off-by: Johannes Schindelin --- repository.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/repository.c b/repository.c index 937fa974b3..3b943d2cb4 100644 --- a/repository.c +++ b/repository.c @@ -271,6 +271,8 @@ void repo_clear(struct repository *repo) parsed_object_pool_clear(repo->parsed_objects); FREE_AND_NULL(repo->parsed_objects); + FREE_AND_NULL(repo->settings.fsmonitor); + if (repo->config) { git_configset_clear(repo->config); FREE_AND_NULL(repo->config); From 3167b60e5bd9fb8e2f9413b38001b624dc2e3da2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 3 Nov 2023 07:27:35 +0000 Subject: [PATCH 05/39] ci: upgrade to using macos-13 In April, GitHub announced that the `macos-13` pool is available: https://github.blog/changelog/2023-04-24-github-actions-macos-13-is-now-available/. It is only a matter of time until the `macos-12` pool is going away, therefore we should switch now, without pressure of a looming deadline. Since the `macos-13` runners no longer include Python2, we also drop specifically testing with Python2 and switch uniformly to Python3, see https://github.com/actions/runner-images/blob/HEAD/images/macos/macos-13-Readme.md for details about the software available on the `macos-13` pool's runners. Also, on macOS 13, Homebrew seems to install a `gcc@9` package that no longer comes with a regular `unistd.h` (there seems only to be a `ssp/unistd.h`), and hence builds would fail with: In file included from base85.c:1: git-compat-util.h:223:10: fatal error: unistd.h: No such file or directory 223 | #include | ^~~~~~~~~~ compilation terminated. The reason why we install GCC v9.x explicitly is historical, and back in the days it was because it was the _newest_ version available via Homebrew: 176441bfb58 (ci: build Git with GCC 9 in the 'osx-gcc' build job, 2019-11-27). To reinstate the spirit of that commit _and_ to fix that build failure, let's switch to the now-newest GCC version: v13.x. Backported-from: 682a868f67 (ci: upgrade to using macos-13, 2023-11-03) Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 6 +++--- ci/lib.sh | 6 ++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index f2fd6cf9cd..2e8b7cd768 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -262,11 +262,11 @@ jobs: pool: ubuntu-20.04 - jobname: osx-clang cc: clang - pool: macos-12 + pool: macos-13 - jobname: osx-gcc cc: gcc - cc_package: gcc-9 - pool: macos-12 + cc_package: gcc-13 + pool: macos-13 - jobname: linux-gcc-default cc: gcc pool: ubuntu-latest diff --git a/ci/lib.sh b/ci/lib.sh index 706e3ba7e9..611564f773 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -253,11 +253,9 @@ ubuntu-*) export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH" ;; macos-*) - if [ "$jobname" = osx-gcc ] + MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)" + if [ "$jobname" != osx-gcc ] then - MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)" - else - MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)" MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks" MAKEFLAGS="$MAKEFLAGS NO_OPENSSL=NoThanks" fi From c67cf4c434039f9b5c796f7e34345b75e0c14450 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 28 Aug 2023 14:37:35 -0400 Subject: [PATCH 06/39] test-lib: ignore uninteresting LSan output When I run the tests in leak-checking mode the same way our CI job does, like: make SANITIZE=leak \ GIT_TEST_PASSING_SANITIZE_LEAK=true \ GIT_TEST_SANITIZE_LEAK_LOG=true \ test then LSan can racily produce useless entries in the log files that look like this: ==git==3034393==Unable to get registers from thread 3034307. I think they're mostly harmless based on the source here: https://github.com/llvm/llvm-project/blob/7e0a52e8e9ef6394bb62e0b56e17fa23e7262411/compiler-rt/lib/lsan/lsan_common.cpp#L414 which reads: PtraceRegistersStatus have_registers = suspended_threads.GetRegistersAndSP(i, ®isters, &sp); if (have_registers != REGISTERS_AVAILABLE) { Report("Unable to get registers from thread %llu.\n", os_id); // If unable to get SP, consider the entire stack to be reachable unless // GetRegistersAndSP failed with ESRCH. if (have_registers == REGISTERS_UNAVAILABLE_FATAL) continue; sp = stack_begin; } The program itself still runs fine and LSan doesn't cause us to abort. But test-lib.sh looks for any non-empty LSan logs and marks the test as a failure anyway, under the assumption that we simply missed the failing exit code somehow. I don't think I've ever seen this happen in the CI job, but running locally using clang-14 on an 8-core machine, I can't seem to make it through a full run of the test suite without having at least one failure. And it's a different one every time (though they do seem to often be related to packing tests, which makes sense, since that is one of our biggest users of threaded code). We can hack around this by only counting LSan log files that contain a line that doesn't match our known-uninteresting pattern. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- t/test-lib.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/test-lib.sh b/t/test-lib.sh index 6db377f68b..251b22ba5a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -334,6 +334,7 @@ nr_san_dir_leaks_ () { find "$TEST_RESULTS_SAN_DIR" \ -type f \ -name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null | + xargs grep -lv "Unable to get registers from thread" | wc -l } From cf5dcd817ab22a3e98bb219b0ef3854c41a57843 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Mar 2024 16:28:18 +0100 Subject: [PATCH 07/39] ci(linux-asan/linux-ubsan): let's save some time Every once in a while, the `git-p4` tests flake for reasons outside of our control. It typically fails with "Connection refused" e.g. here: https://github.com/git/git/actions/runs/5969707156/job/16196057724 [...] + git p4 clone --dest=/home/runner/work/git/git/t/trash directory.t9807-git-p4-submit/git //depot Initialized empty Git repository in /home/runner/work/git/git/t/trash directory.t9807-git-p4-submit/git/.git/ Perforce client error: Connect to server failed; check $P4PORT. TCP connect to localhost:9807 failed. connect: 127.0.0.1:9807: Connection refused failure accessing depot: could not run p4 Importing from //depot into /home/runner/work/git/git/t/trash directory.t9807-git-p4-submit/git [...] This happens in other jobs, too, but in the `linux-asan`/`linux-ubsan` jobs it hurts the most because those jobs often take an _awfully_ long time to run, therefore re-running a failed `linux-asan`/`linux-ubsan` jobs is _very_ costly. The purpose of the `linux-asan`/`linux-ubsan` jobs is to exercise the C code of Git, anyway, and any part of Git's source code that the `git-p4` tests run and that would benefit from the attention of ASAN/UBSAN are run better in other tests anyway, as debugging C code run via Python scripts can get a bit hairy. In fact, it is not even just `git-p4` that is the problem (even if it flakes often enough to be problematic in the CI builds), but really the part about Python scripts. So let's just skip any Python parts of the tests from being run in that job. For good measure, also skip the Subversion tests because debugging C code run via Perl scripts is as much fun as debugging C code run via Python scripts. And it will reduce the time this very expensive job takes, which is a big benefit. Backported to `maint-2.39` as another step to get that branch's CI builds back to a healthy state. Backported-from: 6ba913629f (ci(linux-asan-ubsan): let's save some time, 2023-08-29) Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- ci/lib.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ci/lib.sh b/ci/lib.sh index 706e3ba7e9..052a90585c 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -281,9 +281,13 @@ linux-leaks) ;; linux-asan) export SANITIZE=address + export NO_SVN_TESTS=LetsSaveSomeTime + MAKEFLAGS="$MAKEFLAGS NO_PYTHON=YepBecauseP4FlakesTooOften" ;; linux-ubsan) export SANITIZE=undefined + export NO_SVN_TESTS=LetsSaveSomeTime + MAKEFLAGS="$MAKEFLAGS NO_PYTHON=YepBecauseP4FlakesTooOften" ;; esac From ce47f7c85f6a69b3c9a97f6b97dbed66069142c4 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 2 Feb 2024 12:39:34 -0800 Subject: [PATCH 08/39] GitHub Actions: update to checkout@v4 We seem to be getting "Node.js 16 actions are deprecated." warnings for jobs that use checkout@v3. Except for the i686 containers job that is kept at checkout@v1 [*], update to checkout@v4, which is said to use Node.js 20. [*] 6cf4d908 (ci(main): upgrade actions/checkout to v3, 2022-12-05) refers to https://github.com/actions/runner/issues/2115 and explains why container jobs are kept at checkout@v1. We may want to check the current status of the issue and move it to the same version as other jobs, but that is outside the scope of this step. Backported-from: e94dec0c1d (GitHub Actions: update to checkout@v4, 2024-02-02) Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- .github/workflows/check-whitespace.yml | 2 +- .github/workflows/main.yml | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml index a58e2dc8ad..a241a63428 100644 --- a/.github/workflows/check-whitespace.yml +++ b/.github/workflows/check-whitespace.yml @@ -19,7 +19,7 @@ jobs: check-whitespace: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0 diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2e8b7cd768..ea6000b40c 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -95,7 +95,7 @@ jobs: group: windows-build-${{ github.ref }} cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }} steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: build shell: bash @@ -156,10 +156,10 @@ jobs: group: vs-build-${{ github.ref }} cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }} steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: repository: 'microsoft/vcpkg' path: 'compat/vcbuild/vcpkg' @@ -286,7 +286,7 @@ jobs: runs_on_pool: ${{matrix.vector.pool}} runs-on: ${{matrix.vector.pool}} steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - run: ci/install-dependencies.sh - run: ci/run-build-and-tests.sh - run: ci/print-test-failures.sh @@ -319,7 +319,7 @@ jobs: runs-on: ubuntu-latest container: ${{matrix.vector.image}} steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 if: matrix.vector.jobname != 'linux32' - uses: actions/checkout@v1 if: matrix.vector.jobname == 'linux32' @@ -349,7 +349,7 @@ jobs: group: static-analysis-${{ github.ref }} cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }} steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - run: ci/install-dependencies.sh - run: ci/run-static-analysis.sh - run: ci/check-directional-formatting.bash @@ -372,7 +372,7 @@ jobs: artifact: sparse-20.04 - name: Install the current `sparse` package run: sudo dpkg -i sparse-20.04/sparse_*.deb - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Install other dependencies run: ci/install-dependencies.sh - run: make sparse @@ -387,6 +387,6 @@ jobs: jobname: Documentation runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - run: ci/install-dependencies.sh - run: ci/test-documentation.sh From 7e1bcc8d631c597d72b175aa649eb5e604e4dd68 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 11 Feb 2024 12:11:28 +0000 Subject: [PATCH 09/39] ci: bump remaining outdated Actions versions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After activating automatic Dependabot updates in the git-for-windows/git repository, Dependabot noticed a couple of yet-unaddressed updates. They avoid "Node.js 16 Actions" deprecation messages by bumping the following Actions' versions: - actions/upload-artifact from 3 to 4 - actions/download-artifact from 3 to 4 - actions/cache from 3 to 4 Backported-from: 820a340085 (ci: bump remaining outdated Actions versions, 2024-02-11) Helped-by: Matthias Aßhauer Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index aedb6b0ced..359cf19eb4 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -106,7 +106,7 @@ jobs: - name: zip up tracked files run: git archive -o artifacts/tracked.tar.gz HEAD - name: upload tracked files and build artifacts - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: windows-artifacts path: artifacts @@ -123,7 +123,7 @@ jobs: cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }} steps: - name: download tracked files and build artifacts - uses: actions/download-artifact@v3 + uses: actions/download-artifact@v4 with: name: windows-artifacts path: ${{github.workspace}} @@ -140,7 +140,7 @@ jobs: run: ci/print-test-failures.sh - name: Upload failed tests' directories if: failure() && env.FAILED_TEST_ARTIFACTS != '' - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: failed-tests-windows path: ${{env.FAILED_TEST_ARTIFACTS}} @@ -195,7 +195,7 @@ jobs: - name: zip up tracked files run: git archive -o artifacts/tracked.tar.gz HEAD - name: upload tracked files and build artifacts - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: vs-artifacts path: artifacts @@ -213,7 +213,7 @@ jobs: steps: - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download tracked files and build artifacts - uses: actions/download-artifact@v3 + uses: actions/download-artifact@v4 with: name: vs-artifacts path: ${{github.workspace}} @@ -231,7 +231,7 @@ jobs: run: ci/print-test-failures.sh - name: Upload failed tests' directories if: failure() && env.FAILED_TEST_ARTIFACTS != '' - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: failed-tests-windows path: ${{env.FAILED_TEST_ARTIFACTS}} @@ -293,7 +293,7 @@ jobs: if: failure() && env.FAILED_TEST_ARTIFACTS != '' - name: Upload failed tests' directories if: failure() && env.FAILED_TEST_ARTIFACTS != '' - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: failed-tests-${{matrix.vector.jobname}} path: ${{env.FAILED_TEST_ARTIFACTS}} @@ -329,7 +329,7 @@ jobs: if: failure() && env.FAILED_TEST_ARTIFACTS != '' - name: Upload failed tests' directories if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname != 'linux32' - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: failed-tests-${{matrix.vector.jobname}} path: ${{env.FAILED_TEST_ARTIFACTS}} From f6bed64ce2f645c773b1320cb2cd555600468ec7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 2 Feb 2024 12:39:35 -0800 Subject: [PATCH 10/39] GitHub Actions: update to github-script@v7 We seem to be getting "Node.js 16 actions are deprecated." warnings for jobs that use github-script@v6. Update to github-script@v7, which is said to use Node.js 20. Backported-from: c4ddbe043e (GitHub Actions: update to github-script@v7, 2024-02-02) Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ea6000b40c..aedb6b0ced 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -46,7 +46,7 @@ jobs: echo "skip_concurrent=$skip_concurrent" >>$GITHUB_OUTPUT - name: skip if the commit or tree was already tested id: skip-if-redundant - uses: actions/github-script@v6 + uses: actions/github-script@v7 if: steps.check-ref.outputs.enabled == 'yes' with: github-token: ${{secrets.GITHUB_TOKEN}} From 213958f2482a9985764e7deb7ee2bd18d8222e65 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 11 Feb 2024 12:11:29 +0000 Subject: [PATCH 11/39] ci(linux32): add a note about Actions that must not be updated The Docker container used by the `linux32` job comes without Node.js, and therefore the `actions/checkout` and `actions/upload-artifact` Actions cannot be upgraded to the latest versions (because they use Node.js). One time too many, I accidentally tried to update them, where `actions/checkout` at least fails immediately, but the `actions/upload-artifact` step is only used when any test fails, and therefore the CI run usually passes even though that Action was updated to a version that is incompatible with the Docker container in which this job runs. So let's add a big fat warning, mainly for my own benefit, to avoid running into the very same issue over and over again. Backported-from: 20e0ff8835 (ci(linux32): add a note about Actions that must not be updated, 2024-02-11) Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano Signed-off-by: Johannes Schindelin --- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 359cf19eb4..2dc0221f7f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -321,7 +321,7 @@ jobs: steps: - uses: actions/checkout@v4 if: matrix.vector.jobname != 'linux32' - - uses: actions/checkout@v1 + - uses: actions/checkout@v1 # cannot be upgraded because Node.js Actions aren't supported in this container if: matrix.vector.jobname == 'linux32' - run: ci/install-docker-dependencies.sh - run: ci/run-build-and-tests.sh @@ -335,7 +335,7 @@ jobs: path: ${{env.FAILED_TEST_ARTIFACTS}} - name: Upload failed tests' directories if: failure() && env.FAILED_TEST_ARTIFACTS != '' && matrix.vector.jobname == 'linux32' - uses: actions/upload-artifact@v1 + uses: actions/upload-artifact@v1 # cannot be upgraded because Node.js Actions aren't supported in this container with: name: failed-tests-${{matrix.vector.jobname}} path: ${{env.FAILED_TEST_ARTIFACTS}} From 150e6b0aedf57d224c3c49038c306477fa159886 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Apr 2024 13:30:26 +0200 Subject: [PATCH 12/39] builtin/clone: stop resolving symlinks when copying files When a user performs a local clone without `--no-local`, then we end up copying the source repository into the target repository directly. To optimize this even further, we try to hardlink files into place instead of copying data over, which helps both disk usage and speed. There is an important edge case in this context though, namely when we try to hardlink symlinks from the source repository into the target repository. Depending on both platform and filesystem the resulting behaviour here can be different: - On macOS and NetBSD, calling link(3P) with a symlink target creates a hardlink to the file pointed to by the symlink. - On Linux, calling link(3P) instead creates a hardlink to the symlink itself. To unify this behaviour, 36596fd2df (clone: better handle symlinked files at .git/objects/, 2019-07-10) introduced logic to resolve symlinks before we try to link(3P) files. Consequently, the new behaviour was to always create a hard link to the target of the symlink on all platforms. Eventually though, we figured out that following symlinks like this can cause havoc when performing a local clone of a malicious repository, which resulted in CVE-2022-39253. This issue was fixed via 6f054f9fb3 (builtin/clone.c: disallow `--local` clones with symlinks, 2022-07-28), by refusing symlinks in the source repository. But even though we now shouldn't ever link symlinks anymore, the code that resolves symlinks still exists. In the best case the code does not end up doing anything because there are no symlinks anymore. In the worst case though this can be abused by an adversary that rewrites the source file after it has been checked not to be a symlink such that it actually is a symlink when we call link(3P). Thus, it is still possible to recreate CVE-2022-39253 due to this time-of-check-time-of-use bug. Remove the call to `realpath()`. This doesn't yet address the actual vulnerability, which will be handled in a subsequent commit. Reported-by: Apple Product Security Signed-off-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin --- builtin/clone.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 3c2ae31a55..073e6323d7 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -320,7 +320,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, int src_len, dest_len; struct dir_iterator *iter; int iter_status; - struct strbuf realpath = STRBUF_INIT; mkdir_if_missing(dest->buf, 0777); @@ -358,8 +357,7 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, if (unlink(dest->buf) && errno != ENOENT) die_errno(_("failed to unlink '%s'"), dest->buf); if (!option_no_hardlinks) { - strbuf_realpath(&realpath, src->buf, 1); - if (!link(realpath.buf, dest->buf)) + if (!link(src->buf, dest->buf)) continue; if (option_local > 0) die_errno(_("failed to create link '%s'"), dest->buf); @@ -373,8 +371,6 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, strbuf_setlen(src, src_len); die(_("failed to iterate over '%s'"), src->buf); } - - strbuf_release(&realpath); } static void clone_local(const char *src_repo, const char *dest_repo) From d1bb66a546b4bb46005d17ba711caaad26f26c1e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Apr 2024 13:30:31 +0200 Subject: [PATCH 13/39] builtin/clone: abort when hardlinked source and target file differ When performing local clones with hardlinks we refuse to copy source files which are symlinks as a mitigation for CVE-2022-39253. This check can be raced by an adversary though by changing the file to a symlink after we have checked it. Fix the issue by checking whether the hardlinked destination file matches the source file and abort in case it doesn't. This addresses CVE-2024-32021. Reported-by: Apple Product Security Suggested-by: Linus Torvalds Signed-off-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin --- builtin/clone.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index 073e6323d7..4b80fa0870 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -357,8 +357,27 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, if (unlink(dest->buf) && errno != ENOENT) die_errno(_("failed to unlink '%s'"), dest->buf); if (!option_no_hardlinks) { - if (!link(src->buf, dest->buf)) + if (!link(src->buf, dest->buf)) { + struct stat st; + + /* + * Sanity-check whether the created hardlink + * actually links to the expected file now. This + * catches time-of-check-time-of-use bugs in + * case the source file was meanwhile swapped. + */ + if (lstat(dest->buf, &st)) + die(_("hardlink cannot be checked at '%s'"), dest->buf); + if (st.st_mode != iter->st.st_mode || + st.st_ino != iter->st.st_ino || + st.st_dev != iter->st.st_dev || + st.st_size != iter->st.st_size || + st.st_uid != iter->st.st_uid || + st.st_gid != iter->st.st_gid) + die(_("hardlink different from source at '%s'"), dest->buf); + continue; + } if (option_local > 0) die_errno(_("failed to create link '%s'"), dest->buf); option_no_hardlinks = 1; From 8c9c051bef3db0fe267f3fb6a1dab293c5f23b38 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Apr 2024 13:30:36 +0200 Subject: [PATCH 14/39] setup.c: introduce `die_upon_dubious_ownership()` Introduce a new function `die_upon_dubious_ownership()` that uses `ensure_valid_ownership()` to verify whether a repositroy is safe for use, and causes Git to die in case it is not. This function will be used in a subsequent commit. Helped-by: Johannes Schindelin Signed-off-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin --- cache.h | 12 ++++++++++++ setup.c | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/cache.h b/cache.h index fcf49706ad..a46a3e4b6b 100644 --- a/cache.h +++ b/cache.h @@ -606,6 +606,18 @@ void set_git_work_tree(const char *tree); #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" +/* + * Check if a repository is safe and die if it is not, by verifying the + * ownership of the worktree (if any), the git directory, and the gitfile (if + * any). + * + * Exemptions for known-safe repositories can be added via `safe.directory` + * config settings; for non-bare repositories, their worktree needs to be + * added, for bare ones their git directory. + */ +void die_upon_dubious_ownership(const char *gitfile, const char *worktree, + const char *gitdir); + void setup_work_tree(void); /* * Find the commondir and gitdir of the repository that contains the current diff --git a/setup.c b/setup.c index cefd5f63c4..9d401ae4c8 100644 --- a/setup.c +++ b/setup.c @@ -1165,6 +1165,27 @@ static int ensure_valid_ownership(const char *gitfile, return data.is_safe; } +void die_upon_dubious_ownership(const char *gitfile, const char *worktree, + const char *gitdir) +{ + struct strbuf report = STRBUF_INIT, quoted = STRBUF_INIT; + const char *path; + + if (ensure_valid_ownership(gitfile, worktree, gitdir, &report)) + return; + + strbuf_complete(&report, '\n'); + path = gitfile ? gitfile : gitdir; + sq_quote_buf_pretty("ed, path); + + die(_("detected dubious ownership in repository at '%s'\n" + "%s" + "To add an exception for this directory, call:\n" + "\n" + "\tgit config --global --add safe.directory %s"), + path, report.buf, quoted.buf); +} + static int allowed_bare_repo_cb(const char *key, const char *value, void *d) { enum allowed_bare_repo *allowed_bare_repo = d; From 1204e1a824c34071019fe106348eaa6d88f9528d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 15 Apr 2024 13:30:41 +0200 Subject: [PATCH 15/39] builtin/clone: refuse local clones of unsafe repositories When performing a local clone of a repository we end up either copying or hardlinking the source repository into the target repository. This is significantly more performant than if we were to use git-upload-pack(1) and git-fetch-pack(1) to create the new repository and preserves both disk space and compute time. Unfortunately though, performing such a local clone of a repository that is not owned by the current user is inherently unsafe: - It is possible that source files get swapped out underneath us while we are copying or hardlinking them. While we do perform some checks here to assert that we hardlinked the expected file, they cannot reliably thwart time-of-check-time-of-use (TOCTOU) style races. It is thus possible for an adversary to make us copy or hardlink unexpected files into the target directory. Ideally, we would address this by starting to use openat(3P), fstatat(3P) and friends. Due to platform compatibility with Windows we cannot easily do that though. Furthermore, the scope of these fixes would likely be quite broad and thus not fit for an embargoed security release. - Even if we handled TOCTOU-style races perfectly, hardlinking files owned by a different user into the target repository is not a good idea in general. It is possible for an adversary to rewrite those files to contain whatever data they want even after the clone has completed. Address these issues by completely refusing local clones of a repository that is not owned by the current user. This reuses our existing infra we have in place via `ensure_valid_ownership()` and thus allows a user to override the safety guard by adding the source repository path to the "safe.directory" configuration. This addresses CVE-2024-32020. Signed-off-by: Patrick Steinhardt Signed-off-by: Johannes Schindelin --- builtin/clone.c | 14 ++++++++++++++ t/t0033-safe-directory.sh | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index 4b80fa0870..9ec500d427 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -321,6 +321,20 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest, struct dir_iterator *iter; int iter_status; + /* + * Refuse copying directories by default which aren't owned by us. The + * code that performs either the copying or hardlinking is not prepared + * to handle various edge cases where an adversary may for example + * racily swap out files for symlinks. This can cause us to + * inadvertently use the wrong source file. + * + * Furthermore, even if we were prepared to handle such races safely, + * creating hardlinks across user boundaries is an inherently unsafe + * operation as the hardlinked files can be rewritten at will by the + * potentially-untrusted user. We thus refuse to do so by default. + */ + die_upon_dubious_ownership(NULL, NULL, src_repo); + mkdir_if_missing(dest->buf, 0777); iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC); diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh index dc3496897a..11c3e8f28e 100755 --- a/t/t0033-safe-directory.sh +++ b/t/t0033-safe-directory.sh @@ -80,4 +80,28 @@ test_expect_success 'safe.directory in included file' ' git status ' +test_expect_success 'local clone of unowned repo refused in unsafe directory' ' + test_when_finished "rm -rf source" && + git init source && + ( + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + test_commit -C source initial + ) && + test_must_fail git clone --local source target && + test_path_is_missing target +' + +test_expect_success 'local clone of unowned repo accepted in safe directory' ' + test_when_finished "rm -rf source" && + git init source && + ( + sane_unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + test_commit -C source initial + ) && + test_must_fail git clone --local source target && + git config --global --add safe.directory "$(pwd)/source/.git" && + git clone --local source target && + test_path_is_dir target +' + test_done From 5c5a4a1c05932378d259b1fdd9526cab971656a2 Mon Sep 17 00:00:00 2001 From: Filip Hejsek Date: Sun, 28 Jan 2024 04:29:33 +0100 Subject: [PATCH 16/39] t0411: add tests for cloning from partial repo Cloning from a partial repository must not fetch missing objects into the partial repository, because that can lead to arbitrary code execution. Add a couple of test cases, pretending to the `upload-pack` command (and to that command only) that it is working on a repository owned by someone else. Helped-by: Jeff King Signed-off-by: Filip Hejsek Signed-off-by: Johannes Schindelin --- t/t0411-clone-from-partial.sh | 60 +++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100755 t/t0411-clone-from-partial.sh diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh new file mode 100755 index 0000000000..fb72a0a9ff --- /dev/null +++ b/t/t0411-clone-from-partial.sh @@ -0,0 +1,60 @@ +#!/bin/sh + +test_description='check that local clone does not fetch from promisor remotes' + +. ./test-lib.sh + +test_expect_success 'create evil repo' ' + git init tmp && + test_commit -C tmp a && + git -C tmp config uploadpack.allowfilter 1 && + git clone --filter=blob:none --no-local --no-checkout tmp evil && + rm -rf tmp && + + git -C evil config remote.origin.uploadpack \"\$TRASH_DIRECTORY/fake-upload-pack\" && + write_script fake-upload-pack <<-\EOF && + echo >&2 "fake-upload-pack running" + >"$TRASH_DIRECTORY/script-executed" + exit 1 + EOF + export TRASH_DIRECTORY && + + # empty shallow file disables local clone optimization + >evil/.git/shallow +' + +test_expect_failure 'local clone must not fetch from promisor remote and execute script' ' + rm -f script-executed && + test_must_fail git clone \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ + evil clone1 2>err && + ! grep "fake-upload-pack running" err && + test_path_is_missing script-executed +' + +test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' ' + rm -f script-executed && + test_must_fail git clone \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ + "file://$(pwd)/evil" clone2 2>err && + ! grep "fake-upload-pack running" err && + test_path_is_missing script-executed +' + +test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' ' + rm -f script-executed && + test_must_fail git fetch \ + --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ + "file://$(pwd)/evil" 2>err && + ! grep "fake-upload-pack running" err && + test_path_is_missing script-executed +' + +test_expect_success 'pack-objects should fetch from promisor remote and execute script' ' + rm -f script-executed && + echo "HEAD" | test_must_fail git -C evil pack-objects --revs --stdout >/dev/null 2>err && + grep "fake-upload-pack running" err && + test_path_is_file script-executed +' + +test_done From f4aa8c8bb11dae6e769cd930565173808cbb69c8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 14:39:37 +0200 Subject: [PATCH 17/39] fetch/clone: detect dubious ownership of local repositories When cloning from somebody else's repositories, it is possible that, say, the `upload-pack` command is overridden in the repository that is about to be cloned, which would then be run in the user's context who started the clone. To remind the user that this is a potentially unsafe operation, let's extend the ownership checks we have already established for regular gitdir discovery to extend also to local repositories that are about to be cloned. This protection extends also to file:// URLs. The fixes in this commit address CVE-2024-32004. Note: This commit does not touch the `fetch`/`clone` code directly, but instead the function used implicitly by both: `enter_repo()`. This function is also used by `git receive-pack` (i.e. pushes), by `git upload-archive`, by `git daemon` and by `git http-backend`. In setups that want to serve repositories owned by different users than the account running the service, this will require `safe.*` settings to be configured accordingly. Also note: there are tiny time windows where a time-of-check-time-of-use ("TOCTOU") race is possible. The real solution to those would be to work with `fstat()` and `openat()`. However, the latter function is not available on Windows (and would have to be emulated with rather expensive low-level `NtCreateFile()` calls), and the changes would be quite extensive, for my taste too extensive for the little gain given that embargoed releases need to pay extra attention to avoid introducing inadvertent bugs. Signed-off-by: Johannes Schindelin --- cache.h | 12 ++++++++++++ path.c | 2 ++ setup.c | 21 +++++++++++++++++++++ t/t0411-clone-from-partial.sh | 6 +++--- 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index fcf49706ad..a46a3e4b6b 100644 --- a/cache.h +++ b/cache.h @@ -606,6 +606,18 @@ void set_git_work_tree(const char *tree); #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" +/* + * Check if a repository is safe and die if it is not, by verifying the + * ownership of the worktree (if any), the git directory, and the gitfile (if + * any). + * + * Exemptions for known-safe repositories can be added via `safe.directory` + * config settings; for non-bare repositories, their worktree needs to be + * added, for bare ones their git directory. + */ +void die_upon_dubious_ownership(const char *gitfile, const char *worktree, + const char *gitdir); + void setup_work_tree(void); /* * Find the commondir and gitdir of the repository that contains the current diff --git a/path.c b/path.c index 492e17ad12..d61f70e87d 100644 --- a/path.c +++ b/path.c @@ -840,6 +840,7 @@ const char *enter_repo(const char *path, int strict) if (!suffix[i]) return NULL; gitfile = read_gitfile(used_path.buf); + die_upon_dubious_ownership(gitfile, NULL, used_path.buf); if (gitfile) { strbuf_reset(&used_path); strbuf_addstr(&used_path, gitfile); @@ -850,6 +851,7 @@ const char *enter_repo(const char *path, int strict) } else { const char *gitfile = read_gitfile(path); + die_upon_dubious_ownership(gitfile, NULL, path); if (gitfile) path = gitfile; if (chdir(path)) diff --git a/setup.c b/setup.c index cefd5f63c4..9d401ae4c8 100644 --- a/setup.c +++ b/setup.c @@ -1165,6 +1165,27 @@ static int ensure_valid_ownership(const char *gitfile, return data.is_safe; } +void die_upon_dubious_ownership(const char *gitfile, const char *worktree, + const char *gitdir) +{ + struct strbuf report = STRBUF_INIT, quoted = STRBUF_INIT; + const char *path; + + if (ensure_valid_ownership(gitfile, worktree, gitdir, &report)) + return; + + strbuf_complete(&report, '\n'); + path = gitfile ? gitfile : gitdir; + sq_quote_buf_pretty("ed, path); + + die(_("detected dubious ownership in repository at '%s'\n" + "%s" + "To add an exception for this directory, call:\n" + "\n" + "\tgit config --global --add safe.directory %s"), + path, report.buf, quoted.buf); +} + static int allowed_bare_repo_cb(const char *key, const char *value, void *d) { enum allowed_bare_repo *allowed_bare_repo = d; diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh index fb72a0a9ff..eb3360dbca 100755 --- a/t/t0411-clone-from-partial.sh +++ b/t/t0411-clone-from-partial.sh @@ -23,7 +23,7 @@ test_expect_success 'create evil repo' ' >evil/.git/shallow ' -test_expect_failure 'local clone must not fetch from promisor remote and execute script' ' +test_expect_success 'local clone must not fetch from promisor remote and execute script' ' rm -f script-executed && test_must_fail git clone \ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ @@ -32,7 +32,7 @@ test_expect_failure 'local clone must not fetch from promisor remote and execute test_path_is_missing script-executed ' -test_expect_failure 'clone from file://... must not fetch from promisor remote and execute script' ' +test_expect_success 'clone from file://... must not fetch from promisor remote and execute script' ' rm -f script-executed && test_must_fail git clone \ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ @@ -41,7 +41,7 @@ test_expect_failure 'clone from file://... must not fetch from promisor remote a test_path_is_missing script-executed ' -test_expect_failure 'fetch from file://... must not fetch from promisor remote and execute script' ' +test_expect_success 'fetch from file://... must not fetch from promisor remote and execute script' ' rm -f script-executed && test_must_fail git fetch \ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ From 7b70e9efb18c2cc3f219af399bd384c5801ba1d7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Apr 2024 04:35:33 -0400 Subject: [PATCH 18/39] upload-pack: disable lazy-fetching by default The upload-pack command tries to avoid trusting the repository in which it's run (e.g., by not running any hooks and not using any config that contains arbitrary commands). But if the server side of a fetch or a clone is a partial clone, then either upload-pack or its child pack-objects may run a lazy "git fetch" under the hood. And it is very easy to convince fetch to run arbitrary commands. The "server" side can be a local repository owned by someone else, who would be able to configure commands that are run during a clone with the current user's permissions. This issue has been designated CVE-2024-32004. The fix in this commit's parent helps in this scenario, as well as in related scenarios using SSH to clone, where the untrusted .git directory is owned by a different user id. But if you received one as a zip file, on a USB stick, etc, it may be owned by your user but still untrusted. This has been designated CVE-2024-32465. To mitigate the issue more completely, let's disable lazy fetching entirely during `upload-pack`. While fetching from a partial repository should be relatively rare, it is certainly not an unreasonable workflow. And thus we need to provide an escape hatch. This commit works by respecting a GIT_NO_LAZY_FETCH environment variable (to skip the lazy-fetch), and setting it in upload-pack, but only when the user has not already done so (which gives us the escape hatch). The name of the variable is specifically chosen to match what has already been added in 'master' via e6d5479e7a (git: extend --no-lazy-fetch to work across subprocesses, 2024-02-27). Since we're building this fix as a backport for older versions, we could cherry-pick that patch and its earlier steps. However, we don't really need the niceties (like a "--no-lazy-fetch" option) that it offers. By using the same name, everything should just work when the two are eventually merged, but here are a few notes: - the blocking of the fetch in e6d5479e7a is incomplete! It sets fetch_if_missing to 0 when we setup the repository variable, but that isn't enough. pack-objects in particular will call prefetch_to_pack() even if that variable is 0. This patch by contrast checks the environment variable at the lowest level before we call the lazy fetch, where we can be sure to catch all code paths. Possibly the setting of fetch_if_missing from e6d5479e7a can be reverted, but it may be useful to have. For example, some code may want to use that flag to change behavior before it gets to the point of trying to start the fetch. At any rate, that's all outside the scope of this patch. - there's documentation for GIT_NO_LAZY_FETCH in e6d5479e7a. We can live without that here, because for the most part the user shouldn't need to set it themselves. The exception is if they do want to override upload-pack's default, and that requires a separate documentation section (which is added here) - it would be nice to use the NO_LAZY_FETCH_ENVIRONMENT macro added by e6d5479e7a, but those definitions have moved from cache.h to environment.h between 2.39.3 and master. I just used the raw string literals, and we can replace them with the macro once this topic is merged to master. At least with respect to CVE-2024-32004, this does render this commit's parent commit somewhat redundant. However, it is worth retaining that commit as defense in depth, and because it may help other issues (e.g., symlink/hardlink TOCTOU races, where zip files are not really an interesting attack vector). The tests in t0411 still pass, but now we have _two_ mechanisms ensuring that the evil command is not run. Let's beef up the existing ones to check that they failed for the expected reason, that we refused to run upload-pack at all with an alternate user id. And add two new ones for the same-user case that both the restriction and its escape hatch. Signed-off-by: Jeff King Signed-off-by: Johannes Schindelin --- Documentation/git-upload-pack.txt | 16 ++++++++++++++++ builtin/upload-pack.c | 2 ++ promisor-remote.c | 10 ++++++++++ t/t0411-clone-from-partial.sh | 18 ++++++++++++++++++ 4 files changed, 46 insertions(+) diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt index b656b47567..fc4c62d7bc 100644 --- a/Documentation/git-upload-pack.txt +++ b/Documentation/git-upload-pack.txt @@ -55,6 +55,22 @@ ENVIRONMENT admins may need to configure some transports to allow this variable to be passed. See the discussion in linkgit:git[1]. +`GIT_NO_LAZY_FETCH`:: + When cloning or fetching from a partial repository (i.e., one + itself cloned with `--filter`), the server-side `upload-pack` + may need to fetch extra objects from its upstream in order to + complete the request. By default, `upload-pack` will refuse to + perform such a lazy fetch, because `git fetch` may run arbitrary + commands specified in configuration and hooks of the source + repository (and `upload-pack` tries to be safe to run even in + untrusted `.git` directories). ++ +This is implemented by having `upload-pack` internally set the +`GIT_NO_LAZY_FETCH` variable to `1`. If you want to override it +(because you are fetching from a partial clone, and you are sure +you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to +`0`. + SEE ALSO -------- linkgit:gitnamespaces[7] diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c index 25b69da2bf..f446ff04f6 100644 --- a/builtin/upload-pack.c +++ b/builtin/upload-pack.c @@ -35,6 +35,8 @@ int cmd_upload_pack(int argc, const char **argv, const char *prefix) packet_trace_identity("upload-pack"); read_replace_refs = 0; + /* TODO: This should use NO_LAZY_FETCH_ENVIRONMENT */ + xsetenv("GIT_NO_LAZY_FETCH", "1", 0); argc = parse_options(argc, argv, prefix, options, upload_pack_usage, 0); diff --git a/promisor-remote.c b/promisor-remote.c index faa7612941..550a38f752 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -20,6 +20,16 @@ static int fetch_objects(struct repository *repo, int i; FILE *child_in; + /* TODO: This should use NO_LAZY_FETCH_ENVIRONMENT */ + if (git_env_bool("GIT_NO_LAZY_FETCH", 0)) { + static int warning_shown; + if (!warning_shown) { + warning_shown = 1; + warning(_("lazy fetching disabled; some objects may not be available")); + } + return -1; + } + child.git_cmd = 1; child.in = -1; if (repo != the_repository) diff --git a/t/t0411-clone-from-partial.sh b/t/t0411-clone-from-partial.sh index eb3360dbca..b3d6ddc4bc 100755 --- a/t/t0411-clone-from-partial.sh +++ b/t/t0411-clone-from-partial.sh @@ -28,6 +28,7 @@ test_expect_success 'local clone must not fetch from promisor remote and execute test_must_fail git clone \ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ evil clone1 2>err && + grep "detected dubious ownership" err && ! grep "fake-upload-pack running" err && test_path_is_missing script-executed ' @@ -37,6 +38,7 @@ test_expect_success 'clone from file://... must not fetch from promisor remote a test_must_fail git clone \ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ "file://$(pwd)/evil" clone2 2>err && + grep "detected dubious ownership" err && ! grep "fake-upload-pack running" err && test_path_is_missing script-executed ' @@ -46,6 +48,7 @@ test_expect_success 'fetch from file://... must not fetch from promisor remote a test_must_fail git fetch \ --upload-pack="GIT_TEST_ASSUME_DIFFERENT_OWNER=true git-upload-pack" \ "file://$(pwd)/evil" 2>err && + grep "detected dubious ownership" err && ! grep "fake-upload-pack running" err && test_path_is_missing script-executed ' @@ -57,4 +60,19 @@ test_expect_success 'pack-objects should fetch from promisor remote and execute test_path_is_file script-executed ' +test_expect_success 'clone from promisor remote does not lazy-fetch by default' ' + rm -f script-executed && + test_must_fail git clone evil no-lazy 2>err && + grep "lazy fetching disabled" err && + test_path_is_missing script-executed +' + +test_expect_success 'promisor lazy-fetching can be re-enabled' ' + rm -f script-executed && + test_must_fail env GIT_NO_LAZY_FETCH=0 \ + git clone evil lazy-ok 2>err && + grep "fake-upload-pack running" err && + test_path_is_file script-executed +' + test_done From e69ac42fcc866d3d6f84ea42bc656673440a07f5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Apr 2024 04:52:13 -0400 Subject: [PATCH 19/39] docs: document security issues around untrusted .git dirs For a long time our general philosophy has been that it's unsafe to run arbitrary Git commands if you don't trust the hooks or config in .git, but that running upload-pack should be OK. E.g., see 1456b043fc (Remove post-upload-hook, 2009-12-10), or the design of uploadpack.packObjectsHook. But we never really documented this (and even the discussions that led to 1456b043fc were not on the public list!). Let's try to make our approach more clear, but also be realistic that even upload-pack carries some risk. Helped-by: Filip Hejsek Helped-by: Junio C Hamano Signed-off-by: Jeff King Signed-off-by: Johannes Schindelin --- Documentation/git-upload-pack.txt | 15 +++++++++++++++ Documentation/git.txt | 31 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt index fc4c62d7bc..1d30a4f6b4 100644 --- a/Documentation/git-upload-pack.txt +++ b/Documentation/git-upload-pack.txt @@ -71,6 +71,21 @@ This is implemented by having `upload-pack` internally set the you trust it), you can explicitly set `GIT_NO_LAZY_FETCH` to `0`. +SECURITY +-------- + +Most Git commands should not be run in an untrusted `.git` directory +(see the section `SECURITY` in linkgit:git[1]). `upload-pack` tries to +avoid any dangerous configuration options or hooks from the repository +it's serving, making it safe to clone an untrusted directory and run +commands on the resulting clone. + +For an extra level of safety, you may be able to run `upload-pack` as an +alternate user. The details will be platform dependent, but on many +systems you can run: + + git clone --no-local --upload-pack='sudo -u nobody git-upload-pack' ... + SEE ALSO -------- linkgit:gitnamespaces[7] diff --git a/Documentation/git.txt b/Documentation/git.txt index 1d33e083ab..d2969461a4 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -1032,6 +1032,37 @@ The index is also capable of storing multiple entries (called "stages") for a given pathname. These stages are used to hold the various unmerged version of a file when a merge is in progress. +SECURITY +-------- + +Some configuration options and hook files may cause Git to run arbitrary +shell commands. Because configuration and hooks are not copied using +`git clone`, it is generally safe to clone remote repositories with +untrusted content, inspect them with `git log`, and so on. + +However, it is not safe to run Git commands in a `.git` directory (or +the working tree that surrounds it) when that `.git` directory itself +comes from an untrusted source. The commands in its config and hooks +are executed in the usual way. + +By default, Git will refuse to run when the repository is owned by +someone other than the user running the command. See the entry for +`safe.directory` in linkgit:git-config[1]. While this can help protect +you in a multi-user environment, note that you can also acquire +untrusted repositories that are owned by you (for example, if you +extract a zip file or tarball from an untrusted source). In such cases, +you'd need to "sanitize" the untrusted repository first. + +If you have an untrusted `.git` directory, you should first clone it +with `git clone --no-local` to obtain a clean copy. Git does restrict +the set of options and hooks that will be run by `upload-pack`, which +handles the server side of a clone or fetch, but beware that the +surface area for attack against `upload-pack` is large, so this does +carry some risk. The safest thing is to serve the repository as an +unprivileged user (either via linkgit:git-daemon[1], ssh, or using +other tools to change user ids). See the discussion in the `SECURITY` +section of linkgit:git-upload-pack[1]. + FURTHER DOCUMENTATION --------------------- From c30a574a0b50e64f26885f740dd49d2420b9bed7 Mon Sep 17 00:00:00 2001 From: Filip Hejsek Date: Sun, 28 Jan 2024 04:30:25 +0100 Subject: [PATCH 20/39] has_dir_name(): do not get confused by characters < '/' There is a bug in directory/file ("D/F") conflict checking optimization: It assumes that such a conflict cannot happen if a newly added entry's path is lexicgraphically "greater than" the last already-existing index entry _and_ contains a directory separator that comes strictly after the common prefix (`len > len_eq_offset`). This assumption is incorrect, though: `a-` sorts _between_ `a` and `a/b`, their common prefix is `a`, the slash comes after the common prefix, and there is still a file/directory conflict. Let's re-design this logic, taking these facts into consideration: - It is impossible for a file to sort after another file with whose directory it conflicts because the trailing NUL byte is always smaller than any other character. - Since there are quite a number of ASCII characters that sort before the slash (e.g. `-`, `.`, the space character), looking at the last already-existing index entry is not enough to determine whether there is a D/F conflict when the first character different from the existing last index entry's path is a slash. If it is not a slash, there cannot be a file/directory conflict. And if the existing index entry's first different character is a slash, it also cannot be a file/directory conflict because the optimization requires the newly-added entry's path to sort _after_ the existing entry's, and the conflicting file's path would not. So let's fall back to the regular binary search whenever the newly-added item's path differs in a slash character. If it does not, and it sorts after the last index entry, there is no D/F conflict and the new index entry can be safely appended. This fix also nicely simplifies the logic and makes it much easier to reason about, while the impact on performance should be negligible: After this fix, the optimization will be skipped only when index entry's paths differ in a slash and a space, `!`, `"`, `#`, `$`, `%`, `&`, `'`, | ( `)`, `*`, `+`, `,`, `-`, or `.`, which should be a rare situation. Signed-off-by: Filip Hejsek Signed-off-by: Johannes Schindelin --- read-cache.c | 72 +++++++++++++----------------------------------- t/t0000-basic.sh | 28 +++++++++++++++++++ 2 files changed, 47 insertions(+), 53 deletions(-) diff --git a/read-cache.c b/read-cache.c index 46f5e497b1..383ec6d366 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1186,19 +1186,32 @@ static int has_dir_name(struct index_state *istate, istate->cache[istate->cache_nr - 1]->name, &len_eq_last); if (cmp_last > 0) { - if (len_eq_last == 0) { + if (name[len_eq_last] != '/') { /* * The entry sorts AFTER the last one in the - * index and their paths have no common prefix, - * so there cannot be a F/D conflict. + * index. + * + * If there were a conflict with "file", then our + * name would start with "file/" and the last index + * entry would start with "file" but not "file/". + * + * The next character after common prefix is + * not '/', so there can be no conflict. */ return retval; } else { /* * The entry sorts AFTER the last one in the - * index, but has a common prefix. Fall through - * to the loop below to disect the entry's path - * and see where the difference is. + * index, and the next character after common + * prefix is '/'. + * + * Either the last index entry is a file in + * conflict with this entry, or it has a name + * which sorts between this entry and the + * potential conflicting file. + * + * In both cases, we fall through to the loop + * below and let the regular search code handle it. */ } } else if (cmp_last == 0) { @@ -1222,53 +1235,6 @@ static int has_dir_name(struct index_state *istate, } len = slash - name; - if (cmp_last > 0) { - /* - * (len + 1) is a directory boundary (including - * the trailing slash). And since the loop is - * decrementing "slash", the first iteration is - * the longest directory prefix; subsequent - * iterations consider parent directories. - */ - - if (len + 1 <= len_eq_last) { - /* - * The directory prefix (including the trailing - * slash) also appears as a prefix in the last - * entry, so the remainder cannot collide (because - * strcmp said the whole path was greater). - * - * EQ: last: xxx/A - * this: xxx/B - * - * LT: last: xxx/file_A - * this: xxx/file_B - */ - return retval; - } - - if (len > len_eq_last) { - /* - * This part of the directory prefix (excluding - * the trailing slash) is longer than the known - * equal portions, so this sub-directory cannot - * collide with a file. - * - * GT: last: xxxA - * this: xxxB/file - */ - return retval; - } - - /* - * This is a possible collision. Fall through and - * let the regular search code handle it. - * - * last: xxx - * this: xxx/file - */ - } - pos = index_name_stage_pos(istate, name, len, stage, EXPAND_SPARSE); if (pos >= 0) { /* diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 502b4bcf9e..2ba219b18b 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -1200,6 +1200,34 @@ test_expect_success 'very long name in the index handled sanely' ' test $len = 4098 ' +# D/F conflict checking uses an optimization when adding to the end. +# make sure it does not get confused by `a-` sorting _between_ +# `a` and `a/`. +test_expect_success 'more update-index D/F conflicts' ' + # empty the index to make sure our entry is last + git read-tree --empty && + cacheinfo=100644,$(test_oid empty_blob) && + git update-index --add --cacheinfo $cacheinfo,path5/a && + + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/file && + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/file && + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/c/file && + + # "a-" sorts between "a" and "a/" + git update-index --add --cacheinfo $cacheinfo,path5/a- && + + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/file && + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/file && + test_must_fail git update-index --add --cacheinfo $cacheinfo,path5/a/b/c/file && + + cat >expected <<-\EOF && + path5/a + path5/a- + EOF + git ls-files >actual && + test_cmp expected actual +' + test_expect_success 'test_must_fail on a failing git command' ' test_must_fail git notacommand ' From b20c10fd9b035f46e48112d2cd33d7cb740012b6 Mon Sep 17 00:00:00 2001 From: Filip Hejsek Date: Sun, 28 Jan 2024 04:32:47 +0100 Subject: [PATCH 21/39] t7423: add tests for symlinked submodule directories Submodule operations must not follow symlinks in working tree, because otherwise files might be written to unintended places, leading to vulnerabilities. Signed-off-by: Filip Hejsek Signed-off-by: Johannes Schindelin --- t/t7423-submodule-symlinks.sh | 66 +++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100755 t/t7423-submodule-symlinks.sh diff --git a/t/t7423-submodule-symlinks.sh b/t/t7423-submodule-symlinks.sh new file mode 100755 index 0000000000..a72f3cbcab --- /dev/null +++ b/t/t7423-submodule-symlinks.sh @@ -0,0 +1,66 @@ +#!/bin/sh + +test_description='check that submodule operations do not follow symlinks' + +. ./test-lib.sh + +test_expect_success 'prepare' ' + git config --global protocol.file.allow always && + test_commit initial && + git init upstream && + test_commit -C upstream upstream submodule_file && + git submodule add ./upstream a/sm && + test_tick && + git commit -m submodule +' + +test_expect_failure SYMLINKS 'git submodule update must not create submodule behind symlink' ' + rm -rf a b && + mkdir b && + ln -s b a && + test_must_fail git submodule update && + test_path_is_missing b/sm +' + +test_expect_failure SYMLINKS,CASE_INSENSITIVE_FS 'git submodule update must not create submodule behind symlink on case insensitive fs' ' + rm -rf a b && + mkdir b && + ln -s b A && + test_must_fail git submodule update && + test_path_is_missing b/sm +' + +prepare_symlink_to_repo() { + rm -rf a && + mkdir a && + git init a/target && + git -C a/target fetch ../../upstream && + ln -s target a/sm +} + +test_expect_success SYMLINKS 'git restore --recurse-submodules must not be confused by a symlink' ' + prepare_symlink_to_repo && + test_must_fail git restore --recurse-submodules a/sm && + test_path_is_missing a/sm/submodule_file && + test_path_is_dir a/target/.git && + test_path_is_missing a/target/submodule_file +' + +test_expect_failure SYMLINKS 'git restore --recurse-submodules must not migrate git dir of symlinked repo' ' + prepare_symlink_to_repo && + rm -rf .git/modules && + test_must_fail git restore --recurse-submodules a/sm && + test_path_is_dir a/target/.git && + test_path_is_missing .git/modules/a/sm && + test_path_is_missing a/target/submodule_file +' + +test_expect_failure SYMLINKS 'git checkout -f --recurse-submodules must not migrate git dir of symlinked repo when removing submodule' ' + prepare_symlink_to_repo && + rm -rf .git/modules && + test_must_fail git checkout -f --recurse-submodules initial && + test_path_is_dir a/target/.git && + test_path_is_missing .git/modules/a/sm +' + +test_done From 9cf85473209ea8ae2b56c13145c4704d12ee1374 Mon Sep 17 00:00:00 2001 From: Filip Hejsek Date: Sun, 28 Jan 2024 05:09:17 +0100 Subject: [PATCH 22/39] clone: prevent clashing git dirs when cloning submodule in parallel While it is expected to have several git dirs within the `.git/modules/` tree, it is important that they do not interfere with each other. For example, if one submodule was called "captain" and another submodule "captain/hooks", their respective git dirs would clash, as they would be located in `.git/modules/captain/` and `.git/modules/captain/hooks/`, respectively, i.e. the latter's files could clash with the actual Git hooks of the former. To prevent these clashes, and in particular to prevent hooks from being written and then executed as part of a recursive clone, we introduced checks as part of the fix for CVE-2019-1387 in a8dee3ca61 (Disallow dubiously-nested submodule git directories, 2019-10-01). It is currently possible to bypass the check for clashing submodule git dirs in two ways: 1. parallel cloning 2. checkout --recurse-submodules Let's check not only before, but also after parallel cloning (and before checking out the submodule), that the git dir is not clashing with another one, otherwise fail. This addresses the parallel cloning issue. As to the parallel checkout issue: It requires quite a few manual steps to create clashing git dirs because Git itself would refuse to initialize the inner one, as demonstrated by the test case. Nevertheless, let's teach the recursive checkout (namely, the `submodule_move_head()` function that is used by the recursive checkout) to be careful to verify that it does not use a clashing git dir, and if it does, disable it (by deleting the `HEAD` file so that subsequent Git calls won't recognize it as a git dir anymore). Note: The parallel cloning test case contains a `cat err` that proved to be highly useful when analyzing the racy nature of the operation (the operation can fail with three different error messages, depending on timing), and was left on purpose to ease future debugging should the need arise. Signed-off-by: Filip Hejsek Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 17 +++++++++++++++++ submodule.c | 17 +++++++++++++++++ t/t7450-bad-git-dotfiles.sh | 34 ++++++++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6743fb27bd..b76e13ddce 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1717,6 +1717,23 @@ static int clone_submodule(const struct module_clone_data *clone_data, free(path); } + /* + * We already performed this check at the beginning of this function, + * before cloning the objects. This tries to detect racy behavior e.g. + * in parallel clones, where another process could easily have made the + * gitdir nested _after_ it was created. + * + * To prevent further harm coming from this unintentionally-nested + * gitdir, let's disable it by deleting the `HEAD` file. + */ + if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0) { + char *head = xstrfmt("%s/HEAD", sm_gitdir); + unlink(head); + free(head); + die(_("refusing to create/use '%s' in another submodule's " + "git dir"), sm_gitdir); + } + connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0); p = git_pathdup_submodule(clone_data_path, "config"); diff --git a/submodule.c b/submodule.c index fae24ef34a..71ec23ad98 100644 --- a/submodule.c +++ b/submodule.c @@ -2146,10 +2146,27 @@ int submodule_move_head(const char *path, if (old_head) { if (!submodule_uses_gitfile(path)) absorb_git_dir_into_superproject(path); + else { + char *dotgit = xstrfmt("%s/.git", path); + char *git_dir = xstrdup(read_gitfile(dotgit)); + + free(dotgit); + if (validate_submodule_git_dir(git_dir, + sub->name) < 0) + die(_("refusing to create/use '%s' in " + "another submodule's git dir"), + git_dir); + free(git_dir); + } } else { struct strbuf gitdir = STRBUF_INIT; submodule_name_to_gitdir(&gitdir, the_repository, sub->name); + if (validate_submodule_git_dir(gitdir.buf, + sub->name) < 0) + die(_("refusing to create/use '%s' in another " + "submodule's git dir"), + gitdir.buf); connect_work_tree_and_git_dir(path, gitdir.buf, 0); strbuf_release(&gitdir); diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh index ba1f569bcb..8f94129e74 100755 --- a/t/t7450-bad-git-dotfiles.sh +++ b/t/t7450-bad-git-dotfiles.sh @@ -292,7 +292,7 @@ test_expect_success WINDOWS 'prevent git~1 squatting on Windows' ' fi ' -test_expect_success 'git dirs of sibling submodules must not be nested' ' +test_expect_success 'setup submodules with nested git dirs' ' git init nested && test_commit -C nested nested && ( @@ -310,9 +310,39 @@ test_expect_success 'git dirs of sibling submodules must not be nested' ' git add .gitmodules thing1 thing2 && test_tick && git commit -m nested - ) && + ) +' + +test_expect_success 'git dirs of sibling submodules must not be nested' ' test_must_fail git clone --recurse-submodules nested clone 2>err && test_i18ngrep "is inside git dir" err ' +test_expect_success 'submodule git dir nesting detection must work with parallel cloning' ' + test_must_fail git clone --recurse-submodules --jobs=2 nested clone_parallel 2>err && + cat err && + grep -E "(already exists|is inside git dir|not a git repository)" err && + { + test_path_is_missing .git/modules/hippo/HEAD || + test_path_is_missing .git/modules/hippo/hooks/HEAD + } +' + +test_expect_success 'checkout -f --recurse-submodules must not use a nested gitdir' ' + git clone nested nested_checkout && + ( + cd nested_checkout && + git submodule init && + git submodule update thing1 && + mkdir -p .git/modules/hippo/hooks/refs && + mkdir -p .git/modules/hippo/hooks/objects/info && + echo "../../../../objects" >.git/modules/hippo/hooks/objects/info/alternates && + echo "ref: refs/heads/master" >.git/modules/hippo/hooks/HEAD + ) && + test_must_fail git -C nested_checkout checkout -f --recurse-submodules HEAD 2>err && + cat err && + grep "is inside git dir" err && + test_path_is_missing nested_checkout/thing2/.git +' + test_done From 97065761333fd62db1912d81b489db938d8c991d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 22 Mar 2024 11:19:22 +0100 Subject: [PATCH 23/39] submodules: submodule paths must not contain symlinks When creating a submodule path, we must be careful not to follow symbolic links. Otherwise we may follow a symbolic link pointing to a gitdir (which are valid symbolic links!) e.g. while cloning. On case-insensitive filesystems, however, we blindly replace a directory that has been created as part of the `clone` operation with a symlink when the path to the latter differs only in case from the former's path. Let's simply avoid this situation by expecting not ever having to overwrite any existing file/directory/symlink upon cloning. That way, we won't even replace a directory that we just created. This addresses CVE-2024-32002. Reported-by: Filip Hejsek Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 35 +++++++++++++++++++++++++++ t/t7406-submodule-update.sh | 48 +++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index b76e13ddce..4c1a7dbcda 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1641,12 +1641,35 @@ static char *clone_submodule_sm_gitdir(const char *name) return sm_gitdir; } +static int dir_contains_only_dotgit(const char *path) +{ + DIR *dir = opendir(path); + struct dirent *e; + int ret = 1; + + if (!dir) + return 0; + + e = readdir_skip_dot_and_dotdot(dir); + if (!e) + ret = 0; + else if (strcmp(DEFAULT_GIT_DIR_ENVIRONMENT, e->d_name) || + (e = readdir_skip_dot_and_dotdot(dir))) { + error("unexpected item '%s' in '%s'", e->d_name, path); + ret = 0; + } + + closedir(dir); + return ret; +} + static int clone_submodule(const struct module_clone_data *clone_data, struct string_list *reference) { char *p; char *sm_gitdir = clone_submodule_sm_gitdir(clone_data->name); char *sm_alternate = NULL, *error_strategy = NULL; + struct stat st; struct child_process cp = CHILD_PROCESS_INIT; const char *clone_data_path = clone_data->path; char *to_free = NULL; @@ -1660,6 +1683,10 @@ static int clone_submodule(const struct module_clone_data *clone_data, "git dir"), sm_gitdir); if (!file_exists(sm_gitdir)) { + if (clone_data->require_init && !stat(clone_data_path, &st) && + !is_empty_dir(clone_data_path)) + die(_("directory not empty: '%s'"), clone_data_path); + if (safe_create_leading_directories_const(sm_gitdir) < 0) die(_("could not create directory '%s'"), sm_gitdir); @@ -1704,6 +1731,14 @@ static int clone_submodule(const struct module_clone_data *clone_data, if(run_command(&cp)) die(_("clone of '%s' into submodule path '%s' failed"), clone_data->url, clone_data_path); + + if (clone_data->require_init && !stat(clone_data_path, &st) && + !dir_contains_only_dotgit(clone_data_path)) { + char *dot_git = xstrfmt("%s/.git", clone_data_path); + unlink(dot_git); + free(dot_git); + die(_("directory not empty: '%s'"), clone_data_path); + } } else { char *path; diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index f094e3d7f3..63c24f7f7c 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -1179,4 +1179,52 @@ test_expect_success 'submodule update --recursive skip submodules with strategy= test_cmp expect.err actual.err ' +test_expect_success CASE_INSENSITIVE_FS,SYMLINKS \ + 'submodule paths must not follow symlinks' ' + + # This is only needed because we want to run this in a self-contained + # test without having to spin up an HTTP server; However, it would not + # be needed in a real-world scenario where the submodule is simply + # hosted on a public site. + test_config_global protocol.file.allow always && + + # Make sure that Git tries to use symlinks on Windows + test_config_global core.symlinks true && + + tell_tale_path="$PWD/tell.tale" && + git init hook && + ( + cd hook && + mkdir -p y/hooks && + write_script y/hooks/post-checkout <<-EOF && + echo HOOK-RUN >&2 + echo hook-run >"$tell_tale_path" + EOF + git add y/hooks/post-checkout && + test_tick && + git commit -m post-checkout + ) && + + hook_repo_path="$(pwd)/hook" && + git init captain && + ( + cd captain && + git submodule add --name x/y "$hook_repo_path" A/modules/x && + test_tick && + git commit -m add-submodule && + + printf .git >dotgit.txt && + git hash-object -w --stdin dot-git.hash && + printf "120000 %s 0\ta\n" "$(cat dot-git.hash)" >index.info && + git update-index --index-info err && + grep "directory not empty" err && + test_path_is_missing "$tell_tale_path" +' + test_done From eafffd9ad417bdf0a3c63e5276d5a18f563cd291 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 12 Apr 2024 21:00:44 +0200 Subject: [PATCH 24/39] clone_submodule: avoid using `access()` on directories In 0060fd1511b (clone --recurse-submodules: prevent name squatting on Windows, 2019-09-12), I introduced code to verify that a git dir either does not exist, or is at least empty, to fend off attacks where an inadvertently (and likely maliciously) pre-populated git dir would be used while cloning submodules recursively. The logic used `access(, X_OK)` to verify that a directory exists before calling `is_empty_dir()` on it. That is a curious way to check for a directory's existence and might well fail for unwanted reasons. Even the original author (it was I ;-) ) struggles to explain why this function was used rather than `stat()`. This code was _almost_ copypastad in the previous commit, but that `access()` call was caught during review. Let's use `stat()` instead also in the code that was almost copied verbatim. Let's not use `lstat()` because in the unlikely event that somebody snuck a symbolic link in, pointing to a crafted directory, we want to verify that that directory is empty. Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 4c1a7dbcda..9eacc43574 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1742,7 +1742,7 @@ static int clone_submodule(const struct module_clone_data *clone_data, } else { char *path; - if (clone_data->require_init && !access(clone_data_path, X_OK) && + if (clone_data->require_init && !stat(clone_data_path, &st) && !is_empty_dir(clone_data_path)) die(_("directory not empty: '%s'"), clone_data_path); if (safe_create_leading_directories_const(clone_data_path) < 0) From e8d0608944486019ea0e1ed2ed29776811a565c2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Mar 2024 14:37:25 +0100 Subject: [PATCH 25/39] submodule: require the submodule path to contain directories only Submodules are stored in subdirectories of their superproject. When these subdirectories have been replaced with symlinks by a malicious actor, all kinds of mayhem can be caused. This _should_ not be possible, but many CVEs in the past showed that _when_ possible, it allows attackers to slip in code that gets executed during, say, a `git clone --recursive` operation. Let's add some defense-in-depth to disallow submodule paths to have anything except directories in them. Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 32 +++++++++++++++- submodule.c | 72 +++++++++++++++++++++++++++++++++++ submodule.h | 5 +++ t/t7423-submodule-symlinks.sh | 9 +++-- 4 files changed, 113 insertions(+), 5 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 9eacc43574..941afe1568 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -294,6 +294,9 @@ static void runcommand_in_submodule_cb(const struct cache_entry *list_item, struct child_process cp = CHILD_PROCESS_INIT; char *displaypath; + if (validate_submodule_path(path) < 0) + exit(128); + displaypath = get_submodule_displaypath(path, info->prefix); sub = submodule_from_path(the_repository, null_oid(), path); @@ -620,6 +623,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, .free_removed_argv_elements = 1, }; + if (validate_submodule_path(path) < 0) + exit(128); + if (!submodule_from_path(the_repository, null_oid(), path)) die(_("no submodule mapping found in .gitmodules for path '%s'"), path); @@ -1220,6 +1226,9 @@ static void sync_submodule(const char *path, const char *prefix, if (!is_submodule_active(the_repository, path)) return; + if (validate_submodule_path(path) < 0) + exit(128); + sub = submodule_from_path(the_repository, null_oid(), path); if (sub && sub->url) { @@ -1360,6 +1369,9 @@ static void deinit_submodule(const char *path, const char *prefix, struct strbuf sb_config = STRBUF_INIT; char *sub_git_dir = xstrfmt("%s/.git", path); + if (validate_submodule_path(path) < 0) + exit(128); + sub = submodule_from_path(the_repository, null_oid(), path); if (!sub || !sub->name) @@ -1674,6 +1686,9 @@ static int clone_submodule(const struct module_clone_data *clone_data, const char *clone_data_path = clone_data->path; char *to_free = NULL; + if (validate_submodule_path(clone_data_path) < 0) + exit(128); + if (!is_absolute_path(clone_data->path)) clone_data_path = to_free = xstrfmt("%s/%s", get_git_work_tree(), clone_data->path); @@ -2542,6 +2557,9 @@ static int update_submodule(struct update_data *update_data) { int ret; + if (validate_submodule_path(update_data->sm_path) < 0) + return -1; + ret = determine_submodule_update_strategy(the_repository, update_data->just_cloned, update_data->sm_path, @@ -2649,12 +2667,21 @@ static int update_submodules(struct update_data *update_data) for (i = 0; i < suc.update_clone_nr; i++) { struct update_clone_data ucd = suc.update_clone[i]; - int code; + int code = 128; oidcpy(&update_data->oid, &ucd.oid); update_data->just_cloned = ucd.just_cloned; update_data->sm_path = ucd.sub->path; + /* + * Verify that the submodule path does not contain any + * symlinks; if it does, it might have been tampered with. + * TODO: allow exempting it via + * `safe.submodule.path` or something + */ + if (validate_submodule_path(update_data->sm_path) < 0) + goto fail; + code = ensure_core_worktree(update_data->sm_path); if (code) goto fail; @@ -3361,6 +3388,9 @@ static int module_add(int argc, const char **argv, const char *prefix) normalize_path_copy(add_data.sm_path, add_data.sm_path); strip_dir_trailing_slashes(add_data.sm_path); + if (validate_submodule_path(add_data.sm_path) < 0) + exit(128); + die_on_index_match(add_data.sm_path, force); die_on_repo_without_commits(add_data.sm_path); diff --git a/submodule.c b/submodule.c index 71ec23ad98..0b87ae6340 100644 --- a/submodule.c +++ b/submodule.c @@ -1005,6 +1005,9 @@ static int submodule_has_commits(struct repository *r, .super_oid = super_oid }; + if (validate_submodule_path(path) < 0) + exit(128); + oid_array_for_each_unique(commits, check_has_commit, &has_commit); if (has_commit.result) { @@ -1127,6 +1130,9 @@ static int push_submodule(const char *path, const struct string_list *push_options, int dry_run) { + if (validate_submodule_path(path) < 0) + exit(128); + if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) { struct child_process cp = CHILD_PROCESS_INIT; strvec_push(&cp.args, "push"); @@ -1176,6 +1182,9 @@ static void submodule_push_check(const char *path, const char *head, struct child_process cp = CHILD_PROCESS_INIT; int i; + if (validate_submodule_path(path) < 0) + exit(128); + strvec_push(&cp.args, "submodule--helper"); strvec_push(&cp.args, "push-check"); strvec_push(&cp.args, head); @@ -1507,6 +1516,9 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf struct fetch_task *task = xmalloc(sizeof(*task)); memset(task, 0, sizeof(*task)); + if (validate_submodule_path(path) < 0) + exit(128); + task->sub = submodule_from_path(spf->r, treeish_name, path); if (!task->sub) { @@ -1879,6 +1891,9 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked) const char *git_dir; int ignore_cp_exit_code = 0; + if (validate_submodule_path(path) < 0) + exit(128); + strbuf_addf(&buf, "%s/.git", path); git_dir = read_gitfile(buf.buf); if (!git_dir) @@ -1955,6 +1970,9 @@ int submodule_uses_gitfile(const char *path) struct strbuf buf = STRBUF_INIT; const char *git_dir; + if (validate_submodule_path(path) < 0) + exit(128); + strbuf_addf(&buf, "%s/.git", path); git_dir = read_gitfile(buf.buf); if (!git_dir) { @@ -1994,6 +2012,9 @@ int bad_to_remove_submodule(const char *path, unsigned flags) struct strbuf buf = STRBUF_INIT; int ret = 0; + if (validate_submodule_path(path) < 0) + exit(128); + if (!file_exists(path) || is_empty_dir(path)) return 0; @@ -2044,6 +2065,9 @@ void submodule_unset_core_worktree(const struct submodule *sub) { struct strbuf config_path = STRBUF_INIT; + if (validate_submodule_path(sub->path) < 0) + exit(128); + submodule_name_to_gitdir(&config_path, the_repository, sub->name); strbuf_addstr(&config_path, "/config"); @@ -2066,6 +2090,9 @@ static int submodule_has_dirty_index(const struct submodule *sub) { struct child_process cp = CHILD_PROCESS_INIT; + if (validate_submodule_path(sub->path) < 0) + exit(128); + prepare_submodule_repo_env(&cp.env); cp.git_cmd = 1; @@ -2083,6 +2110,10 @@ static int submodule_has_dirty_index(const struct submodule *sub) static void submodule_reset_index(const char *path) { struct child_process cp = CHILD_PROCESS_INIT; + + if (validate_submodule_path(path) < 0) + exit(128); + prepare_submodule_repo_env(&cp.env); cp.git_cmd = 1; @@ -2287,6 +2318,34 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name) return 0; } +int validate_submodule_path(const char *path) +{ + char *p = xstrdup(path); + struct stat st; + int i, ret = 0; + char sep; + + for (i = 0; !ret && p[i]; i++) { + if (!is_dir_sep(p[i])) + continue; + + sep = p[i]; + p[i] = '\0'; + /* allow missing components, but no symlinks */ + ret = lstat(p, &st) || !S_ISLNK(st.st_mode) ? 0 : -1; + p[i] = sep; + if (ret) + error(_("expected '%.*s' in submodule path '%s' not to " + "be a symbolic link"), i, p, p); + } + if (!lstat(p, &st) && S_ISLNK(st.st_mode)) + ret = error(_("expected submodule path '%s' not to be a " + "symbolic link"), p); + free(p); + return ret; +} + + /* * Embeds a single submodules git directory into the superprojects git dir, * non recursively. @@ -2297,6 +2356,9 @@ static void relocate_single_git_dir_into_superproject(const char *path) struct strbuf new_gitdir = STRBUF_INIT; const struct submodule *sub; + if (validate_submodule_path(path) < 0) + exit(128); + if (submodule_uses_worktrees(path)) die(_("relocate_gitdir for submodule '%s' with " "more than one worktree not supported"), path); @@ -2337,6 +2399,9 @@ static void absorb_git_dir_into_superproject_recurse(const char *path) struct child_process cp = CHILD_PROCESS_INIT; + if (validate_submodule_path(path) < 0) + exit(128); + cp.dir = path; cp.git_cmd = 1; cp.no_stdin = 1; @@ -2359,6 +2424,10 @@ void absorb_git_dir_into_superproject(const char *path) int err_code; const char *sub_git_dir; struct strbuf gitdir = STRBUF_INIT; + + if (validate_submodule_path(path) < 0) + exit(128); + strbuf_addf(&gitdir, "%s/.git", path); sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code); @@ -2501,6 +2570,9 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule) const char *git_dir; int ret = 0; + if (validate_submodule_path(submodule) < 0) + exit(128); + strbuf_reset(buf); strbuf_addstr(buf, submodule); strbuf_complete(buf, '/'); diff --git a/submodule.h b/submodule.h index b52a4ff1e7..fb770f1687 100644 --- a/submodule.h +++ b/submodule.h @@ -148,6 +148,11 @@ void submodule_name_to_gitdir(struct strbuf *buf, struct repository *r, */ int validate_submodule_git_dir(char *git_dir, const char *submodule_name); +/* + * Make sure that the given submodule path does not follow symlinks. + */ +int validate_submodule_path(const char *path); + #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0) #define SUBMODULE_MOVE_HEAD_FORCE (1<<1) int submodule_move_head(const char *path, diff --git a/t/t7423-submodule-symlinks.sh b/t/t7423-submodule-symlinks.sh index a72f3cbcab..3d3c7af3ce 100755 --- a/t/t7423-submodule-symlinks.sh +++ b/t/t7423-submodule-symlinks.sh @@ -14,15 +14,16 @@ test_expect_success 'prepare' ' git commit -m submodule ' -test_expect_failure SYMLINKS 'git submodule update must not create submodule behind symlink' ' +test_expect_success SYMLINKS 'git submodule update must not create submodule behind symlink' ' rm -rf a b && mkdir b && ln -s b a && + test_path_is_missing b/sm && test_must_fail git submodule update && test_path_is_missing b/sm ' -test_expect_failure SYMLINKS,CASE_INSENSITIVE_FS 'git submodule update must not create submodule behind symlink on case insensitive fs' ' +test_expect_success SYMLINKS,CASE_INSENSITIVE_FS 'git submodule update must not create submodule behind symlink on case insensitive fs' ' rm -rf a b && mkdir b && ln -s b A && @@ -46,7 +47,7 @@ test_expect_success SYMLINKS 'git restore --recurse-submodules must not be confu test_path_is_missing a/target/submodule_file ' -test_expect_failure SYMLINKS 'git restore --recurse-submodules must not migrate git dir of symlinked repo' ' +test_expect_success SYMLINKS 'git restore --recurse-submodules must not migrate git dir of symlinked repo' ' prepare_symlink_to_repo && rm -rf .git/modules && test_must_fail git restore --recurse-submodules a/sm && @@ -55,7 +56,7 @@ test_expect_failure SYMLINKS 'git restore --recurse-submodules must not migrate test_path_is_missing a/target/submodule_file ' -test_expect_failure SYMLINKS 'git checkout -f --recurse-submodules must not migrate git dir of symlinked repo when removing submodule' ' +test_expect_success SYMLINKS 'git checkout -f --recurse-submodules must not migrate git dir of symlinked repo when removing submodule' ' prepare_symlink_to_repo && rm -rf .git/modules && test_must_fail git checkout -f --recurse-submodules initial && From e4930e86c0d521aa6c3c3da9f590e852f6eeac21 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2024 14:13:41 +0100 Subject: [PATCH 26/39] t5510: verify that D/F confusion cannot lead to an RCE The most critical vulnerabilities in Git lead to a Remote Code Execution ("RCE"), i.e. the ability for an attacker to have malicious code being run as part of a Git operation that is not expected to run said code, such has hooks delivered as part of a `git clone`. A couple of parent commits ago, a bug was fixed that let Git be confused by the presence of a path `a-` to mistakenly assume that a directory `a/` can safely be created without removing an existing `a` that is a symbolic link. This bug did not represent an exploitable vulnerability on its own; Let's make sure it stays that way. Signed-off-by: Johannes Schindelin --- t/t5510-fetch.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index c0b745e33b..211afe13e9 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1240,6 +1240,30 @@ EOF test_cmp fatal-expect fatal-actual ' +test_expect_success SYMLINKS 'clone does not get confused by a D/F conflict' ' + git init df-conflict && + ( + cd df-conflict && + ln -s .git a && + git add a && + test_tick && + git commit -m symlink && + test_commit a- && + rm a && + mkdir -p a/hooks && + write_script a/hooks/post-checkout <<-EOF && + echo WHOOPSIE >&2 + echo whoopsie >"$TRASH_DIRECTORY"/whoops + EOF + git add a/hooks/post-checkout && + test_tick && + git commit -m post-checkout + ) && + git clone df-conflict clone 2>err && + ! grep WHOOPS err && + test_path_is_missing whoops +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd From 850c3a220e7a0b1bf740fba9ac8f3f2b0486a1af Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 28 Mar 2024 09:44:28 +0100 Subject: [PATCH 27/39] entry: report more colliding paths In b878579ae7 (clone: report duplicate entries on case-insensitive filesystems, 2018-08-17) code was added to warn about index entries that resolve to the same file system entity (usually the cause is a case-insensitive filesystem). In Git for Windows, where inodes are not trusted (because of a performance trade-off, inodes are equal to 0 by default), that check does not compare inode numbers but the verbatim path. This logic works well when index entries' paths differ only in case. However, for file/directory conflicts only the file's path was reported, leaving the user puzzled with what that path collides. Let's try ot catch colliding paths even if one path is the prefix of the other. We do this also in setups where the file system is case-sensitive because the inode check would not be able to catch those collisions. While not a complete solution (for example, on macOS, Unicode normalization could also lead to file/directory conflicts but be missed by this logic), it is at least another defensive layer on top of what the previous commits added. Signed-off-by: Johannes Schindelin --- dir.c | 12 ++++++++++++ dir.h | 7 +++++++ entry.c | 2 +- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/dir.c b/dir.c index f8a11aa1ec..fd689bbe66 100644 --- a/dir.c +++ b/dir.c @@ -88,6 +88,18 @@ int fspathncmp(const char *a, const char *b, size_t count) return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count); } +int paths_collide(const char *a, const char *b) +{ + size_t len_a = strlen(a), len_b = strlen(b); + + if (len_a == len_b) + return fspatheq(a, b); + + if (len_a < len_b) + return is_dir_sep(b[len_a]) && !fspathncmp(a, b, len_a); + return is_dir_sep(a[len_b]) && !fspathncmp(a, b, len_b); +} + unsigned int fspathhash(const char *str) { return ignore_case ? strihash(str) : strhash(str); diff --git a/dir.h b/dir.h index 674747d93a..62e89a053d 100644 --- a/dir.h +++ b/dir.h @@ -519,6 +519,13 @@ int fspatheq(const char *a, const char *b); int fspathncmp(const char *a, const char *b, size_t count); unsigned int fspathhash(const char *str); +/* + * Reports whether paths collide. This may be because the paths differ only in + * case on a case-sensitive filesystem, or that one path refers to a symlink + * that collides with one of the parent directories of the other. + */ +int paths_collide(const char *a, const char *b); + /* * The prefix part of pattern must not contains wildcards. */ diff --git a/entry.c b/entry.c index 616e4f073c..a4c18c5645 100644 --- a/entry.c +++ b/entry.c @@ -454,7 +454,7 @@ static void mark_colliding_entries(const struct checkout *state, continue; if ((trust_ino && !match_stat_data(&dup->ce_stat_data, st)) || - (!trust_ino && !fspathcmp(ce->name, dup->name))) { + paths_collide(ce->name, dup->name)) { dup->ce_flags |= CE_MATCHED; break; } From 31572dc420afee36db8fbbbe060dd78c9a48778c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 28 Mar 2024 10:55:07 +0100 Subject: [PATCH 28/39] clone: when symbolic links collide with directories, keep the latter When recursively cloning a repository with submodules, we must ensure that the submodules paths do not suddenly contain symbolic links that would let Git write into unintended locations. We just plugged that vulnerability, but let's add some more defense-in-depth. Since we can only keep one item on disk if multiple index entries' paths collide, we may just as well avoid keeping a symbolic link (because that would allow attack vectors where Git follows those links by mistake). Technically, we handle more situations than cloning submodules into paths that were (partially) replaced by symbolic links. This provides defense-in-depth in case someone finds a case-folding confusion vulnerability in the future that does not even involve submodules. Signed-off-by: Johannes Schindelin --- entry.c | 14 ++++++++++++++ t/t5601-clone.sh | 15 +++++++++++++++ t/t7406-submodule-update.sh | 4 ++-- 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/entry.c b/entry.c index a4c18c5645..1d78e54168 100644 --- a/entry.c +++ b/entry.c @@ -541,6 +541,20 @@ int checkout_entry_ca(struct cache_entry *ce, struct conv_attrs *ca, /* If it is a gitlink, leave it alone! */ if (S_ISGITLINK(ce->ce_mode)) return 0; + /* + * We must avoid replacing submodules' leading + * directories with symbolic links, lest recursive + * clones can write into arbitrary locations. + * + * Technically, this logic is not limited + * to recursive clones, or for that matter to + * submodules' paths colliding with symbolic links' + * paths. Yet it strikes a balance in favor of + * simplicity, and if paths are colliding, we might + * just as well keep the directories during a clone. + */ + if (state->clone && S_ISLNK(ce->ce_mode)) + return 0; remove_subtree(&path); } else if (unlink(path.buf)) return error_errno("unable to unlink old '%s'", path.buf); diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index b2524a24c2..fd02984330 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -633,6 +633,21 @@ test_expect_success CASE_INSENSITIVE_FS 'colliding file detection' ' test_i18ngrep "the following paths have collided" icasefs/warning ' +test_expect_success CASE_INSENSITIVE_FS,SYMLINKS \ + 'colliding symlink/directory keeps directory' ' + git init icasefs-colliding-symlink && + ( + cd icasefs-colliding-symlink && + a=$(printf a | git hash-object -w --stdin) && + printf "100644 %s 0\tA/dir/b\n120000 %s 0\ta\n" $a $a >idx && + git update-index --index-info err && - grep "directory not empty" err && + git clone --recursive captain hooked 2>err && + ! grep HOOK-RUN err && test_path_is_missing "$tell_tale_path" ' From 48c171d927407132875283db3afb05b08b98a078 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 28 Mar 2024 19:02:30 +0100 Subject: [PATCH 29/39] find_hook(): refactor the `STRIP_EXTENSION` logic When looking for a hook and not finding one, and when `STRIP_EXTENSION` is available (read: if we're on Windows and `.exe` is the required extension for executable programs), we want to look also for a hook with that extension. Previously, we added that handling into the conditional block that was meant to handle when no hook was found (possibly providing some advice for the user's benefit). If the hook with that file extension was found, we'd return early from that function instead of writing out said advice, of course. However, we're about to introduce a safety valve to prevent hooks from being run during a clone, to reduce the attack surface of bugs that allow writing files to be written into arbitrary locations. To prepare for that, refactor the logic to avoid the early return, by separating the `STRIP_EXTENSION` handling from the conditional block handling the case when no hook was found. This commit is best viewed with `--patience`. Signed-off-by: Johannes Schindelin --- hook.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/hook.c b/hook.c index a4fa1031f2..22b274b60b 100644 --- a/hook.c +++ b/hook.c @@ -7,20 +7,24 @@ const char *find_hook(const char *name) { static struct strbuf path = STRBUF_INIT; + int found_hook; + strbuf_reset(&path); strbuf_git_path(&path, "hooks/%s", name); - if (access(path.buf, X_OK) < 0) { + found_hook = access(path.buf, X_OK) >= 0; +#ifdef STRIP_EXTENSION + if (!found_hook) { int err = errno; -#ifdef STRIP_EXTENSION strbuf_addstr(&path, STRIP_EXTENSION); - if (access(path.buf, X_OK) >= 0) - return path.buf; - if (errno == EACCES) - err = errno; + found_hook = access(path.buf, X_OK) >= 0; + if (!found_hook) + errno = err; + } #endif - if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) { + if (!found_hook) { + if (errno == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) { static struct string_list advise_given = STRING_LIST_INIT_DUP; if (!string_list_lookup(&advise_given, name)) { From df93e407f0618e4a8265ac619dc7f4c7005155bc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Mar 2024 11:45:01 +0100 Subject: [PATCH 30/39] init: refactor the template directory discovery into its own function We will need to call this function from `hook.c` to be able to prevent hooks from running that were written as part of a `clone` but did not originate from the template directory. Signed-off-by: Johannes Schindelin --- builtin/init-db.c | 22 ++++------------------ cache.h | 1 + setup.c | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 18 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index dcaaf102ea..a101e7f94c 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -11,10 +11,6 @@ #include "parse-options.h" #include "worktree.h" -#ifndef DEFAULT_GIT_TEMPLATE_DIR -#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates" -#endif - #ifdef NO_TRUSTABLE_FILEMODE #define TEST_FILEMODE 0 #else @@ -93,8 +89,9 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path, } } -static void copy_templates(const char *template_dir, const char *init_template_dir) +static void copy_templates(const char *option_template) { + const char *template_dir = get_template_dir(option_template); struct strbuf path = STRBUF_INIT; struct strbuf template_path = STRBUF_INIT; size_t template_len; @@ -103,16 +100,8 @@ static void copy_templates(const char *template_dir, const char *init_template_d DIR *dir; char *to_free = NULL; - if (!template_dir) - template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); - if (!template_dir) - template_dir = init_template_dir; - if (!template_dir) - template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR); - if (!template_dir[0]) { - free(to_free); + if (!template_dir || !*template_dir) return; - } strbuf_addstr(&template_path, template_dir); strbuf_complete(&template_path, '/'); @@ -200,7 +189,6 @@ static int create_default_files(const char *template_path, int reinit; int filemode; struct strbuf err = STRBUF_INIT; - const char *init_template_dir = NULL; const char *work_tree = get_git_work_tree(); /* @@ -212,9 +200,7 @@ static int create_default_files(const char *template_path, * values (since we've just potentially changed what's available on * disk). */ - git_config_get_pathname("init.templatedir", &init_template_dir); - copy_templates(template_path, init_template_dir); - free((char *)init_template_dir); + copy_templates(template_path); git_config_clear(); reset_shared_repository(); git_config(git_default_config, NULL); diff --git a/cache.h b/cache.h index a46a3e4b6b..8c5fb1e1ba 100644 --- a/cache.h +++ b/cache.h @@ -656,6 +656,7 @@ int path_inside_repo(const char *prefix, const char *path); #define INIT_DB_QUIET 0x0001 #define INIT_DB_EXIST_OK 0x0002 +const char *get_template_dir(const char *option_template); int init_db(const char *git_dir, const char *real_git_dir, const char *template_dir, int hash_algo, const char *initial_branch, unsigned int flags); diff --git a/setup.c b/setup.c index 9d401ae4c8..e6e749ec4b 100644 --- a/setup.c +++ b/setup.c @@ -6,6 +6,7 @@ #include "chdir-notify.h" #include "promisor-remote.h" #include "quote.h" +#include "exec-cmd.h" static int inside_git_dir = -1; static int inside_work_tree = -1; @@ -1720,3 +1721,34 @@ int daemonize(void) return 0; #endif } + +#ifndef DEFAULT_GIT_TEMPLATE_DIR +#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates" +#endif + +const char *get_template_dir(const char *option_template) +{ + const char *template_dir = option_template; + + if (!template_dir) + template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); + if (!template_dir) { + static const char *init_template_dir; + static int initialized; + + if (!initialized) { + git_config_get_pathname("init.templatedir", + &init_template_dir); + initialized = 1; + } + template_dir = init_template_dir; + } + if (!template_dir) { + static char *dir; + + if (!dir) + dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); + template_dir = dir; + } + return template_dir; +} From 584de0b4c235209fa60ca4a733678472263bdce0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 30 Mar 2024 15:59:20 +0100 Subject: [PATCH 31/39] Add a helper function to compare file contents In the next commit, Git will learn to disallow hooks during `git clone` operations _except_ when those hooks come from the templates (which are inherently supposed to be trusted). To that end, we add a function to compare the contents of two files. Signed-off-by: Johannes Schindelin --- cache.h | 14 +++++++++ copy.c | 58 ++++++++++++++++++++++++++++++++++++++ t/helper/test-path-utils.c | 10 +++++++ t/t0060-path-utils.sh | 41 +++++++++++++++++++++++++++ 4 files changed, 123 insertions(+) diff --git a/cache.h b/cache.h index 8c5fb1e1ba..16b34799bf 100644 --- a/cache.h +++ b/cache.h @@ -1785,6 +1785,20 @@ int copy_fd(int ifd, int ofd); int copy_file(const char *dst, const char *src, int mode); int copy_file_with_time(const char *dst, const char *src, int mode); +/* + * Compare the file mode and contents of two given files. + * + * If both files are actually symbolic links, the function returns 1 if the link + * targets are identical or 0 if they are not. + * + * If any of the two files cannot be accessed or in case of read failures, this + * function returns 0. + * + * If the file modes and contents are identical, the function returns 1, + * otherwise it returns 0. + */ +int do_files_match(const char *path1, const char *path2); + void write_or_die(int fd, const void *buf, size_t count); void fsync_or_die(int fd, const char *); int fsync_component(enum fsync_component component, int fd); diff --git a/copy.c b/copy.c index 4de6a110f0..8492f6fc83 100644 --- a/copy.c +++ b/copy.c @@ -65,3 +65,61 @@ int copy_file_with_time(const char *dst, const char *src, int mode) return copy_times(dst, src); return status; } + +static int do_symlinks_match(const char *path1, const char *path2) +{ + struct strbuf buf1 = STRBUF_INIT, buf2 = STRBUF_INIT; + int ret = 0; + + if (!strbuf_readlink(&buf1, path1, 0) && + !strbuf_readlink(&buf2, path2, 0)) + ret = !strcmp(buf1.buf, buf2.buf); + + strbuf_release(&buf1); + strbuf_release(&buf2); + return ret; +} + +int do_files_match(const char *path1, const char *path2) +{ + struct stat st1, st2; + int fd1 = -1, fd2 = -1, ret = 1; + char buf1[8192], buf2[8192]; + + if ((fd1 = open_nofollow(path1, O_RDONLY)) < 0 || + fstat(fd1, &st1) || !S_ISREG(st1.st_mode)) { + if (fd1 < 0 && errno == ELOOP) + /* maybe this is a symbolic link? */ + return do_symlinks_match(path1, path2); + ret = 0; + } else if ((fd2 = open_nofollow(path2, O_RDONLY)) < 0 || + fstat(fd2, &st2) || !S_ISREG(st2.st_mode)) { + ret = 0; + } + + if (ret) + /* to match, neither must be executable, or both */ + ret = !(st1.st_mode & 0111) == !(st2.st_mode & 0111); + + if (ret) + ret = st1.st_size == st2.st_size; + + while (ret) { + ssize_t len1 = read_in_full(fd1, buf1, sizeof(buf1)); + ssize_t len2 = read_in_full(fd2, buf2, sizeof(buf2)); + + if (len1 < 0 || len2 < 0 || len1 != len2) + ret = 0; /* read error or different file size */ + else if (!len1) /* len2 is also 0; hit EOF on both */ + break; /* ret is still true */ + else + ret = !memcmp(buf1, buf2, len1); + } + + if (fd1 >= 0) + close(fd1); + if (fd2 >= 0) + close(fd2); + + return ret; +} diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c index f69709d674..0e0de21807 100644 --- a/t/helper/test-path-utils.c +++ b/t/helper/test-path-utils.c @@ -495,6 +495,16 @@ int cmd__path_utils(int argc, const char **argv) return !!res; } + if (argc == 4 && !strcmp(argv[1], "do_files_match")) { + int ret = do_files_match(argv[2], argv[3]); + + if (ret) + printf("equal\n"); + else + printf("different\n"); + return !ret; + } + fprintf(stderr, "%s: unknown function name: %s\n", argv[0], argv[1] ? argv[1] : "(there was none)"); return 1; diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 68e29c904a..73d0e1a7f1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -560,4 +560,45 @@ test_expect_success !VALGRIND,RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' test_cmp expect actual ' +test_expect_success 'do_files_match()' ' + test_seq 0 10 >0-10.txt && + test_seq -1 10 >-1-10.txt && + test_seq 1 10 >1-10.txt && + test_seq 1 9 >1-9.txt && + test_seq 0 8 >0-8.txt && + + test-tool path-utils do_files_match 0-10.txt 0-10.txt >out && + + assert_fails() { + test_must_fail \ + test-tool path-utils do_files_match "$1" "$2" >out && + grep different out + } && + + assert_fails 0-8.txt 1-9.txt && + assert_fails -1-10.txt 0-10.txt && + assert_fails 1-10.txt 1-9.txt && + assert_fails 1-10.txt .git && + assert_fails does-not-exist 1-10.txt && + + if test_have_prereq FILEMODE + then + cp 0-10.txt 0-10.x && + chmod a+x 0-10.x && + assert_fails 0-10.txt 0-10.x + fi && + + if test_have_prereq SYMLINKS + then + ln -sf 0-10.txt symlink && + ln -s 0-10.txt another-symlink && + ln -s over-the-ocean yet-another-symlink && + ln -s "$PWD/0-10.txt" absolute-symlink && + assert_fails 0-10.txt symlink && + test-tool path-utils do_files_match symlink another-symlink && + assert_fails symlink yet-another-symlink && + assert_fails symlink absolute-symlink + fi +' + test_done From 8db1e8743c0f1ed241f6a1b8bf55b6fef07d6751 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 28 Mar 2024 19:21:06 +0100 Subject: [PATCH 32/39] clone: prevent hooks from running during a clone Critical security issues typically combine relatively common vulnerabilities such as case confusion in file paths with other weaknesses in order to raise the severity of the attack. One such weakness that has haunted the Git project in many a submodule-related CVE is that any hooks that are found are executed during a clone operation. Examples are the `post-checkout` and `fsmonitor` hooks. However, Git's design calls for hooks to be disabled by default, as only disabled example hooks are copied over from the templates in `/share/git-core/templates/`. As a defense-in-depth measure, let's prevent those hooks from running. Obviously, administrators can choose to drop enabled hooks into the template directory, though, _and_ it is also possible to override `core.hooksPath`, in which case the new check needs to be disabled. Signed-off-by: Johannes Schindelin --- builtin/clone.c | 12 +++++++++++- hook.c | 32 ++++++++++++++++++++++++++++++ t/t5601-clone.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index 3c2ae31a55..35a73ed0a7 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -908,6 +908,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) int err = 0, complete_refs_before_fetch = 1; int submodule_progress; int filter_submodules = 0; + const char *template_dir; + char *template_dir_dup = NULL; struct transport_ls_refs_options transport_ls_refs_options = TRANSPORT_LS_REFS_OPTIONS_INIT; @@ -927,6 +929,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix) usage_msg_opt(_("You must specify a repository to clone."), builtin_clone_usage, builtin_clone_options); + xsetenv("GIT_CLONE_PROTECTION_ACTIVE", "true", 0 /* allow user override */); + template_dir = get_template_dir(option_template); + if (*template_dir && !is_absolute_path(template_dir)) + template_dir = template_dir_dup = + absolute_pathdup(template_dir); + xsetenv("GIT_CLONE_TEMPLATE_DIR", template_dir, 1); + if (option_depth || option_since || option_not.nr) deepen = 1; if (option_single_branch == -1) @@ -1074,7 +1083,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) } } - init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL, + init_db(git_dir, real_git_dir, template_dir, GIT_HASH_UNKNOWN, NULL, INIT_DB_QUIET); if (real_git_dir) { @@ -1392,6 +1401,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) free(unborn_head); free(dir); free(path); + free(template_dir_dup); UNLEAK(repo); junk_mode = JUNK_LEAVE_ALL; diff --git a/hook.c b/hook.c index 22b274b60b..632b537b99 100644 --- a/hook.c +++ b/hook.c @@ -3,6 +3,30 @@ #include "run-command.h" #include "config.h" +static int identical_to_template_hook(const char *name, const char *path) +{ + const char *env = getenv("GIT_CLONE_TEMPLATE_DIR"); + const char *template_dir = get_template_dir(env && *env ? env : NULL); + struct strbuf template_path = STRBUF_INIT; + int found_template_hook, ret; + + strbuf_addf(&template_path, "%s/hooks/%s", template_dir, name); + found_template_hook = access(template_path.buf, X_OK) >= 0; +#ifdef STRIP_EXTENSION + if (!found_template_hook) { + strbuf_addstr(&template_path, STRIP_EXTENSION); + found_template_hook = access(template_path.buf, X_OK) >= 0; + } +#endif + if (!found_template_hook) + return 0; + + ret = do_files_match(template_path.buf, path); + + strbuf_release(&template_path); + return ret; +} + const char *find_hook(const char *name) { static struct strbuf path = STRBUF_INIT; @@ -38,6 +62,14 @@ const char *find_hook(const char *name) } return NULL; } + if (!git_hooks_path && git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0) && + !identical_to_template_hook(name, path.buf)) + die(_("active `%s` hook found during `git clone`:\n\t%s\n" + "For security reasons, this is disallowed by default.\n" + "If this is intentional and the hook should actually " + "be run, please\nrun the command again with " + "`GIT_CLONE_PROTECTION_ACTIVE=false`"), + name, path.buf); return path.buf; } diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index fd02984330..20deca0231 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -771,6 +771,57 @@ test_expect_success 'batch missing blob request does not inadvertently try to fe git clone --filter=blob:limit=0 "file://$(pwd)/server" client ' +test_expect_success 'clone with init.templatedir runs hooks' ' + git init tmpl/hooks && + write_script tmpl/hooks/post-checkout <<-EOF && + echo HOOK-RUN >&2 + echo I was here >hook.run + EOF + git -C tmpl/hooks add . && + test_tick && + git -C tmpl/hooks commit -m post-checkout && + + test_when_finished "git config --global --unset init.templateDir || :" && + test_when_finished "git config --unset init.templateDir || :" && + ( + sane_unset GIT_TEMPLATE_DIR && + NO_SET_GIT_TEMPLATE_DIR=t && + export NO_SET_GIT_TEMPLATE_DIR && + + git -c core.hooksPath="$(pwd)/tmpl/hooks" \ + clone tmpl/hooks hook-run-hookspath 2>err && + ! grep "active .* hook found" err && + test_path_is_file hook-run-hookspath/hook.run && + + git -c init.templateDir="$(pwd)/tmpl" \ + clone tmpl/hooks hook-run-config 2>err && + ! grep "active .* hook found" err && + test_path_is_file hook-run-config/hook.run && + + git clone --template=tmpl tmpl/hooks hook-run-option 2>err && + ! grep "active .* hook found" err && + test_path_is_file hook-run-option/hook.run && + + git config --global init.templateDir "$(pwd)/tmpl" && + git clone tmpl/hooks hook-run-global-config 2>err && + git config --global --unset init.templateDir && + ! grep "active .* hook found" err && + test_path_is_file hook-run-global-config/hook.run && + + # clone ignores local `init.templateDir`; need to create + # a new repository because we deleted `.git/` in the + # `setup` test case above + git init local-clone && + cd local-clone && + + git config init.templateDir "$(pwd)/../tmpl" && + git clone ../tmpl/hooks hook-run-local-config 2>err && + git config --unset init.templateDir && + ! grep "active .* hook found" err && + test_path_is_missing hook-run-local-config/hook.run + ) +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd From 4412a04fe6f7e632269a6668a4f367230ca2c0e0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Mar 2024 13:15:32 +0100 Subject: [PATCH 33/39] init.templateDir: consider this config setting protected The ability to configuring the template directory is a delicate feature: It allows defining hooks that will be run e.g. during a `git clone` operation, such as the `post-checkout` hook. As such, it is of utmost importance that Git would not allow that config setting to be changed during a `git clone` by mistake, allowing an attacker a chance for a Remote Code Execution, allowing attackers to run arbitrary code on unsuspecting users' machines. As a defense-in-depth measure, to prevent minor vulnerabilities in the `git clone` code from ballooning into higher-serverity attack vectors, let's make this a protected setting just like `safe.directory` and friends, i.e. ignore any `init.templateDir` entries from any local config. Note: This does not change the behavior of any recursive clone (modulo bugs), as the local repository config is not even supposed to be written while cloning the superproject, except in one scenario: If a config template is configured that sets the template directory. This might be done because `git clone --recurse-submodules --template=` does not pass that template directory on to the submodules' initialization. Another scenario where this commit changes behavior is where repositories are _not_ cloned recursively, and then some (intentional, benign) automation configures the template directory to be used before initializing the submodules. So the caveat is that this could theoretically break existing processes. In both scenarios, there is a way out, though: configuring the template directory via the environment variable `GIT_TEMPLATE_DIR`. This change in behavior is a trade-off between security and backwards-compatibility that is struck in favor of security. Signed-off-by: Johannes Schindelin --- setup.c | 37 ++++++++++++++++++++++++++++++------- t/t7400-submodule-basic.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/setup.c b/setup.c index e6e749ec4b..c3301f5ab8 100644 --- a/setup.c +++ b/setup.c @@ -1726,6 +1726,31 @@ int daemonize(void) #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates" #endif +struct template_dir_cb_data { + char *path; + int initialized; +}; + +static int template_dir_cb(const char *key, const char *value, void *d) +{ + struct template_dir_cb_data *data = d; + + if (strcmp(key, "init.templatedir")) + return 0; + + if (!value) { + data->path = NULL; + } else { + char *path = NULL; + + FREE_AND_NULL(data->path); + if (!git_config_pathname((const char **)&path, key, value)) + data->path = path ? path : xstrdup(value); + } + + return 0; +} + const char *get_template_dir(const char *option_template) { const char *template_dir = option_template; @@ -1733,15 +1758,13 @@ const char *get_template_dir(const char *option_template) if (!template_dir) template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); if (!template_dir) { - static const char *init_template_dir; - static int initialized; + static struct template_dir_cb_data data; - if (!initialized) { - git_config_get_pathname("init.templatedir", - &init_template_dir); - initialized = 1; + if (!data.initialized) { + git_protected_config(template_dir_cb, &data); + data.initialized = 1; } - template_dir = init_template_dir; + template_dir = data.path; } if (!template_dir) { static char *dir; diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..3e8cf9b885 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1436,4 +1436,35 @@ test_expect_success 'recursive clone respects -q' ' test_must_be_empty actual ' +test_expect_success '`submodule init` and `init.templateDir`' ' + mkdir -p tmpl/hooks && + write_script tmpl/hooks/post-checkout <<-EOF && + echo HOOK-RUN >&2 + echo I was here >hook.run + exit 1 + EOF + + test_config init.templateDir "$(pwd)/tmpl" && + test_when_finished \ + "git config --global --unset init.templateDir || true" && + ( + sane_unset GIT_TEMPLATE_DIR && + NO_SET_GIT_TEMPLATE_DIR=t && + export NO_SET_GIT_TEMPLATE_DIR && + + git config --global init.templateDir "$(pwd)/tmpl" && + test_must_fail git submodule \ + add "$submodurl" sub-global 2>err && + git config --global --unset init.templateDir && + grep HOOK-RUN err && + test_path_is_file sub-global/hook.run && + + git config init.templateDir "$(pwd)/tmpl" && + git submodule add "$submodurl" sub-local 2>err && + git config --unset init.templateDir && + ! grep HOOK-RUN err && + test_path_is_missing sub-local/hook.run + ) +' + test_done From 20f3588efc6cbcae5bbaabf65ee12df87b51a9ea Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 30 Mar 2024 19:12:50 +0100 Subject: [PATCH 34/39] core.hooksPath: add some protection while cloning Quite frequently, when vulnerabilities were found in Git's (quite complex) clone machinery, a relatively common way to escalate the severity was to trick Git into running a hook which is actually a script that has just been laid on disk as part of that clone. This constitutes a Remote Code Execution vulnerability, the highest severity observed in Git's vulnerabilities so far. Some previously-fixed vulnerabilities allowed malicious repositories to be crafted such that Git would check out files not in the worktree, but in, say, a submodule's `/hooks/` directory. A vulnerability that "merely" allows to modify the Git config would allow a related attack vector, to manipulate Git into looking in the worktree for hooks, e.g. redirecting the location where Git looks for hooks, via setting `core.hooksPath` (which would be classified as CWE-427: Uncontrolled Search Path Element and CWE-114: Process Control, for more details see https://cwe.mitre.org/data/definitions/427.html and https://cwe.mitre.org/data/definitions/114.html). To prevent that attack vector, let's error out and complain loudly if an active `core.hooksPath` configuration is seen in the repository-local Git config during a `git clone`. There is one caveat: This changes Git's behavior in a slightly backwards-incompatible manner. While it is probably a rare scenario (if it exists at all) to configure `core.hooksPath` via a config in the Git templates, it _is_ conceivable that some valid setup requires this to work. In the hopefully very unlikely case that a user runs into this, there is an escape hatch: set the `GIT_CLONE_PROTECTION_ACTIVE=false` environment variable. Obviously, this should be done only with utmost caution. Signed-off-by: Johannes Schindelin --- config.c | 13 ++++++++++++- t/t1800-hook.sh | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 8c1c4071f0..85b37f2ee0 100644 --- a/config.c +++ b/config.c @@ -1525,8 +1525,19 @@ static int git_default_core_config(const char *var, const char *value, void *cb) if (!strcmp(var, "core.attributesfile")) return git_config_pathname(&git_attributes_file, var, value); - if (!strcmp(var, "core.hookspath")) + if (!strcmp(var, "core.hookspath")) { + if (current_config_scope() == CONFIG_SCOPE_LOCAL && + git_env_bool("GIT_CLONE_PROTECTION_ACTIVE", 0)) + die(_("active `core.hooksPath` found in the local " + "repository config:\n\t%s\nFor security " + "reasons, this is disallowed by default.\nIf " + "this is intentional and the hook should " + "actually be run, please\nrun the command " + "again with " + "`GIT_CLONE_PROTECTION_ACTIVE=false`"), + value); return git_config_pathname(&git_hooks_path, var, value); + } if (!strcmp(var, "core.bare")) { is_bare_repository_cfg = git_config_bool(var, value); diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 2ef3579fa7..7ee12e6f48 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -177,4 +177,19 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_cmp expect actual ' +test_expect_success 'clone protections' ' + test_config core.hooksPath "$(pwd)/my-hooks" && + mkdir -p my-hooks && + write_script my-hooks/test-hook <<-\EOF && + echo Hook ran $1 + EOF + + git hook run test-hook 2>err && + grep "Hook ran" err && + test_must_fail env GIT_CLONE_PROTECTION_ACTIVE=true \ + git hook run test-hook 2>err && + grep "active .core.hooksPath" err && + ! grep "Hook ran" err +' + test_done From a33fea0886cfa016d313d2bd66bdd08615bffbc9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 18:01:13 +0200 Subject: [PATCH 35/39] fsck: warn about symlink pointing inside a gitdir In the wake of fixing a vulnerability where `git clone` mistakenly followed a symbolic link that it had just written while checking out files, writing into a gitdir, let's add some defense-in-depth by teaching `git fsck` to report symbolic links stored in its trees that point inside `.git/`. Even though the Git project never made any promises about the exact shape of the `.git/` directory's contents, there are likely repositories out there containing symbolic links that point inside the gitdir. For that reason, let's only report these as warnings, not as errors. Security-conscious users are encouraged to configure `fsck.symlinkPointsToGitDir = error`. Signed-off-by: Johannes Schindelin --- Documentation/fsck-msgids.txt | 12 ++++++++ fsck.c | 56 +++++++++++++++++++++++++++++++++++ fsck.h | 12 ++++++++ t/t1450-fsck.sh | 37 +++++++++++++++++++++++ 4 files changed, 117 insertions(+) diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt index 12eae8a222..b06ec385af 100644 --- a/Documentation/fsck-msgids.txt +++ b/Documentation/fsck-msgids.txt @@ -157,6 +157,18 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`symlinkPointsToGitDir`:: + (WARN) Symbolic link points inside a gitdir. + +`symlinkTargetBlob`:: + (ERROR) A non-blob found instead of a symbolic link's target. + +`symlinkTargetLength`:: + (WARN) Symbolic link target longer than maximum path length. + +`symlinkTargetMissing`:: + (ERROR) Unable to read symbolic link target's blob. + `treeNotSorted`:: (ERROR) A tree is not properly sorted. diff --git a/fsck.c b/fsck.c index 47eaeedd70..b85868e122 100644 --- a/fsck.c +++ b/fsck.c @@ -636,6 +636,8 @@ static int fsck_tree(const struct object_id *tree_oid, retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_MAILMAP_SYMLINK, ".mailmap is a symlink"); + oidset_insert(&options->symlink_targets_found, + entry_oid); } if ((backslash = strchr(name, '\\'))) { @@ -1228,6 +1230,56 @@ static int fsck_blob(const struct object_id *oid, const char *buf, } } + if (oidset_contains(&options->symlink_targets_found, oid)) { + const char *ptr = buf; + const struct object_id *reported = NULL; + + oidset_insert(&options->symlink_targets_done, oid); + + if (!buf || size > PATH_MAX) { + /* + * A missing buffer here is a sign that the caller found the + * blob too gigantic to load into memory. Let's just consider + * that an error. + */ + return report(options, oid, OBJ_BLOB, + FSCK_MSG_SYMLINK_TARGET_LENGTH, + "symlink target too long"); + } + + while (!reported && ptr) { + const char *p = ptr; + char c, *slash = strchrnul(ptr, '/'); + char *backslash = memchr(ptr, '\\', slash - ptr); + + c = *slash; + *slash = '\0'; + + while (!reported && backslash) { + *backslash = '\0'; + if (is_ntfs_dotgit(p)) + ret |= report(options, reported = oid, OBJ_BLOB, + FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR, + "symlink target points to git dir"); + *backslash = '\\'; + p = backslash + 1; + backslash = memchr(p, '\\', slash - p); + } + if (!reported && is_ntfs_dotgit(p)) + ret |= report(options, reported = oid, OBJ_BLOB, + FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR, + "symlink target points to git dir"); + + if (!reported && is_hfs_dotgit(ptr)) + ret |= report(options, reported = oid, OBJ_BLOB, + FSCK_MSG_SYMLINK_POINTS_TO_GIT_DIR, + "symlink target points to git dir"); + + *slash = c; + ptr = c ? slash + 1 : NULL; + } + } + return ret; } @@ -1319,6 +1371,10 @@ int fsck_finish(struct fsck_options *options) FSCK_MSG_GITATTRIBUTES_MISSING, FSCK_MSG_GITATTRIBUTES_BLOB, options, ".gitattributes"); + ret |= fsck_blobs(&options->symlink_targets_found, &options->symlink_targets_done, + FSCK_MSG_SYMLINK_TARGET_MISSING, FSCK_MSG_SYMLINK_TARGET_BLOB, + options, ""); + return ret; } diff --git a/fsck.h b/fsck.h index fcecf4101c..130fa8d8f9 100644 --- a/fsck.h +++ b/fsck.h @@ -63,6 +63,8 @@ enum fsck_msg_type { FUNC(GITATTRIBUTES_LARGE, ERROR) \ FUNC(GITATTRIBUTES_LINE_LENGTH, ERROR) \ FUNC(GITATTRIBUTES_BLOB, ERROR) \ + FUNC(SYMLINK_TARGET_MISSING, ERROR) \ + FUNC(SYMLINK_TARGET_BLOB, ERROR) \ /* warnings */ \ FUNC(EMPTY_NAME, WARN) \ FUNC(FULL_PATHNAME, WARN) \ @@ -72,6 +74,8 @@ enum fsck_msg_type { FUNC(NULL_SHA1, WARN) \ FUNC(ZERO_PADDED_FILEMODE, WARN) \ FUNC(NUL_IN_COMMIT, WARN) \ + FUNC(SYMLINK_TARGET_LENGTH, WARN) \ + FUNC(SYMLINK_POINTS_TO_GIT_DIR, WARN) \ /* infos (reported as warnings, but ignored by default) */ \ FUNC(BAD_FILEMODE, INFO) \ FUNC(GITMODULES_PARSE, INFO) \ @@ -139,6 +143,8 @@ struct fsck_options { struct oidset gitmodules_done; struct oidset gitattributes_found; struct oidset gitattributes_done; + struct oidset symlink_targets_found; + struct oidset symlink_targets_done; kh_oid_map_t *object_names; }; @@ -148,6 +154,8 @@ struct fsck_options { .gitmodules_done = OIDSET_INIT, \ .gitattributes_found = OIDSET_INIT, \ .gitattributes_done = OIDSET_INIT, \ + .symlink_targets_found = OIDSET_INIT, \ + .symlink_targets_done = OIDSET_INIT, \ .error_func = fsck_error_function \ } #define FSCK_OPTIONS_STRICT { \ @@ -156,6 +164,8 @@ struct fsck_options { .gitmodules_done = OIDSET_INIT, \ .gitattributes_found = OIDSET_INIT, \ .gitattributes_done = OIDSET_INIT, \ + .symlink_targets_found = OIDSET_INIT, \ + .symlink_targets_done = OIDSET_INIT, \ .error_func = fsck_error_function, \ } #define FSCK_OPTIONS_MISSING_GITMODULES { \ @@ -164,6 +174,8 @@ struct fsck_options { .gitmodules_done = OIDSET_INIT, \ .gitattributes_found = OIDSET_INIT, \ .gitattributes_done = OIDSET_INIT, \ + .symlink_targets_found = OIDSET_INIT, \ + .symlink_targets_done = OIDSET_INIT, \ .error_func = fsck_error_cb_print_missing_gitmodules, \ } diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index de0f6d5e7f..5669872bc8 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -1023,4 +1023,41 @@ test_expect_success 'fsck error on gitattributes with excessive size' ' test_cmp expected actual ' +test_expect_success 'fsck warning on symlink target with excessive length' ' + symlink_target=$(printf "pattern %032769d" 1 | git hash-object -w --stdin) && + test_when_finished "remove_object $symlink_target" && + tree=$(printf "120000 blob %s\t%s\n" $symlink_target symlink | git mktree) && + test_when_finished "remove_object $tree" && + cat >expected <<-EOF && + warning in blob $symlink_target: symlinkTargetLength: symlink target too long + EOF + git fsck --no-dangling >actual 2>&1 && + test_cmp expected actual +' + +test_expect_success 'fsck warning on symlink target pointing inside git dir' ' + gitdir=$(printf ".git" | git hash-object -w --stdin) && + ntfs_gitdir=$(printf "GIT~1" | git hash-object -w --stdin) && + hfs_gitdir=$(printf ".${u200c}git" | git hash-object -w --stdin) && + inside_gitdir=$(printf "nested/.git/config" | git hash-object -w --stdin) && + benign_target=$(printf "legit/config" | git hash-object -w --stdin) && + tree=$(printf "120000 blob %s\t%s\n" \ + $benign_target benign_target \ + $gitdir gitdir \ + $hfs_gitdir hfs_gitdir \ + $inside_gitdir inside_gitdir \ + $ntfs_gitdir ntfs_gitdir | + git mktree) && + for o in $gitdir $ntfs_gitdir $hfs_gitdir $inside_gitdir $benign_target $tree + do + test_when_finished "remove_object $o" || return 1 + done && + printf "warning in blob %s: symlinkPointsToGitDir: symlink target points to git dir\n" \ + $gitdir $hfs_gitdir $inside_gitdir $ntfs_gitdir | + sort >expected && + git fsck --no-dangling >actual 2>&1 && + sort actual >actual.sorted && + test_cmp expected actual.sorted +' + test_done From 47b6d90e91835082010da926f6a844d4441c57a6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 20:37:40 +0200 Subject: [PATCH 36/39] Git 2.39.4 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.39.4.txt | 79 +++++++++++++++++++++++++++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.39.4.txt diff --git a/Documentation/RelNotes/2.39.4.txt b/Documentation/RelNotes/2.39.4.txt new file mode 100644 index 0000000000..7f54521fea --- /dev/null +++ b/Documentation/RelNotes/2.39.4.txt @@ -0,0 +1,79 @@ +Git v2.39.4 Release Notes +========================= + +This addresses the security issues CVE-2024-32002, CVE-2024-32004, +CVE-2024-32020 and CVE-2024-32021. + +This release also backports fixes necessary to let the CI builds pass +successfully. + +Fixes since v2.39.3 +------------------- + + * CVE-2024-32002: + + Recursive clones on case-insensitive filesystems that support symbolic + links are susceptible to case confusion that can be exploited to + execute just-cloned code during the clone operation. + + * CVE-2024-32004: + + Repositories can be configured to execute arbitrary code during local + clones. To address this, the ownership checks introduced in v2.30.3 + are now extended to cover cloning local repositories. + + * CVE-2024-32020: + + Local clones may end up hardlinking files into the target repository's + object database when source and target repository reside on the same + disk. If the source repository is owned by a different user, then + those hardlinked files may be rewritten at any point in time by the + untrusted user. + + * CVE-2024-32021: + + When cloning a local source repository that contains symlinks via the + filesystem, Git may create hardlinks to arbitrary user-readable files + on the same filesystem as the target repository in the objects/ + directory. + + * CVE-2024-32465: + + It is supposed to be safe to clone untrusted repositories, even those + unpacked from zip archives or tarballs originating from untrusted + sources, but Git can be tricked to run arbitrary code as part of the + clone. + + * Defense-in-depth: submodule: require the submodule path to contain + directories only. + + * Defense-in-depth: clone: when symbolic links collide with directories, keep + the latter. + + * Defense-in-depth: clone: prevent hooks from running during a clone. + + * Defense-in-depth: core.hooksPath: add some protection while cloning. + + * Defense-in-depth: fsck: warn about symlink pointing inside a gitdir. + + * Various fix-ups on HTTP tests. + + * Test update. + + * HTTP Header redaction code has been adjusted for a newer version of + cURL library that shows its traces differently from earlier + versions. + + * Fix was added to work around a regression in libcURL 8.7.0 (which has + already been fixed in their tip of the tree). + + * Replace macos-12 used at GitHub CI with macos-13. + + * ci(linux-asan/linux-ubsan): let's save some time + + * Tests with LSan from time to time seem to emit harmless message that makes + our tests unnecessarily flakey; we work it around by filtering the + uninteresting output. + + * Update GitHub Actions jobs to avoid warnings against using deprecated + version of Node.js. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index eec953731c..c124ef0b41 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.39.3 +DEF_VER=v2.39.4 LF=' ' diff --git a/RelNotes b/RelNotes index e32fcea009..34ee294000 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.39.3.txt \ No newline at end of file +Documentation/RelNotes/2.39.4.txt \ No newline at end of file From b9b439e0e3a543ddb920e4cf8d3c9d53f730111f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 20:56:02 +0200 Subject: [PATCH 37/39] Git 2.40.2 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.40.2.txt | 7 +++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.40.2.txt diff --git a/Documentation/RelNotes/2.40.2.txt b/Documentation/RelNotes/2.40.2.txt new file mode 100644 index 0000000000..646a2cc3eb --- /dev/null +++ b/Documentation/RelNotes/2.40.2.txt @@ -0,0 +1,7 @@ +Git v2.40.2 Release Notes +========================= + +This release merges up the fix that appears in v2.39.4 to address +the security issues CVE-2024-32002, CVE-2024-32004, CVE-2024-32020, +CVE-2024-32021 and CVE-2024-32465; see the release notes for that +version for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 71425bd821..fc9c36a511 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.40.1 +DEF_VER=v2.40.2 LF=' ' diff --git a/RelNotes b/RelNotes index 829f78a6eb..6d30158d7d 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.40.1.txt \ No newline at end of file +Documentation/RelNotes/2.40.2.txt \ No newline at end of file From 0f158320593bd57fe2c3fe55fbce751e9415ffc2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 21:06:57 +0200 Subject: [PATCH 38/39] Git 2.41.1 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.41.1.txt | 7 +++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.41.1.txt diff --git a/Documentation/RelNotes/2.41.1.txt b/Documentation/RelNotes/2.41.1.txt new file mode 100644 index 0000000000..9fb4c218b2 --- /dev/null +++ b/Documentation/RelNotes/2.41.1.txt @@ -0,0 +1,7 @@ +Git v2.41.1 Release Notes +========================= + +This release merges up the fix that appears in v2.39.4 and v2.40.2 +to address the security issues CVE-2024-32002, CVE-2024-32004, +CVE-2024-32020, CVE-2024-32021 and CVE-2024-32465; see the release +notes for these versions for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index b37f72a552..a142c55c85 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.41.0 +DEF_VER=v2.41.1 LF=' ' diff --git a/RelNotes b/RelNotes index 4da73c9a6d..41c25a6bc1 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.41.0.txt \ No newline at end of file +Documentation/RelNotes/2.41.1.txt \ No newline at end of file From babb4e5d7107ba730beff8d224e4bcf065533e0b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 10 Apr 2024 21:51:47 +0200 Subject: [PATCH 39/39] Git 2.42.2 Signed-off-by: Johannes Schindelin --- Documentation/RelNotes/2.42.2.txt | 7 +++++++ GIT-VERSION-GEN | 2 +- RelNotes | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 Documentation/RelNotes/2.42.2.txt diff --git a/Documentation/RelNotes/2.42.2.txt b/Documentation/RelNotes/2.42.2.txt new file mode 100644 index 0000000000..dbf761a01d --- /dev/null +++ b/Documentation/RelNotes/2.42.2.txt @@ -0,0 +1,7 @@ +Git v2.42.2 Release Notes +========================= + +This release merges up the fix that appears in v2.39.4, v2.40.2 +and v2.41.1 to address the security issues CVE-2024-32002, +CVE-2024-32004, CVE-2024-32020, CVE-2024-32021 and CVE-2024-32465; +see the release notes for these versions for details. diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN index 226c8c19ff..4407ea29da 100755 --- a/GIT-VERSION-GEN +++ b/GIT-VERSION-GEN @@ -1,7 +1,7 @@ #!/bin/sh GVF=GIT-VERSION-FILE -DEF_VER=v2.42.1 +DEF_VER=v2.42.2 LF=' ' diff --git a/RelNotes b/RelNotes index 234837ed95..fd58bd7691 120000 --- a/RelNotes +++ b/RelNotes @@ -1 +1 @@ -Documentation/RelNotes/2.42.1.txt \ No newline at end of file +Documentation/RelNotes/2.42.2.txt \ No newline at end of file