Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0

[Junio C Hamano]

Gustavo Grieco <gustavo.gri...@imag.fr> writes:

> We found a stack read out-of-bounds parsing object files using git 2.10.0. It 
> was tested on ArchLinux x86_64. To reproduce, first recompile git with ASAN 
> support and then execute:
>
> $ git init ; mkdir -p .git/objects/b2 ; printf 'x' > 
> .git/objects/b2/93584ddd61af21260be75ee9f73e9d53f08cd0

Interesting.  If you prepare such a broken loose object file in your
local repository, I would expect that either unpack_sha1_header() or
unpack_sha1_header_to_strbuf() that sha1_loose_object_info() calls
would detect and barf by noticing that an error came from libz while
it attempts to inflate and would not even call parse_sha1_header.

But it is nevertheless bad to assume that whatever happens to
inflate without an error must be formatted correctly to allow
parsing (i.e. has ' ' and then NUL termination within the first 32
bytes after inflation), which is exactly what the hdr[32] is saying.

Perhaps we need something like the following to tighten the
codepath.

Note that this is totally unteseted and not thought through; I
briefly thought about what unpack_sha1_header_to_strbuf() does with
this change (it first lets unpack_sha1_header() to attempt with a
small buffer but it seems to discard the error code from it before
seeing if the returned buffer has NUL in it); there may be bad
interactions with it.


 sha1_file.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 60ff21f..dfcbd76 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1648,6 +1648,8 @@ unsigned long unpack_object_header_buffer(const unsigned 
char *buf,
 
 int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long 
mapsize, void *buffer, unsigned long bufsiz)
 {
+       int status;
+
        /* Get the data stream */
        memset(stream, 0, sizeof(*stream));
        stream->next_in = map;
@@ -1656,7 +1658,15 @@ int unpack_sha1_header(git_zstream *stream, unsigned 
char *map, unsigned long ma
        stream->avail_out = bufsiz;
 
        git_inflate_init(stream);
-       return git_inflate(stream, 0);
+       status = git_inflate(stream, 0);
+       if (status)
+               return status;
+
+       /* Make sure we got the terminating NUL for the object header */
+       if (!memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
+               return -1;
+
+       return 0;
 }
 
 static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char 
*map,
@@ -1758,6 +1768,8 @@ static int parse_sha1_header_extended(const char *hdr, 
struct object_info *oi,
                char c = *hdr++;
                if (c == ' ')
                        break;
+               if (!c)
+                       die("invalid object header");
                type_len++;
        }