[PATCH 0/6] loose object size checks and a few size truncations

[Elia Pinto <gitter.spiros@gmail.com>]

These patches come from the same search for truncated sizes on LLP64
(64-bit Windows, where "long" is 32 bits but size_t is 64 bits) as the
"use size_t for a few more object and buffer sizes" series:

  https://lore.kernel.org/git/cover.1793352943.git.gitter.spiros@gmail.com/

Its cover letter explains the method (a MinGW cross build and
clang-tidy with -Wshorten-64-to-32). For these I followed each value
back to where it comes from and forward to where it is used, because
they can lead to out-of-bounds memory accesses. For that reason I sent
all of them but 3/6 to the git-security list first; they do not
consider them security issues, so here they are. While checking the
loose object reader I also found two missing size checks that have
nothing to do with LLP64 (1/6 and 3/6).

 1/6 A loose object whose zlib stream is shorter than the size in its
     header is accepted, and the uninitialized tail of the buffer is
     handed out as object content (all platforms; e.g. "git cat-file
     commit" prints heap memory). A longer stream is silently cut.

 2/6 On Windows the size from the loose object header is truncated, so
     the caller gets a buffer much smaller than the size it is told,
     and reads beyond its end.

 3/6 The same check as 1/6 for the streaming reader of loose objects,
     which today streams out a short or long object without an error.

 4/6 "git cat-file --batch" writes memory past the end of objects
     between 2GiB and 4GiB (all platforms; non-blob objects, or blobs
     with --filters/--textconv).

 5/6 On Windows, concatenating notes that add up to 4GiB or more
     overflows a heap buffer.

 6/6 On Windows, diffing a 2-4GiB file as text reads one byte before
     its buffer.

1/6 and 3/6 come with tests, which are cheap, and fail without the
fix. The others would need objects of several GiB (and 2/6, 5/6 and
6/6 a 32-bit "long"), so their commit messages describe how to
reproduce them instead. I have not run anything on Windows: the
Windows-only issues are based on reading the code and on the
cross-compiled build.

5/6 passes the combined size to odb_write_object() through
cast_size_t_to_ulong(), as the length parameter is still an "unsigned
long" on master. With 1/4 of the series above, which makes it a
size_t, the cast becomes unnecessary but harmless; the two series do
not conflict.

The series is based on master (b9e5cff180).

Elia Pinto (6):
  object-file: check size of inflated loose object against its header
  object-file: use size_t for loose object sizes
  odb/source-loose: check object size when streaming loose objects
  cat-file: use size_t for the length passed to batch_write()
  notes: avoid truncating the size of concatenated notes
  diff: use size_t in count_trailing_blank()

 builtin/cat-file.c           |  2 +-
 diff.c                       |  2 +-
 notes.c                      |  7 +++----
 object-file.c                | 29 ++++++++++++++++++-----------
 object-file.h                |  6 +++---
 odb/source-loose.c           | 22 ++++++++++++++++++----
 t/lib-loose.sh               |  5 ++++-
 t/t1006-cat-file.sh          | 18 ++++++++++++++++++
 t/t1060-object-corruption.sh | 28 ++++++++++++++++++++++++++++
 9 files changed, 94 insertions(+), 25 deletions(-)


base-commit: b9e5cff1803ab24582201ef059516eb16014b9d9
-- 
2.53.0