From: Stefan Sperling Subject: fix gotd group auth To: gameoftrees@openbsd.org Date: Thu, 17 Nov 2022 10:03:36 +0100 gotd group auth is broken. It matches against the groups of the user running gotd instead of the user which is connecting to the socket :-) Fix this and add tests for various cases where gotd should deny repository read access. ok? diff refs/heads/main refs/heads/gotd-auth-group commit - c2a4f618fa3c9683fbb5f384b117f7e45a041122 commit + c8b61813bb25b1170687311d5deba5f694b21a93 blob - b8b141d19d580a01c1cc68f758b798a5f2947b11 blob + ce2c2df55ff1550b6443b71f5873da9002df3eac --- gotd/auth.c +++ gotd/auth.c @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -29,10 +30,12 @@ #include #include #include +#include #include "got_error.h" #include "gotd.h" +#include "log.h" #include "auth.h" static int @@ -93,8 +96,10 @@ match_identifier(const char *identifier, gid_t *groups gid_t rgid; if (parsegid(identifier + 1, &rgid) == -1) return 0; + if (rgid == egid) + return 1; for (i = 0; i < ngroups; i++) { - if (rgid == groups[i] && egid == rgid) + if (rgid == groups[i]) break; } if (i == ngroups) @@ -107,12 +112,22 @@ gotd_auth_check(struct gotd_access_rule_list *rules, c const struct got_error * gotd_auth_check(struct gotd_access_rule_list *rules, const char *repo_name, - gid_t *groups, int ngroups, uid_t euid, gid_t egid, - int required_auth) + uid_t euid, gid_t egid, int required_auth) { struct gotd_access_rule *rule; enum gotd_access access = GOTD_ACCESS_DENIED; + struct passwd *pw; + gid_t groups[NGROUPS_MAX]; + int ngroups; + errno = 0; /* allow us to tell apart getpwuid does not clear */ + pw = getpwuid(euid); + if (pw == NULL) + return got_error_from_errno("getpwuid"); + + if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1) + log_warnx("group membership list truncated"); + STAILQ_FOREACH(rule, rules, entry) { if (!match_identifier(rule->identifier, groups, ngroups, euid, egid)) blob - 480be955e04852bc1940c62233137deaeebcaedf blob + 506f5e8b84ac2182f7114a5f4569b3b8d6963cc2 --- gotd/auth.h +++ gotd/auth.h @@ -16,4 +16,4 @@ gotd_auth_check(struct gotd_access_rule_list *rules, c const struct got_error * gotd_auth_check(struct gotd_access_rule_list *rules, const char *repo_name, - gid_t *groups, int ngroups, uid_t euid, gid_t egid, int required_auth); + uid_t euid, gid_t egid, int required_auth); blob - 526949eca1f9f3357614b701fd37f29764969911 blob + c9a5e9898ab184ee0b9168d135e56c9ae529e183 --- gotd/gotd.c +++ gotd/gotd.c @@ -630,8 +630,7 @@ forward_list_refs_request(struct gotd_client *client, if (repo == NULL) return got_error(GOT_ERR_NOT_GIT_REPO); err = gotd_auth_check(&repo->rules, repo->name, - gotd.groups, gotd.ngroups, client->euid, client->egid, - GOTD_AUTH_READ); + client->euid, client->egid, GOTD_AUTH_READ); if (err) return err; client->repo_read = find_proc_by_repo_name(PROC_REPO_READ, @@ -645,9 +644,8 @@ forward_list_refs_request(struct gotd_client *client, repo = find_repo_by_name(ireq.repo_name); if (repo == NULL) return got_error(GOT_ERR_NOT_GIT_REPO); - err = gotd_auth_check(&repo->rules, repo->name, - gotd.groups, gotd.ngroups, client->euid, client->egid, - GOTD_AUTH_READ | GOTD_AUTH_WRITE); + err = gotd_auth_check(&repo->rules, repo->name, client->euid, + client->egid, GOTD_AUTH_READ | GOTD_AUTH_WRITE); if (err) return err; client->repo_write = find_proc_by_repo_name(PROC_REPO_WRITE, @@ -2097,6 +2095,7 @@ main(int argc, char **argv) const char *confpath = GOTD_CONF_PATH; char *argv0 = argv[0]; char title[2048]; + gid_t groups[NGROUPS_MAX]; int ngroups = NGROUPS_MAX; struct passwd *pw = NULL; struct group *gr = NULL; @@ -2167,11 +2166,10 @@ main(int argc, char **argv) getprogname(), pw->pw_name, getprogname()); } - if (getgrouplist(pw->pw_name, pw->pw_gid, gotd.groups, &ngroups) == -1) + if (getgrouplist(pw->pw_name, pw->pw_gid, groups, &ngroups) == -1) log_warnx("group membership list truncated"); - gotd.ngroups = ngroups; - gr = match_group(gotd.groups, ngroups, gotd.unix_group_name); + gr = match_group(groups, ngroups, gotd.unix_group_name); if (gr == NULL) { fatalx("cannot start %s: the user running %s " "must be a secondary member of group %s", blob - fa1f5d66ce3b4c6691a931b445425921470634a2 blob + 1137c32a92e87b805d1195b4b06d153a9a53d1fa --- gotd/gotd.h +++ gotd/gotd.h @@ -117,8 +117,6 @@ struct gotd { struct event pause; struct gotd_child_proc *procs; int nprocs; - gid_t groups[NGROUPS_MAX]; - int ngroups; }; enum gotd_imsg_type { blob - 773e9c0dfd3c2e287025d41ffed9f3749b380941 blob + cd20ee9bf8631fcd757767edf78df5f410e56161 --- regress/gotd/Makefile +++ regress/gotd/Makefile @@ -1,4 +1,7 @@ -REGRESS_TARGETS=test_repo_read test_repo_write test_repo_write_empty +REGRESS_TARGETS=test_repo_read test_repo_read_group \ + test_repo_read_denied_user test_repo_read_denied_group \ + test_repo_read_bad_user test_repo_read_bad_group \ + test_repo_write test_repo_write_empty NOOBJ=Yes .PHONY: ensure_root prepare_test_repo check_test_repo start_gotd @@ -46,6 +49,67 @@ start_gotd_rw: ensure_root @$(GOTD_TRAP); $(GOTD_START_CMD) @$(GOTD_TRAP); sleep .5 +start_gotd_ro_group: ensure_root + @echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf + @echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf + @echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf + @echo 'repository "test-repo" {' >> $(PWD)/gotd.conf + @echo ' path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf + @echo ' permit ro :$(GOTD_DEVUSER)' >> $(PWD)/gotd.conf + @echo "}" >> $(PWD)/gotd.conf + @$(GOTD_TRAP); $(GOTD_START_CMD) + @$(GOTD_TRAP); sleep .5 + +# try a permit rule followed by a deny rule; last matched rule wins +start_gotd_ro_denied_user: ensure_root + @echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf + @echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf + @echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf + @echo 'repository "test-repo" {' >> $(PWD)/gotd.conf + @echo ' path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf + @echo ' permit ro $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf + @echo ' deny $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf + @echo "}" >> $(PWD)/gotd.conf + @$(GOTD_TRAP); $(GOTD_START_CMD) + @$(GOTD_TRAP); sleep .5 + +# try a permit rule followed by a deny rule; last matched rule wins +start_gotd_ro_denied_group: ensure_root + @echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf + @echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf + @echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf + @echo 'repository "test-repo" {' >> $(PWD)/gotd.conf + @echo ' path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf + @echo ' permit ro $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf + @echo ' deny :$(GOTD_DEVUSER)' >> $(PWD)/gotd.conf + @echo "}" >> $(PWD)/gotd.conf + @$(GOTD_TRAP); $(GOTD_START_CMD) + @$(GOTD_TRAP); sleep .5 + +# $GOTD_DEVUSER should not equal $GOTD_USER +start_gotd_ro_bad_user: ensure_root + @echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf + @echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf + @echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf + @echo 'repository "test-repo" {' >> $(PWD)/gotd.conf + @echo ' path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf + @echo ' permit ro $(GOTD_USER)' >> $(PWD)/gotd.conf + @echo "}" >> $(PWD)/gotd.conf + @$(GOTD_TRAP); $(GOTD_START_CMD) + @$(GOTD_TRAP); sleep .5 + +# $GOTD_DEVUSER should not be in group wheel +start_gotd_ro_bad_group: ensure_root + @echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf + @echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf + @echo "user $(GOTD_USER)" >> $(PWD)/gotd.conf + @echo 'repository "test-repo" {' >> $(PWD)/gotd.conf + @echo ' path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf + @echo ' permit ro :wheel' >> $(PWD)/gotd.conf + @echo "}" >> $(PWD)/gotd.conf + @$(GOTD_TRAP); $(GOTD_START_CMD) + @$(GOTD_TRAP); sleep .5 + start_gotd_rw: ensure_root @echo 'unix_socket "$(GOTD_SOCK)"' > $(PWD)/gotd.conf @echo "unix_group $(GOTD_GROUP)" >> $(PWD)/gotd.conf @@ -71,6 +135,36 @@ test_repo_write: prepare_test_repo start_gotd_rw @$(GOTD_STOP_CMD) 2>/dev/null @su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh' +test_repo_read_group: prepare_test_repo start_gotd_ro_group + @-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \ + 'env $(GOTD_TEST_ENV) sh ./repo_read.sh' + @$(GOTD_STOP_CMD) 2>/dev/null + @su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh' + +test_repo_read_denied_user: prepare_test_repo start_gotd_ro_denied_user + @-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \ + 'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh' + @$(GOTD_STOP_CMD) 2>/dev/null + @su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh' + +test_repo_read_denied_group: prepare_test_repo start_gotd_ro_denied_group + @-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \ + 'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh' + @$(GOTD_STOP_CMD) 2>/dev/null + @su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh' + +test_repo_read_bad_user: prepare_test_repo start_gotd_ro_bad_user + @-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \ + 'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh' + @$(GOTD_STOP_CMD) 2>/dev/null + @su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh' + +test_repo_read_bad_group: prepare_test_repo start_gotd_ro_bad_group + @-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \ + 'env $(GOTD_TEST_ENV) sh ./repo_read_access_denied.sh' + @$(GOTD_STOP_CMD) 2>/dev/null + @su -m ${GOTD_USER} -c 'env $(GOTD_TEST_ENV) sh ./check_test_repo.sh' + test_repo_write: prepare_test_repo start_gotd_rw @-$(GOTD_TRAP); su ${GOTD_TEST_USER} -c \ 'env $(GOTD_TEST_ENV) sh ./repo_write.sh' blob - /dev/null blob + c8b4fc889c65a723539f0957fff96fda7cd7d32b (mode 644) --- /dev/null +++ regress/gotd/repo_read_access_denied.sh @@ -0,0 +1,47 @@ +#!/bin/sh +# +# Copyright (c) 2022 Stefan Sperling +# +# Permission to use, copy, modify, and distribute this software for any +# purpose with or without fee is hereby granted, provided that the above +# copyright notice and this permission notice appear in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES +# WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN +# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF +# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + +. ../cmdline/common.sh +. ./common.sh + +test_clone_basic_access_denied() { + local testroot=`test_init clone_basic_access_denied 1` + + cp -r ${GOTD_TEST_REPO} $testroot/repo-copy + + got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone \ + 2> $testroot/stderr + ret=$? + if [ $ret -eq 0 ]; then + echo "got clone succeeded unexpectedly" >&2 + test_done "$testroot" "1" + return 1 + fi + + # Verify that the clone operation failed. + echo 'got-fetch-pack: test-repo: Permission denied' \ + > $testroot/stderr.expected + echo 'got: fetch failed' >> $testroot/stderr.expected + cmp -s $testroot/stderr.expected $testroot/stderr + ret=$? + if [ $ret -ne 0 ]; then + diff -u $testroot/stderr.expected $testroot/stderr + fi + test_done "$testroot" "$ret" +} + +test_parseargs "$@" +run_test test_clone_basic_access_denied