session: don't spawn cockpit-bridge with the shell

src/ws/session-utils.c

@@ -401,52 +401,6 @@ open_session (pam_handle_t *pamh)
   return PAM_SUCCESS;
 }
 
-int
-fork_session (char **env, int (*session)(char**))
-{
-  int status;
-
-  fflush (stderr);
-  assert (pwd != NULL);
-
-  child = fork ();
-  if (child < 0)
-    {
-      warn ("can't fork");
-      return 1 << 8;
-    }
-
-  if (child == 0)
-    {
-      if (setgid (pwd->pw_gid) < 0)
-        {
-          warn ("setgid() failed");
-          _exit (42);
-        }
-
-      if (setuid (pwd->pw_uid) < 0)
-        {
-          warn ("setuid() failed");
-          _exit (42);
-        }
-
-      if (getuid() != geteuid() &&
-          getgid() != getegid())
-        {
-          warnx ("couldn't drop privileges");
-          _exit (42);
-        }
-
-      debug ("dropped privileges");
-
-      _exit (session (env));
-    }
-
-  close (0);
-  close (1);
-  waitpid (child, &status, 0);
-  return status;
-}
 
 static bool
 do_lastlog (uid_t                 uid,
@@ -946,7 +900,7 @@ abort_with_message (const char *format,
  *
  * Commonly used after fork() and before exec().
  */
-void
+static void
 fd_remap (const int *remap_fds,
           int        n_remap_fds)
 {
@@ -983,3 +937,82 @@ fd_remap (const int *remap_fds,
   if (fdwalk (closefd, &n_remap_fds) < 0)
     abort_with_message ("couldn't close all file descriptors");
 }
+
+int
+spawn_and_wait (const char **argv, const char **envp,
+                const int *remap_fds, int n_remap_fds,
+                uid_t uid, gid_t gid)
+{
+  pid_t child;
+
+  child = fork ();
+  if (child == -1)
+    abort_with_message ("cockpit-session: fork() failed: %m");
+
+  if (child == 0)
+    {
+      /* This is the child process.  Do preparation, and exec(). */
+      if (setresgid (gid, gid, gid) != 0)
+        abort_with_message ("setresgid: couldn't set gid to %u: %m\n", (int) gid);
+
+      if (setresuid (uid, uid, uid) != 0)
+        abort_with_message ("setresgid: couldn't set uid to %u: %m\n", (int) gid);
+
+      /* paranoid */
+      {
+        uid_t real, effective, saved;
+        int r;
+
+        r = getresuid (&real, &effective, &saved);
+        assert (r == 0 && real == uid && effective == uid && saved == uid);
+      }
+
+      {
+        gid_t real, effective, saved;
+        int r;
+
+        r = getresgid (&real, &effective, &saved);
+        assert (r == 0 && real == gid && effective == gid && saved == gid);
+      }
+
+      if (n_remap_fds != -1)
+        fd_remap (remap_fds, n_remap_fds);
+
+      execvpe (argv[0], (char **) argv, (char **) envp);
+      _exit(127);
+    }
+
+  else
+    {
+      /* This is the parent process.  Wait for the child to exit. */
+      int wstatus;
+      int r;
+
+      do
+        r = waitpid (child, &wstatus, 0);
+      while (r == -1 && errno == EINTR);
+
+      if (r == -1)
+        abort_with_message ("waitpid(%d) on cockpit-bridge process failed: %m", (int) child);
+
+      /* 0 can only be returned of WNOHANG was given */
+      assert (r == child);
+
+      return wstatus;
+    }
+}
+
+bool
+user_has_valid_login_shell (const char **envp)
+{
+  /* <lis> >>> random.randint(0,127)
+   * <lis> 71
+   * <pitti> https://xkcd.com/221/
+   */
+  const char *argv[] = { pwd->pw_shell, "-c", "exit 71;", NULL };
+  const int remap_fds[] = { -1, 2, -1 }; /* send stdout to stderr */
+  int wstatus;
+
+  wstatus = spawn_and_wait (argv, envp, remap_fds, 3, pwd->pw_uid, pwd->pw_gid);
+  return WIFEXITED(wstatus) && WEXITSTATUS(wstatus) == 71;
+}

src/ws/session-utils.h

@@ -25,6 +25,7 @@
 #include <err.h>
 #include <stdio.h>
 #include <stdint.h>
+#include <stdnoreturn.h>
 #include <string.h>
 #include <pwd.h>
 #include <grp.h>
@@ -88,9 +89,17 @@ GNUC_NORETURN void exit_init_problem (int result_code);
 #endif
 
 int open_session (pam_handle_t *pamh);
-int fork_session (char **env, int (*session)(char**));
 
 FILE *open_memfd (const char *name);
 int seal_memfd (FILE **memfd);
 
-void fd_remap (const int *remap_fds, int n_remap_fds);
+int
+spawn_and_wait (const char **argv,
+                const char **envp,
+                const int *remap_fds,
+                int n_remap_fds,
+                uid_t uid,
+                gid_t gid);
+
+bool
+user_has_valid_login_shell (const char **envp);

src/ws/session.c

@@ -29,7 +29,6 @@
 
 static char *last_txt_msg = NULL;
 static char *conversation = NULL;
-static int login_messages_fd = -1;
 
 /* This program opens a session for a given user and runs the bridge in
  * it.  It is used to manage localhost; for remote hosts sshd does
@@ -608,44 +607,6 @@ store_krb_credentials (gss_cred_id_t creds, uid_t uid, gid_t gid)
     err (127, "Unable to restore permissions after storing gss credentials");
 }
 
-static int
-session (char **env)
-{
-  char *argv[] = { NULL /* user's shell */, "-c", "exec cockpit-bridge >&3", NULL };
-  struct passwd *pwd;
-
-  /* determine the user's shell and run the bridge through it, so that we catch
-   * disabled accounts like /bin/false or /bin/nologin shells */
-  pwd = getpwuid (geteuid ());
-  if (!pwd)
-    errx (EX, "failed to get passwd entry for user: %m");
-  argv[0] = pwd->pw_shell;
-  if (!argv[0])
-    errx (EX, "passwd entry for user has NULL shell");
-
-  debug ("executing cockpit-bridge through user shell %s", pwd->pw_shell);
-
-  /* connect our and cockpit-bridge's stdout via fd 3, to avoid stdout output
-   * from ~/.profile and friends to interfere with the protocol; route shell's
-   * stdout to its stderr, so that we can still see it in the logs */
-  int remap_fds[5] = { -1, 2, -1, 1 };
-  int n_remap_fds = 4;
-
-  /* This is the "COCKPIT_LOGIN_MESSAGES_MEMFD" blob (ie: fd 4) */
-  if (login_messages_fd != -1)
-    remap_fds[n_remap_fds++] = login_messages_fd;
-
-  fd_remap (remap_fds, n_remap_fds);
-
-  if (env)
-    execvpe (argv[0], argv, env);
-  else
-    execvp (argv[0], argv);
-
-  warn ("can't exec %s", argv[0]);
-  return EX;
-}
-
 int
 main (int argc,
       char **argv)
@@ -655,7 +616,7 @@ main (int argc,
   const char *rhost;
   char *authorization;
   char *type = NULL;
-  char **env;
+  const char **env;
   int status;
   int res;
   int i;
@@ -728,21 +689,26 @@ main (int argc,
 
   if (want_session) /* no session → no login messages → no memfd */
     {
-      if (pam_putenv (pamh, "COCKPIT_LOGIN_MESSAGES_MEMFD=4") != PAM_SUCCESS)
-        errx (EX, "Failed to set COCKPIT_LOGIN_MESSAGES_MEMFD=4 in PAM environment");
+      if (pam_putenv (pamh, "COCKPIT_LOGIN_MESSAGES_MEMFD=3") != PAM_SUCCESS)
+        errx (EX, "Failed to set COCKPIT_LOGIN_MESSAGES_MEMFD=3 in PAM environment");
     }
 
-  env = pam_getenvlist (pamh);
+  env = (const char **) pam_getenvlist (pamh);
   if (env == NULL)
     errx (EX, "get pam environment failed");
 
+  const char *bridge_argv[] = { "cockpit-bridge", NULL };
+
   if (want_session)
     {
       assert (pwd != NULL);
 
       if (initgroups (pwd->pw_name, pwd->pw_gid) < 0)
         err (EX, "%s: can't init groups", pwd->pw_name);
 
+      if (!user_has_valid_login_shell (env))
+        exit_init_problem (PAM_PERM_DENIED);
+
       signal (SIGTERM, pass_to_child);
       signal (SIGINT, pass_to_child);
       signal (SIGQUIT, pass_to_child);
@@ -755,12 +721,13 @@ main (int argc,
 
       fprintf (login_messages, "}");
 
-      login_messages_fd = seal_memfd (&login_messages);
+      int login_messages_fd = seal_memfd (&login_messages);
 
       if (creds != GSS_C_NO_CREDENTIAL)
         store_krb_credentials (creds, pwd->pw_uid, pwd->pw_gid);
 
-      status = fork_session (env, session);
+      const int remap_fds[] = { -1, -1, -1, login_messages_fd };
+      status = spawn_and_wait (bridge_argv, env, remap_fds, 4, pwd->pw_uid, pwd->pw_gid);
 
       utmp_log (0, rhost, NULL);
 
@@ -779,7 +746,7 @@ main (int argc,
     }
   else
     {
-      status = session (env);
+      status = spawn_and_wait (bridge_argv, env, NULL, -1, pwd->pw_uid, pwd->pw_gid);
     }
 
   pam_end (pamh, PAM_SUCCESS);

test/verify/check-login

@@ -145,6 +145,13 @@ account    required     pam_succeed_if.so user ingroup %s""" % m.get_admin_group
             b.open("/system")
             b.wait_not_visible("#option-group")
 
+            # test login with tcsh
+            if not m.image.startswith("rhel") and not m.image.startswith("centos"):  # no tcsh in RHEL
+                m.execute("sed -r -i.bak '/^admin:/ s_:[^:]+$_:/bin/tcsh_' /etc/passwd")
+                b.login_and_go()
+                b.logout()
+                m.execute("mv /etc/passwd.bak /etc/passwd")
+
             # login with user shell that prints some stdout/err noise
             # having stdout output in ~/.bashrc confuses docker, so don't run on Atomic
             m.execute("set -e; cd ~admin; cp -a .bashrc .bashrc.bak; [ ! -e .profile ] || cp -a .profile .profile.bak; "