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

[Derrick Stolee <derrickstolee@github.com>]

On 4/26/2022 2:31 PM, Carlo Marcelo Arenas Belón wrote:
> bdc77d1d685 (Add a function to determine whether a path is owned by the
> current user, 2022-03-02) checks for the effective uid of the running
> process using geteuid() but didn't account for cases where that uid was
> root (because git was invoked through sudo or doas) and the effetive id

s/effetive/effective

> that repositiry trusted for its config was different, therefore failing

s/repositiry/repository

> the following common call:
> 
>   guy@renard ~/Software/uncrustify $ sudo git describe --always --dirty
>   [sudo] password for guy:
>   fatal: unsafe repository ('/home/guy/Software/uncrustify' is owned by someone else)
> 
> Attempt to detect those cases by using the environment variables that
> those tools create to keep track of the original user id, and do the
> ownership check using those instead.
> 
> This assumes the environment the user is running with after going
> priviledged can't be tampered with, and also does the check only for

s/priviledged/privileged 

> root to keep the most common case less complicated, but as a side effect
> will miss cases where sudo (or an equivalent) was used to change to
> another unpriviledged user.

s/unpriviledged/unpriviledged 

> Sent as an RFC as it has been only lightly tested and because some of
> the assumptions I have to make, made me unconfortable.

 
> Ex, in order to make the atoi() calls safe, I was originally doing
> is_digit(), but that would require this function to move further down
> to work.
>
> It is also now big enough that would make sense for it to move into
> its own compat file and outside for git-compat-util.h, but if that is
> done we might not keep the "root uid is not always 0" bits that seem
> useful to have for the future.
>
> getent() is not thread safe, so it might be worth to use an alternative
> but that would require a bigger change.

These points make sense. It would be worth taking our time and
doing some refactoring in advance of this functional change.
 
> IMHO it should have a test added, but not sure where it would fit.

I wonder how we could test a multi-user scenario. The tests I
added in e47363e5a (t0033: add tests for safe.directory, 2022-04-13)
purposefully avoid the user-id functionality.

> Original discussion in :
> 
>   https://lore.kernel.org/git/4ef9287b-6260-9538-7c89-cffb611520ee@maurel.de/

I agree that the idea behind this change is a good one. The escalation
of privilege isn't a huge concern when the "real" user is the same.
The only way to trick the root user here is to set an environment
variable, in which case they might as well modify PATH and be done with
it.

> +	euid = geteuid();
> +	if (euid == ROOT_UID) {
> +		/* we might have raised our priviledges with sudo or doas */

Similar spelling error here.

> +		const char *real_uid = getenv("SUDO_UID");
> +		if (real_uid && *real_uid)
> +			euid = atoi(real_uid);
> +		else {
> +			real_uid = getenv("DOAS_UID");
> +			if (real_uid && *real_uid)
> +				euid = atoi(real_uid);
> +		}

I imagine that something else could be added here to help Windows
users who have elevated to administrator privileges. It will use a
completely different mechanism, though, if needed at all. We can
delay that for now.

Thanks,
-Stolee