From cec01cc0a2523453e5da5a486bc6d977c3768db1 Mon Sep 17 00:00:00 2001 From: TJ Saunders Date: Wed, 13 Nov 2024 06:33:35 -0800 Subject: [PATCH] Issue #1830: When no supplemental groups are provided by the underlying authentication providers, fall back to using the primary group/GID. (#1835) This prevents surprise due to inheritance of the parent processes' supplemental group membership, which might inadvertently provided undesired access. --- contrib/mod_sftp/auth.c | 14 +- modules/mod_auth.c | 19 +- src/auth.c | 14 +- .../ProFTPD/Tests/Modules/mod_sql_sqlite.pm | 174 ++++++++++++++++++ 4 files changed, 209 insertions(+), 12 deletions(-) diff --git a/contrib/mod_sftp/auth.c b/contrib/mod_sftp/auth.c index c7a694e..6196fec 100644 --- a/contrib/mod_sftp/auth.c +++ b/contrib/mod_sftp/auth.c @@ -388,8 +388,20 @@ static int setup_env(pool *p, const char *user) { session.groups == NULL) { res = pr_auth_getgroups(p, pw->pw_name, &session.gids, &session.groups); if (res < 1) { + /* If no supplemental groups are provided, default to using the process + * primary GID as the supplemental group. This prevents access + * regressions as seen in Issue #1830. + */ (void) pr_log_writefile(sftp_logfd, MOD_SFTP_VERSION, - "no supplemental groups found for user '%s'", pw->pw_name); + "no supplemental groups found for user '%s', " + "using primary group %s (GID %lu)", pw->pw_name, session.group, + (unsigned long) session.login_gid); + + session.gids = make_array(p, 2, sizeof(gid_t)); + session.groups = make_array(p, 2, sizeof(char *)); + + *((gid_t *) push_array(session.gids)) = session.login_gid; + *((char **) push_array(session.groups)) = pstrdup(p, session.group); } } diff --git a/modules/mod_auth.c b/modules/mod_auth.c index a85be06..9eb9b48 100644 --- a/modules/mod_auth.c +++ b/modules/mod_auth.c @@ -1113,8 +1113,8 @@ static int setup_env(pool *p, cmd_rec *cmd, const char *user, char *pass) { session.groups = NULL; } - if (!session.gids && - !session.groups) { + if (session.gids == NULL && + session.groups == NULL) { /* Get the supplemental groups. Note that we only look up the * supplemental group credentials if we have not cached the group * credentials before, in session.gids and session.groups. @@ -1124,8 +1124,19 @@ static int setup_env(pool *p, cmd_rec *cmd, const char *user, char *pass) { */ res = pr_auth_getgroups(p, pw->pw_name, &session.gids, &session.groups); if (res < 1) { - pr_log_debug(DEBUG5, "no supplemental groups found for user '%s'", - pw->pw_name); + /* If no supplemental groups are provided, default to using the process + * primary GID as the supplemental group. This prevents access + * regressions as seen in Issue #1830. + */ + pr_log_debug(DEBUG5, "no supplemental groups found for user '%s', " + "using primary group %s (GID %lu)", pw->pw_name, session.group, + (unsigned long) session.login_gid); + + session.gids = make_array(p, 2, sizeof(gid_t)); + session.groups = make_array(p, 2, sizeof(char *)); + + *((gid_t *) push_array(session.gids)) = session.login_gid; + *((char **) push_array(session.groups)) = pstrdup(p, session.group); } } diff --git a/src/auth.c b/src/auth.c index b90fe41..af39fc0 100644 --- a/src/auth.c +++ b/src/auth.c @@ -1471,12 +1471,12 @@ int pr_auth_getgroups(pool *p, const char *name, array_header **group_ids, } /* Allocate memory for the array_headers of GIDs and group names. */ - if (group_ids) { - *group_ids = make_array(permanent_pool, 2, sizeof(gid_t)); + if (group_ids != NULL) { + *group_ids = make_array(p, 2, sizeof(gid_t)); } - if (group_names) { - *group_names = make_array(permanent_pool, 2, sizeof(char *)); + if (group_names != NULL) { + *group_names = make_array(p, 2, sizeof(char *)); } cmd = make_cmd(p, 3, name, group_ids ? *group_ids : NULL, @@ -1495,7 +1495,7 @@ int pr_auth_getgroups(pool *p, const char *name, array_header **group_ids, * for the benefit of auth_getgroup() implementors. */ - if (group_ids) { + if (group_ids != NULL) { register unsigned int i; char *strgids = ""; gid_t *gids = (*group_ids)->elts; @@ -1511,7 +1511,7 @@ int pr_auth_getgroups(pool *p, const char *name, array_header **group_ids, *strgids ? strgids : "(None; corrupted group file?)"); } - if (group_names) { + if (group_names != NULL) { register unsigned int i; char *strgroups = ""; char **groups = (*group_names)->elts; @@ -1527,7 +1527,7 @@ int pr_auth_getgroups(pool *p, const char *name, array_header **group_ids, } } - if (cmd->tmp_pool) { + if (cmd->tmp_pool != NULL) { destroy_pool(cmd->tmp_pool); cmd->tmp_pool = NULL; } diff --git a/tests/t/lib/ProFTPD/Tests/Modules/mod_sql_sqlite.pm b/tests/t/lib/ProFTPD/Tests/Modules/mod_sql_sqlite.pm index 08c1542..42ba967 100644 --- a/tests/t/lib/ProFTPD/Tests/Modules/mod_sql_sqlite.pm +++ b/tests/t/lib/ProFTPD/Tests/Modules/mod_sql_sqlite.pm @@ -467,6 +467,11 @@ my $TESTS = { order => ++$order, test_class => [qw(forking bug mod_tls)], }, + + sql_user_info_no_suppl_groups_issue1830 => { + order => ++$order, + test_class => [qw(forking bug rootprivs)], + }, }; sub new { @@ -15764,4 +15769,173 @@ EOC test_cleanup($setup->{log_file}, $ex); } +sub sql_user_info_no_suppl_groups_issue1830 { + my $self = shift; + my $tmpdir = $self->{tmpdir}; + my $setup = test_setup($tmpdir, 'sqlite'); + + my $db_file = File::Spec->rel2abs("$tmpdir/proftpd.db"); + + # Build up sqlite3 command to create users, groups tables and populate them + my $db_script = File::Spec->rel2abs("$tmpdir/proftpd.sql"); + + if (open(my $fh, "> $db_script")) { + print $fh <{user}', '$setup->{passwd}', $setup->{uid}, $setup->{gid}, '$setup->{home_dir}', '/bin/bash'); + +CREATE TABLE groups ( + groupname TEXT, + gid INTEGER, + members TEXT +); +INSERT INTO groups (groupname, gid, members) VALUES ('$setup->{group}', $setup->{gid}, '$setup->{user}'); +EOS + + unless (close($fh)) { + die("Can't write $db_script: $!"); + } + + } else { + die("Can't open $db_script: $!"); + } + + my $cmd = "sqlite3 $db_file < $db_script"; + build_db($cmd, $db_script); + + # Make sure that, if we're running as root, the database file has + # the permissions/privs set for use by proftpd + if ($< == 0) { + unless (chmod(0666, $db_file)) { + die("Can't set perms on $db_file to 0666: $!"); + } + } + + my $config = { + PidFile => $setup->{pid_file}, + ScoreboardFile => $setup->{scoreboard_file}, + SystemLog => $setup->{log_file}, + TraceLog => $setup->{log_file}, + Trace => 'auth:20 sql:20', + + # Required for logging the expected message + DebugLevel => 5, + + IfModules => { + 'mod_delay.c' => { + DelayEngine => 'off', + }, + + 'mod_sql.c' => { + AuthOrder => 'mod_sql.c', + + SQLAuthenticate => 'users', + SQLAuthTypes => 'plaintext', + SQLBackend => 'sqlite3', + SQLConnectInfo => $db_file, + SQLLogFile => $setup->{log_file}, + + # Set these, so that our lower UID/GID will be used + SQLMinUserUID => 100, + SQLMinUserGID => 100, + }, + }, + }; + + my ($port, $config_user, $config_group) = config_write($setup->{config_file}, + $config); + + # Open pipes, for use between the parent and child processes. Specifically, + # the child will indicate when it's done with its test by writing a message + # to the parent. + my ($rfh, $wfh); + unless (pipe($rfh, $wfh)) { + die("Can't open pipe: $!"); + } + + my $ex; + + # Fork child + $self->handle_sigchld(); + defined(my $pid = fork()) or die("Can't fork: $!"); + if ($pid) { + eval { + sleep(2); + + my $client = ProFTPD::TestSuite::FTP->new('127.0.0.1', $port); + $client->login($setup->{user}, $setup->{passwd}); + + my $resp_msgs = $client->response_msgs(); + my $nmsgs = scalar(@$resp_msgs); + + my $expected = 1; + $self->assert($expected == $nmsgs, + test_msg("Expected $expected, got $nmsgs")); + + $expected = "User $setup->{user} logged in"; + $self->assert($expected eq $resp_msgs->[0], + test_msg("Expected response '$expected', got '$resp_msgs->[0]'")); + + $client->quit(); + }; + if ($@) { + $ex = $@; + } + + $wfh->print("done\n"); + $wfh->flush(); + + } else { + eval { server_wait($setup->{config_file}, $rfh) }; + if ($@) { + warn($@); + exit 1; + } + + exit 0; + } + + # Stop server + server_stop($setup->{pid_file}); + $self->assert_child_ok($pid); + + eval { + if (open(my $fh, "< $setup->{log_file}")) { + my $ok = 0; + + while (my $line = <$fh>) { + chomp($line); + + if ($ENV{TEST_VERBOSE}) { + print STDERR "# $line\n"; + } + + if ($line =~ /no supplemental groups found for user '$setup->{user}', using primary group/) { + $ok = 1; + last; + } + } + + close($fh); + + $self->assert($ok, test_msg("Did not see expected log message")); + + } else { + die("Can't read $setup->{log_file}: $!"); + } + }; + if ($@) { + $ex = $@ unless $ex; + } + + test_cleanup($setup->{log_file}, $ex); +} + 1; -- 2.33.0