"GOT", but the "O" is a cute, smiling sun Index | Thread

From:
Stefan Sperling <stsp@stsp.name>
Subject:
ignore config files with non-exclusive ownership
To:
gameoftrees@openbsd.org
Date:
Mon, 24 Oct 2022 22:37:34 +0200

Download raw body.

I keep wondering whether it is safe for got(1) to read configuration
files by default when such files are writable by other users.

We already prevent a code execution vector by not offering a way to
configure an editor command in the config file. So Bob cannot set the
editor command to a script that will invisibly delete all of Alice's files
in the background while also starting an editor for Alice to type into.

However, it would still be possible for Bob to trick Alice into making
contact with a Git server controlled by Bob, by changing remote repo URLs
in a config file both users have write access to. Which could potentially
be abused to trigger a bug or try some phishing once Alice makes contact.

Am I being too paranoid?

diff /home/stsp/src/got
commit - 88dec1791eeb2f779795789b119d5bf675c24b6a
path + /home/stsp/src/got
blob - 7aaa7e057f73895b5da6fc8f9f74a6dc5e45b4c3
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -3001,6 +3001,17 @@ information found in Git configuration files will be i
 .It Ev GOT_IGNORE_GITCONFIG
 If this variable is set then any remote repository definitions or author
 information found in Git configuration files will be ignored.
+.It GOT_IGNORE_CONFIG_FILE_PERMISSIONS
+Unless this variable is set, the
+.Xr got.conf 5 ,
+.Pa .git/config ,
+and
+.Pa ~/.gitconfig
+configuration files will not be read if they are owned by a different user ID
+or are writable by other users.
+This default behaviour prevents making contact with remote repositories added
+to the configuration file by someone other than the user running
+.Nm .
 .El
 .Sh FILES
 .Bl -tag -width packed-refs -compact
blob - d46762d0f8fd908e9beebe23dd87ff3740db4068
file + lib/read_gitconfig.c
--- lib/read_gitconfig.c
+++ lib/read_gitconfig.c
@@ -123,6 +123,23 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 		}
 	}
 
+	/*
+	 * We must always read repository format version and the list
+	 * of format extensions for interop with Git. Ignore the rest
+	 * of the config if file permissions don't look safe.
+	 */
+	if (getenv("GOT_IGNORE_CONFIG_FILE_PERMISSIONS") == NULL) {
+		struct stat sb;
+		if (fstat(fd, &sb) == -1) {
+			err = got_error_from_errno("fstat");
+			goto done;
+		}
+		if (sb.st_uid != geteuid() || sb.st_mode & (S_IWGRP | S_IWOTH)) {
+			err = NULL;
+			goto done;
+		}
+	}
+
 	author = got_gitconfig_get_str(gitconfig, "user", "name");
 	if (author) {
 		*gitconfig_author_name = strdup(author);
blob - 0f29cc09331dd7d207bd1ec47e0b2310708660ee
file + lib/read_gitconfig_privsep.c
--- lib/read_gitconfig_privsep.c
+++ lib/read_gitconfig_privsep.c
@@ -17,6 +17,7 @@
 #include <sys/types.h>
 #include <sys/tree.h>
 #include <sys/socket.h>
+#include <sys/stat.h>
 #include <sys/queue.h>
 #include <sys/uio.h>
 
@@ -54,6 +55,7 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 	int imsg_fds[2] = { -1, -1 };
 	pid_t pid;
 	struct imsgbuf *ibuf;
+	int ignore_config = 0;
 
 	*gitconfig_repository_format_version = 0;
 	if (extensions)
@@ -104,6 +106,16 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 	imsg_fds[1] = -1;
 	imsg_init(ibuf, imsg_fds[0]);
 
+	if (getenv("GOT_IGNORE_CONFIG_FILE_PERMISSIONS") == NULL) {
+		struct stat sb;
+		if (fstat(fd, &sb) == -1) {
+			err = got_error_from_errno("fstat");
+			goto done;
+		}
+		if (sb.st_uid != geteuid() || sb.st_mode & (S_IWGRP | S_IWOTH))
+			ignore_config = 1;
+	}
+
 	err = got_privsep_send_gitconfig_parse_req(ibuf, fd);
 	if (err)
 		goto done;
@@ -144,6 +156,16 @@ got_repo_read_gitconfig(int *gitconfig_repository_form
 		}
 	}
 
+	/*
+	 * We must always read repository format version and the list
+	 * of format extensions for interop with Git. Ignore the rest
+	 * of the config if file permissions don't look safe.
+	 */
+	 if (ignore_config) {
+		err = NULL;
+		goto done;
+	}
+
 	err = got_privsep_send_gitconfig_author_name_req(ibuf);
 	if (err)
 		goto done;
blob - 00dbdebf54e0335e148296057d897cb9f431e29f
file + lib/read_gotconfig_privsep.c
--- lib/read_gotconfig_privsep.c
+++ lib/read_gotconfig_privsep.c
@@ -18,6 +18,7 @@
 #include <sys/queue.h>
 #include <sys/uio.h>
 #include <sys/socket.h>
+#include <sys/stat.h>
 
 #include <unistd.h>
 #include <fcntl.h>
@@ -47,7 +48,7 @@ got_gotconfig_read(struct got_gotconfig **conf, const 
 	int fd = -1;
 	int imsg_fds[2] = { -1, -1 };
 	pid_t pid;
-	struct imsgbuf *ibuf;
+	struct imsgbuf *ibuf = NULL;
 
 	*conf = calloc(1, sizeof(**conf));
 	if (*conf == NULL)
@@ -57,9 +58,22 @@ got_gotconfig_read(struct got_gotconfig **conf, const 
 	if (fd == -1) {
 		if (errno == ENOENT)
 			return NULL;
-		return got_error_from_errno2("open", gotconfig_path);
+		err = got_error_from_errno2("open", gotconfig_path);
+		goto done;
 	}
 
+	if (getenv("GOT_IGNORE_CONFIG_FILE_PERMISSIONS") == NULL) {
+		struct stat sb;
+		if (fstat(fd, &sb) == -1) {
+			err = got_error_from_errno("fstat");
+			goto done;
+		}
+		if (sb.st_uid != geteuid() || sb.st_mode & (S_IWGRP | S_IWOTH)) {
+			err = NULL;
+			goto done;
+		}
+	}
+
 	ibuf = calloc(1, sizeof(*ibuf));
 	if (ibuf == NULL) {
 		err = got_error_from_errno("calloc");
blob - 8d6881ec92c2a3aff5b101a606dfd3264744dcca
file + regress/cmdline/fetch.sh
--- regress/cmdline/fetch.sh
+++ regress/cmdline/fetch.sh
@@ -1109,6 +1109,21 @@ EOF
 		return 1
 	fi
 
+	# configuration files not exclusively writable by our UID are ignored
+	chmod g+w $testroot/repo-clone/got.conf
+	echo "got: foobar: remote repository not found" \
+		> $testroot/stderr.expected
+	(cd $testroot/repo-clone && got fetch -q foobar \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "got fetch command succeeded unexpectedly" >&2
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "1"
+		return 1
+	fi
+	chmod g-w $testroot/repo-clone/got.conf
+
 	(cd $testroot/repo-clone && got fetch -q -l foobar \
 		> $testroot/stdout)
 	ret=$?