On May 9, 2022 2:49 PM, Carlo Arenas wrote: >On Mon, May 9, 2022 at 9:54 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> >> It is not just "not likely", but such an attack, with a potential >> victim not futzing with SUDO_UID environment themselves, would not >> work at all. The value of SUDO_UID and the original uid of the >> potential victim by definition should fit in the uid_t type. So if >> you, the aspiring cracker, have UID 1000, nobody else on the system >> has UID that is congruent modulo uid_t and wrap-around attack does not >> exist. As long as the type we use to read SUDO_UID string into a >> variable is not narrower than uid_t, there. > >correct; would adding a less elegant "static_assert" (obviously not available in C99) >in the code an alternative?, or you are suggesting that documenting that constrain >should be done in a comment and hope it doesn't get ignored in the future, or >maybe gets instead made less likely to fail by some additional work, like the one I >suggested to do later by moving this code around and using intmax_t instead? > >not that there are 2 different scenarios which we might seem to be flip flopping >about : > >1) victim gets attacked by tricking them into using the wrong SUDO_UID > >unlikely to be useful as you pointed out, and totally useless if we still restrict this >code to run only as root > >2) attacker using this code to elevate privileges themselves > >only useful if this code is not run as root. > >My concern again, is not with this code AS-IS but how it might be used in the >future. > >> Of course you can tell any user who runs "sudo" to set SUDO_UID to >> 1000 + 64k and cause wrap-around, but then you can tell them to set >> SUDO_UID to 1000 without relying on wrap-around and have the same >> effect. So, let's stop worrying about this bogus scenario. > >bogus only if we are still only running this code as root, of course. > >> As to the "we can break compilation with -Wsign-compare on a system >> with signed uid_t", I agree that is true if we have > >Apologies for not documenting it until now, but I had -Wtautological-constant-out- >of-range-compare in mind instead, but your are correct either one would work >and the point was that (without having to add even an "static assert") we were >able to find them and warn them that they need to patch. > >> env_id <= (uid_t) -1 > >If that was not enough, that simple code ALSO disabled the code in that case to >make sure they MUST patch locally if they need to make it work, or come back to >us to figure out a portable way to accommodate them in the future, with all the >information about their system we currently lack. > >> there. But I am not sure if that is the most effective way to smoke >> out platforms where this code has trouble working correctly. Also, I >> would think that a system with signed uid_t is a possibility, but a >> user with a negative UID? > >It doesn't need to be a real user (specially if someone independently decides to >remove the restriction that keeps this code available only to root). > >The fact that it was negative but was treated as a positive number on our code just >makes the wraparound we decided to ignore before more likely to work without >triggering alarms somewhere else and because we decided to ignore an unlikely to >be useful positive wraparound, we also would fall from the negative wraparound >here that would had protected this code from both if that humble line of code >wouldn't had been removed. > >In your hypothetical system where uid_t is 16-bit (hence 15-bit for valid non- >negative ids if uid_t is signed, since no sane system would have negative ones), >either 65536 or -65536 would become 0, as well as at least the other 2^16 >possibilities that a larger long would provide. > >If the size difference is even smaller (ex: uid_t is signed int), so the type we are >using to parse those values is only 1 bit wider it will still be a concern IMHO. Just to be pedantic uid_t is signed int is not always smaller. It is 32-bit on NonStop. uid_t signed short or signed char would be smaller. I have wondered why uid_t was not defined as unsigned int or unsigned long (although unsigned long is 64 bits on the x86 wide model NonStop) because they cannot be negative. Making assumptions about size or sign when doing this check is not correct IMHO. It should be a direct comparison of env_id != ROOT_UID, where you know what ROOT_UID is on the specific platform. I do not like the <= concept for user id comparison because it is making assumptions. Structurally, on NonStop, (uid & 0x0000FF00) == 0x0000FF00 can be used to check whether the user is in the root group but that is coincidental and subject to change without notice. No one worth their salt does that comparison on platform, rather they compare getgid() == 255 to do that test. > >I know people that wrote code to check if "/" was writable as means to make sure >they were "root", because that is what any sane system would do, and then came >Windows and even in 2022 anyone can write to "/" >there and create subdirectories. On NonStop: / is often writable by non-root users in the root group. Non-root users in the root group sometimes have repositories. In cygwin, / is owned by the user who installed cygwin or the OS rather than root. It is a relatively random number, and definitely not 0. It is also possible that a dedicated VM in Linux can be spun up for sandbox testing allowing anyone to write anywhere. Even if you run git as Administrator on Windows 10+, it will not have the userId of 0 in cygwin git. I do not think the / writable assumption is portable.
