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 fix. 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 instead. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
