From d59dc2d6245ffcfd79276890f9092721b169c947 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 16:32:00 -0500 Subject: [PATCH 1/9] Fix checking for memory allocation errors These should never happen, but call exit() if they do. Also avoid freeing an uninitialized PAM handle in such an error case. I do not consider this a security vulnerability because there is no reasonable way I know of for an attacker to trigger this failure, but this commit should still be backported. --- agent/qrexec-agent.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index 871a57b8..fc1d201a 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -196,23 +196,31 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) */ 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; + if (!arg0) { + PERROR("malloc"); + exit(QREXEC_EXIT_PROBLEM); + } arg0[0] = '-'; strcpy (arg0 + 1, shell_basename); retval = pam_start("qrexec", user, &conv, &pamh); - if (retval != PAM_SUCCESS) + if (retval != PAM_SUCCESS) { + LOG("PAM handle could not be acquired"); + pamh = NULL; goto error; + } retval = pam_authenticate(pamh, 0); if (retval != PAM_SUCCESS) From fe57a3b9f0af98ca10f2cad9db93e3b77bc84c37 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 16:36:32 -0500 Subject: [PATCH 2/9] Set PAM error if snprintf() fails --- agent/qrexec-agent.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index fc1d201a..b60c99a3 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -241,28 +241,38 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) 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; From d19d60a4f139d88917bf959a6c1118cf4916446f Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 11:47:20 -0500 Subject: [PATCH 3/9] Get effective UID at startup Saves an (admittedly cheap) system call. --- agent/qrexec-agent.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index b60c99a3..361f4c91 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -77,6 +77,8 @@ 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; @@ -158,10 +160,10 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) 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(QREXEC_EXIT_PROBLEM); @@ -404,6 +406,7 @@ static void init(void) 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); From 03324b1e8aca2b28aa52c543dd0f45be2f4ecc34 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 11:57:37 -0500 Subject: [PATCH 4/9] Move waiting on the child to a helper function No functional change intended. --- agent/qrexec-agent.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index 361f4c91..8562c1f4 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -122,6 +122,24 @@ static struct pam_conv conv = { NULL }; #endif + +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: @@ -146,7 +164,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) 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; @@ -319,15 +337,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) } /* 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); From 06c2f3331ee8df0c7714cdfc8505a34e4cdf2dcd Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 12:00:40 -0500 Subject: [PATCH 5/9] agent: Move exec to helper function This will be used by tests later. No functional change intended. --- agent/qrexec-agent.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index 8562c1f4..ab170874 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -123,6 +123,28 @@ static struct pam_conv conv = { }; #endif +_Noreturn static void really_exec(const char *prog, + const struct passwd *pw, char **env, + const char *cmd, const char *arg0) +{ + /* 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 */ + execle(pw->pw_shell, arg0, "-c", cmd, (char*)NULL, env); + _exit(QREXEC_EXIT_PROBLEM); +} + static int really_wait(pid_t child) { int status; @@ -310,23 +332,10 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) _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, arg0); default: /* parent */ /* close std*, so when child process closes them, qrexec-agent will receive EOF */ From faa222042adf52bf07740563a2f6ae5110611342 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 12:01:22 -0500 Subject: [PATCH 6/9] Move closing fds to helper function This will be used by tests later. No functional change intended. --- agent/qrexec-agent.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index ab170874..b35bf566 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -145,6 +145,15 @@ _Noreturn static void really_exec(const char *prog, _exit(QREXEC_EXIT_PROBLEM); } +static void close_std(void) +{ + /* 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); +} + static int really_wait(pid_t child) { int status; @@ -338,11 +347,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) really_exec(prog, pw, env, cmd, arg0); 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(); } /* reachable only in parent */ From c77788ff9a2eeb9154d16ecd936ff3c1c032e86c Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 16:36:22 -0500 Subject: [PATCH 7/9] Move basename handling to common function This also fixes a bug: basename can mutate its argument, so a copy must be passed to it. --- agent/qrexec-agent.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index b35bf566..ab8d2187 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -123,9 +123,8 @@ static struct pam_conv conv = { }; #endif -_Noreturn static void really_exec(const char *prog, - const struct passwd *pw, char **env, - const char *cmd, const char *arg0) +_Noreturn static void really_exec(const char *prog, const struct passwd *pw, + char **env, const char *cmd) { /* child */ setsid(); @@ -141,6 +140,17 @@ _Noreturn static void really_exec(const char *prog, 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); } @@ -198,8 +208,6 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) pid_t child; char **env; char env_buf[64]; - char *arg0; - char *shell_basename; #endif sigset_t sigmask; @@ -256,16 +264,6 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) } 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) { - PERROR("malloc"); - exit(QREXEC_EXIT_PROBLEM); - } - arg0[0] = '-'; - strcpy (arg0 + 1, shell_basename); - retval = pam_start("qrexec", user, &conv, &pamh); if (retval != PAM_SUCCESS) { LOG("PAM handle could not be acquired"); @@ -344,7 +342,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) /* This is a copy but don't care to free as we exec later anyway. */ env = pam_getenvlist (pamh); - really_exec(prog, pw, env, cmd, arg0); + really_exec(prog, pw, env, cmd); default: /* parent */ close_std(); From 3a54a87cfb88c16ce14beb9a10bd13954c457789 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sat, 11 Jan 2025 12:02:17 -0500 Subject: [PATCH 8/9] Use fork()/exec() in unit test code This makes the unit test code more like the actual code used by end-users, and therefore makes the tests more accurate. This trips a bug in the code which will be fixed later, requiring a test to be changed to compensate. --- agent/qrexec-agent.c | 22 +++++++++++++--------- qrexec/tests/socket/agent.py | 2 +- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index ab8d2187..9c6c6706 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -230,16 +230,20 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) user, pw->pw_name); 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); - } - /* otherwise exec shell */ - execl("/bin/sh", "sh", "-c", cmd, NULL); - PERROR("execl"); - exit(QREXEC_EXIT_PROBLEM); + /* FORK HERE */ + child = fork(); + + switch (child) { + case -1: + goto error; + case 0: + really_exec(prog, pw, environ, cmd); + default: + /* parent */ + close_std(); + exit(really_wait(child)); + } } pw = getpwnam(user); diff --git a/qrexec/tests/socket/agent.py b/qrexec/tests/socket/agent.py index ba7718a6..e231669f 100644 --- a/qrexec/tests/socket/agent.py +++ b/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]: From ca34aa7987575b759c9f839ec9ef988a92e17882 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 9 Jan 2025 16:47:15 -0500 Subject: [PATCH 9/9] Do not close FDs 0, 1, or 2 If they are closed, another file descriptor could be created with these numbers, and so standard library functions that use them might write to an unwanted place. dup2() a file descriptor to /dev/null over them instead. Also statically initialize trigger_fd to -1, which is the conventional value for an invalid file descriptor. This requires care to avoid closing the file descriptor to /dev/null in fix_fds(), which took over an hour to debug. --- agent/qrexec-agent.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/agent/qrexec-agent.c b/agent/qrexec-agent.c index 9c6c6706..2588aba1 100644 --- a/agent/qrexec-agent.c +++ b/agent/qrexec-agent.c @@ -71,7 +71,7 @@ static libvchan_t *ctrl_vchan; static pid_t wait_for_session_pid = -1; -static int trigger_fd; +static int trigger_fd = -1; static int terminate_requested; @@ -155,13 +155,18 @@ _Noreturn static void really_exec(const char *prog, const struct passwd *pw, _exit(QREXEC_EXIT_PROBLEM); } -static void close_std(void) +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... */ - close(0); - close(1); - close(2); + 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) @@ -231,6 +236,12 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) exit(QREXEC_EXIT_PROBLEM); } + int null_fd = open("/dev/null", O_RDWR|O_CLOEXEC); + if (null_fd == -1) { + LOG(ERROR, "cannot open /dev/null"); + exit(QREXEC_EXIT_PROBLEM); + } + /* FORK HERE */ child = fork(); @@ -241,7 +252,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) really_exec(prog, pw, environ, cmd); default: /* parent */ - close_std(); + close_std(null_fd); exit(really_wait(child)); } } @@ -330,6 +341,12 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) 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(); @@ -349,7 +366,7 @@ _Noreturn void do_exec(const char *prog, const char *cmd, const char *user) really_exec(prog, pw, env, cmd); default: /* parent */ - close_std(); + close_std(null_fd); } /* reachable only in parent */