[PATCH] diff-no-index: fix stdin path in subdirectory

[PATCH] diff-no-index: fix stdin path in subdirectory

[Gregoire Geis]

`git diff --no-index` tried to normalize paths before printing them,
skipping a prefix before reading a string. However, `-` doesn't start
with this prefix, leading to a buffer overflow:

$ make SANITIZE=address
$ mkdir a && cd a
$ echo a | ../git diff --no-index /dev/null -
=================================================================
==14536==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b000001f92 at pc 0x000102612a30 bp 0x00016db386d0 sp 0x00016db386c8
READ of size 1 at 0x60b000001f92 thread T0
    #0 0x102612a2c in diff_flush_patch diff.c:6154
    #1 0x10260dbfc in diff_flush diff.c:6829
    #2 0x1025f218c in diff_no_index diff-no-index.c:422
    #3 0x102356338 in cmd_diff diff.c:501
    #4 0x1022cc780 in run_builtin git.c:480
    #5 0x1022c9bec in handle_builtin git.c:746
    #6 0x1022c8670 in cmd_main git.c:953
    #7 0x1024fe230 in main common-main.c:9

Signed-off-by: Gregoire Geis <opensource@gregoirege.is>
---
Hello!

I noticed that running `git diff --no-index /dev/null -` in a
subdirectory leads to garbage file names (instead of `a/-` and `b/-`).

Valgrind and ASAN confirmed that this is a heap buffer overflow. I
fixed it by checking against "-" when writing the file names in the
patch, which leads to results similar to using an absolute path (e.g.
`a/dev/stdin`, `a/-`, no matter the working directory). "-" is a
special input for `git diff --no-index`, so I don't think we need any
kind of normalization before this check.

I added a test which currently fails on `master`. I chose a
subdirectory `aa/bb/cc` to make sure that the path is long enough for
garbage to appear even if git is compiled without ASAN.

 diff.c                        |  4 ++--
 t/meson.build                 |  1 +
 t/t4072-diff-no-index-dash.sh | 17 +++++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)
 create mode 100755 t/t4072-diff-no-index-dash.sh

diff --git a/diff.c b/diff.c
index dca87e1..88e93e5 100644
--- a/diff.c
+++ b/diff.c
@@ -4626,12 +4626,12 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is
 static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
 {
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
-	if (*namep && !is_absolute_path(*namep)) {
+	if (*namep && !is_absolute_path(*namep) && strcmp(*namep, "-")) {
 		*namep += prefix_length;
 		if (**namep == '/')
 			++*namep;
 	}
-	if (*otherp && !is_absolute_path(*otherp)) {
+	if (*otherp && !is_absolute_path(*otherp) && strcmp(*otherp, "-")) {
 		*otherp += prefix_length;
 		if (**otherp == '/')
 			++*otherp;
diff --git a/t/meson.build b/t/meson.build
index bbeba1a..9042f3f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -486,6 +486,7 @@ integration_tests = [
   't4069-remerge-diff.sh',
   't4070-diff-pairs.sh',
   't4071-diff-minimal.sh',
+  't4072-diff-no-index-dash.sh',
   't4100-apply-stat.sh',
   't4101-apply-nonl.sh',
   't4102-apply-rename.sh',
diff --git a/t/t4072-diff-no-index-dash.sh b/t/t4072-diff-no-index-dash.sh
new file mode 100755
index 0000000..35170a4
--- /dev/null
+++ b/t/t4072-diff-no-index-dash.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='`git diff file -` does not crash'
+
+. ./test-lib.sh
+
+test_expect_success '`git diff --no-index /dev/null -` works in root' '
+  echo foo | git diff --no-index /dev/null - | grep "a/- b/-"
+'
+
+test_expect_success '`git diff --no-index /dev/null -` works in subdirectory' '
+  mkdir -p aa/bb/cc &&
+  cd aa/bb/cc &&
+  echo foo | git diff --no-index /dev/null - | grep "a/- b/-"
+'
+
+test_done

base-commit: 64cbe5e2e8a7b0f92c780b210e602496bd5cad0f
-- 
2.48.1

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

[Junio C Hamano]

Gregoire Geis <opensource@gregoirege.is> writes:

> `git diff --no-index` tried to normalize paths before printing them,
> skipping a prefix before reading a string. However, `-` doesn't start
> with this prefix, leading to a buffer overflow:

Thanks for a report, but ...

> I noticed that running `git diff --no-index /dev/null -` in a
> subdirectory leads to garbage file names (instead of `a/-` and `b/-`).

... there fundamentally is something wrong here.  Why does the code
even need to "strip" the "prefix" in the first place?  Once the user
said "git diff --no-index", there is no concept of "subdirectory"
anymore.  The user just told Git to pretend as if it is operating
outside any Git-controlled working tree, so whereever you are is
your current directory, and you would never be "in a subdirectory"
of anything to begin with.  But apparently diff_no_index() code is
pretending as if it is part of Git proper and doing the prepend and
strip the prefix dance.

Would something more direct like the attached patch work?

--- >8 ---
Subject: diff: --no-index should ignore the worktree

The act of giving "--no-index" tells Git to pretend that the current
directory is not under control of any Git index or repository, so
even when you happen to be in a Git controlled working tree, where
in that working tree should not matter.

But the start-up sequence tries to discover the top of the working
tree and chdir(2)'s there, even before Git passes control to the
subcommand being run.  When diff_no_index() starts running, it
starts at a wrong (from the end-user's point of view who thinks "git
diff --no-index" is merely a better version of GNU diff) directory,
and the original directory the user started the command is at
"prefix".

Because the paths given from argv[] have already been adjusted to
account for this path shuffling by prepending the prefix, and
showing the resulting path by stripping the prefix, the effect of
these nonsense operations (nonsense in the context of "--no-index",
that is) is usually not observable.

Except for special cases like "-", where it is not preprocessed by
prepending the prefix.

Instead of papering over by adding more special cases only to cater
to the no-index codepath in the generic code, drive the diff
machinery more faithfully to what is going on.  If the user started
"git diff --no-index" in directory X/Y/Z in a working tree
controlled by Git, and the start up sequence of Git chdir(2)'ed up
to directory X and left Y/Z in the prefix, revert the effect of the
start up sequence by chdir'ing back to Y/Z and emptying the prefix.

Reported-by: Gregoire Geis <opensource@gregoirege.is>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * After we worked on our own "diff" enhancements like rename
   detecion, colored output, etc., we thought it may make the world
   a better place if we gave non-Git using people the benefit of our
   "diff" implementation.  The correct thing to back then do may
   have been to give patches to integrate our extended diff code to
   diff implementations of other people (like GNU), but we took the
   lazy route to bolt the code to slurp two sets of paths to compare
   out of filesystem files on to our own diff implementation.  After
   fixing many fallouts from this impedance-mismatched code, it is
   not surprising that a bug like this still remained.

 builtin/diff.c           | 14 ++++++++++++++
 t/t4053-diff-no-index.sh | 17 +++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git c/builtin/diff.c w/builtin/diff.c
index 9a89e25a98..3eb4cbb057 100644
--- c/builtin/diff.c
+++ w/builtin/diff.c
@@ -487,6 +487,20 @@ int cmd_diff(int argc,
 
 	init_diff_ui_defaults();
 	repo_config(the_repository, git_diff_ui_config, NULL);
+
+	/*
+	 * If we are ignoring the fact that our current directory may
+	 * be part of a working tree controlled by a Git repository to
+	 * pretend to be a "better GNU diff", we should undo the
+	 * effect of the setup code that did a chdir() to the top of
+	 * the working tree.  Where we came from is recorded in the
+	 * prefix.
+	 */
+	if (no_index && prefix) {
+		chdir(prefix);
+		prefix = NULL;
+	}
+
 	prefix = precompose_argv_prefix(argc, argv, prefix);
 
 	repo_init_revisions(the_repository, &rev, prefix);
diff --git c/t/t4053-diff-no-index.sh w/t/t4053-diff-no-index.sh
index 01db9243ab..44b4b13f5d 100755
--- c/t/t4053-diff-no-index.sh
+++ w/t/t4053-diff-no-index.sh
@@ -26,6 +26,23 @@ test_expect_success 'git diff --no-index directories' '
 	test_line_count = 14 cnt
 '
 
+test_expect_success 'git diff --no-index with -' '
+	cat >expect <<-\EOF &&
+	diff --git a/- b/-
+	new file mode 100644
+	--- /dev/null
+	+++ b/-
+	@@ -0,0 +1 @@
+	+frotz
+	EOF
+	(
+		cd a &&
+		echo frotz |
+		test_expect_code 1 git diff --no-index /dev/null - >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git diff --no-index relative path outside repo' '
 	(
 		cd repo &&

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

[opensource]

Junio C Hamano <gitster@pobox.com> wrote:

> ... there fundamentally is something wrong here.

Thank you for the thorough response and for explaining the
reasoning / implementation of the more correct patch.

> Would something more direct like the attached patch work?

I am not familiar with the inner workings of Git at all, but I
cannot think of any problem with this approach. This works for me!

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

[Junio C Hamano]

opensource@gregoirege.is writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>
>> ... there fundamentally is something wrong here.
>
> Thank you for the thorough response and for explaining the
> reasoning / implementation of the more correct patch.
>
>> Would something more direct like the attached patch work?
>
> I am not familiar with the inner workings of Git at all, but I
> cannot think of any problem with this approach. This works for me!

Ah, what I meant was that I did not test the patch, so I did not
know if it solves the problem you observed in your environment and
with your development tools, hence I was asking you to apply it and
test, like you did when you originally noticed the problem ;-)

Thanks.

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

[opensource]

Junio C Hamano <gitster@pobox.com> wrote:

> Ah, what I meant was that I did not test the patch, so I did not
> know if it solves the problem you observed in your environment and
> with your development tools, hence I was asking you to apply it and
> test, like you did when you originally noticed the problem ;-)

Ah, I definitely should have been more explicit, sorry. I did test
your patch, and it did work where git previously encountered a buffer
overflow.

I didn't try a lot of inputs, but different combinations of `-`,
`/dev/null` and files in parent / subdirectories yield the same
outputs as before, and additionally don't trigger any buffer overflow
with ASAN.

`prove --shuffle -j8 t*-diff*.sh` also succeeded with your patch,
whereas `t4053-diff-no-index.sh` fails on current `master`. I did
skip `t4056-diff-order.sh` as it never terminates on my computer
both with and without your patch.