Am 22.01.23 um 08:48 schrieb Jeff King:

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

quoted

Am 19.01.23 um 02:39 schrieb Jeff King:

quoted

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

quoted

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.

Sure.

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.

Solvable by limiting the search for the end of the string in
decode_tree_entry() by using strnlen(3) or memchr(3) instead of
strlen(3).  You just need to define some (configurable?) limit.

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.

Makes sense.

quoted

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.

quoted

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.

That's a cup half full/empty thing, perhaps.  What's the harm in leading
zeros? vs. Why allow leading zeros?

René