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

[Jeff King <peff@peff.net>]

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

> 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).

> 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