RE: [RFC PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged

>On Mon, May 9, 2022 at 9:54 AM Junio C Hamano <gitster@pobox.com> 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.