From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Gustavo Grieco <gustavo.grieco@imag.fr>, git@vger.kernel.org
Subject: Re: Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0
Date: Mon, 26 Sep 2016 09:50:55 -0400 [thread overview]
Message-ID: <20160926135055.zeagw6mj3udzhv7s@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqbmzbwmfc.fsf@gitster.mtv.corp.google.com>
On Sun, Sep 25, 2016 at 05:10:31PM -0700, Junio C Hamano wrote:
> 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.
Yeah. I also was surprised that we didn't barf on a zlib failure. But
based on previous debugging of corrupted zlib data, my recollection
is that there are a large number of weird corruptions that zlib will
happily pass back and only later complain about a checksum error. So
presumably "x" is one of those, and it might not hold for other
corruptions (but I didn't try).
> 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.
Yeah, that seems wrong. I don't think it would involve an out of bounds
read, but we probably could fail to correctly report zlib corruption.
> 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;
This doesn't look too invasive as an approach, though I would have done
it differently. We're making the assumption that once there is a NUL,
the header-parser won't do anything stupid, which creates a coupling
between those two bits of code. My inclination would have been to just
treat the header as a ptr/len pair, and make sure the parser never reads
past the end.
But I implemented that, and it _is_ rather invasive. And it's not like
coupling unpack_sha1_header() and parse_sha1_header() is all that
terrible; they are meant to be paired.
I haven't read through your follow-up yet; I'll do that before posting
my version.
> 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++;
> }
We keep reading from hdr after this, though I think those bits would all
bail correctly on seeing NUL.
-Peff
next prev parent reply other threads:[~2016-09-26 13:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1399913289.8224468.1474810664933.JavaMail.zimbra@imag.fr>
2016-09-25 14:12 ` Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0 Gustavo Grieco
2016-09-26 0:10 ` Junio C Hamano
2016-09-26 4:29 ` [PATCH] unpack_sha1_header(): detect malformed object header Junio C Hamano
2016-09-26 14:03 ` Jeff King
2016-09-26 16:15 ` Junio C Hamano
2016-09-26 17:33 ` Junio C Hamano
2016-09-26 17:35 ` Jeff King
2016-09-26 17:39 ` Junio C Hamano
2016-09-26 17:34 ` Junio C Hamano
2016-09-26 17:38 ` Jeff King
2016-09-26 13:50 ` Jeff King [this message]
2016-09-26 17:48 ` Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0 Gustavo Grieco
2016-09-26 17:55 ` Junio C Hamano
2016-09-26 18:01 ` Gustavo Grieco
2016-09-26 18:06 ` Junio C Hamano
2016-09-26 18:10 ` Junio C Hamano
2016-09-27 2:13 ` Gustavo Grieco
2016-09-27 7:19 ` Jeff King
2016-09-27 2:30 ` Possible integer overflow parsing malformed objects in " Gustavo Grieco
2016-09-27 8:07 ` Jeff King
2016-09-27 15:57 ` Junio C Hamano
2016-09-27 19:14 ` Gustavo Grieco
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160926135055.zeagw6mj3udzhv7s@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=gustavo.grieco@imag.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).