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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 27, 2022, 18:54 UTC
Message-ID: <xmqqzgk6l5ly.fsf@gitster.g>
In-Reply-To: <b0a80f47-f441-fc6e-3695-58ea83dea2b8@gmail.com>

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

Show 20 quoted lines

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