use DynamicUser=yes for all cockpit components

.gitignore

@@ -76,7 +76,7 @@ Makefile.in
 /src/cockpit.egg-info/
 /src/common/fail-html.c
 /src/systemd/cockpit*.service
-/src/systemd/cockpit*.socket
+/src/systemd/cockpit.socket
 /src/systemd/tmpfiles.d/cockpit-ws.conf
 /src/tls/cockpit-certificate-helper
 /src/ws/cockpit-desktop

doc/authentication.md

@@ -51,9 +51,13 @@ The response will look something like this
     "command": "authorize",
     "cookie": "cookie",
     "response": "Basic dXNlcjpwYXNzd29yZAo=",
+    "remote-peer": "1.2.3.4",
 }
 ```
 
+`remote-peer` is the IP address of the connecting user, if known (otherwise
+unset).
+
 A `*` challenge requests whatever credentials the parent process has. Most auth
 commands will want to begin by issuing a `*` challenge.
 
@@ -188,11 +192,8 @@ for more details. This will affect how many custom authentication processes can
 Environment Variables
 ---------------------
 
-The following environment variables are set by cockpit-ws when spawning an auth process:
-
- * **COCKPIT_REMOTE_PEER** Set to the ip address of the connecting user.
-
-The following environment variables are used to set options for SSH connections:
+The following environment variables are set by cockpit-ws when spawning an auth process for
+SSH connections:
 
  * **COCKPIT_SSH_CONNECT_TO_UNKNOWN_HOSTS** Set to `1` to  allow connecting to
    hosts that are not present in the current `known_hosts` files. If not set,

selinux/cockpit.te

@@ -31,6 +31,7 @@ type cockpit_session_t;
 type cockpit_session_exec_t;
 domain_type(cockpit_session_t)
 domain_entry_file(cockpit_session_t,cockpit_session_exec_t)
+init_daemon_domain(cockpit_session_t,cockpit_session_exec_t)
 
 ########################################
 #
@@ -109,7 +110,8 @@ sysnet_exec_ifconfig(cockpit_ws_t)
 cockpit_session_domtrans(cockpit_ws_t)
 allow cockpit_ws_t cockpit_session_t:process signal_perms;
 
-# cockpit-session communicates back with cockpit-ws
+# cockpit-session communicates with cockpit-ws over a unix socket
+allow cockpit_ws_t cockpit_session_t:unix_stream_socket connectto;
 allow cockpit_session_t cockpit_ws_t:unix_stream_socket rw_stream_socket_perms;
 
 # cockpit-tls and cockpit-ws communicate over a Unix socket
@@ -169,6 +171,13 @@ list_dirs_pattern(cockpit_session_t, cockpit_var_run_t, cockpit_var_run_t)
 
 kernel_read_network_state(cockpit_session_t)
 
+# SELinux-restricted users can communicate with existing cockpit-session socket connection
+gen_require(`
+    type user_t;
+    type sysadm_t;
+')
+allow { user_t sysadm_t } cockpit_session_t:unix_stream_socket rw_stream_socket_perms;
+
 # cockpit-session runs a full pam stack, including pam_selinux.so
 auth_login_pgm_domain(cockpit_session_t)
 # cockpit-session resseting expired passwords

src/common/cockpitframe.c

@@ -277,8 +277,9 @@ cockpit_frame_read (int fd,
       return -1;
     }
 
-  /* We now have size equal to the number of bytes we need to return. */
-  unsigned char *buffer = malloc (size);
+  /* We now have size equal to the number of bytes we need to return. Add an extra byte for a NUL terminator,
+   * to be friendly to callers for text frames. */
+  unsigned char *buffer = calloc (1, size + 1);
   if (buffer == NULL)
     return -1; /* ENOMEM */
 

src/common/test-frame.c

@@ -122,6 +122,8 @@ test_valid (Fixture *pipe, const TestCase *tc)
       g_assert_cmpint (size, ==, i);
       for (gint j = 0; j < size; j++)
         g_assert (output[j] == ' ');
+      /* ensure it's NUL terminated */
+      g_assert (output[size] == '\0');
 
       /* Make sure our pattern is there */
       char buffer[7];

src/session/Makefile-session.am

@@ -20,8 +20,3 @@ cockpit_session_SOURCES = \
 	src/session/session-utils.h \
 	src/session/session.c \
 	$(NULL)
-
-# set up cockpit-session to be setuid root, but only runnable by cockpit-session
-install-exec-hook::
-	chown -f root:cockpit-wsinstance $(DESTDIR)$(libexecdir)/cockpit-session || true
-	chmod -f 4750 $(DESTDIR)$(libexecdir)/cockpit-session || true

src/session/client-certificate.c

@@ -31,7 +31,9 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <sys/socket.h>
 #include <sys/stat.h>
+#include <sys/un.h>
 #include <syslog.h>
 #include <unistd.h>
 
@@ -41,6 +43,19 @@
 
 #define CLIENT_CERTIFICATE_DIRECTORY   "/run/cockpit/tls/clients"
 
+static int
+open_proc_pid (pid_t pid)
+{
+    char path[100];
+    int r = snprintf (path, sizeof (path), "/proc/%d", pid);
+    if (r < 0 || r >= sizeof (path))
+        errx (EX, "memory error");
+    int fd = open (path, O_DIRECTORY | O_NOFOLLOW | O_PATH);
+    if (fd < 0)
+      err (EX,  "failed to open %s", path);
+    return fd;
+}
+
 /* This is a bit lame, but having a hard limit on peer certificates is
  * desirable: Let's not get DoSed by huge certs */
 #define MAX_PEER_CERT_SIZE 100000
@@ -49,20 +64,34 @@
  * including "0::" prefix and newline.
  * NB: the kernel doesn't allow newlines in cgroup names. */
 static char *
-read_proc_self_cgroup (size_t *out_length)
+read_proc_pid_cgroup (int dirfd, size_t *out_length)
 {
-  FILE *fp = fopen ("/proc/self/cgroup", "r");
-
-  if (fp == NULL)
+  int fd = openat (dirfd, "cgroup", O_RDONLY | O_CLOEXEC);
+  if (fd < 0)
     {
-      warn ("Failed to open /proc/self/cgroup");
+      warn ("Failed to open /proc/pid/cgroup");
       return NULL;
     }
 
   char buffer[1024];
-  *out_length = fread (buffer, 1, sizeof (buffer) - 1, fp);
+  int ret;
+  while (true)
+    {
+      ret = read (fd, buffer, sizeof (buffer));
+      if (ret < 0)
+        {
+          if (errno == EINTR)
+            continue;
+          warn ("Failed to read /proc/pid/cgroup");
+          close (fd);
+          return NULL;
+        }
+      break;
+    }
+
+  *out_length = ret;
   buffer[*out_length] = '\0';
-  fclose (fp);
+  close (fd);
 
   if (*out_length > 5 &&  /* at least "0::/\n" */
       *out_length <= sizeof (buffer) - 1 && /* not too big */
@@ -74,6 +103,43 @@ read_proc_self_cgroup (size_t *out_length)
   return NULL;
 }
 
+static
+unsigned long long get_proc_pid_start_time (int dirfd)
+{
+  int fd = openat (dirfd, "stat", O_RDONLY);
+  if (fd < 0)
+    err (EX, "Failed to open /proc/pid/stat");
+
+  char buffer[4096];
+  ssize_t size = read (fd, buffer, sizeof buffer);
+  if (size < 0)
+    err (EX, "Failed to read /proc/pid/stat");
+  close (fd);
+
+  /* ensure this is NUL terminated */
+  buffer[size] = '\0';
+
+  /* start time is the token at index 19 after the '(process name)' entry - since only this
+  * field can contain the ')' character, search backwards for this to avoid malicious
+  * processes trying to fool us; See proc_pid_stat(5) */
+  char *p = strrchr (buffer, ')');
+  if (p == NULL)
+    err (EX, "Failed to find process name in /proc/pid/stat");
+  for (int i = 0; i <= 19; i++) /* NB: ')' is the first token */
+    {
+      p = strchr (p, ' ');
+      if (p == NULL)
+        err (EX, "Failed to find start time in /proc/pid/stat");
+      ++p; /* skip over the space */
+    }
+
+  char *endptr;
+  unsigned long long start_time = strtoull (p, &endptr, 10);
+  if (*endptr != ' ')
+    errx (EX, "Failed to parse start time in /proc/pid/stat from %s", p);
+  return start_time;
+}
+
 /* valid_256_bit_hex_string:
  * @str: a string
  *
@@ -119,7 +185,7 @@ read_cert_file (const char *filename,
       goto out;
     }
 
-  dirfd = open (CLIENT_CERTIFICATE_DIRECTORY, O_PATH | O_DIRECTORY | O_NOFOLLOW);
+  dirfd = open (CLIENT_CERTIFICATE_DIRECTORY, O_PATH | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC);
   if (dirfd == -1)
     {
       warn ("Failed to open " CLIENT_CERTIFICATE_DIRECTORY);
@@ -300,27 +366,58 @@ cockpit_session_client_certificate_map_user (const char *client_certificate_file
       return NULL;
     }
 
-  size_t my_cgroup_length;
-  char *my_cgroup = read_proc_self_cgroup (&my_cgroup_length);
-  if (my_cgroup == NULL)
+  /* check if stdin is a Unix socket; then we are being called via [email protected] and need to
+   * check the cgroup of our peer */
+  struct ucred ucred;
+  int ws_pid_dirfd;
+  if (getsockopt (STDIN_FILENO, SOL_SOCKET, SO_PEERCRED, &ucred, &(socklen_t){sizeof ucred}) == 0)
+    {
+      debug ("unix socket mode, reading cgroup from peer pid %d", ucred.pid);
+      ws_pid_dirfd = open_proc_pid (ucred.pid);
+      int my_pid_dirfd = open_proc_pid (getpid ());
+      unsigned long ws_start_time = get_proc_pid_start_time (ws_pid_dirfd);
+      unsigned long my_start_time = get_proc_pid_start_time (my_pid_dirfd);
+      debug ("peer start time: %lu, my start time: %lu", ws_start_time, my_start_time);
+      close (my_pid_dirfd);
+
+      /* Guard against pid recycling: If a malicious user captures ws, keeps the socket in a forked child and exits
+       * the original pid, they can trap a different user to login, get the old pid (pointing ot their cgroup), and
+       * capture their session. To prevent that, require that ws must have started earlier than ourselves. */
+      if (my_start_time < ws_start_time)
+        {
+          warnx ("start time of this process (%lu) is older than cockpit-ws (%lu), pid recycling attack?",
+                  my_start_time, ws_start_time);
+          close (ws_pid_dirfd);
+          return NULL;
+        }
+    }
+  else {
+    ws_pid_dirfd = open_proc_pid (getpid ());
+    debug ("failed to read stdin peer credentials: %m; not in socket mode, reading cgroup from my own pid %d", ws_pid_dirfd);
+  }
+
+  size_t ws_cgroup_length;
+  char *ws_cgroup = read_proc_pid_cgroup (ws_pid_dirfd, &ws_cgroup_length);
+  close (ws_pid_dirfd);
+  if (ws_cgroup == NULL)
     {
       warnx ("Could not determine cgroup of this process");
       return NULL;
     }
-  /* A simple prefix comparison is appropriate here because my_cgroup
+  /* A simple prefix comparison is appropriate here because ws_cgroup
    * will contain exactly one newline (at the end), and the expected
-   * value of my_cgroup is on the first line in cert_pem.
+   * value of ws_cgroup is on the first line in cert_pem.
    */
-  if (strncmp (cert_pem, my_cgroup, my_cgroup_length) != 0)
+  if (strncmp (cert_pem, ws_cgroup, ws_cgroup_length) != 0)
     {
       warnx ("This client certificate is only meant to be used from another cgroup");
-      free (my_cgroup);
+      free (ws_cgroup);
       return NULL;
     }
-  free (my_cgroup);
+  free (ws_cgroup);
 
   /* ask sssd to map cert to a user */
-  if (!sssd_map_certificate (cert_pem + my_cgroup_length, &sssd_user))
+  if (!sssd_map_certificate (cert_pem + ws_cgroup_length, &sssd_user))
     return NULL;
 
   return sssd_user;

src/session/session-utils.c

@@ -24,6 +24,7 @@
 #include "common/cockpitframe.h"
 #include "common/cockpitjsonprint.h"
 #include "common/cockpithacks.h"
+#include "common/cockpitmemory.h"
 
 #include <fcntl.h>
 #include <paths.h>
@@ -52,36 +53,52 @@ static FILE *authf = NULL;
 char *
 read_authorize_response (const char *what)
 {
-  const char *auth_response = ",\"response\":\"";
-  size_t auth_response_size = 13;
-  const char *auth_suffix = "\"}";
-  size_t auth_suffix_size = 2;
   unsigned char *message;
-  ssize_t len;
-
   debug ("reading %s authorize message", what);
-
-  len = cockpit_frame_read (STDIN_FILENO, &message);
+  ssize_t len = cockpit_frame_read (STDIN_FILENO, &message);
   if (len < 0)
     err (EX, "couldn't read %s", what);
+  if (len == 0)
+    {
+       debug ("EOF reading %s authorize message", what);
+       exit (0);
+    }
+  return (char *)message;
+}
 
+char *get_authorize_key (const char *json, const char *key, bool required)
+{
   /*
-   * The authorize messages we receive always have an exact prefix and suffix:
+   * The authorize messages we receive always have this structure:
+   *
+   * \n{"command":"authorize","cookie":"NNN","response":"....","remote-peer":"..."}
    *
-   * \n{"command":"authorize","cookie":"NNN","response":"...."}
+   * We make many assumptions/restrictions here to keep the parser simple. In particular, no extra
+   * whitespace, everything is a string, and no escaped ".
    */
-  if (len <= auth_prefix_size + auth_response_size + auth_suffix_size ||
-      memcmp (message, auth_prefix, auth_prefix_size) != 0 ||
-      memcmp (message + auth_prefix_size, auth_response, auth_response_size) != 0 ||
-      memcmp (message + (len - auth_suffix_size), auth_suffix, auth_suffix_size) != 0)
+  if (json == NULL || json[0] != '\n' || json[1] != '{')
+    errx (EX, "authorize message doesn't start with \\n{");
+
+  char *key_match;
+  int key_match_len = asprintfx (&key_match, "\"%s\":\"", key);
+
+  const char *match = strstr (json, key_match);
+  free (key_match);
+  if (!match)
     {
-      errx (EX, "didn't receive expected \"authorize\" message");
+      if (required)
+        errx (EX, "authorize message missing required key %s", key);
+      debug ("authorize message missing optional key %s", key);
+      return NULL;
     }
 
-  len -= auth_prefix_size + auth_response_size + auth_suffix_size;
-  memmove (message, message + auth_prefix_size + auth_response_size, len);
-  message[len] = '\0';
-  return (char *)message;
+  /* advance to the start of the value */
+  match += key_match_len;
+  /* find end of value */
+  size_t value_len = strcspn (match, "\"");
+  if (match[value_len] != '"')
+    errx (EX, "syntax error, expected closing quote");
+  return strndup (match, value_len);
 }
 
 void

src/session/session-utils.h

@@ -64,6 +64,7 @@ void utmp_log (int login, const char *rhost, FILE *messages);
 void btmp_log (const char *username, const char *rhost);
 
 char* read_authorize_response (const char *what);
+char* get_authorize_key (const char *json, const char *key, bool required);
 void write_authorize_begin (void);
 void write_control_string (const char *field, const char *str);
 void write_control_bool (const char *field, bool val);