Git fork

unpack_loose_header(): fix infinite loop on broken zlib input

When reading a loose object, we first try to expand the first 32 bytes
to read the type+size header. This is enough for any of the normal Git
types. But since 46f034483e (sha1_file: support reading from a loose
object of unknown type, 2015-05-03), the caller can also ask us to parse
any unknown names, which can be much longer. In this case we keep
inflating until we find the NUL at the end of the header, or hit
Z_STREAM_END.

But what if zlib can't make forward progress? For example, if the loose
object file is truncated, we'll have no more data to feed it. It will
return Z_BUF_ERROR, and we'll just loop infinitely, calling
git_inflate() over and over but never seeing new bytes nor an
end-of-stream marker.

We can fix this by only looping when we think we can make forward
progress. This will always be Z_OK in this case. In other code we might
also be able to continue on Z_BUF_ERROR, but:

- We will never see Z_BUF_ERROR because the output buffer is full; we
always feed a fresh 32-byte buffer on each call to git_inflate().

- We may see Z_BUF_ERROR if we run out of input. But since we've fed
the whole mmap'd buffer to zlib, if it runs out of input there is
nothing more we can do.

So if we don't see Z_OK (and didn't see the end-of-header NUL, otherwise
we'd have broken out of the loop), then we should stop looping and
return an error.

The test case shows an example where the input is truncated (which gives
us the input Z_BUF_ERROR case above).

Although we do operate on objects we might get from an untrusted remote,
I don't think the security implications of this bug are too great. It
can only trigger if both of these are true:

- You're reading a loose object whose on-disk representation was
written by an attacker. So fetching an object (or receiving a push)
are mostly OK, because even with unpack-objects it is our local,
trusted code that writes out the object file. The exception may be
fetching from an untrusted local repo, or using dumb-http, which
copies objects verbatim. But...

- The only code path which triggers the inflate loop is cat-file's
--allow-unknown-type option. This is unlikely to be called at all
outside of debugging. But I also suspect that objects with
non-standard types (or that are truncated) would not survive the
usual fetch/receive checks in the first place.

So I think it would be quite hard to trick somebody into running the
infinite loop, and we can just fix the bug.

Co-authored-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

authored by

Jeff King
Taylor Blau
and committed by
Junio C Hamano
b748ddb7 e7ac344d

+20 -1
+1 -1
object-file.c
··· 1307 1307 strbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer); 1308 1308 if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) 1309 1309 return 0; 1310 - } while (status != Z_STREAM_END); 1310 + } while (status == Z_OK); 1311 1311 return ULHR_BAD; 1312 1312 } 1313 1313
+19
t/t1006-cat-file.sh
··· 817 817 ) 818 818 ' 819 819 820 + test_expect_success 'truncated object with --allow-unknown-type' - <<\EOT 821 + objtype='a really long type name that exceeds the 32-byte limit' && 822 + blob=$(git hash-object -w --literally -t "$objtype" /dev/null) && 823 + objpath=.git/objects/$(test_oid_to_path "$blob") && 824 + 825 + # We want to truncate the object far enough in that we don't hit the 826 + # end while inflating the first 32 bytes (since we want to have to dig 827 + # for the trailing NUL of the header). But we don't want to go too far, 828 + # since our header isn't very big. And of course we are counting 829 + # deflated zlib bytes in the on-disk file, so it's a bit of a guess. 830 + # Empirically 50 seems to work. 831 + mv "$objpath" obj.bak && 832 + test_when_finished 'mv obj.bak "$objpath"' && 833 + test_copy_bytes 50 <obj.bak >"$objpath" && 834 + 835 + test_must_fail git cat-file --allow-unknown-type -t $blob 2>err && 836 + test_grep "unable to unpack $blob header" err 837 + EOT 838 + 820 839 # Tests for git cat-file --follow-symlinks 821 840 test_expect_success 'prep for symlink tests' ' 822 841 echo_without_newline "$hello_content" >morx &&