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 <peff@peff.net>
Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/patch-delta.c b/patch-delta.c
index 283fb4b7591a179c9e248ec3d9ac8ac47f3a4c9f..b5c8594db60dd19fbe18b95ea79439c7bad3fa40 100644 (file)
--- 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 41dc947d3f17950f97d0d92e9c6a6b8b89d767b4..b68bbeedccbff453de04ed1d92943dcfeee93d6f 100755 (executable)
--- 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 &&