Re: [PATCH 1/3] patch-delta: fix oob read

[Jeff King <peff@peff.net>]

On Wed, Aug 29, 2018 at 05:20:25PM -0400, Jeff King wrote:

> Nice catch. The patch looks good to me, but just to lay out my thought
> process looking for other related problems:
> 
> We have two types of instructions:
> 
>   1. Take N bytes from position P within the source.
> 
>   2. Take the next N bytes from the delta stream.
> 
> In both cases we need to check that:
> 
>   a. We have enough space in the destination buffer.
> 
>   b. We have enough source bytes.

Actually, there's one other case to consider: reading the actual offset
for a copy operation. E.g., if we see '0x80' at the end of the delta,
I think we'd read garbage into cp_offset? That would typically generate
nonsense that would be caught by the other checks, but I think it's
possible that the memory could happen to produce a valid copy
instruction from the base, leading to a confusing result (though it
would still need to match the expected result size).

Thinking on it more, one other interesting tidbit here: in actual git
code (i.e., not test-delta), we'd always be reading from an mmap'd
packfile. And we're guaranteed to have at least 20 trailing bytes in the
mmap due to the pack format (which is enforced when we parse the pack).

So I don't think this can ever read truly out-of-bounds memory, though
obviously it's something we should fix regardless.

-Peff