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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Apr 28, 2022, 17:56 UTC
Message-ID: <fabb3940-7d6d-efb4-dba0-07531caf9d5e@gmail.com>
In-Reply-To: <xmqqzgk6l5ly.fsf@gitster.g>

On 27/04/2022 19:54, Junio C Hamano wrote:

[ I've trimmed your discussion for brevity but thanks for taking the time to write it out, it was helpful in clarifying what we're trying to protect ourselves against]

Show 17 quoted lines

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

I think that unsigned long should be wide enough. On x86 linux uid_t is uint32_t for both 32 & 64 bit processors. That means that we could truncate the value we read from SUDO_UID when long is 64 bits if it is cast to uid_t but we're supposed to be able to trust that SUDO_UID is a valid id so that shouldn't be a problem. If an attacker can change what's in SUDO_UID we're doomed anyway.

Best Wishes

Phillip