On Fri, Dec 20, 2019 at 09:00:40AM -0800, Elijah Newren wrote: > > > > > + 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. > 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@xxxxxxxxxxxxxx/). 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
