From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Gustavo Grieco <gustavo.grieco@imag.fr>, git@vger.kernel.org
Subject: Re: Possible integer overflow parsing malformed objects in git 2.10.0
Date: Tue, 27 Sep 2016 08:57:53 -0700 [thread overview]
Message-ID: <xmqqponpqqri.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160927080755.evlq3sfkyoolixop@sigill.intra.peff.net> (Jeff King's message of "Tue, 27 Sep 2016 04:07:55 -0400")
Jeff King <peff@peff.net> writes:
> That being said, the parse_sha1_header() function clearly does not
> detect overflow at all when parsing the size. So on a 32-bit system, you
> end up with:
>
> $ git fsck
> fatal: Out of memory, malloc failed (tried to allocate 4294967141 bytes)
>
> which is not correct, but I'm not sure it's a security problem. Integer
> overflows are an issue if they cause us to under-allocate, and then to
> write more bytes than we allocated. In this case, I would expect
> unpack_sha1_rest() to never write more bytes than the "size" we parsed
> and allocated (and to complain if the number of bytes we get from the
> zlib sequence do not exactly match the claimed size).
>
> So a more interesting example is more like "ULONG_MAX + 5", where we
> would overflow to 5 bytes. And we'd hope that unpack_sha1_rest does not
> ever write more than 5 bytes. From my reading and a few tests with gdb,
> it does not. However, it also does not notice that there were more bytes
> that we didn't use.
>
> So I think there's room for improved diagnosis of bogus situations
> (including integer overflows), but I don't see any actual security bugs.
I agree with the overall conclusion. This does look like an attempt
to throw random fuzz at Git and see if and how it breaks, and in this
particular one Git is simply doing the right thing (and the fault lies
in the way how ASAN is used and how its result was interpreted).
Throwing random fuzz to see what breaks is not a bad thing to do
per-se, but anybody who does so without wearing a black hat needs to
keep two things in mind:
* When a random fuzz attempt does uncover a security issue,
reporting it here on this list is a grossly irresponsible way to
disclose the issue. We have the git-security list for that.
* A random fuzz may stop Git and that may be perfectly legit thing
to happen, e.g. the data may request a large but still valid
amount of memory to be allocated that happens not to fit in the
hardware the fuzz attempt is being run, and xmalloc() may detect
the situation and die, like the above example. False positives
are expected and you want to make sure you cull them before
making your reports. Otherwise, they will unnecessary burden
people who are doing real work, i.e. reproduce and correct
problems that may be security related that are irresponsibly
disclosed here quickly enough to minimize damage.
Thanks.
next prev parent reply other threads:[~2016-09-27 15:58 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 ` Stack read out-of-bounds in parse_sha1_header_extended using git 2.10.0 Jeff King
2016-09-26 17:48 ` 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 [this message]
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=xmqqponpqqri.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=gustavo.grieco@imag.fr \
--cc=peff@peff.net \
/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).