202 lines
8.6 KiB
Diff
202 lines
8.6 KiB
Diff
From 7b70e9efb18c2cc3f219af399bd384c5801ba1d7 Mon Sep 17 00:00:00 2001
|
|
From: Jeff King <peff@peff.net>
|
|
Date: Tue, 16 Apr 2024 04:35:33 -0400
|
|
Subject: [PATCH] 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 <peff@peff.net>
|
|
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
|
---
|
|
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 b656b4756..fc4c62d7b 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 25b69da2b..f446ff04f 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");
|
|
disable_replace_refs();
|
|
+ /* 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 faa761294..550a38f75 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 eb3360dbc..b3d6ddc4b 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
|
|
--
|
|
2.33.0
|
|
|