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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, May 12, 2022 at 06:00:18PM -0700, 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.

Thanks for working on this!

Unfortunately, I haven't been able to follow the discussion on this
patch series at all, but by a cursory look now I didn't notice any
discussion about what should happen if someone were to use 'sudo' to
access a repository owned by root.  I think it should work, and it did
in fact work in the past, even after bdc77d1d685, but this patch
broke it.

Case in point are tools like 'etckeeper', which keeps track of '/etc'
in a root-owned Git repository, and hooks into package managers to
automatically create a commit when a package installation/update
changes something in '/etc'.  After this patch when 'sudo apt install
<pkg>' invokes 'etckeeper', it complains about the unsafe repository
in '/etc', and doesn't make that commit.


>  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();

Perhaps all we'd need is just a simple condition here:

        if (st.st_uid == euid)
                return 1;

which does make it work again in my manual tests, e.g.:

  $ id -u
  1000
  $ sudo ./git -C /etc/ rev-parse --absolute-git-dir
  /etc/.git

Alas, I couldn't get 't0034-root-safe-directory.sh' work for me at
all, so I'm not sure it doesn't break something else.

And it would also allow 'sudo -u somebody git ...' to access a
repository owned by that somebody, which, I think, should work as
well.

> +	if (euid == ROOT_UID)
> +		extract_id_from_env("SUDO_UID", &euid);
> +
> +	return st.st_uid == euid;
>  }
>  
>  #define is_path_owned_by_current_user is_path_owned_by_current_uid