[PATCH] apply: keep buffer/size pair in sync when parsing binary hunks

From: Jeff King <peff@peff.net>
Date: Aug 10, 2021, 01:01 UTC
Message-ID: <YRHQAFDXs5xvyDND@coredump.intra.peff.net>

We parse through binary hunks by looping through the buffer with code like:

    llen = linelen(buffer, size);

    ...do something with the line...

    buffer += llen;
    size -= llen;

However, before we enter the loop, there is one call that increments "buffer" but forgets to decrement "size". As a result, our "size" is off by the length of that line, and subsequent calls to linelen() may look past the end of the buffer for a newline.

The fix is easy: we just need to decrement size as we do elsewhere.

This bug goes all the way back to 0660626caf (binary diff: further updates., 2006-05-05). Presumably nobody noticed because it only triggers if the patch is corrupted, and even then we are often "saved" by luck. We use a strbuf to store the incoming patch, so we overallocate there, plus we add a 16-byte run of NULs as slop for memory comparisons. So if this happened accidentally, the common case is that we'd just read a few uninitialized bytes from the end of the strbuf before producing the expected "this patch is corrupted" error complaint.

However, it is possible to carefully construct a case which reads off the end of the buffer. The included test does so. It will pass both before and after this patch when run normally, but using a tool like ASan shows that we get an out-of-bounds read before this patch, but not after.

Reported-by: Xingman Chen <xichixingman@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
This was reported to the security list, but I think the impact is small
enough to just take it as a normal fix.

I hate the test, as it is brittle and subject to bitrot if we do things like change the default strbuf hint size. If that does happen, it will still pass; it just won't do anything interesting. But I would like some record of how to reproduce, and I prefer this to putting something hard-to-run in the commit message.

 apply.c                 |  1 +
 t/t4103-apply-binary.sh | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/apply.c b/apply.c
index 44bc31d6eb..4ed4b27169 100644
--- a/apply.c
+++ b/apply.c
@@ -1917,6 +1917,7 @@ static struct fragment *parse_binary_hunk(struct apply_state *state,
 
 	state->linenr++;
 	buffer += llen;
+	size -= llen;
 	while (1) {
 		int byte_length, max_byte_length, newsize;
 		llen = linelen(buffer, size);
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index fad6d3f542..d370ecfe0d 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -158,4 +158,27 @@ test_expect_success 'apply binary -p0 diff' '
 	test -z "$(git diff --name-status binary -- file3)"
 '
 
+test_expect_success 'reject truncated binary diff' '
+	do_reset &&
+
+	# this length is calculated to get us very close to
+	# the 8192-byte strbuf we will use to read in the patch.
+	test-tool genrandom foo 6205 >file1 &&
+	git diff --binary >patch &&
+
+	# truncate the patch at the second "literal" line,
+	# but exclude the trailing newline. We must use perl
+	# for this, since tools like "sed" cannot reliably
+	# produce output without the trailing newline.
+	perl -pe "
+		if (/^literal/ && \$count++ >= 1) {
+			chomp;
+			print;
+			exit 0;
+		}
+	" <patch >patch.trunc &&
+
+	do_reset &&
+	test_must_fail git apply patch.trunc
+'
 test_done

-- 
2.33.0.rc1.475.g023efe0ae4