proftpd/backport-CVE-2024-48651.patch

From cec01cc0a2523453e5da5a486bc6d977c3768db1 Mon Sep 17 00:00:00 2001
From: TJ Saunders <tj@castaglia.org>
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 <<EOS;
+CREATE TABLE users (
+  userid TEXT,
+  passwd TEXT,
+  uid INTEGER,
+  gid INTEGER,
+  homedir TEXT,
+  shell TEXT
+);
+INSERT INTO users (userid, passwd, uid, gid, homedir, shell) VALUES ('$setup->{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