Web lists-archives.com

Re: What's cooking in git.git (Jan 2019, #01; Mon, 7)

On Wed, Jan 09, 2019 at 10:06:08PM +0100, Martin Ågren wrote:
> I found some more time to look into this.
> It seems we have a buffer with raw data and we set up a `struct
> object_id *` pointing into it, at a (supposed) OID value. Then
> `update_tree_entry_internal()` verifies that the buffer contains
> sufficiently many bytes, i.e., at least `the_hash_algo->rawsz` (=20).
> We immediately call `oidset_insert()` which copies an entire struct,
> i.e., we copy sizeof(struct object_id) (=32) bytes. Which is 12 more
> than what is known to be safe. For this particular input data, we read
> outside allocated memory.

Anything pointing to a struct object_id has to support at least
GIT_MAX_RAWSZ bytes, and that code doesn't, because it's a tree buffer.

I ran into this later on in my SHA-256 work and have a series that fixes
the tree-walk code, but it's a bit involved and requires copying the
struct object_id out of the buffer.

I thought we were going to be triggering this case only with some new
code I was introducing, but apparently somebody else got there first.

> I can think of three possible approaches:
> * Allocate with a margin (GIT_MAX_RAWSZ - the_hash_algo->rawsz) where
>   "necessary" (TM). Maybe not so maintainable.

I think there are actually several places where we allocate for these
buffers, so this is not likely to be a great solution. Even worse, in
some cases, we intentionally use a too-short buffer knowing that we'll
never dereference the data.

> * Teach `oidset_insert()` (i.e., khash) to only copy
>   `the_hash_algo->rawsz` bytes. Maybe not so good for performance.

This is probably the best fix for the moment if you want an immediate

As for my series, I'll need to run the testsuite on it, but I'll try to
get it out tonight or at the latest tomorrow if people want to use that
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Attachment: signature.asc
Description: PGP signature