Re: [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 27/04/2022 16:58, Carlo Arenas wrote:
>> On Wed, Apr 27, 2022 at 5:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> You are ignoring any errors when parsing the environment variable - that
>>> is not a good idea in a security check.
>> which errors are you concerned about?, if anything in this code
>
> I was confused by the fact that the helper function returns a
> success/failure value which we ignore. However euid is not overwritten 
> if strtol fails so it is safe I think.
>
>> worries me from a security point of view is the fact that we are
>> relying in getenv not being racy (as mentioned in the original RFC),
>> but there are no errors set there AFAIK.
>> not ignoring errno in strtol is an option, but as mentioned before I
>> decided instead to reject bogus values and therefore not the clobber a
>> previous errno,
>
> strtol() will set errno if there is a range error ignoring it does not
> change that. In any case is_path_owned_by_current_uid() already
> clobbers errno if stat() fails.

IOW we are not preserving errno anyway, so ignoring errno (or
resetting before calling strtol) does not by us anything.

We probably should step back a bit, take a deep breath and think
what we are trying to protect our users against.  When I said early
in the discussion how much we trust the value in SUDO_UID, I hoped
people would do so, and I was especially hoping such a discussion to
happen when they realized that the width of uid_t is unknown and
there can be truncations in either direction.

So let's start one now.

If we didn't trust the value in SUDO_UID, what valid use case do we
help, and what avenue of attack do we open ourselves to?  This is
not a suggestion of an alternative, but is a discussion starter to
illustrate the line along which we want to think about the issues.

The original "sudo make install" (which runs "git describe" inside)
use case does not really care.  The process running with SUDO_UID is
not spelunking into random directories uncontrollably without being
aware that there may be hostile repositories.  The directories they
visit are part of legitimate journey "make install" takes.

The "sudo sh to get a shell, chdir to /var/tmp/foo to do something"
use case does care---it needs to make sure that whereever it goes is
not part of a hostile repository.  So "if SUDO_UID is there, anything
goes" is not a good protection for such a use case.

If we read SUDO_UID into an int16_t and uid_t happens to be much
wider than that type, then what happens?  Again, I am not advocating
to deliberately use shorter type to cause truncation.  This is
merely to illustrate how much truncation matters.

The "sudo make install" use case may be harmed unless the comparison
is done carefully.  If the repository owner's UID is beyond 32k then
asking if (st.st_uid == SUDO_UID) would say "no" due to truncation
and refuse access to the legitimate user.  If we compare both after
casting them to int16_t then the repository owner will be allowed in
again.  A friendly stranger who happens to share the low 16-bit of
UID will also be allowed to install from the repository, but that is
not an intended consequence.

The "sudo sh and go spelunking" use case is weakend by truncation.
It is protected only if the victim's UID does not share the lower
16-bit with the owner of hostile repository.

So what attack does this allow?  An attacker needs to learn their
own UID, find another user whose UID is the same modulo 16-bit as a
potential victim, and the victim has to be among those who can run
"sudo".  Then the victim somehow has to be talked into running stuff
in a repository the attacker prepares.

Possible?  Yes.  Practical?  I dunno.  Especially if we do not
deliberately truncate by reading into int16_t but use "int" (32-bit
almost everywhere we care about) or "long".  Now how likely is uid_t
narrower than long?  If it is not likely, then we probably are OK to
use long and not worry about the truncation at all.  Or use strtoll()
to make it even less likely.

So, in short, I think it is reasonable if we did:

 - do the extra "if SUDO_UID exists, then..." only when we are root.

 - use strtol to ensure SUDO_UID is in good shape (all digits) and
   read into long; use errno=0 trick to notice overflows.  err on
   the safe side and do not allow if the value cannot be read
   correctly.

 - do the comparison between st.st_uid and strtol() result by
   casting both to (long).  Without this, st.st_uid has a value that
   needs wider than long, we cannot allow the owner of such a
   repository in (and I somehow feel that is unlikely to happen).

 - or omit the third one --- tough luck for those whose UID is
   beyond what a long can represent (and I somehow feel that this is
   probably OK).

Another thing I'd suggest doing is to have a helper function to do
the "we do a bit more than geteuid() to come up with the allowed
owner of repositories we encounter", and call that function instead
of geteuid().

Thanks.