From: Stefan Sperling Subject: ignore config files with non-exclusive ownership To: gameoftrees@openbsd.org Date: Mon, 24 Oct 2022 22:37:34 +0200 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 #include #include +#include #include #include @@ -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 #include #include +#include #include #include @@ -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=$?