Re: [RFC/PATCH 0/6] hash-object: use fsck to check objects

From: Jeff King <peff@peff.net>
Date: Jan 22, 2023, 07:48 UTC
Message-ID: <Y8zqZH+X6fOoCOYV@coredump.intra.peff.net>
In-Reply-To: <97faa323-a5b9-e459-70d7-3f6318446898@web.de>

On Sat, Jan 21, 2023 at 10:36:08AM +0100, René Scharfe wrote:

Show 9 quoted lines

> Am 19.01.23 um 02:39 schrieb Jeff King:
> >
> > Though I do find the use of strlen() in decode_tree_entry()
> > a little suspicious (and that would be true of the current code, as
> > well, since it powers hash-object's existing parsing check).
> 
> strlen() won't overrun the buffer because the first check in
> decode_tree_entry() makes sure there is a NUL in the buffer ahead.
> If get_mode() crosses it then we exit early.

Yeah, that was what I found after digging deeper (see my patch 7).

Show 6 quoted lines

> Storing the result in an unsigned int can overflow on platforms where
> size_t is bigger.  That would result in pathlen values being too short
> for really long paths, but no out-of-bounds access.  They are then
> stored as signed int in struct name_entry and used as such in many
> places -- that seems like a bad idea, but I didn't actually check them
> thoroughly.

Yeah, I agree that the use of a signed int there looks questionable. I do think it's orthogonal to my series here, as that tree-decoding is used by the existing hash-object checks.

But it probably bears further examination, especially because we use it for the fsck checks on incoming objects via receive-pack, etc, which are meant to be the first line of defense for hosters who might receive malicious garbage from users.

We probably ought to reject trees with enormous names via fsck anyway. I actually have a patch to do that, but of course it depends on decode_tree_entry() to get the length, so there's a bit of chicken-and-egg. We probably also should outright reject gigantic trees, which closes out a whole class of integer truncation problems. I know GitHub has rejected trees over 100MB for years for this reason.

> get_mode() can overflow "mode" if there are too many octal digits.  Do
> we need to accept more than two handfuls in the first place?  I'll send
> a patch for at least rejecting overflow.

Seems reasonable. I doubt there's an interesting attack here, just because the mode isn't used to index anything. If you feed a garbage mode that happens to overflow to something useful, you could just as easily have sent the useful mode in the first place.

> Hmm, what would be the performance impact of trees with mode fields
> zero-padded to silly lengths?

Certainly it would waste some time parsing the tree, but you could do that already with a long pathname. Or just having a lot of paths in a tree. Or a lot of trees.

-Peff