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

From: Jeff King <peff@peff.net>
Date: Aug 29, 2018, 22:18 UTC
Message-ID: <20180829221838.GA10400@sigill.intra.peff.net>
In-Reply-To: <20180829212025.GB29880@sigill.intra.peff.net>

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

Show 14 quoted lines

> 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