Re: [PATCH v4 6/8] dir: fix checks on common prefix directory

From: Jeff King <peff@peff.net>
Date: Dec 20, 2019, 21:14 UTC
Message-ID: <20191220211400.GB2752079@coredump.intra.peff.net>
In-Reply-To: <CABPp-BEe7=LfR5KJriu1Lq-v5SVET-zm3GK3ZPjOYuLHrxqimg@mail.gmail.com>

On Fri, Dec 20, 2019 at 09:00:40AM -0800, Elijah Newren wrote:

Show 11 quoted lines

> > > > > +     de = xcalloc(1, sizeof(struct dirent)+len+1);
> > > >
> > > > That "+len+1" may deserve a comment?
> > >
> > > Good point, I'll add one and send a re-roll.
> >
> > Please use st_add3() while you are at it.
> 
> I would, but Junio already took the patches and applied them to next
> already.  (I am curious, though, why we're worried about overflow in a
> context like this?)

If len is large enough to cause integer overflow when computing the total size, then we'd allocate a too-small buffer (and then later overflow the buffer when writing into it).

I'm not sure how possible that is here. On 32-bit systems, overflowing size_t only needs 4GB. you're not likely to have a 4GB path on a filesystem, but malicious folks could shove them into a tree. I'm not sure if this code could be triggered for anything that doesn't actually exist on the filesystem, though.

You're also not likely to actually manage to store a 4GB string in "path" on a 32-bit system in the first place. But "len" is actually an "int". On a 64-bit system it would be easy to do, though, and int is still 32 bits there. But because the result of sizeof() is a size_t, I think the int will be promoted as well during the addition (and assuming it's not negative, will be too small to overflow). (Also, the "len" parameter probably should to be a size_t in the first place, but that's not new).

So I don't think it's exploitable, but as you can see there's a bit of pondering to see that it's so. When I audit I usually look for something like /x[mc]alloc.*[+*] / to find potential problem spots. Even if we're pretty sure a particular site isn't vulnerable, marking it with st_add() errs on the safe side, and makes those audits easier.

Show 9 quoted lines

> It's possible I vastly overestimated how much work ripping out the
> dirent would be; I mean I've mis-estimated absolutely everything in
> dir.c and assumed each "little" thing would all be a small amount of
> work, so maybe I'm just swinging the pendulum too far the other way.
> But, although I think this alternative would be the cleanest, I saw a
> couple things that looked like this was going to turn into a huge can
> of worms when I started to peek at what it all touched.  I'd be happy
> for someone to take this route, but it won't be me (see also
> https://lore.kernel.org/git/CABPp-BEkX9cH1=r3dJ4WLzcJKVcF-KpGUkshL34MMp3Xhhhpuw@mail.gmail.com/).

OK. I certainly don't insist on this direction. I just saw the portability issues and wondered how bad it would be to do so. Hence the patch I sent, which I _think_ is correct, but I really don't know the dir.c code very well. And I'm sure it will not surprise you that I have generally been confused and/or frightened by it when I do look at it. :)

-Peff