Hi Carlo,

On Mon, 2 May 2022, 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 user was
> root (because git was invoked through sudo or a compatible tool) and the
> original uid that repository trusted for its config was no longer known,
> therefore failing the following otherwise safe 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 that instead.
>
> This assumes the environment the user is running on after going
> privileged can't be tampered with, and also adds code to restrict that
> the new behavior only applies if running as root, therefore keeping the
> most common case, which runs unprivileged, from changing, but because of
> that, it will miss cases where sudo (or an equivalent) was used to change
> to another unprivileged user or where the equivalent tool used to raise
> privileges didn't track the original id in a sudo compatible way.

Hmm. I do realize that this is a quite common scenario, and I wish we
would not need to rush for a fix here: Otherwise we could carefully design
an "untrusted" mode in which Git errors out on spawning user-specified
commands and on writing files (and avoids refreshing the index to avoid
having to write a file), but runs normally if none of that is needed.

> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> index 6d764fe0ccf..ee558ced8c7 100644
> --- a/Documentation/config/safe.txt
> +++ b/Documentation/config/safe.txt
> @@ -26,3 +26,12 @@ directory was listed in the `safe.directory` list. If `safe.directory=*`
>  is set in system config and you want to re-enable this protection, then
>  initialize your list with an empty value before listing the repositories
>  that you deem safe.
> ++
> +When git tries to check for ownership of git repositories, it will
> +obviously do so with the uid of the user that is running git itself,
> +but if git is running as root, it will check first if it might have
> +been started through `sudo`, and if that is the case, will instead
> +use the uid of the user that did so.
> +If that is not what you would prefer and want git to only trust
> +repositories that are owned by root instead, then you should remove
> +the `SUDO_UID` variable from root's environment.
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 63ba89dd31d..dfdd3e4f81a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -393,12 +393,50 @@ static inline int git_offset_1st_component(const char *path)
>  #endif
>
>  #ifndef is_path_owned_by_current_user
> +
> +#ifdef __TANDEM
> +#define ROOT_UID 65535
> +#else
> +#define ROOT_UID 0
> +#endif

I do wonder whether we have to play this kind of fragile game. Why not
simply respect `SUDO_UID` if it is set? It's not like we expect attackers
to have control over the environment and could set this malicously.

> +
> +/*
> + * this helper function overrides a ROOT_UID with the one provided by
> + * an environment variable, do not use unless the original user is
> + * root
> + */
> +static inline void extract_id_from_env(const char *env, uid_t *id)
> +{
> +	const char *real_uid = getenv(env);
> +
> +	/* discard any empty values */
> +	if (real_uid && *real_uid) {
> +		char *endptr;
> +		unsigned long env_id;
> +		int saved_errno = errno;
> +
> +		errno = 0;
> +		env_id = strtoul(real_uid, &endptr, 10);
> +		if (!errno && !*endptr && env_id <= (uid_t)-1)

We should not look at `errno` here unless the return value of `strtoul()`
indicates that there might have been an error (i.e. when it is
`ULONG_MAX`).

Likewise, we need to either initialize `endptr` or only look at it when
`strtoul()` succeeded.

We could side-step all of this, of course, if we simply did this:

	euid = getuid();
	if (euid == ROOT_UID)
		euid = git_env_ulong("SUDO_UID", euid);

> +			*id = env_id;
> +
> +		errno = saved_errno;
> +	}

> +}
> +
>  static inline int is_path_owned_by_current_uid(const char *path)
>  {
>  	struct stat st;
> +	uid_t euid;
> +
>  	if (lstat(path, &st))
>  		return 0;
> -	return st.st_uid == geteuid();
> +
> +	euid = geteuid();
> +	if (euid == ROOT_UID)
> +		extract_id_from_env("SUDO_UID", &euid);
> +
> +	return st.st_uid == euid;

Since this code is not even compiled on Windows, I believe we need to
adjust the documentation accordingly ("On systems other than Windows,
where `sudo` is available, ...").

>  }
>
>  #define is_path_owned_by_current_user is_path_owned_by_current_uid
> diff --git a/t/t0034-root-safe-directory.sh b/t/t0034-root-safe-directory.sh
> index 6dac7a05cfd..dd659aed4e1 100755
> --- a/t/t0034-root-safe-directory.sh
> +++ b/t/t0034-root-safe-directory.sh
> @@ -32,7 +32,7 @@ test_expect_success SUDO 'setup' '
>  	)
>  '
>
> -test_expect_failure SUDO 'sudo git status as original owner' '
> +test_expect_success SUDO 'sudo git status as original owner' '
>  	(
>  		cd root/r &&
>  		git status &&
> --
> 2.36.0.352.g0cd7feaf86f

Again, thank you for working on this!
Dscho