Re: Possible integer overflow parsing malformed objects in git 2.10.0

[Gustavo Grieco <gustavo.grieco@imag.fr>]

----- Original Message -----
> 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.

Great, it is exactly the type of analysis i was expecting. 

> 
> 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.

That is reasonable, indeed. As we discussed, this type of issues are very unlikely to be easily exploited (or even possible), so i did not think that it could be irresponsible to post this issue here. I will be happy to post only in git-security if you think my reports can uncover security issues (but so far, it was not the case). It is also interesting to mention that git-security is not linked anywhere in the official website (git-scm.com) or the github repository (github.com/git/git).

> 
>  * 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.

I try to discard false positives as much as possible. Despite we just started adding the git generation module to our tool, we got hundreds of aborts caused by the allocation limits of AddressSanitizer: I only reported the one which specifically contains a size that could be interpreted as negative number (in fact, valgrind will also report this issue a 'fishy size' for malloc).
Also, I think my reports should are clear enough to reproduce any issue and i carefully check every test case for reproducibility, still i am open to suggestion on how to improve my bug reports! 

> 
> Thanks.
> 
>