Do not close FDs 0, 1, or 2

agent/qrexec-agent.c

@@ -71,12 +71,14 @@
 
 static pid_t wait_for_session_pid = -1;
 
-static int trigger_fd;
+static int trigger_fd = -1;
 
 static int terminate_requested;
 
 static int meminfo_write_started = 0;
 
+static uid_t myuid;
+
 static const char *agent_trigger_path = QREXEC_AGENT_TRIGGER_PATH;
 static const char *fork_server_path = QREXEC_FORK_SERVER_SOCKET;
 
@@ -120,6 +122,70 @@
     NULL
 };
 #endif
+
+_Noreturn static void really_exec(const char *prog, const struct passwd *pw,
+                                  char **env, const char *cmd)
+{
+    /* child */
+    setsid();
+
+    /* try to enter home dir, but don't abort if it fails */
+    int retval = chdir(pw->pw_dir);
+    if (retval == -1)
+        warn("chdir(%s)", pw->pw_dir);
+
+    /* call QUBESRPC if requested */
+    if (prog) {
+        /* Set up environment variables for a login shell. */
+        exec_qubes_rpc2(prog, cmd, env, true);
+    }
+    /* otherwise exec shell */
+    char *shell_dup = strdup(pw->pw_shell);
+    if (shell_dup == NULL)
+        _exit(QREXEC_EXIT_PROBLEM);
+    char *shell_basename = basename (shell_dup);
+    /* this process is going to die shortly, so don't care about freeing */
+    size_t len = strlen(shell_basename) + 1;
+    char *arg0 = malloc(len + 1);
+    if (!arg0)
+        _exit(QREXEC_EXIT_PROBLEM);
+    arg0[0] = '-';
+    memcpy(arg0 + 1, shell_basename, len);
+    execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env);
+    _exit(QREXEC_EXIT_PROBLEM);
+}
+
+static void close_std(int null_fd)
+{
+    /* close std*, so when child process closes them, qrexec-agent will receive EOF */
+    /* this is the main purpose of this reimplementation of /bin/su... */
+    for (int i = 0; i < 3; ++i) {
+        int j;
+        do {
+            j = dup2(null_fd, i);
+        } while (j == -1 && (errno == EINTR || errno == EBUSY));
+        if (j != i)
+            abort();
+    }
+}
+
+static int really_wait(pid_t child)
+{
+    int status;
+    pid_t pid;
+    do {
+        pid = waitpid (child, &status, 0);
+    } while (pid == -1 && errno == EINTR);
+    if (pid != (pid_t)-1) {
+        if (WIFSIGNALED (status))
+            status = WTERMSIG (status) + 128;
+        else
+            status = WEXITSTATUS (status);
+    } else
+        status = QREXEC_EXIT_PROBLEM;
+    return status;
+}
+
 /* Start program requested by dom0 in already prepared process
  * (stdin/stdout/stderr already set, etc)
  * Called in two cases:
@@ -144,11 +210,9 @@
     pam_handle_t *pamh=NULL;
     struct passwd *pw;
     struct passwd pw_copy;
-    pid_t child, pid;
+    pid_t child;
     char **env;
     char env_buf[64];
-    char *arg0;
-    char *shell_basename;
 #endif
     sigset_t sigmask;
 
@@ -158,36 +222,46 @@
     signal(SIGPIPE, SIG_DFL);
 
 #ifdef HAVE_PAM
-    if (geteuid() != 0) {
+    if (myuid != 0) {
         /* We're not root, assume this is a testing environment. */
 
-        pw = getpwuid(geteuid());
+        pw = getpwuid(myuid);
         if (!pw) {
             PERROR("getpwuid");
-            exit(1);
+            exit(QREXEC_EXIT_PROBLEM);
         }
         if (strcmp(pw->pw_name, user)) {
             LOG(ERROR, "requested user %s, but qrexec-agent is running as user %s",
                 user, pw->pw_name);
-            exit(1);
+            exit(QREXEC_EXIT_PROBLEM);
         }
-        /* call QUBESRPC if requested */
-        if (prog) {
-            /* no point in creating a login shell for test environments */
-            exec_qubes_rpc2(prog, cmd, environ, false);
+
+        int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC);
+        if (null_fd == -1) {
+            LOG(ERROR, "cannot open /dev/null");
+            exit(QREXEC_EXIT_PROBLEM);
         }
 
-        /* otherwise exec shell */
-        execl("/bin/sh", "sh", "-c", cmd, NULL);
-        PERROR("execl");
-        exit(1);
+        /* FORK HERE */
+        child = fork();
+
+        switch (child) {
+            case -1:
+                goto error;
+            case 0:
+                really_exec(prog, pw, environ, cmd);
+            default:
+                /* parent */
+                close_std(null_fd);
+                exit(really_wait(child));
+        }
     }
 
     pw = getpwnam(user);
     if (! (pw && pw->pw_name && pw->pw_name[0] && pw->pw_dir && pw->pw_dir[0]
                 && pw->pw_passwd)) {
         LOG(ERROR, "user %s does not exist", user);
-        exit(1);
+        exit(QREXEC_EXIT_PROBLEM);
     }
 
     /* Make a copy of the password information and point pw at the local
@@ -196,23 +270,20 @@
      */
     pw_copy = *pw;
     pw = &pw_copy;
-    pw->pw_name = strdup(pw->pw_name);
-    pw->pw_passwd = strdup(pw->pw_passwd);
-    pw->pw_dir = strdup(pw->pw_dir);
-    pw->pw_shell = strdup(pw->pw_shell);
+    if (!((pw->pw_name = strdup(pw->pw_name)) &&
+          (pw->pw_passwd = strdup(pw->pw_passwd)) &&
+          (pw->pw_dir = strdup(pw->pw_dir)) &&
+          (pw->pw_shell = strdup(pw->pw_shell)))) {
+        PERROR("strdup");
+        exit(QREXEC_EXIT_PROBLEM);
+    }
     endpwent();
 
-    shell_basename = basename (pw->pw_shell);
-    /* this process is going to die shortly, so don't care about freeing */
-    arg0 = malloc (strlen (shell_basename) + 2);
-    if (!arg0)
-        goto error;
-    arg0[0] = '-';
-    strcpy (arg0 + 1, shell_basename);
-
     retval = pam_start("qrexec", user, &conv, &pamh);
-    if (retval != PAM_SUCCESS)
+    if (retval != PAM_SUCCESS) {
+        pamh = NULL;
         goto error;
+    }
 
     retval = pam_authenticate(pamh, 0);
     if (retval != PAM_SUCCESS)
@@ -233,32 +304,48 @@
         goto error;
 
     /* provide this variable to child process */
-    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "QREXEC_AGENT_PID=%d", getppid()) >= sizeof(env_buf))
+    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "QREXEC_AGENT_PID=%d", getppid()) >= sizeof(env_buf)) {
+        retval = PAM_ABORT;
         goto error;
+    }
     retval = pam_putenv(pamh, env_buf);
     if (retval != PAM_SUCCESS)
         goto error;
-    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "HOME=%s", pw->pw_dir) >= sizeof(env_buf))
+    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "HOME=%s", pw->pw_dir) >= sizeof(env_buf)) {
+        retval = PAM_ABORT;
         goto error;
+    }
     retval = pam_putenv(pamh, env_buf);
     if (retval != PAM_SUCCESS)
         goto error;
-    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "SHELL=%s", pw->pw_shell) >= sizeof(env_buf))
+    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "SHELL=%s", pw->pw_shell) >= sizeof(env_buf)) {
+        retval = PAM_ABORT;
         goto error;
+    }
     retval = pam_putenv(pamh, env_buf);
     if (retval != PAM_SUCCESS)
         goto error;
-    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "USER=%s", pw->pw_name) >= sizeof(env_buf))
+    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "USER=%s", pw->pw_name) >= sizeof(env_buf)) {
+        retval = PAM_ABORT;
         goto error;
+    }
     retval = pam_putenv(pamh, env_buf);
     if (retval != PAM_SUCCESS)
         goto error;
-    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "LOGNAME=%s", pw->pw_name) >= sizeof(env_buf))
+    if ((unsigned)snprintf(env_buf, sizeof(env_buf), "LOGNAME=%s", pw->pw_name) >= sizeof(env_buf)) {
+        retval = PAM_ABORT;
         goto error;
+    }
     retval = pam_putenv(pamh, env_buf);
     if (retval != PAM_SUCCESS)
         goto error;
 
+    int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC);
+    if (null_fd == -1) {
+        LOG(ERROR, "cannot open /dev/null");
+        goto error;
+    }
+
     /* FORK HERE */
     child = fork();
 
@@ -272,54 +359,29 @@
                 _exit(QREXEC_EXIT_PROBLEM);
             if (setuid (pw->pw_uid))
                 _exit(QREXEC_EXIT_PROBLEM);
-            setsid();
             /* This is a copy but don't care to free as we exec later anyway.  */
             env = pam_getenvlist (pamh);
 
-            /* try to enter home dir, but don't abort if it fails */
-            retval = chdir(pw->pw_dir);
-            if (retval == -1)
-                warn("chdir(%s)", pw->pw_dir);
-
-            /* call QUBESRPC if requested */
-            if (prog) {
-                /* Set up environment variables for a login shell. */
-                exec_qubes_rpc2(prog, cmd, env, true);
-            }
-            /* otherwise exec shell */
-            execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env);
-            _exit(QREXEC_EXIT_PROBLEM);
+            really_exec(prog, pw, env, cmd);
         default:
             /* parent */
-            /* close std*, so when child process closes them, qrexec-agent will receive EOF */
-            /* this is the main purpose of this reimplementation of /bin/su... */
-            close(0);
-            close(1);
-            close(2);
+            close_std(null_fd);
     }
 
     /* reachable only in parent */
-    pid = waitpid (child, &status, 0);
-    if (pid != (pid_t)-1) {
-        if (WIFSIGNALED (status))
-            status = WTERMSIG (status) + 128;
-        else
-            status = WEXITSTATUS (status);
-    } else
-        status = 1;
-
+    status = really_wait(child);
     retval = pam_close_session (pamh, 0);
 
     retval = pam_setcred (pamh, PAM_DELETE_CRED | PAM_SILENT);
 
     if (pam_end(pamh, retval) != PAM_SUCCESS) {     /* close Linux-PAM */
         pamh = NULL;
-        exit(1);
+        exit(QREXEC_EXIT_PROBLEM);
     }
     exit(status);
 error:
     pam_end(pamh, PAM_ABORT);
-    exit(1);
+    exit(QREXEC_EXIT_PROBLEM);
 #else
     /* call QUBESRPC if requested */
     if (prog) {
@@ -329,7 +391,7 @@
     /* otherwise exec shell */
     execl("/bin/su", "su", "-", user, "-c", cmd, NULL);
     PERROR("execl");
-    exit(1);
+    exit(QREXEC_EXIT_PROBLEM);
 #endif
 
 }
@@ -386,6 +448,7 @@
     if (handle_handshake(ctrl_vchan) < 0)
         exit(1);
     old_umask = umask(0);
+    myuid = geteuid();
     trigger_fd = get_server_socket(agent_trigger_path);
     umask(old_umask);
     register_exec_func(do_exec);
@@ -858,7 +921,7 @@
 static struct option longopts[] = {
     { "help", no_argument, 0, 'h' },
     { "agent-socket", required_argument, 0, 'a' },
-    { "fork-server-socket", optional_argument, 0, 's' },
+    { "fork-server-socket", required_argument, 0, 's' },
     { "no-fork-server", no_argument, 0, 'S' },
     { NULL, 0, 0, 0 },
 };

qrexec/tests/socket/agent.py

@@ -602,7 +602,7 @@ def test_exec_service_bad_service(self):
             messages[-2:],
             [
                 (qrexec.MSG_DATA_STDERR, b""),
-                (qrexec.MSG_DATA_EXIT_CODE, b"\175\0\0\0"),
+                (qrexec.MSG_DATA_EXIT_CODE, b"\1\0\0\0"),
             ],
         )
         for msg_type, msg_value in messages[1:-2]: