On Fri, May 06, 2022 at 04:55:36PM -0700, Junio C Hamano wrote: > Carlo Arenas <carenas@xxxxxxxxx> writes: > > > true, but the apparent check for ULONG_MAX (which should have a > > comment added) was really a check not to overflow when assigning the > > value we got into uid_t, by sacrificing an unlikely to be valid > > ULONG_MAX as an uid. > > Are you worried about uid_t wider than ulong? strtoul() with !errno > test covers the case, doesn't it? No, I am worried about uid_t narrower than ulong, which is also the most likely scenatio with a typeof(uid_t) == uint32_t > SUDO_UID cannot have any integer > that cannot be represented in uid_t and if strtoul() does not say > ERANGE, we know whatever value in SUID_UID did not overflow ulong. It is a little subtle, but strtoul doesn't warrant always an ERANGE because it tries to be helpful when given a negative integer and returns instead an equivalent unsigned long as per spec[1] (or in this case the commentary from OpenBSD man page which is also easier to link) "If the minus sign was part of the input sequence, the numeric value calculated from the sequence of digits is negated as if by unary minus in the result type, which applies unsigned integer wraparound rules." > > it ALSO avoids someone trying to sneak a value that would overflow in > > one of the most common configurations we will run where sizeof(long) > > > sizeof(uid_t) && MIN_UID_T >=0, by using an equivalent to MAX_UID_T > > Sorry, -ECANNOTPARSE. If strtoul() can parse everything in uid_t > then where is the room for overflowing? So lets assume a 32bit unsigned uid_t, that wraparounds at 2^32+1, if we get a negative value that is equivalent to something bigger than it, or even a positive value bigger than it, then the assignment will overflow unless we keep it in check by that obviously too clever condition that was removed and we MIGHT even assume an uid_t of 0, which is embarrasing[0]. > We are trying to protect an > unsuspecting user who temporary has become 'root' via sudo, and not > somebody deliberately hurt themselves or others by setting SUDO_UID > deliberately to strange values (once you are 'root', you have easier > ways to hurt other people). You are correct for the current code that even has a big warning telling people NEVER to run that function for anyone other than root, but who knows how this will evolve in the future. Removing it also has other sideeffect, like making this code work in incorrect ways if uid_t is signed, which I mentioned before but probably should had been added as a comment, but that was part of the requirements[2] we had when Phillip argued correctly that I was restricting the valid uid to only half was possible in 32bit systems. FWIW, sudo prints the uid using "%u" so using unsigned long makes more sense and all these problems are unlikely to be practical issues now so I am ok taking your code if you insist, but I still think that the original one was safer in case things change in the future or if there is a platform we currently run on with has signed uid_t, so I will keep it in the RFC with hopefully enough comments to convince you. Carlo [0] https://github.com/systemd/systemd/issues/11026 [1] https://man.openbsd.org/strtoul.3 [2] https://lore.kernel.org/git/CAPUEspjoTYtv9K=rvpkFnyGnEz_uxefED820rx09b6qGG93SqA@xxxxxxxxxxxxxx/
