pam-ssh-add: Fix insecure killing of session ssh-agent [CVE-2024-6126]

Some distributions like Debian 12, or possibly some administrators
enable pam_env's deprecated `user_readenv` option [1]. The user session
can change the `$SSH_AGENT_PID`, so that it can pass an arbitrary pid to
`pam_sm_close_session()`. This is a local authenticated DoS.

Avoid this by storing the agent pid in a global variable. The
cockpit-session process stays around for the entire session time, so we
don't need to put the pid into the PAM data.

It can also happen that the user session's ssh-agent gets killed, and
some other process later on recycles the PID. Temporarily drop
privileges to the target user so that we at least don't kill anyone
else's process.

Add an integration test which checks that changing the env variable
works, pointing it to a different process doesn't kill that, and
ssh-agent (the original pid) is still cleaned up correctly. However, as
pam_so.env in Fedora crashes hard, skip the test there.

Many thanks to Paolo Perego <[email protected]> for discovering,
and Luna Dragon <[email protected]> for reporting this issue!

[1] https://man7.org/linux/man-pages/man8/pam_env.8.html

CVE-2024-6126
https://bugzilla.redhat.com/show_bug.cgi?id=2290859

Author: martinpitt

src/pam-ssh-add/pam-ssh-add.c

@@ -54,6 +54,9 @@ const char *pam_ssh_agent_arg = NULL;
 const char *pam_ssh_add_program = PATH_SSH_ADD;
 const char *pam_ssh_add_arg = NULL;
 
+static unsigned long ssh_agent_pid;
+static uid_t ssh_agent_uid;
+
 /* Environment */
 #define ENVIRON_SIZE 5
 #define PATH "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
@@ -866,6 +869,25 @@ start_agent (pam_handle_t *pamh,
           error ("couldn't set agent environment: %s",
                  pam_strerror (pamh, res));
         }
+
+      /* parse and store the agent pid for later cleanup */
+      if (strncmp (auth_pid, "SSH_AGENT_PID=", 14) == 0)
+        {
+          unsigned long pid = strtoul (auth_pid + 14, NULL, 10);
+          if (pid > 0 && pid != ULONG_MAX)
+            {
+              ssh_agent_pid = pid;
+              ssh_agent_uid = auth_pwd->pw_uid;
+            }
+          else
+            {
+              error ("invalid SSH_AGENT_PID value: %s", auth_pid);
+            }
+        }
+      else
+        {
+          error ("unexpected agent pid format: %s", auth_pid);
+        }
     }
 
   free (auth_socket);
@@ -952,19 +974,25 @@ pam_sm_close_session (pam_handle_t *pamh,
                       int argc,
                       const char *argv[])
 {
-  const char *s_pid;
-  int pid = 0;
   parse_args (argc, argv);
 
   /* Kill the ssh agent we started */
-  s_pid = pam_getenv (pamh, "SSH_AGENT_PID");
-  if (s_pid)
-    pid = atoi (s_pid);
-
-  if (pid > 0)
+  if (ssh_agent_pid > 0)
     {
-      debug ("Closing %d", pid);
-      kill (pid, SIGTERM);
+      debug ("Closing %lu", ssh_agent_pid);
+      /* kill as user to guard against crashing ssh-agent and PID reuse */
+      if (setresuid (ssh_agent_uid, ssh_agent_uid,  -1) < 0)
+        {
+          error ("could not drop privileges for killing ssh agent: %m");
+          return PAM_SESSION_ERR;
+        }
+      if (kill (ssh_agent_pid, SIGTERM) < 0 && errno != ESRCH)
+        message ("could not kill ssh agent %lu: %m", ssh_agent_pid);
+      if (setresuid (0, 0, -1) < 0)
+        {
+          error ("could not restore privileges after killing ssh agent: %m");
+          return PAM_SESSION_ERR;
+        }
     }
   return PAM_SUCCESS;
 }

test/verify/check-session

@@ -86,6 +86,39 @@ class TestSession(testlib.MachineCase):
         b.logout()
         wait_session(should_exist=False)
 
+        # try to pwn $SSH_AGENT_PID via pam_env's user_readenv=1 (CVE-2024-6126)
+
+        if m.image in ["fedora-39", "fedora-40", "centos-10", "rhel-10-0"]:
+            # pam_env user_readenv crashes in Fedora/RHEL 10, skip the test
+            # https://bugzilla.redhat.com/show_bug.cgi?id=2293045
+            return
+        if m.ostree_image:
+            # not using cockpit's PAM config
+            return
+
+        # this is enabled by default in tools/cockpit.debian.pam, as well as
+        # Debian/Ubuntu's /etc/pam.d/sshd; but not in Fedora/RHEL
+        if "debian" not in m.image and "ubuntu" not in m.image:
+            self.write_file("/etc/pam.d/cockpit", "session required pam_env.so user_readenv=1\n", append=True)
+        victim_pid = m.spawn("sleep infinity", "sleep.log")
+        self.addCleanup(m.execute, f"kill {victim_pid} || true")
+        self.write_file("/home/admin/.pam_environment", f"SSH_AGENT_PID={victim_pid}\n", owner="admin")
+
+        b.login_and_go()
+        wait_session(should_exist=True)
+        # starts ssh-agent in session
+        m.execute("pgrep -u admin ssh-agent")
+        # but the session has the modified SSH_AGENT_PID
+        bridge = m.execute("pgrep -u admin cockpit-bridge").strip()
+        agent = m.execute(f"grep --null-data SSH_AGENT_PID /proc/{bridge}/environ | xargs -0 | sed 's/.*=//'").strip()
+        self.assertEqual(agent, str(victim_pid))
+
+        # logging out still kills the actual ssh-agent, not the victim pid
+        b.logout()
+        wait_session(should_exist=False)
+        m.execute("while pgrep -u admin ssh-agent; do sleep 1; done", timeout=10)
+        m.execute(f"test -e /proc/{victim_pid}")
+
 
 if __name__ == '__main__':
     testlib.test_main()