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