[PATCH 1/3] patch-delta: fix oob read

[PATCH 1/3] patch-delta: fix oob read

[Jann Horn]

If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
`memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
into `dst_buf`.

This is not an exploitable bug because triggering the bug increments the
`data` pointer beyond `top`, causing the `data != top` sanity check after
the loop to trigger and discard the destination buffer - which means that
the result of the out-of-bounds read is never used for anything.

Also, directly jump into the error handler instead of just breaking out of
the loop - otherwise, data corruption would be silently ignored if the
delta buffer ends with a command and the destination buffer is already
full.

Signed-off-by: Jann Horn <jannh@google.com>
---
 patch-delta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/patch-delta.c b/patch-delta.c
index 56e0a5ede..283fb4b75 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 			if (unsigned_add_overflows(cp_off, cp_size) ||
 			    cp_off + cp_size > src_size ||
 			    cp_size > size)
-				break;
+				goto bad_length;
 			memcpy(out, (char *) src_buf + cp_off, cp_size);
 			out += cp_size;
 			size -= cp_size;
 		} else if (cmd) {
-			if (cmd > size)
-				break;
+			if (cmd > size || cmd > top - data)
+				goto bad_length;
 			memcpy(out, data, cmd);
 			out += cmd;
 			data += cmd;
@@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 
 	/* sanity check */
 	if (data != top || size != 0) {
+		bad_length:
 		error("delta replay has gone wild");
 		bad:
 		free(dst_buf);
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

[PATCH 2/3] t/helper/test-delta: segfault on OOB access

[Jann Horn]

This ensures that any attempts to access memory directly after the input
buffer or delta buffer in a delta test will cause a segmentation fault.

Inspired by vsftpd.

Signed-off-by: Jann Horn <jannh@google.com>
---
 t/helper/test-delta.c | 78 +++++++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 25 deletions(-)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 34c725924..64d0ec902 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -16,45 +16,73 @@
 static const char usage_str[] =
 	"test-tool delta (-d|-p) <from_file> <data_file> <out_file>";
 
-int cmd__delta(int argc, const char **argv)
+/*
+ * We want to detect OOB reads behind the resulting buffer, even in non-ASAN
+ * builds. This helper reads some data into memory, aligns the *end* of the
+ * buffer on a page boundary, and reserves the next virtual page. This ensures
+ * that a single-byte OOB access segfaults.
+ */
+static void *map_with_adjacent_trailing_guard(const char *path,
+					      unsigned long *sizep)
 {
 	int fd;
 	struct stat st;
-	void *from_buf, *data_buf, *out_buf;
-	unsigned long from_size, data_size, out_size;
+	unsigned long page_size = getpagesize();
+	unsigned long padded_size, padding;
 
-	if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) {
-		fprintf(stderr, "usage: %s\n", usage_str);
-		return 1;
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		perror(path);
+		return NULL;
 	}
+	if (fstat(fd, &st)) {
+		perror(path);
+		close(fd);
+		return NULL;
+	}
+	*sizep = st.st_size;
 
-	fd = open(argv[2], O_RDONLY);
-	if (fd < 0 || fstat(fd, &st)) {
-		perror(argv[2]);
-		return 1;
+	/* pad in front for alignment and add trailing page */
+	padded_size = ((page_size-1) + st.st_size + page_size) & ~(page_size-1);
+	padding = padded_size - (st.st_size + page_size);
+
+	char *mapping = mmap(NULL, padded_size, PROT_READ|PROT_WRITE,
+			     MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+	if (mapping == MAP_FAILED ||
+	    mprotect(mapping + padded_size - page_size, page_size, PROT_NONE)) {
+		perror("mmap");
+		close(fd);
+		return NULL;
 	}
-	from_size = st.st_size;
-	from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (from_buf == MAP_FAILED) {
-		perror(argv[2]);
+	if (read_in_full(fd, mapping + padding, st.st_size) != st.st_size) {
+		perror("read_in_full");
+		munmap(mapping, padded_size);
 		close(fd);
-		return 1;
+		return NULL;
 	}
+	mprotect(mapping, padded_size - page_size, PROT_READ);
 	close(fd);
+	return mapping + padding;
+}
 
-	fd = open(argv[3], O_RDONLY);
-	if (fd < 0 || fstat(fd, &st)) {
-		perror(argv[3]);
+int cmd__delta(int argc, const char **argv)
+{
+	int fd;
+	void *from_buf, *data_buf, *out_buf;
+	unsigned long from_size, data_size, out_size;
+
+	if (argc != 5 || (strcmp(argv[1], "-d") && strcmp(argv[1], "-p"))) {
+		fprintf(stderr, "usage: %s\n", usage_str);
 		return 1;
 	}
-	data_size = st.st_size;
-	data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (data_buf == MAP_FAILED) {
-		perror(argv[3]);
-		close(fd);
+
+	from_buf = map_with_adjacent_trailing_guard(argv[2], &from_size);
+	if (from_buf == NULL)
+		return 1;
+
+	data_buf = map_with_adjacent_trailing_guard(argv[3], &data_size);
+	if (data_buf == NULL)
 		return 1;
-	}
-	close(fd);
 
 	if (argv[1][1] == 'd')
 		out_buf = diff_delta(from_buf, from_size,
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access

[Jeff King]

On Wed, Aug 29, 2018 at 10:58:56PM +0200, Jann Horn wrote:

> This ensures that any attempts to access memory directly after the input
> buffer or delta buffer in a delta test will cause a segmentation fault.
> 
> Inspired by vsftpd.

Neat trick, but it seems funny to protect this one buffer in
non-production code. Obviously you were interested in demonstrating the
issue for your tests, but do we want to carry this all the time?

If we want to detect this kind of thing in tests, we should probably be
relying on tools like ASan, which would cover all mmaps.

It would be nice if there was a low-cost way to detect this in
production use, but it looks like this replaces mmap with
read_in_full(), which I think is a non-starter for most uses.

-Peff

Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access

[Jann Horn]

On Wed, Aug 29, 2018 at 11:34 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, Aug 29, 2018 at 10:58:56PM +0200, Jann Horn wrote:
>
> > This ensures that any attempts to access memory directly after the input
> > buffer or delta buffer in a delta test will cause a segmentation fault.
> >
> > Inspired by vsftpd.
>
> Neat trick, but it seems funny to protect this one buffer in
> non-production code. Obviously you were interested in demonstrating the
> issue for your tests, but do we want to carry this all the time?
>
> If we want to detect this kind of thing in tests, we should probably be
> relying on tools like ASan, which would cover all mmaps.
>
> It would be nice if there was a low-cost way to detect this in
> production use, but it looks like this replaces mmap with
> read_in_full(), which I think is a non-starter for most uses.

I think even with ASAN, you'd still need read_in_full() or an mmap()
wrapper that fiddles with the ASAN shadow, because mmap() always maps
whole pages:

$ cat mmap-read-asan-blah.c
#include <sys/mman.h>
#include <stdlib.h>
int main(void) {
  volatile char *p = mmap(NULL, 1, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
  p[200] = 1;
}
$ gcc -o mmap-read-asan-blah mmap-read-asan-blah.c -fsanitize=address
$ ./mmap-read-asan-blah
$

But that aside, you do have a point about having some custom hack for
a single patch.

Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access

[Jeff King]

On Wed, Aug 29, 2018 at 11:40:41PM +0200, Jann Horn wrote:

> > If we want to detect this kind of thing in tests, we should probably be
> > relying on tools like ASan, which would cover all mmaps.
> >
> > It would be nice if there was a low-cost way to detect this in
> > production use, but it looks like this replaces mmap with
> > read_in_full(), which I think is a non-starter for most uses.
> 
> I think even with ASAN, you'd still need read_in_full() or an mmap()
> wrapper that fiddles with the ASAN shadow, because mmap() always maps
> whole pages:
> 
> $ cat mmap-read-asan-blah.c
> #include <sys/mman.h>
> #include <stdlib.h>
> int main(void) {
>   volatile char *p = mmap(NULL, 1, PROT_READ|PROT_WRITE,
> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
>   p[200] = 1;
> }
> $ gcc -o mmap-read-asan-blah mmap-read-asan-blah.c -fsanitize=address
> $ ./mmap-read-asan-blah
> $

Yeah, I was just trying to run your tests with ASan and couldn't
convince it to complain. I also tried MSan, but no luck.

> But that aside, you do have a point about having some custom hack for
> a single patch.

I'm also not sure how portable it is. Looks like we have a Windows
wrapper for getpagesize(), but I don't see any other uses of mprotect().

-Peff

Re: [PATCH 2/3] t/helper/test-delta: segfault on OOB access

[Jeff King]

On Wed, Aug 29, 2018 at 05:46:21PM -0400, Jeff King wrote:

> > I think even with ASAN, you'd still need read_in_full() or an mmap()
> > wrapper that fiddles with the ASAN shadow, because mmap() always maps
> > whole pages:
> > 
> > $ cat mmap-read-asan-blah.c
> > #include <sys/mman.h>
> > #include <stdlib.h>
> > int main(void) {
> >   volatile char *p = mmap(NULL, 1, PROT_READ|PROT_WRITE,
> > MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> >   p[200] = 1;
> > }
> > $ gcc -o mmap-read-asan-blah mmap-read-asan-blah.c -fsanitize=address
> > $ ./mmap-read-asan-blah
> > $
> 
> Yeah, I was just trying to run your tests with ASan and couldn't
> convince it to complain. I also tried MSan, but no luck.
> 
> > But that aside, you do have a point about having some custom hack for
> > a single patch.
> 
> I'm also not sure how portable it is. Looks like we have a Windows
> wrapper for getpagesize(), but I don't see any other uses of mprotect().

Actually, there is no real need for this test helper to use mmap. I
suppose we could swap it out for malloc + read_in_full() and let ASan
(or even valgrind) handle the tricky parts.

-Peff

[PATCH 3/3] t5303: add tests for corrupted deltas

[Jann Horn]

This verifies the changes from commit "patch-delta: fix oob read".

Signed-off-by: Jann Horn <jannh@google.com>
---
 t/t5303-pack-corruption-resilience.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 3634e258f..7152376b6 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -311,4 +311,22 @@ test_expect_success \
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
 
+test_expect_success \
+    'apply good minimal delta' \
+    'printf "\x00\x01\x01X" > minimal_delta &&
+     test-tool delta -p /dev/null minimal_delta /dev/null
+     '
+
+test_expect_success \
+    'apply truncated delta' \
+    'printf "\x00\x02\x02X" > truncated_delta &&
+     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null
+     '
+
+test_expect_success \
+    'apply delta with trailing garbage command' \
+    'printf "\x00\x01\x01X\x01" > tail_garbage_delta &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_delta /dev/null
+     '
+
 test_done
-- 
2.19.0.rc0.228.g281dcd1b4d0-goog

Re: [PATCH 3/3] t5303: add tests for corrupted deltas

[Jeff King]

On Wed, Aug 29, 2018 at 10:58:57PM +0200, Jann Horn wrote:

> This verifies the changes from commit "patch-delta: fix oob read".

A minor nit, but usually we'd either introduce tests along with the
fix, or introduce them beforehand as test_expect_failure and then flip
them to success along with the fix.

> diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
> index 3634e258f..7152376b6 100755
> --- a/t/t5303-pack-corruption-resilience.sh
> +++ b/t/t5303-pack-corruption-resilience.sh
> @@ -311,4 +311,22 @@ test_expect_success \
>       test_must_fail git cat-file blob $blob_2 > /dev/null &&
>       test_must_fail git cat-file blob $blob_3 > /dev/null'
>  
> +test_expect_success \
> +    'apply good minimal delta' \
> +    'printf "\x00\x01\x01X" > minimal_delta &&
> +     test-tool delta -p /dev/null minimal_delta /dev/null
> +     '

Without your second patch applied, this complains about mmap-ing
/dev/null (or any zero-length file).

Also, \x escapes are sadly not portable (dash, for example, does not
respect them). You have to use octal instead (which is not too onerous
for these small numbers).

I needed the patch below to get it to behave as expected (I also
annotated the deltas to make it more comprehensible to somebody who
hasn't just been digging in the patch code ;) ).

I wonder if we should more fully test the 4 cases I outlined in my
earlier mail, too.

-Peff

---
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 7152376b67..df28cce68b 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -311,22 +311,35 @@ test_expect_success \
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
 
+# \5 - five bytes in base (though we do not use it)
+# \1 - one byte in result
+# \1 - copy one byte (X)
 test_expect_success \
     'apply good minimal delta' \
-    'printf "\x00\x01\x01X" > minimal_delta &&
-     test-tool delta -p /dev/null minimal_delta /dev/null
+    'printf "\5\1\1X" > minimal_delta &&
+     echo base >base &&
+     test-tool delta -p base minimal_delta /dev/null
      '
 
+# \5 - five bytes in base (though we do not use it)
+# \2 - two bytes in result
+# \2 - copy two bytes (we are short one)
 test_expect_success \
     'apply truncated delta' \
-    'printf "\x00\x02\x02X" > truncated_delta &&
-     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null
+    'printf "\5\2\2X" > truncated_delta &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base truncated_delta /dev/null
      '
 
+# \5 - five bytes in base (though we do not use it)
+# \1 - one byte in result
+# \1 - copy one byte (X)
+# \1 - trailing garbage command
 test_expect_success \
     'apply delta with trailing garbage command' \
-    'printf "\x00\x01\x01X\x01" > tail_garbage_delta &&
-     test_must_fail test-tool delta -p /dev/null tail_garbage_delta /dev/null
+    'printf "\5\1\1X\1" > tail_garbage_delta &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base tail_garbage_delta /dev/null
      '
 
 test_done

Re: [PATCH 3/3] t5303: add tests for corrupted deltas

[Jeff King]

On Wed, Aug 29, 2018 at 06:03:53PM -0400, Jeff King wrote:

> Without your second patch applied, this complains about mmap-ing
> /dev/null (or any zero-length file).
> 
> Also, \x escapes are sadly not portable (dash, for example, does not
> respect them). You have to use octal instead (which is not too onerous
> for these small numbers).
> 
> I needed the patch below to get it to behave as expected (I also
> annotated the deltas to make it more comprehensible to somebody who
> hasn't just been digging in the patch code ;) ).
> 
> I wonder if we should more fully test the 4 cases I outlined in my
> earlier mail, too.

So here's a version which checks each of those cases (plus your minimal
base-case and the trailing garbage case from your original).

I did it as a straight patch rather than on top of yours, since I think
that's easier to read (and I'd expect us to squash together whatever we
end up with anyway).

This doesn't cover out-of-bounds reads while parsing the offset/size.
It's dinner-time here, but I may take a look at that later tonight.

---
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 3634e258f8..305922eeb3 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -311,4 +311,81 @@ test_expect_success \
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
 
+# Base sanity check; this is the smallest possible delta.
+#
+# \5 - five bytes in base (though we do not use it)
+# \1 - one byte in result
+# \1 - copy one byte (X)
+test_expect_success \
+    'apply good minimal delta' \
+    'printf "\5\1\1X" > minimal_delta &&
+     echo base >base &&
+     test-tool delta -p base minimal_delta /dev/null
+     '
+
+# This delta has too many literal bytes to fit in destination.
+#
+# \5 - five bytes in base (though we do not use it)
+# \1 - 1 byte in result
+# \2 - copy two bytes (one too many)
+test_expect_success \
+    'apply truncated delta' \
+    'printf "\5\1\2XX" > too_big_literal &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base too_big_literal /dev/null
+     '
+
+# This delta has too many copy bytes to fit in destination.
+#
+# \5 - five bytes in base
+# \1 - one byte in result
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \2 - copy two bytes (one too many)
+test_expect_success \
+    'apply delta with trailing garbage command' \
+    'printf "\5\1\221\0\2" > too_big_copy &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base too_big_copy /dev/null
+     '
+
+# This delta has too few bytes in the delta itself.
+#
+# \5 - five bytes in base (though we do not use it)
+# \2 - two bytes in result
+# \2 - copy two bytes (we are short one)
+test_expect_success \
+    'apply truncated delta' \
+    'printf "\5\2\2X" > truncated_delta &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base truncated_delta /dev/null
+     '
+
+# This delta has too few bytes in the base.
+#
+# \5 - five bytes in base
+# \6 - six bytes in result
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \6 - copy six bytes (one too many)
+test_expect_success \
+    'apply delta with trailing garbage command' \
+    'printf "\5\6\221\0\6" > truncated_base &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base truncated_base /dev/null
+     '
+
+# Trailing garbage command.
+#
+# \5 - five bytes in base (though we do not use it)
+# \1 - one byte in result
+# \1 - copy one byte (X)
+# \1 - trailing garbage command
+test_expect_success \
+    'apply delta with trailing garbage command' \
+    'printf "\5\1\1X\1" > tail_garbage_delta &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base tail_garbage_delta /dev/null
+     '
+
 test_done

Re: [PATCH 1/3] patch-delta: fix oob read

[Jeff King]

On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:

> If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
> `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
> into `dst_buf`.
> 
> This is not an exploitable bug because triggering the bug increments the
> `data` pointer beyond `top`, causing the `data != top` sanity check after
> the loop to trigger and discard the destination buffer - which means that
> the result of the out-of-bounds read is never used for anything.
> 
> Also, directly jump into the error handler instead of just breaking out of
> the loop - otherwise, data corruption would be silently ignored if the
> delta buffer ends with a command and the destination buffer is already
> full.

Nice catch. The patch looks good to me, but just to lay out my thought
process looking for other related problems:

We have two types of instructions:

  1. Take N bytes from position P within the source.

  2. Take the next N bytes from the delta stream.

In both cases we need to check that:

  a. We have enough space in the destination buffer.

  b. We have enough source bytes.

So this:

> diff --git a/patch-delta.c b/patch-delta.c
> index 56e0a5ede..283fb4b75 100644
> --- a/patch-delta.c
> +++ b/patch-delta.c
> @@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
>  			if (unsigned_add_overflows(cp_off, cp_size) ||
>  			    cp_off + cp_size > src_size ||
>  			    cp_size > size)
> -				break;
> +				goto bad_length;
>  			memcpy(out, (char *) src_buf + cp_off, cp_size);

Covers 1a (cp_size > size) and 1b (cp_off + cp_size > src_size), plus
the obvious overflow possibility.

And then here:

>  		} else if (cmd) {
> -			if (cmd > size)
> -				break;
> +			if (cmd > size || cmd > top - data)
> +				goto bad_length;
>  			memcpy(out, data, cmd);

We had previously dealt with 2a (cmd > size), but failed to handle 2b,
which you've added. We don't need to deal with over/underflow here,
because our subtraction is on pointers to the same buffer.

> @@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
>  
>  	/* sanity check */
>  	if (data != top || size != 0) {
> +		bad_length:
>  		error("delta replay has gone wild");
>  		bad:
>  		free(dst_buf);

And I agree that jumping straight here is a good idea.

Overall, very nicely done.

-Peff

Re: [PATCH 1/3] patch-delta: fix oob read

[Jeff King]

On Wed, Aug 29, 2018 at 05:20:25PM -0400, Jeff King wrote:

> Nice catch. The patch looks good to me, but just to lay out my thought
> process looking for other related problems:
> 
> We have two types of instructions:
> 
>   1. Take N bytes from position P within the source.
> 
>   2. Take the next N bytes from the delta stream.
> 
> In both cases we need to check that:
> 
>   a. We have enough space in the destination buffer.
> 
>   b. We have enough source bytes.

Actually, there's one other case to consider: reading the actual offset
for a copy operation. E.g., if we see '0x80' at the end of the delta,
I think we'd read garbage into cp_offset? That would typically generate
nonsense that would be caught by the other checks, but I think it's
possible that the memory could happen to produce a valid copy
instruction from the base, leading to a confusing result (though it
would still need to match the expected result size).

Thinking on it more, one other interesting tidbit here: in actual git
code (i.e., not test-delta), we'd always be reading from an mmap'd
packfile. And we're guaranteed to have at least 20 trailing bytes in the
mmap due to the pack format (which is enforced when we parse the pack).

So I don't think this can ever read truly out-of-bounds memory, though
obviously it's something we should fix regardless.

-Peff

[PATCH 0/5] handle corruption in patch-delta

[Jeff King]

On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:

> If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
> `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
> into `dst_buf`.
> 
> This is not an exploitable bug because triggering the bug increments the
> `data` pointer beyond `top`, causing the `data != top` sanity check after
> the loop to trigger and discard the destination buffer - which means that
> the result of the out-of-bounds read is never used for anything.
> 
> Also, directly jump into the error handler instead of just breaking out of
> the loop - otherwise, data corruption would be silently ignored if the
> delta buffer ends with a command and the destination buffer is already
> full.

Based on my earlier observations, here's a replacement patch series I
came up with. It has:

  [1/5]: test-delta: read input into a heap buffer

    A simpler replacement for your patch 2 which avoids portability
    issues.

  [2/5]: t5303: test some corrupt deltas

    A more complete set of boundary tests based on the 4 cases I laid
    out, plus the cp_size problem I found.

  [3/5]: patch-delta: fix oob read

    Your actual fix.

  [4/5]: patch-delta: consistently report corruption

    Your related trailing-garbage fix. I split this into two in order to
    better demonstrate the cases this part covers.

  [5/5]: patch-delta: handle truncated copy parameters

    My fix for the cp_size read.

I hope you don't mind me hacking up your patches a bit. Thanks again for
your original report and patch.

-Peff

[PATCH 1/5] test-delta: read input into a heap buffer

[Jeff King]

We currently read the input to test-delta by mmap()-ing it.
However, memory-checking tools like valgrind and ASan are
less able to detect reads/writes past the end of an mmap'd
buffer, because the OS is likely to give us extra bytes to
pad out the final page size. So instead, let's read into a
heap buffer.

As a bonus, this also makes it possible to write tests with
empty bases, as mmap() will complain about a zero-length
map.

This is based on a patch by Jann Horn <jannh@google.com>
which actually aligned the data at the end of a page, and
followed it with another page marked with mprotect(). That
would detect problems even without a tool like ASan, but it
was significantly more complex and may have introduced
portability problems. By comparison, this approach pushes
the complexity onto existing memory-checking tools.

Note that this could be done even more simply by using
strbuf_read_file(), but that would defeat the purpose:
strbufs generally overallocate (and at the very least
include a trailing NUL which we do not care about), which
would defeat most memory checkers.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-delta.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 34c7259248..e749a49c88 100644
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -34,8 +34,8 @@ int cmd__delta(int argc, const char **argv)
 		return 1;
 	}
 	from_size = st.st_size;
-	from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (from_buf == MAP_FAILED) {
+	from_buf = xmalloc(from_size);
+	if (read_in_full(fd, from_buf, from_size) < 0) {
 		perror(argv[2]);
 		close(fd);
 		return 1;
@@ -48,8 +48,8 @@ int cmd__delta(int argc, const char **argv)
 		return 1;
 	}
 	data_size = st.st_size;
-	data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if (data_buf == MAP_FAILED) {
+	data_buf = xmalloc(data_size);
+	if (read_in_full(fd, data_buf, data_size) < 0) {
 		perror(argv[3]);
 		close(fd);
 		return 1;
-- 
2.19.0.rc1.539.g3876d0831e

[PATCH 2/5] t5303: test some corrupt deltas

[Jeff King]

We don't have any tests that specifically check boundary
cases in patch_delta(). It obviously gets exercised by tests
which read from packfiles, but it's hard to create packfiles
with bogus deltas.

So let's cover some obvious boundary cases:

  1. commands that overflow the result buffer

     a. literal content from the delta

     b. copies from a base

  2. commands where the source isn't large enough

     a. literal content from a truncated delta

     b. copies that need more bytes than the base has

  3. copy commands who parameters are truncated

And indeed, we have problems with both 2a and 3. I've marked
these both as expect_failure, though note that because they
involve reading past the end of a buffer, they will
typically only be caught when run under valgrind or ASan.

There's one more test here, too, which just applies a basic
delta. Since all of the other tests expect failure and we
don't otherwise use "test-tool delta" in the test suite,
this gives a sanity check that the tool works at all.

These are based on an earlier patch by Jann Horn
<jannh@google.com>.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5303-pack-corruption-resilience.sh | 59 +++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 3634e258f8..912e659acf 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -311,4 +311,63 @@ test_expect_success \
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
 
+# \0 - empty base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+test_expect_success \
+    'apply good minimal delta' \
+    'printf "\0\1\1X" > minimal_delta &&
+     test-tool delta -p /dev/null minimal_delta /dev/null'
+
+# \0 - empty base
+# \1 - 1 byte in result
+# \2 - two literal bytes (one too many)
+test_expect_success \
+    'apply delta with too many literal bytes' \
+    'printf "\0\1\2XX" > too_big_literal &&
+     test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'
+
+# \5 - five bytes in base
+# \1 - one byte in result
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \2 - copy two bytes (one too many)
+test_expect_success \
+    'apply delta with too many copied bytes' \
+    'printf "\5\1\221\0\2" > too_big_copy &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base too_big_copy /dev/null'
+
+# \0 - empty base
+# \2 - two bytes in result
+# \2 - two literal bytes (we are short one)
+test_expect_failure \
+    'apply delta with too few literal bytes' \
+    'printf "\0\2\2X" > truncated_delta &&
+     test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null'
+
+# \0 - empty base
+# \1 - one byte in result
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \1 - copy one byte (we are short one)
+test_expect_success \
+    'apply delta with too few bytes in base' \
+    'printf "\0\1\221\0\1" > truncated_base &&
+     test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'
+
+# \5 - five bytes in base
+# \5 - five bytes in result
+# \1 - one literal byte (X)
+# \221 - copy, one byte offset, one byte size
+#        (offset/size missing)
+#
+# Note that the literal byte is necessary to get past the uninteresting minimum
+# delta size check.
+test_expect_failure \
+    'apply delta with truncated copy parameters' \
+    'printf "\5\5\1X\221" > truncated_copy_delta &&
+     echo base >base &&
+     test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'
+
 test_done
-- 
2.19.0.rc1.539.g3876d0831e

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +test_expect_success \
> +    'apply delta with too many copied bytes' \
> +    'printf "\5\1\221\0\2" > too_big_copy &&
> +     echo base >base &&
> +     test_must_fail test-tool delta -p base too_big_copy /dev/null'

Would "echo base >base" give us 5-byte long base even on Windows?
Or the test does not care if it is either "base\n" or "base\r\n"?

Just double-checking.

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Jeff King]

On Thu, Aug 30, 2018 at 10:38:21AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +test_expect_success \
> > +    'apply delta with too many copied bytes' \
> > +    'printf "\5\1\221\0\2" > too_big_copy &&
> > +     echo base >base &&
> > +     test_must_fail test-tool delta -p base too_big_copy /dev/null'
> 
> Would "echo base >base" give us 5-byte long base even on Windows?
> Or the test does not care if it is either "base\n" or "base\r\n"?
> 
> Just double-checking.

Good question. On the first one, I don't know. On the second one, yes,
it does matter. We'd feed "6" to patch_delta(), and it would complain
about the mismatch before actually hitting the code we're trying to
exercise. The test would still pass (the error result is the same either
way), but would quietly not test what we wanted.

Maybe something like this to be on the safe side?

(note that we can leave the \5 in the result size of the "truncated copy
parameters" test; it really just needs to be larger than 1).

---
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 912e659acf..e80934a18e 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -327,15 +327,15 @@ test_expect_success \
     'printf "\0\1\2XX" > too_big_literal &&
      test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'
 
-# \5 - five bytes in base
+# \4 - four bytes in base
 # \1 - one byte in result
 # \221 - copy, one byte offset, one byte size
 #   \0 - copy from offset 0
 #   \2 - copy two bytes (one too many)
 test_expect_success \
     'apply delta with too many copied bytes' \
-    'printf "\5\1\221\0\2" > too_big_copy &&
-     echo base >base &&
+    'printf "\4\1\221\0\2" > too_big_copy &&
+     printf base >base &&
      test_must_fail test-tool delta -p base too_big_copy /dev/null'
 
 # \0 - empty base
@@ -356,7 +356,7 @@ test_expect_success \
     'printf "\0\1\221\0\1" > truncated_base &&
      test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'
 
-# \5 - five bytes in base
+# \4 - four bytes in base
 # \5 - five bytes in result
 # \1 - one literal byte (X)
 # \221 - copy, one byte offset, one byte size
@@ -366,8 +366,8 @@ test_expect_success \
 # delta size check.
 test_expect_failure \
     'apply delta with truncated copy parameters' \
-    'printf "\5\5\1X\221" > truncated_copy_delta &&
-     echo base >base &&
+    'printf "\4\5\1X\221" > truncated_copy_delta &&
+     printf base >base &&
      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'
 
 test_done

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Jeff King]

On Thu, Aug 30, 2018 at 02:42:01PM -0400, Jeff King wrote:

> > Would "echo base >base" give us 5-byte long base even on Windows?
> > Or the test does not care if it is either "base\n" or "base\r\n"?
> > 
> > Just double-checking.
> 
> Good question. On the first one, I don't know. On the second one, yes,
> it does matter. We'd feed "6" to patch_delta(), and it would complain
> about the mismatch before actually hitting the code we're trying to
> exercise. The test would still pass (the error result is the same either
> way), but would quietly not test what we wanted.
> 
> Maybe something like this to be on the safe side?

That could be squashed into patch 2. Patch 4 would need this one
additional case:

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 0c537958e7..e91d6f5770 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -379,7 +379,7 @@ test_expect_success \
     'printf "\0\1\1X\1" > tail_garbage_literal &&
      test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'
 
-# \5 - five bytes in base
+# \4 - four bytes in base
 # \1 - one byte in result
 # \1 - one literal byte (X)
 # \221 - copy, one byte offset, one byte size
@@ -387,8 +387,8 @@ test_expect_success \
 #   \1 - copy 1 byte
 test_expect_success \
     'apply delta with trailing garbage copy' \
-    'printf "\5\1\1X\221\0\1" > tail_garbage_copy &&
-     echo base >base &&
+    'printf "\4\1\1X\221\0\1" > tail_garbage_copy &&
+     printf base >base &&
      test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'
 
 # \0 - empty base

I can re-roll, or even prepare a patch on top (it's sufficiently subtle
that it may merit calling out explicitly in a commit).

-Peff

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I can re-roll, or even prepare a patch on top (it's sufficiently subtle
> that it may merit calling out explicitly in a commit).

Yeah, I tend to agree with your reasoning to do it on top as a
separate patch.

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Jeff King]

On Thu, Aug 30, 2018 at 11:50:56AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I can re-roll, or even prepare a patch on top (it's sufficiently subtle
> > that it may merit calling out explicitly in a commit).
> 
> Yeah, I tend to agree with your reasoning to do it on top as a
> separate patch.

Here it is, then.

-- >8 --
Subject: [PATCH 6/5] t5303: use printf to generate delta bases

The exact byte count of the delta base file is important.
The test-delta helper will feed it to patch_delta(), which
will barf if it doesn't match the size byte given in the
delta. Using "echo" may end up with unexpected line endings
on some platforms (e.g,. "\r\n" instead of just "\n").

This actually wouldn't cause the test to fail (since we
already expect test-delta to complain about these bogus
deltas), but would mean that we're not exercising the code
we think we are.

Let's use printf instead (which we already trust to give us
byte-perfect output when we generate the deltas).

While we're here, let's tighten the 5-byte result size used
in the "truncated copy parameters" test. This just needs to
have enough room to attempt to parse the bogus copy command,
meaning 2 is sufficient. Using 5 was arbitrary and just
copied from the base size; since those no longer match, it's
simply confusing. Let's use a more meaningful number.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5303-pack-corruption-resilience.sh | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index b68bbeedcc..41e6dc4dcf 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -327,15 +327,15 @@ test_expect_success \
     'printf "\0\1\2XX" > too_big_literal &&
      test_must_fail test-tool delta -p /dev/null too_big_literal /dev/null'
 
-# \5 - five bytes in base
+# \4 - four bytes in base
 # \1 - one byte in result
 # \221 - copy, one byte offset, one byte size
 #   \0 - copy from offset 0
 #   \2 - copy two bytes (one too many)
 test_expect_success \
     'apply delta with too many copied bytes' \
-    'printf "\5\1\221\0\2" > too_big_copy &&
-     echo base >base &&
+    'printf "\4\1\221\0\2" > too_big_copy &&
+     printf base >base &&
      test_must_fail test-tool delta -p base too_big_copy /dev/null'
 
 # \0 - empty base
@@ -356,8 +356,8 @@ test_expect_success \
     'printf "\0\1\221\0\1" > truncated_base &&
      test_must_fail test-tool delta -p /dev/null truncated_base /dev/null'
 
-# \5 - five bytes in base
-# \5 - five bytes in result
+# \4 - four bytes in base
+# \2 - two bytes in result
 # \1 - one literal byte (X)
 # \221 - copy, one byte offset, one byte size
 #        (offset/size missing)
@@ -366,8 +366,8 @@ test_expect_success \
 # delta size check.
 test_expect_success \
     'apply delta with truncated copy parameters' \
-    'printf "\5\5\1X\221" > truncated_copy_delta &&
-     echo base >base &&
+    'printf "\4\2\1X\221" > truncated_copy_delta &&
+     printf base >base &&
      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'
 
 # \0 - empty base
@@ -379,7 +379,7 @@ test_expect_success \
     'printf "\0\1\1X\1" > tail_garbage_literal &&
      test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'
 
-# \5 - five bytes in base
+# \4 - four bytes in base
 # \1 - one byte in result
 # \1 - one literal byte (X)
 # \221 - copy, one byte offset, one byte size
@@ -387,8 +387,8 @@ test_expect_success \
 #   \1 - copy 1 byte
 test_expect_success \
     'apply delta with trailing garbage copy' \
-    'printf "\5\1\1X\221\0\1" > tail_garbage_copy &&
-     echo base >base &&
+    'printf "\4\1\1X\221\0\1" > tail_garbage_copy &&
+     printf base >base &&
      test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'
 
 # \0 - empty base
-- 
2.19.0.rc1.546.g3fcb3c0d7c

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Johannes Schindelin]

Hi Junio,

On Thu, 30 Aug 2018, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +test_expect_success \
> > +    'apply delta with too many copied bytes' \
> > +    'printf "\5\1\221\0\2" > too_big_copy &&
> > +     echo base >base &&
> > +     test_must_fail test-tool delta -p base too_big_copy /dev/null'
> 
> Would "echo base >base" give us 5-byte long base even on Windows?

Please note that Unix shell scripting is a foreign thing on Windows. As
such, there is not really any "native" shell we can use [*1*], and
therefore we use MSYS2's Bash which outputs Unix line endings.

Ciao,
Dscho

Footnote *1*: I keep trying (and failing) to find the time to work on the
pure-Win32 port of BusyBox, which would give us "sort of a native Unix
shell". That probably *would* output CR/LF in this case. But as there are
still parts of the test suite that require Perl (which is not included in
BusyBox), I think we are still a loooong way from running the test suite
in a pure Win32 fashion. With all the time tax that incurs for us Windows
users.

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Would "echo base >base" give us 5-byte long base even on Windows?
>
> Please note that Unix shell scripting is a foreign thing on Windows. As
> such, there is not really any "native" shell we can use [*1*], and

Yeah, I know that; otherwise I wouldn't have asked.  Because ...

> therefore we use MSYS2's Bash which outputs Unix line endings.

... I didn't know what MSYS folks chose, and/or if you have chosen
to tweak their choice, and/or if you switched to somebody else's shell
(e.g. busybox) and/or you chose to tweak what they do out of the box,
it was worth asking and getting yes/no question.  You do not have to
tell me why I should be asking.

So instead of typing 3 lines, you can just say "yes we use echo that
emulates Unix".

Thanks.

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Jeff King]

On Fri, Aug 31, 2018 at 08:33:26AM -0700, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Would "echo base >base" give us 5-byte long base even on Windows?
> >
> > Please note that Unix shell scripting is a foreign thing on Windows. As
> > such, there is not really any "native" shell we can use [*1*], and
> 
> Yeah, I know that; otherwise I wouldn't have asked.  Because ...
> 
> > therefore we use MSYS2's Bash which outputs Unix line endings.
> 
> ... I didn't know what MSYS folks chose, and/or if you have chosen
> to tweak their choice, and/or if you switched to somebody else's shell
> (e.g. busybox) and/or you chose to tweak what they do out of the box,
> it was worth asking and getting yes/no question.  You do not have to
> tell me why I should be asking.
> 
> So instead of typing 3 lines, you can just say "yes we use echo that
> emulates Unix".

I actually found Dscho's response much more informative than a simple
yes/no.

At any rate, it sounds like we are probably OK with echo, but I think it
is still worth doing the defensive patch-on-top that I posted.

-Peff

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> So instead of typing 3 lines, you can just say "yes we use echo that
>> emulates Unix".
>
> I actually found Dscho's response much more informative than a simple
> yes/no.
>
> At any rate, it sounds like we are probably OK with echo, but I think it
> is still worth doing the defensive patch-on-top that I posted.

Actually, Dscho's footnote was much more informative than the "echo
is from UNIX, dammit" that didn't convey anything useful---yes, we
all know that echo is from UNIX, but what does he do to it?  Does he
want it to be like UNIX, or want to tweak it to suit Windows taste?

Reading the footnote again, from there, you can read that 'echo' may
currently behave like UNIX, but he guarantees that it can change any
time without notice; in his mind, 'echo' in the ideal world in the
fiture on Windows is to use CRLF.

So we must protect the test from his whim by using printf.

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Johannes Schindelin]

Hi Junio,

On Fri, 31 Aug 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Would "echo base >base" give us 5-byte long base even on Windows?
> >
> > Please note that Unix shell scripting is a foreign thing on Windows. As
> > such, there is not really any "native" shell we can use [*1*], and
> 
> Yeah, I know that; otherwise I wouldn't have asked.  Because ...
> 
> > therefore we use MSYS2's Bash which outputs Unix line endings.
> 
> ... I didn't know what MSYS folks chose, and/or if you have chosen
> to tweak their choice, and/or if you switched to somebody else's shell
> (e.g. busybox) and/or you chose to tweak what they do out of the box,
> it was worth asking and getting yes/no question.  You do not have to
> tell me why I should be asking.
> 
> So instead of typing 3 lines, you can just say "yes we use echo that
> emulates Unix".
> 
> Thanks.

You could just express your gratitude that I do more than just answer a
question, and impart more knowledge that will help you be a better
maintainer.

Ciao,
Dscho

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Jeff King]

On Fri, Aug 31, 2018 at 11:14:20PM +0200, Johannes Schindelin wrote:

> On Fri, 31 Aug 2018, Junio C Hamano wrote:
> [...]
> > So instead of typing 3 lines, you can just say "yes we use echo that
> > emulates Unix".
> 
> You could just express your gratitude that I do more than just answer a
> question, and impart more knowledge that will help you be a better
> maintainer.

It is my observation as a third party that there has been a lot of
bickering between you two lately (here and in other threads). Can you
both please try to make sure your comments are truly constructive?

I don't want to get into details of who I think has been unreasonable
when, or lay any kind of blame. I'm just concerned that exchanges like
this one worsen the general tone of the mailing list, and I'd like to:

  - make it clear to other observers that there's at least one person
    who hopes this is not the norm for this list

  - make a general reminder that collaborating on the Internet is hard,
    but it's worth spending the extra effort to consider how comments
    will be taken by the intended receiver, as well as others on the
    list

You're free to disagree, of course. And I'm sorry for being somewhat
vague. I really want to avoid opening up a specific flamewar, and just
make a general call for people to remember to be friendly.

If vger allowed HTML mail, I would link to a picture of a unicorn
running across a rainbow.

-Peff

Re: [PATCH 2/5] t5303: test some corrupt deltas

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   - make it clear to other observers that there's at least one person
>     who hopes this is not the norm for this list

Make it at least two by counting me ;-)

>   - make a general reminder that collaborating on the Internet is hard,
>     but it's worth spending the extra effort to consider how comments
>     will be taken by the intended receiver, as well as others on the
>     list

Well, in this particular case both of us know exactly how the sender
wants the message to be taken by the other side.  At least I do.

[PATCH 3/5] patch-delta: fix oob read

[Jeff King]

From: Jann Horn <jannh@google.com>

If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
`memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
into `dst_buf`.

This is not an exploitable bug because triggering the bug increments the
`data` pointer beyond `top`, causing the `data != top` sanity check after
the loop to trigger and discard the destination buffer - which means that
the result of the out-of-bounds read is never used for anything.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 patch-delta.c                         | 2 +-
 t/t5303-pack-corruption-resilience.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/patch-delta.c b/patch-delta.c
index 56e0a5ede2..b937afd2c9 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -56,7 +56,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 			out += cp_size;
 			size -= cp_size;
 		} else if (cmd) {
-			if (cmd > size)
+			if (cmd > size || cmd > top - data)
 				break;
 			memcpy(out, data, cmd);
 			out += cmd;
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 912e659acf..7114c31ade 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -341,7 +341,7 @@ test_expect_success \
 # \0 - empty base
 # \2 - two bytes in result
 # \2 - two literal bytes (we are short one)
-test_expect_failure \
+test_expect_success \
     'apply delta with too few literal bytes' \
     'printf "\0\2\2X" > truncated_delta &&
      test_must_fail test-tool delta -p /dev/null truncated_delta /dev/null'
-- 
2.19.0.rc1.539.g3876d0831e

[PATCH 4/5] patch-delta: consistently report corruption

[Jeff King]

From: Jann Horn <jannh@google.com>

When applying a delta, if we see an opcode that cannot be
fulfilled (e.g., asking to write more bytes than the
destination has left), we break out of our parsing loop but
don't signal an explicit error. We rely on the sanity check
after the loop to see if we have leftover delta bytes or
didn't fill our result buffer.

This can silently ignore corruption when the delta buffer
ends with a bogus command and the destination buffer is
already full. Instead, let's jump into the error handler
directly when we see this case.

Note that the tests also cover the "bad opcode" case, which
already handles this correctly.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 patch-delta.c                         |  5 +++--
 t/t5303-pack-corruption-resilience.sh | 30 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/patch-delta.c b/patch-delta.c
index b937afd2c9..283fb4b759 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 			if (unsigned_add_overflows(cp_off, cp_size) ||
 			    cp_off + cp_size > src_size ||
 			    cp_size > size)
-				break;
+				goto bad_length;
 			memcpy(out, (char *) src_buf + cp_off, cp_size);
 			out += cp_size;
 			size -= cp_size;
 		} else if (cmd) {
 			if (cmd > size || cmd > top - data)
-				break;
+				goto bad_length;
 			memcpy(out, data, cmd);
 			out += cmd;
 			data += cmd;
@@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 
 	/* sanity check */
 	if (data != top || size != 0) {
+		bad_length:
 		error("delta replay has gone wild");
 		bad:
 		free(dst_buf);
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 7114c31ade..41dc947d3f 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -370,4 +370,34 @@ test_expect_failure \
      echo base >base &&
      test_must_fail test-tool delta -p base truncated_copy_delta /dev/null'
 
+# \0 - empty base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+# \1 - trailing garbage command
+test_expect_success \
+    'apply delta with trailing garbage literal' \
+    'printf "\0\1\1X\1" > tail_garbage_literal &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_literal /dev/null'
+
+# \5 - five bytes in base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+# \221 - copy, one byte offset, one byte size
+#   \0 - copy from offset 0
+#   \1 - copy 1 byte
+test_expect_success \
+    'apply delta with trailing garbage copy' \
+    'printf "\5\1\1X\221\0\1" > tail_garbage_copy &&
+     echo base >base &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_copy /dev/null'
+
+# \0 - empty base
+# \1 - one byte in result
+# \1 - one literal byte (X)
+# \0 - bogus opcode
+test_expect_success \
+    'apply delta with trailing garbage opcode' \
+    'printf "\0\1\1X\0" > tail_garbage_opcode &&
+     test_must_fail test-tool delta -p /dev/null tail_garbage_opcode /dev/null'
+
 test_done
-- 
2.19.0.rc1.539.g3876d0831e

[PATCH 5/5] patch-delta: handle truncated copy parameters

[Jeff King]

When we see a delta command instructing us to copy bytes
from the base, we have to read the offset and size from the
delta stream. We do this without checking whether we're at
the end of the stream, meaning we may read past the end of
the buffer.

In practice this isn't exploitable in any interesting way
because:

  1. Deltas are always in packfiles, so we have at least a
     20-byte trailer that we'll end up reading.

  2. The worst case is that we try to perform a nonsense
     copy from the base object into the result, based on
     whatever was in the pack stream next. In most cases
     this will simply fail due to our bounds-checks against
     the base or the result.

     But even if you carefully constructed a pack stream for
     which it succeeds, it wouldn't perform any delta
     operation that you couldn't have simply included in a
     non-broken form.

But obviously it's poor form to read past the end of the
buffer we've been given. Unfortunately there's no easy way
to do a single length check, since the number of bytes we
need depends on the number of bits set in the initial
command byte. So we'll just check each byte as we parse. We
can hide the complexity in a macro; it's ugly, but not as
ugly as writing out each individual conditional.

Signed-off-by: Jeff King <peff@peff.net>
---
 patch-delta.c                         | 21 ++++++++++++++-------
 t/t5303-pack-corruption-resilience.sh |  2 +-
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/patch-delta.c b/patch-delta.c
index 283fb4b759..b5c8594db6 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -40,13 +40,20 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 		cmd = *data++;
 		if (cmd & 0x80) {
 			unsigned long cp_off = 0, cp_size = 0;
-			if (cmd & 0x01) cp_off = *data++;
-			if (cmd & 0x02) cp_off |= (*data++ << 8);
-			if (cmd & 0x04) cp_off |= (*data++ << 16);
-			if (cmd & 0x08) cp_off |= ((unsigned) *data++ << 24);
-			if (cmd & 0x10) cp_size = *data++;
-			if (cmd & 0x20) cp_size |= (*data++ << 8);
-			if (cmd & 0x40) cp_size |= (*data++ << 16);
+#define PARSE_CP_PARAM(bit, var, shift) do { \
+			if (cmd & (bit)) { \
+				if (data >= top) \
+					goto bad_length; \
+				var |= ((unsigned) *data++ << (shift)); \
+			} } while (0)
+			PARSE_CP_PARAM(0x01, cp_off, 0);
+			PARSE_CP_PARAM(0x02, cp_off, 8);
+			PARSE_CP_PARAM(0x04, cp_off, 16);
+			PARSE_CP_PARAM(0x08, cp_off, 24);
+			PARSE_CP_PARAM(0x10, cp_size, 0);
+			PARSE_CP_PARAM(0x20, cp_size, 8);
+			PARSE_CP_PARAM(0x40, cp_size, 16);
+#undef PARSE_CP_PARAM
 			if (cp_size == 0) cp_size = 0x10000;
 			if (unsigned_add_overflows(cp_off, cp_size) ||
 			    cp_off + cp_size > src_size ||
diff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh
index 41dc947d3f..b68bbeedcc 100755
--- a/t/t5303-pack-corruption-resilience.sh
+++ b/t/t5303-pack-corruption-resilience.sh
@@ -364,7 +364,7 @@ test_expect_success \
 #
 # Note that the literal byte is necessary to get past the uninteresting minimum
 # delta size check.
-test_expect_failure \
+test_expect_success \
     'apply delta with truncated copy parameters' \
     'printf "\5\5\1X\221" > truncated_copy_delta &&
      echo base >base &&
-- 
2.19.0.rc1.539.g3876d0831e

Re: [PATCH 0/5] handle corruption in patch-delta

[Jann Horn]

On Thu, Aug 30, 2018 at 9:05 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:
>
> > If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
> > `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
> > into `dst_buf`.
> >
> > This is not an exploitable bug because triggering the bug increments the
> > `data` pointer beyond `top`, causing the `data != top` sanity check after
> > the loop to trigger and discard the destination buffer - which means that
> > the result of the out-of-bounds read is never used for anything.
> >
> > Also, directly jump into the error handler instead of just breaking out of
> > the loop - otherwise, data corruption would be silently ignored if the
> > delta buffer ends with a command and the destination buffer is already
> > full.
>
> Based on my earlier observations, here's a replacement patch series I
> came up with. It has:
[...]
> I hope you don't mind me hacking up your patches a bit.

Not at all. I'm happy that I don't have to write a v2 series.

> Thanks again for your original report and patch.

Thanks for turning my patch into something decent so quickly!

Re: [PATCH 0/5] handle corruption in patch-delta

[Nicolas Pitre]

On Thu, 30 Aug 2018, Jeff King wrote:

> On Wed, Aug 29, 2018 at 10:58:55PM +0200, Jann Horn wrote:
> 
> > If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the
> > `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf`
> > into `dst_buf`.
> > 
> > This is not an exploitable bug because triggering the bug increments the
> > `data` pointer beyond `top`, causing the `data != top` sanity check after
> > the loop to trigger and discard the destination buffer - which means that
> > the result of the out-of-bounds read is never used for anything.
> > 
> > Also, directly jump into the error handler instead of just breaking out of
> > the loop - otherwise, data corruption would be silently ignored if the
> > delta buffer ends with a command and the destination buffer is already
> > full.
> 
> Based on my earlier observations, here's a replacement patch series I
> came up with. It has:
> 
>   [1/5]: test-delta: read input into a heap buffer
> 
>     A simpler replacement for your patch 2 which avoids portability
>     issues.
> 
>   [2/5]: t5303: test some corrupt deltas
> 
>     A more complete set of boundary tests based on the 4 cases I laid
>     out, plus the cp_size problem I found.
> 
>   [3/5]: patch-delta: fix oob read
> 
>     Your actual fix.
> 
>   [4/5]: patch-delta: consistently report corruption
> 
>     Your related trailing-garbage fix. I split this into two in order to
>     better demonstrate the cases this part covers.
> 
>   [5/5]: patch-delta: handle truncated copy parameters
> 
>     My fix for the cp_size read.
> 
> I hope you don't mind me hacking up your patches a bit. Thanks again for
> your original report and patch.

Looks good to me (feels like traveling back in time).

Reviewed-by: Nicolas Pitre <nico@fluxnic.net>



> 
> -Peff
>