Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 26, 2016, 00:10 UTC
- Message-ID
- <xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1825523389.8224664.1474812766424.JavaMail.zimbra@imag.fr>
Gustavo Grieco <gustavo.grieco@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++; }