git/backport-CVE-2023-23946-apply-fix-writing-behind-newly-created-symbolic-link.patch

From fade728df1221598f42d391cf377e9e84a32053f Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 2 Feb 2023 11:54:34 +0100
Subject: [PATCH] apply: fix writing behind newly created symbolic links

When writing files git-apply(1) initially makes sure that none of the
files it is about to create are behind a symlink:

```
 $ git init repo
 Initialized empty Git repository in /tmp/repo/.git/
 $ cd repo/
 $ ln -s dir symlink
 $ git apply - <<EOF
 diff --git a/symlink/file b/symlink/file
 new file mode 100644
 index 0000000..e69de29
 EOF
 error: affected file 'symlink/file' is beyond a symbolic link
```

This safety mechanism is crucial to ensure that we don't write outside
of the repository's working directory. It can be fooled though when the
patch that is being applied creates the symbolic link in the first
place, which can lead to writing files in arbitrary locations.

Fix this by checking whether the path we're about to create is
beyond a symlink or not. Tightening these checks like this should be
fine as we already have these precautions in Git as explained
above. Ideally, we should update the check we do up-front before
starting to reflect the computed changes to the working tree so that
we catch this case as well, but as part of embargoed security work,
adding an equivalent check just before we try to write out a file
should serve us well as a reasonable first step.

Digging back into history shows that this vulnerability has existed
since at least Git v2.9.0. As Git v2.8.0 and older don't build on my
system anymore I cannot tell whether older versions are affected, as
well.

Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 apply.c                  | 27 ++++++++++++++
 t/t4115-apply-symlink.sh | 81 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/apply.c b/apply.c
index 668b16e989..d80382c940 100644
--- a/apply.c
+++ b/apply.c
@@ -4400,6 +4400,33 @@ static int create_one_file(struct apply_state *state,
 	if (state->cached)
 		return 0;
 
+	/*
+	 * We already try to detect whether files are beyond a symlink in our
+	 * up-front checks. But in the case where symlinks are created by any
+	 * of the intermediate hunks it can happen that our up-front checks
+	 * didn't yet see the symlink, but at the point of arriving here there
+	 * in fact is one. We thus repeat the check for symlinks here.
+	 *
+	 * Note that this does not make the up-front check obsolete as the
+	 * failure mode is different:
+	 *
+	 * - The up-front checks cause us to abort before we have written
+	 *   anything into the working directory. So when we exit this way the
+	 *   working directory remains clean.
+	 *
+	 * - The checks here happen in the middle of the action where we have
+	 *   already started to apply the patch. The end result will be a dirty
+	 *   working directory.
+	 *
+	 * Ideally, we should update the up-front checks to catch what would
+	 * happen when we apply the patch before we damage the working tree.
+	 * We have all the information necessary to do so.  But for now, as a
+	 * part of embargoed security work, having this check would serve as a
+	 * reasonable first step.
+	 */
+	if (path_is_beyond_symlink(state, path))
+		return error(_("affected file '%s' is beyond a symbolic link"), path);
+
 	res = try_create_file(state, path, mode, buf, size);
 	if (res < 0)
 		return -1;
diff --git a/t/t4115-apply-symlink.sh b/t/t4115-apply-symlink.sh
index 872fcda6cb..1acb7b2582 100755
--- a/t/t4115-apply-symlink.sh
+++ b/t/t4115-apply-symlink.sh
@@ -44,4 +44,85 @@ test_expect_success 'apply --index symlink patch' '
 
 '
 
+test_expect_success 'symlink setup' '
+	ln -s .git symlink &&
+	git add symlink &&
+	git commit -m "add symlink"
+'
+
+test_expect_success SYMLINKS 'symlink escape when creating new files' '
+	test_when_finished "git reset --hard && git clean -dfx" &&
+
+	cat >patch <<-EOF &&
+	diff --git a/symlink b/renamed-symlink
+	similarity index 100%
+	rename from symlink
+	rename to renamed-symlink
+	--
+	diff --git /dev/null b/renamed-symlink/create-me
+	new file mode 100644
+	index 0000000..039727e
+	--- /dev/null
+	+++ b/renamed-symlink/create-me
+	@@ -0,0 +1,1 @@
+	+busted
+	EOF
+
+	test_must_fail git apply patch 2>stderr &&
+	cat >expected_stderr <<-EOF &&
+	error: affected file ${SQ}renamed-symlink/create-me${SQ} is beyond a symbolic link
+	EOF
+	test_cmp expected_stderr stderr &&
+	! test_path_exists .git/create-me
+'
+
+test_expect_success SYMLINKS 'symlink escape when modifying file' '
+	test_when_finished "git reset --hard && git clean -dfx" &&
+	touch .git/modify-me &&
+
+	cat >patch <<-EOF &&
+	diff --git a/symlink b/renamed-symlink
+	similarity index 100%
+	rename from symlink
+	rename to renamed-symlink
+	--
+	diff --git a/renamed-symlink/modify-me b/renamed-symlink/modify-me
+	index 1111111..2222222 100644
+	--- a/renamed-symlink/modify-me
+	+++ b/renamed-symlink/modify-me
+	@@ -0,0 +1,1 @@
+	+busted
+	EOF
+
+	test_must_fail git apply patch 2>stderr &&
+	cat >expected_stderr <<-EOF &&
+	error: renamed-symlink/modify-me: No such file or directory
+	EOF
+	test_cmp expected_stderr stderr &&
+	test_must_be_empty .git/modify-me
+'
+
+test_expect_success SYMLINKS 'symlink escape when deleting file' '
+	test_when_finished "git reset --hard && git clean -dfx && rm .git/delete-me" &&
+	touch .git/delete-me &&
+
+	cat >patch <<-EOF &&
+	diff --git a/symlink b/renamed-symlink
+	similarity index 100%
+	rename from symlink
+	rename to renamed-symlink
+	--
+	diff --git a/renamed-symlink/delete-me b/renamed-symlink/delete-me
+	deleted file mode 100644
+	index 1111111..0000000 100644
+	EOF
+
+	test_must_fail git apply patch 2>stderr &&
+	cat >expected_stderr <<-EOF &&
+	error: renamed-symlink/delete-me: No such file or directory
+	EOF
+	test_cmp expected_stderr stderr &&
+	test_path_is_file .git/delete-me
+'
+
 test_done
-- 
2.27.0