Re: [PATCH 1/2] Documentation: explain how safe.directory works when running under sudo

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> In a previous patch, the behaviour of git was changed so it will be able
> to find the "effective uid" that is required when git was invoked with
> sudo to root, for example:
>
>   $ sudo make install
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  Documentation/config/safe.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> index 6d764fe0ccf..67f8ef5d766 100644
> --- a/Documentation/config/safe.txt
> +++ b/Documentation/config/safe.txt
> @@ -26,3 +26,11 @@ 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

Comma before "it will obviously".

> +use the user that is being used to run git itself, but if git is running
> +as root, it will first check if it might had been started through `sudo`,
> +and if that is the case, will use the user id that invoked sudo instead.

This raises a design question.  In a repository is owned by root,
shouldn't "sudo git describe" work?  IOW, I am wondering if the
"instead" at the end of the description is what we want, or if we
want to check both the original user and "root".

There is not much point in protecting against a malicious repository
a repository that is owned by "root"---an attacker that can create
such a repository and futz with its config or hooks can attack you
more directly, without social engineering you into using git on such
a repository.  So from that point of view, it may be reasonable to
say that we can trust repositories owned by 

 - euid (both when euid is root and euid is not root)
 - SUDO_UID (when euid is root)

I think.  And even if we adopt such a tweak, ...

> +If that is not what you would prefer and want git to instead only trust
> +repositories that are owned by root, then you should remove the `SUDO_UID`
> +variable from root's environment.

... this is still true.

I guess the necessary tweak to the code, if we were to take that
suggestion, would be quite small.  We are happy when euid owns the
path (regardless of who euid is), and in addition, only when euid
is root, we check with SUDO_UID, too.

 git-compat-util.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git c/git-compat-util.h w/git-compat-util.h
index dfdd3e4f81..18660553b3 100644
--- c/git-compat-util.h
+++ w/git-compat-util.h
@@ -401,13 +401,13 @@ static inline int git_offset_1st_component(const char *path)
 #endif
 
 /*
- * this helper function overrides a ROOT_UID with the one provided by
- * an environment variable, do not use unless the original user is
- * root
+ * The environment variable (e.g. SUDO_UID) gives an integer;
+ * is it the same as the given uid_t id?
  */
-static inline void extract_id_from_env(const char *env, uid_t *id)
+static inline int id_from_env_matches(const char *env, uid_t id)
 {
 	const char *real_uid = getenv(env);
+	int matches = 0;
 
 	/* discard any empty values */
 	if (real_uid && *real_uid) {
@@ -417,11 +417,12 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
 
 		errno = 0;
 		env_id = strtoul(real_uid, &endptr, 10);
-		if (!errno && !*endptr && env_id <= (uid_t)-1)
-			*id = env_id;
-
+		if (!errno && !*endptr && env_id <= (uid_t)-1 &&
+		    (uid_t)env_id == id)
+			matches = 1;
 		errno = saved_errno;
 	}
+	return matches;
 }
 
 static inline int is_path_owned_by_current_uid(const char *path)
@@ -433,10 +434,11 @@ static inline int is_path_owned_by_current_uid(const char *path)
 		return 0;
 
 	euid = geteuid();
+	if (st.st_uid == euid)
+		return 1;
 	if (euid == ROOT_UID)
-		extract_id_from_env("SUDO_UID", &euid);
-
-	return st.st_uid == euid;
+		return id_from_env_matches("SUDO_UID", st.st_uid);
+	return 0;
 }
 
 #define is_path_owned_by_current_user is_path_owned_by_current_uid