Preparations for launching cockpit-session via unix socket activation

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/client-certificate.c

@@ -328,27 +328,27 @@ 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)
+  size_t ws_cgroup_length;
+  char *ws_cgroup = read_proc_self_cgroup (&ws_cgroup_length);
+  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":"...."}
+   *
+   * 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);

src/session/session.c

@@ -42,6 +42,9 @@ static char *conversation = NULL;
 
 #define COCKPIT_KTAB PACKAGE_SYSCONF_DIR "/cockpit/krb5.keytab"
 
+/* exit status if cockpit-bridge binary does not exist */
+#define EXIT_NOT_FOUND 127
+
 static gss_cred_id_t creds = GSS_C_NO_CREDENTIAL;
 
 /* Environment variables to transfer */
@@ -136,8 +139,6 @@ pam_conv_func (int num_msg,
                void *appdata_ptr)
 {
   char **password = (char **)appdata_ptr;
-  char *authorization = NULL;
-  char *prompt_resp = NULL;
 
   /* For keeping track of messages returned by PAM */
   char *err_msg = NULL;
@@ -216,8 +217,11 @@ pam_conv_func (int num_msg,
               txt_msg = NULL;
             }
 
-          authorization = read_authorize_response (msg[i]->msg);
-          prompt_resp = cockpit_authorize_parse_x_conversation (authorization, NULL);
+          char *authorization = read_authorize_response (msg[i]->msg);
+          char *response = get_authorize_key (authorization, "response", true);
+          char *prompt_resp = cockpit_authorize_parse_x_conversation (response, NULL);
+          cockpit_memory_clear (response, -1);
+          free (response);
 
           debug ("got prompt response");
           if (prompt_resp)
@@ -231,8 +235,7 @@ pam_conv_func (int num_msg,
               success = 0;
             }
 
-          if (authorization)
-            cockpit_memory_clear (authorization, -1);
+          cockpit_memory_clear (authorization, -1);
           free (authorization);
         }
     }
@@ -389,13 +392,21 @@ exit_init_problem (int result_code)
     problem = "access-denied";
   else if (result_code == PAM_AUTHINFO_UNAVAIL)
     problem = "authentication-unavailable";
+  else if (result_code == EXIT_NOT_FOUND)
+    {
+      problem = "no-cockpit";
+      message = "cockpit-bridge not found";
+    }
   else
     problem = "internal-error";
 
-  if (last_err_msg)
-    message = last_err_msg;
-  else
-    message = pam_strerror (NULL, result_code);
+  if (!message)
+    {
+        if (last_err_msg)
+            message = last_err_msg;
+        else
+            message = pam_strerror (NULL, result_code);
+    }
 
   if (asprintf (&payload, "\n{\"command\":\"init\",\"version\":1,\"problem\":\"%s\",\"message\":\"%s\"}",
                 problem, message) < 0)
@@ -577,7 +588,6 @@ perform_gssapi (const char *rhost,
   gss_ctx_id_t context = GSS_C_NO_CONTEXT;
   gss_OID mech_type = GSS_C_NO_OID;
   pam_handle_t *pamh = NULL;
-  char *response = NULL;
   char *challenge;
   OM_uint32 flags = 0;
   char *str = NULL;
@@ -646,11 +656,17 @@ perform_gssapi (const char *rhost,
       input.length = 0;
 
       debug ("need to continue gssapi negotiation");
-      response = read_authorize_response ("negotiate");
+      char *authorize = read_authorize_response ("negotiate");
+      char *response = get_authorize_key (authorize, "response", false);
+      cockpit_memory_clear (authorize, -1);
+      free (authorize);
+
       input.value = cockpit_authorize_parse_negotiate (response, &input.length);
       if (response)
-        cockpit_memory_clear (response, -1);
-      free (response);
+        {
+          cockpit_memory_clear (response, -1);
+          free (response);
+        }
     }
 
   str = map_gssapi_to_local (name, mech_type);
@@ -897,7 +913,6 @@ main (int argc,
   pam_handle_t *pamh = NULL;
   OM_uint32 minor;
   const char *rhost;
-  char *authorization;
   char *type = NULL;
   const char **env;
   char *ccache = NULL;
@@ -943,19 +958,23 @@ main (int argc,
   write_control_end ();
 
   /* And get back the authorization header */
-  authorization = read_authorize_response ("authorization");
-  if (!cockpit_authorize_type (authorization, &type))
+  char *authorization = read_authorize_response ("authorization");
+  char *response = get_authorize_key (authorization, "response", true);
+  cockpit_memory_clear (authorization, -1);
+  free (authorization);
+
+  if (!cockpit_authorize_type (response, &type))
     errx (EX, "invalid authorization header received");
 
   if (strcmp (type, "basic") == 0)
-    pamh = perform_basic (rhost, authorization);
+    pamh = perform_basic (rhost, response);
   else if (strcmp (type, "negotiate") == 0)
-    pamh = perform_gssapi (rhost, authorization);
+    pamh = perform_gssapi (rhost, response);
   else if (strcmp (type, "tls-cert") == 0)
-    pamh = perform_tlscert (rhost, authorization);
+    pamh = perform_tlscert (rhost, response);
 
-  cockpit_memory_clear (authorization, -1);
-  free (authorization);
+  cockpit_memory_clear (response, -1);
+  free (response);
 
   if (!pamh)
     errx (2, "unrecognized authentication method: %s", type);
@@ -1035,6 +1054,13 @@ main (int argc,
       status = spawn_and_wait (bridge_argv, env, NULL, -1, pwd->pw_uid, pwd->pw_gid);
     }
 
+  if (WIFEXITED (status) && WEXITSTATUS (status) == EXIT_NOT_FOUND)
+    {
+      debug ("bridge exited with status %d, no-cockpit", status);
+      exit_init_problem (EXIT_NOT_FOUND);
+    }
+  debug ("bridge exited with status %d", status);
+
   pam_end (pamh, PAM_SUCCESS);
 
   free (last_err_msg);

src/systemd/cockpit-session@.service.in

@@ -7,3 +7,5 @@ StandardInput=socket
 StandardOutput=inherit
 StandardError=journal
 User=root
+# authentication failure or timeout, that's not a problem with the unit
+SuccessExitStatus=1 5 127

src/ws/Makefile-ws.am

@@ -110,7 +110,7 @@ mock_echo_SOURCES = src/ws/mock-echo.c
 check_PROGRAMS += mock-auth-command
 mock_auth_command_CPPFLAGS = $(libcockpit_common_a_CPPFLAGS) $(TEST_CPP)
 mock_auth_command_LDADD = $(libcockpit_common_a_LIBS) $(TEST_LIBS)
-mock_auth_command_SOURCES = src/ws/mock-auth-command.c
+mock_auth_command_SOURCES = src/ws/mock-auth-command.c src/session/session-utils.c
 
 TEST_PROGRAM += test-auth
 test_auth_CPPFLAGS = $(libcockpit_ws_a_CPPFLAGS) $(TEST_CPP)