Re: [PATCH V2 1/2] Fix delta integer overflows
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 11, 2017, 18:43 UTC
- Message-ID
- <xmqqy3qqymgm.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170810203612.lt342yq3gnfadjlp@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Thu, Aug 10, 2017 at 01:07:07PM -0700, Junio C Hamano wrote: > >> Perhaps we should teach the receiving end to notice that the varint >> data it reads encodes a size that is too large for it to grok and >> die. With that, we can safely move forward with whatever size_t >> each platform uses. > > Yes, this is very important even for "unsigned long". I'd worry that > malicious input could cause us to wrap to 0, and we'd potentially write > into a too-small buffer[1]. > > There's some prior art with checking this against bitsizeof() in > unpack_object_header_buffer() but get_delta_hdr_size() does not seem to > have a check. > > -Peff > > [1] In most cases it's _probably_ not a vulnerability to wrap here, > because we'd just read less data than we ought to. But it makes me > nervous nonetheless.
As I said in my other message in the thread, as long as the callers of get_delta_hdr_size() are written correctly, it should be OK. And patch_delta() should be OK, even for "unsigned long" when it is too small. It just will not produce correct result and instead abort, and the patch under discussion fixes that.