On 2022/10/24 22:37:34 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> 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?

I dislike piling up env variables.  In some case it's the easiest way
or mostly needed for the regress so it's kind of ok, but for potential
security issues like this I prefer a line in the sand approach: we
either don't care or straight up refuse to work.

If Alice and Bob share a git repository then Alice is likely to set
GOT_IGNORE_CONFIG_FILE_PERMISSIONS to work with Bob and this opens up
the doors for Eve to to trick Alice.  woops.

Both `got send' and `got fetch' are clear enough to communicate what
they're doing:

	% got send
	Connecting to "origin" git@foobar.com
	...

However once the user reads that line it's very likely too late and
the connection to the remote server has been made.

So, if we consider insecure connecting to unknown servers, why don't
just fail?  Are shared bare repositories common?


more below

> 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;
> +	}
> +

not an issue, but I'd move this a bit earlier, right after open(2), so
it's easier to read.

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

This bit should be committed regardless of what gets decided: `return'
here leaks the previous calloc.

>  	}
>  
> +	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=$?