packages/git
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix CVE-2022-39253 CVE-2022-39260

Browse Source
This commit is contained in:
fly_fzc 2022-10-21 15:15:54 +08:00
parent e4a1345042
commit 0f87ea5967
5 changed files with 500 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

194
backport-CVE-2022-39253-builtin-clone.c-disallow-local-clones-with-symlinks.patch Normal file
View File

@ -0,0 +1,194 @@
From 6f054f9fb3a501c35b55c65e547a244f14c38d56 Mon Sep 17 00:00:00 2001
From: Taylor Blau <me@ttaylorr.com>
Date: Thu, 28 Jul 2022 17:35:17 -0400
Subject: [PATCH] builtin/clone.c: disallow `--local` clones with symlinks
When cloning a repository with `--local`, Git relies on either making a
hardlink or copy to every file in the "objects" directory of the source
repository. This is done through the callpath `cmd_clone()` ->
`clone_local()` -> `copy_or_link_directory()`.
The way this optimization works is by enumerating every file and
directory recursively in the source repository's `$GIT_DIR/objects`
directory, and then either making a copy or hardlink of each file. The
only exception to this rule is when copying the "alternates" file, in
which case paths are rewritten to be absolute before writing a new
"alternates" file in the destination repo.
One quirk of this implementation is that it dereferences symlinks when
cloning. This behavior was most recently modified in 36596fd2df (clone:
better handle symlinked files at .git/objects/, 2019-07-10), which
attempted to support `--local` clones of repositories with symlinks in
their objects directory in a platform-independent way.
Unfortunately, this behavior of dereferencing symlinks (that is,
creating a hardlink or copy of the source's link target in the
destination repository) can be used as a component in attacking a
victim by inadvertently exposing the contents of file stored outside of
the repository.
Take, for example, a repository that stores a Dockerfile and is used to
build Docker images. When building an image, Docker copies the directory
contents into the VM, and then instructs the VM to execute the
Dockerfile at the root of the copied directory. This protects against
directory traversal attacks by copying symbolic links as-is without
dereferencing them.
That is, if a user has a symlink pointing at their private key material
(where the symlink is present in the same directory as the Dockerfile,
but the key itself is present outside of that directory), the key is
unreadable to a Docker image, since the link will appear broken from the
container's point of view.
This behavior enables an attack whereby a victim is convinced to clone a
repository containing an embedded submodule (with a URL like
"file:///proc/self/cwd/path/to/submodule") which has a symlink pointing
at a path containing sensitive information on the victim's machine. If a
user is tricked into doing this, the contents at the destination of
those symbolic links are exposed to the Docker image at runtime.
One approach to preventing this behavior is to recreate symlinks in the
destination repository. But this is problematic, since symlinking the
objects directory are not well-supported. (One potential problem is that
when sharing, e.g. a "pack" directory via symlinks, different writers
performing garbage collection may consider different sets of objects to
be reachable, enabling a situation whereby garbage collecting one
repository may remove reachable objects in another repository).
Instead, prohibit the local clone optimization when any symlinks are
present in the `$GIT_DIR/objects` directory of the source repository.
Users may clone the repository again by prepending the "file://" scheme
to their clone URL, or by adding the `--no-local` option to their `git
clone` invocation.
The directory iterator used by `copy_or_link_directory()` must no longer
dereference symlinks (i.e., it *must* call `lstat()` instead of `stat()`
in order to discover whether or not there are symlinks present). This has
no bearing on the overall behavior, since we will immediately `die()` on
encounter a symlink.
Note that t5604.33 suggests that we do support local clones with
symbolic links in the source repository's objects directory, but this
was likely unintentional, or at least did not take into consideration
the problem with sharing parts of the objects directory with symbolic
links at the time. Update this test to reflect which options are and
aren't supported.
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/clone.c | 8 +++---
t/t5604-clone-reference.sh | 50 ++++++++++++++------------------------
2 files changed, 23 insertions(+), 35 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index e335734b4c..e626073b1f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -420,13 +420,11 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
int src_len, dest_len;
struct dir_iterator *iter;
int iter_status;
- unsigned int flags;
struct strbuf realpath = STRBUF_INIT;
mkdir_if_missing(dest->buf, 0777);
- flags = DIR_ITERATOR_PEDANTIC | DIR_ITERATOR_FOLLOW_SYMLINKS;
- iter = dir_iterator_begin(src->buf, flags);
+ iter = dir_iterator_begin(src->buf, DIR_ITERATOR_PEDANTIC);
if (!iter)
die_errno(_("failed to start iterator over '%s'"), src->buf);
@@ -442,6 +440,10 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
strbuf_setlen(dest, dest_len);
strbuf_addstr(dest, iter->relative_path);
+ if (S_ISLNK(iter->st.st_mode))
+ die(_("symlink '%s' exists, refusing to clone with --local"),
+ iter->relative_path);
+
if (S_ISDIR(iter->st.st_mode)) {
mkdir_if_missing(dest->buf, 0777);
continue;
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 2f7be23044..9d32f1c4a4 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -300,8 +300,6 @@ test_expect_success SYMLINKS 'setup repo with manually symlinked or unknown file
ln -s ../an-object $obj &&
cd ../ &&
- find . -type f | sort >../../../T.objects-files.raw &&
- find . -type l | sort >../../../T.objects-symlinks.raw &&
echo unknown_content >unknown_file
) &&
git -C T fsck &&
@@ -310,19 +308,27 @@ test_expect_success SYMLINKS 'setup repo with manually symlinked or unknown file
test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at objects/' '
- for option in --local --no-hardlinks --shared --dissociate
+ # None of these options work when cloning locally, since T has
+ # symlinks in its `$GIT_DIR/objects` directory
+ for option in --local --no-hardlinks --dissociate
do
- git clone $option T T$option || return 1 &&
- git -C T$option fsck || return 1 &&
- git -C T$option rev-list --all --objects >T$option.objects &&
- test_cmp T.objects T$option.objects &&
- (
- cd T$option/.git/objects &&
- find . -type f | sort >../../../T$option.objects-files.raw &&
- find . -type l | sort >../../../T$option.objects-symlinks.raw
- )
+ test_must_fail git clone $option T T$option 2>err || return 1 &&
+ test_i18ngrep "symlink.*exists" err || return 1
done &&
+ # But `--shared` clones should still work, even when specifying
+ # a local path *and* that repository has symlinks present in its
+ # `$GIT_DIR/objects` directory.
+ git clone --shared T T--shared &&
+ git -C T--shared fsck &&
+ git -C T--shared rev-list --all --objects >T--shared.objects &&
+ test_cmp T.objects T--shared.objects &&
+ (
+ cd T--shared/.git/objects &&
+ find . -type f | sort >../../../T--shared.objects-files.raw &&
+ find . -type l | sort >../../../T--shared.objects-symlinks.raw
+ ) &&
+
for raw in $(ls T*.raw)
do
sed -e "s!/../!/Y/!; s![0-9a-f]\{38,\}!Z!" -e "/commit-graph/d" \
@@ -330,26 +336,6 @@ test_expect_success SYMLINKS 'clone repo with symlinked or unknown files at obje
sort $raw.de-sha-1 >$raw.de-sha || return 1
done &&
- cat >expected-files <<-EOF &&
- ./Y/Z
- ./Y/Z
- ./Y/Z
- ./a-loose-dir/Z
- ./an-object
- ./info/packs
- ./pack/pack-Z.idx
- ./pack/pack-Z.pack
- ./packs/pack-Z.idx
- ./packs/pack-Z.pack
- ./unknown_file
- EOF
-
- for option in --local --no-hardlinks --dissociate
- do
- test_cmp expected-files T$option.objects-files.raw.de-sha || return 1 &&
- test_must_be_empty T$option.objects-symlinks.raw.de-sha || return 1
- done &&
-
echo ./info/alternates >expected-files &&
test_cmp expected-files T--shared.objects-files.raw &&
test_must_be_empty T--shared.objects-symlinks.raw
--
2.27.0

85
backport-CVE-2022-39260-alias.c-reject-too-long-cmdline-strings-in-split_cmd.patch Normal file
View File

@ -0,0 +1,85 @@
From 0ca6ead81edd4fb1984b69aae87c1189e3025530 Mon Sep 17 00:00:00 2001
From: Kevin Backhouse <kevinbackhouse@github.com>
Date: Wed, 28 Sep 2022 18:53:32 -0400
Subject: [PATCH] alias.c: reject too-long cmdline strings in split_cmdline()
This function improperly uses an int to represent the number of entries
in the resulting argument array. This allows a malicious actor to
intentionally overflow the return value, leading to arbitrary heap
writes.
Because the resulting argv array is typically passed to execv(), it may
be possible to leverage this attack to gain remote code execution on a
victim machine. This was almost certainly the case for certain
configurations of git-shell until the previous commit limited the size
of input it would accept. Other calls to split_cmdline() are typically
limited by the size of argv the OS is willing to hand us, so are
similarly protected.
So this is not strictly fixing a known vulnerability, but is a hardening
of the function that is worth doing to protect against possible unknown
vulnerabilities.
One approach to fixing this would be modifying the signature of
`split_cmdline()` to look something like:
int split_cmdline(char *cmdline, const char ***argv, size_t *argc);
Where the return value of `split_cmdline()` is negative for errors, and
zero otherwise. If non-NULL, the `*argc` pointer is modified to contain
the size of the `**argv` array.
But this implies an absurdly large `argv` array, which more than likely
larger than the system's argument limit. So even if split_cmdline()
allowed this, it would fail immediately afterwards when we called
execv(). So instead of converting all of `split_cmdline()`'s callers to
work with `size_t` types in this patch, instead pursue the minimal fix
here to prevent ever returning an array with more than INT_MAX entries
in it.
Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
alias.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/alias.c b/alias.c
index c471538020..00abde0817 100644
--- a/alias.c
+++ b/alias.c
@@ -46,14 +46,16 @@ void list_aliases(struct string_list *list)
#define SPLIT_CMDLINE_BAD_ENDING 1
#define SPLIT_CMDLINE_UNCLOSED_QUOTE 2
+#define SPLIT_CMDLINE_ARGC_OVERFLOW 3
static const char *split_cmdline_errors[] = {
N_("cmdline ends with \\"),
- N_("unclosed quote")
+ N_("unclosed quote"),
+ N_("too many arguments"),
};
int split_cmdline(char *cmdline, const char ***argv)
{
- int src, dst, count = 0, size = 16;
+ size_t src, dst, count = 0, size = 16;
char quoted = 0;
ALLOC_ARRAY(*argv, size);
@@ -96,6 +98,11 @@ int split_cmdline(char *cmdline, const char ***argv)
return -SPLIT_CMDLINE_UNCLOSED_QUOTE;
}
+ if (count >= INT_MAX) {
+ FREE_AND_NULL(*argv);
+ return -SPLIT_CMDLINE_ARGC_OVERFLOW;
+ }
+
ALLOC_GROW(*argv, count + 1, size);
(*argv)[count] = NULL;
--
2.27.0

57
backport-CVE-2022-39260-shell-add-basic-tests.patch Normal file
View File

@ -0,0 +1,57 @@
From 32696a4cbe90929ae79ea442f5102c513ce3dfaa Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Wed, 28 Sep 2022 18:50:36 -0400
Subject: [PATCH] shell: add basic tests
We have no tests of even basic functionality of git-shell. Let's add a
couple of obvious ones. This will serve as a framework for adding tests
for new things we fix, as well as making sure we don't screw anything up
too badly while doing so.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
t/t9850-shell.sh | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100755 t/t9850-shell.sh
diff --git a/t/t9850-shell.sh b/t/t9850-shell.sh
new file mode 100755
index 0000000000..2af476c3af
--- /dev/null
+++ b/t/t9850-shell.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+
+test_description='git shell tests'
+. ./test-lib.sh
+
+test_expect_success 'shell allows upload-pack' '
+ printf 0000 >input &&
+ git upload-pack . <input >expect &&
+ git shell -c "git-upload-pack $SQ.$SQ" <input >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'shell forbids other commands' '
+ test_must_fail git shell -c "git config foo.bar baz"
+'
+
+test_expect_success 'shell forbids interactive use by default' '
+ test_must_fail git shell
+'
+
+test_expect_success 'shell allows interactive command' '
+ mkdir git-shell-commands &&
+ write_script git-shell-commands/ping <<-\EOF &&
+ echo pong
+ EOF
+ echo pong >expect &&
+ echo ping | git shell >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.27.0

153
backport-CVE-2022-39260-shell-limit-size-of-interactive-commands.patch Normal file
View File

@ -0,0 +1,153 @@
From 71ad7fe1bcec2a115bd0ab187240348358aa7f21 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Wed, 28 Sep 2022 18:52:48 -0400
Subject: [PATCH] shell: limit size of interactive commands
When git-shell is run in interactive mode (which must be enabled by
creating $HOME/git-shell-commands), it reads commands from stdin, one
per line, and executes them.
We read the commands with git_read_line_interactively(), which uses a
strbuf under the hood. That means we'll accept an input of arbitrary
size (limited only by how much heap we can allocate). That creates two
problems:
- the rest of the code is not prepared to handle large inputs. The
most serious issue here is that split_cmdline() uses "int" for most
of its types, which can lead to integer overflow and out-of-bounds
array reads and writes. But even with that fixed, we assume that we
can feed the command name to snprintf() (via xstrfmt()), which is
stuck for historical reasons using "int", and causes it to fail (and
even trigger a BUG() call).
- since the point of git-shell is to take input from untrusted or
semi-trusted clients, it's a mild denial-of-service. We'll allocate
as many bytes as the client sends us (actually twice as many, since
we immediately duplicate the buffer).
We can fix both by just limiting the amount of per-command input we're
willing to receive.
We should also fix split_cmdline(), of course, which is an accident
waiting to happen, but that can come on top. Most calls to
split_cmdline(), including the other one in git-shell, are OK because
they are reading from an OS-provided argv, which is limited in practice.
This patch should eliminate the immediate vulnerabilities.
I picked 4MB as an arbitrary limit. It's big enough that nobody should
ever run into it in practice (since the point is to run the commands via
exec, we're subject to OS limits which are typically much lower). But
it's small enough that allocating it isn't that big a deal.
The code is mostly just swapping out fgets() for the strbuf call, but we
have to add a few niceties like flushing and trimming line endings. We
could simplify things further by putting the buffer on the stack, but
4MB is probably a bit much there. Note that we'll _always_ allocate 4MB,
which for normal, non-malicious requests is more than we would before
this patch. But on the other hand, other git programs are happy to use
96MB for a delta cache. And since we'd never touch most of those pages,
on a lazy-allocating OS like Linux they won't even get allocated to
actual RAM.
The ideal would be a version of strbuf_getline() that accepted a maximum
value. But for a minimal vulnerability fix, let's keep things localized
and simple. We can always refactor further on top.
The included test fails in an obvious way with ASan or UBSan (which
notice the integer overflow and out-of-bounds reads). Without them, it
fails in a less obvious way: we may segfault, or we may try to xstrfmt()
a long string, leading to a BUG(). Either way, it fails reliably before
this patch, and passes with it. Note that we don't need an EXPENSIVE
prereq on it. It does take 10-15s to fail before this patch, but with
the new limit, we fail almost immediately (and the perl process
generating 2GB of data exits via SIGPIPE).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
shell.c | 34 ++++++++++++++++++++++++++++++----
t/t9850-shell.sh | 6 ++++++
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/shell.c b/shell.c
index cef7ffdc9e..02cfd9627f 100644
--- a/shell.c
+++ b/shell.c
@@ -47,6 +47,8 @@ static void cd_to_homedir(void)
die("could not chdir to user's home directory");
}
+#define MAX_INTERACTIVE_COMMAND (4*1024*1024)
+
static void run_shell(void)
{
int done = 0;
@@ -67,22 +69,46 @@ static void run_shell(void)
run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
do {
- struct strbuf line = STRBUF_INIT;
const char *prog;
char *full_cmd;
char *rawargs;
+ size_t len;
char *split_args;
const char **argv;
int code;
int count;
fprintf(stderr, "git> ");
- if (git_read_line_interactively(&line) == EOF) {
+
+ /*
+ * Avoid using a strbuf or git_read_line_interactively() here.
+ * We don't want to allocate arbitrary amounts of memory on
+ * behalf of a possibly untrusted client, and we're subject to
+ * OS limits on command length anyway.
+ */
+ fflush(stdout);
+ rawargs = xmalloc(MAX_INTERACTIVE_COMMAND);
+ if (!fgets(rawargs, MAX_INTERACTIVE_COMMAND, stdin)) {
fprintf(stderr, "\n");
- strbuf_release(&line);
+ free(rawargs);
break;
}
- rawargs = strbuf_detach(&line, NULL);
+ len = strlen(rawargs);
+
+ /*
+ * If we truncated due to our input buffer size, reject the
+ * command. That's better than running bogus input, and
+ * there's a good chance it's just malicious garbage anyway.
+ */
+ if (len >= MAX_INTERACTIVE_COMMAND - 1)
+ die("invalid command format: input too long");
+
+ if (len > 0 && rawargs[len - 1] == '\n') {
+ if (--len > 0 && rawargs[len - 1] == '\r')
+ --len;
+ rawargs[len] = '\0';
+ }
+
split_args = xstrdup(rawargs);
count = split_cmdline(split_args, &argv);
if (count < 0) {
diff --git a/t/t9850-shell.sh b/t/t9850-shell.sh
index 2af476c3af..cfc71c3bd4 100755
--- a/t/t9850-shell.sh
+++ b/t/t9850-shell.sh
@@ -28,4 +28,10 @@ test_expect_success 'shell allows interactive command' '
test_cmp expect actual
'
+test_expect_success 'shell complains of overlong commands' '
+ perl -e "print \"a\" x 2**12 for (0..2**19)" |
+ test_must_fail git shell 2>err &&
+ grep "too long" err
+'
+
test_done
--
2.27.0

12
git.spec
View File

@ -1,7 +1,7 @@
%global gitexecdir %{_libexecdir}/git-core
Name: git
Version: 2.33.0
Release: 4
Release: 5
Summary: A popular and widely used Version Control System
License: GPLv2+ or LGPLv2.1
URL: https://git-scm.com/
@ -20,6 +20,10 @@ Patch4: backport-t0033-add-tests-for-safe.directory.patch
Patch5: backport-0005-CVE-2022-24765.patch
Patch6: backport-0006-CVE-2022-24765.patch
Patch7: backport-CVE-2022-29187.patch
Patch8: backport-CVE-2022-39253-builtin-clone.c-disallow-local-clones-with-symlinks.patch
Patch9: backport-CVE-2022-39260-shell-add-basic-tests.patch
Patch10: backport-CVE-2022-39260-shell-limit-size-of-interactive-commands.patch
Patch11: backport-CVE-2022-39260-alias.c-reject-too-long-cmdline-strings-in-split_cmd.patch
BuildRequires: gcc gettext
BuildRequires: openssl-devel libcurl-devel expat-devel systemd asciidoc xmlto glib2-devel libsecret-devel pcre-devel desktop-file-utils
@ -300,6 +304,12 @@ make %{?_smp_mflags} test
%{_mandir}/man7/git*.7.*
%changelog
* Fri Oct 21 2022 fuanan <fuanan3@h-partners.com> - 2.33.0-5
- Type:CVE
- ID:CVE-2022-39253 CVE-2022-39260
- SUG:NA
- DESC:Fix CVE-2022-39253 CVE-2022-39260
* Mon Sep 19 2022 fuanan <fuanan3@h-partners.com> - 2.33.0-4
- add subpackage git-core