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

[Carlo Arenas <carenas@gmail.com>]

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.

Carlo