Merge branch 'cb/path-owner-check-with-sudo-plus' into seen

"sudo git foo" used to consider a repository owned by the original
user a safe one to access; it now also considers a repository owned
by root a safe one, too (after all, if an attacker can craft a
malicious repository owned by root, the box is 0wned already).

What's our take on this one?  IIRC, the last time we discussed,
Carlo was hestant to push this step forward?
source: <[email protected]>

* cb/path-owner-check-with-sudo-plus:
  git-compat-util: allow root to access both SUDO_UID and root owned

Author: gitster

Documentation/config/safe.txt

@@ -31,11 +31,12 @@ As explained, Git only allows you to access repositories owned by
 yourself, i.e. the user who is running Git, by default.  When Git
 is running as 'root' in a non Windows platform that provides sudo,
  however, git checks the SUDO_UID environment variable that sudo creates
-and will allow access to the uid recorded as its value instead.
+and will allow access to the uid recorded as its value in addition to
+the id from 'root'.
 This is to make it easy to perform a common sequence during installation
 "make && sudo make install".  A git process running under 'sudo' runs as
 'root' but the 'sudo' command exports the environment variable to record
 which id the original user has.
 If that is not what you would prefer and want git to only trust
-repositories that are owned by root instead, then you must remove
+repositories that are owned by root instead, then you can remove
 the `SUDO_UID` variable from root's environment before invoking git.

git-compat-util.h

@@ -470,9 +470,10 @@ static inline int git_offset_1st_component(const char *path)
  * maybe provide you with a patch that would prevent this issue again
  * in the future.
  */
-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 anything empty to avoid a more complex check below */
 	if (real_uid && *real_uid) {
@@ -482,9 +483,10 @@ static inline void extract_id_from_env(const char *env, uid_t *id)
 		errno = 0;
 		/* silent overflow errors could trigger a bug here */
 		env_id = strtoul(real_uid, &endptr, 10);
-		if (!*endptr && !errno)
-			*id = env_id;
+		if (!*endptr && !errno && (uid_t)env_id == id)
+			matches = 1;
 	}
+	return matches;
 }
 
 static inline int is_path_owned_by_current_uid(const char *path)
@@ -496,10 +498,13 @@ 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 id_from_env_matches("SUDO_UID", st.st_uid);
 
-	return st.st_uid == euid;
+	return 0;
 }
 
 #define is_path_owned_by_current_user is_path_owned_by_current_uid

t/t0034-root-safe-directory.sh

@@ -68,7 +68,7 @@ test_expect_success 'can access if addressed explicitly' '
 	)
 '
 
-test_expect_failure SUDO 'can access with sudo if root' '
+test_expect_success SUDO 'can access with sudo if root' '
 	(
 		cd root/p &&
 		sudo git status
@@ -85,19 +85,6 @@ test_expect_success SUDO 'can access with sudo if root by removing SUDO_UID' '
 	)
 '
 
-test_lazy_prereq SUDO_SUDO '
-	sudo sudo id -u >u &&
-	id -u root >r &&
-	test_cmp u r
-'
-
-test_expect_success SUDO_SUDO 'can access with sudo abusing SUDO_UID' '
-	(
-		cd root/p &&
-		sudo sudo git status
-	)
-'
-
 # this MUST be always the last test
 test_expect_success SUDO 'cleanup' '
 	sudo rm -rf root