Re: [PATCH v3 2/3] git-compat-util: avoid failing dir ownership checks if running privileged
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- May 5, 2022, 14:01 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2205051545370.355@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20220503065442.95699-3-carenas@gmail.com>
Hi Carlo,
On Mon, 2 May 2022, Carlo Marcelo Arenas Belón wrote:
Show 22 quoted lines
> 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.
Show 31 quoted lines
> 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.
Show 19 quoted lines
> +
> +/*
> + * 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; > + }
Show 16 quoted lines
> +}
> +
> 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, ...").
Show 18 quoted lines
> } > > #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