From 9caf0107a86d11f059554e55c461f8e7657d89bf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Aug 2018 03:09:32 -0400 Subject: t5303: test some corrupt deltas 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 . Signed-off-by: Jeff King Reviewed-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- t/t5303-pack-corruption-resilience.sh | 59 +++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) (limited to 't/t5303-pack-corruption-resilience.sh') 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 -- cgit v1.2.3 From 21870efc4aab4732ba2c422ef116597c54e4a8ec Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Thu, 30 Aug 2018 03:09:45 -0400 Subject: patch-delta: fix oob read 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 Signed-off-by: Jeff King Reviewed-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- t/t5303-pack-corruption-resilience.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 't/t5303-pack-corruption-resilience.sh') 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' -- cgit v1.2.3 From fa72f90e7a5cfbbc32860c6336628c96791b5af3 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Thu, 30 Aug 2018 03:10:26 -0400 Subject: patch-delta: consistently report corruption 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 Signed-off-by: Jeff King Reviewed-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- t/t5303-pack-corruption-resilience.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) (limited to 't/t5303-pack-corruption-resilience.sh') 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 -- cgit v1.2.3 From 9514b0b22624b9a6de80a480ab480d61ce29833b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Aug 2018 03:12:52 -0400 Subject: patch-delta: handle truncated copy parameters 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 Reviewed-by: Nicolas Pitre Signed-off-by: Junio C Hamano --- t/t5303-pack-corruption-resilience.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 't/t5303-pack-corruption-resilience.sh') 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 && -- cgit v1.2.3 From 18f60f2d3dba291e1ea8e2751c1fce70580de8a9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Aug 2018 15:13:39 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- t/t5303-pack-corruption-resilience.sh | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 't/t5303-pack-corruption-resilience.sh') 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 -- cgit v1.2.3