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
