From 13fcdb4f5f3ced21c806b07507b1a64cfdde6e9f Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Fri, 24 Feb 2017 15:14:56 -0700 Subject: [PATCH] Set the child pid to -1 after we've waited for it and take care to avoid killing pid -1. This makes it a bit more explicit and removes the need for a separate variable to track the child's status. Sudo already stops processing signals after it receives SIGCHLD so it is not vulnerable to CVE-2017-2616. --- src/exec.c | 91 ++++++++++++++++++++++++++------------------------ src/exec_pty.c | 32 ++++++++++-------- 2 files changed, 65 insertions(+), 58 deletions(-) diff --git a/src/exec.c b/src/exec.c index f74e73edf..9d24da5a6 100644 --- a/src/exec.c +++ b/src/exec.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009-2016 Todd C. Miller + * Copyright (c) 2009-2017 Todd C. Miller * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -243,7 +243,7 @@ backchannel_cb(int fd, int what, void *v) case CMD_PID: /* * Once we know the command's pid we can unblock - * signals which ere blocked in fork_pty(). This + * signals which were blocked in fork_pty(). This * avoids a race between exec of the command and * receipt of a fatal signal from it. */ @@ -498,9 +498,9 @@ sudo_execute(struct command_details *details, struct command_status *cstat) if (sudo_ev_got_break(evbase)) { /* error from callback */ sudo_debug_printf(SUDO_DEBUG_ERROR, "event loop exited prematurely"); - /* kill command if still running and not I/O logging */ - if (!log_io && kill(child, 0) == 0) - terminate_command(child, true); + /* kill command if not I/O logging */ + if (!log_io) + terminate_command(ec.child, true); } if (log_io) { @@ -530,18 +530,20 @@ done: } /* - * Forward a signal to the command (non-pty version). + * Forward a signal to the command (non-pty version) or handle + * changes to the command's status (SIGCHLD). */ -static int -dispatch_signal(struct sudo_event_base *evbase, pid_t child, - int signo, char *signame, struct command_status *cstat) +static void +dispatch_signal(struct exec_closure *ec, int signo, char *signame) { - int rc = 1; debug_decl(dispatch_signal, SUDO_DEBUG_EXEC) sudo_debug_printf(SUDO_DEBUG_INFO, "%s: evbase %p, child: %d, signo %s(%d), cstat %p", - __func__, evbase, (int)child, signame, signo, cstat); + __func__, ec->evbase, (int)ec->child, signame, signo, ec->cstat); + + if (ec->child == -1) + goto done; if (signo == SIGCHLD) { pid_t pid; @@ -550,9 +552,9 @@ dispatch_signal(struct sudo_event_base *evbase, pid_t child, * The command stopped or exited. */ do { - pid = waitpid(child, &status, WUNTRACED|WNOHANG); + pid = waitpid(ec->child, &status, WUNTRACED|WNOHANG); } while (pid == -1 && errno == EINTR); - if (pid == child) { + if (pid == ec->child) { if (WIFSTOPPED(status)) { /* * Save the controlling terminal's process group @@ -583,7 +585,7 @@ dispatch_signal(struct sudo_event_base *evbase, pid_t child, */ if (signo == SIGTTOU || signo == SIGTTIN) { if (saved_pgrp == ppgrp) { - pid_t child_pgrp = getpgid(child); + pid_t child_pgrp = getpgid(ec->child); if (child_pgrp != ppgrp) { if (tcsetpgrp(fd, child_pgrp) == 0) { if (killpg(child_pgrp, SIGCONT) != 0) { @@ -626,39 +628,41 @@ dispatch_signal(struct sudo_event_base *evbase, pid_t child, } } else { /* Child has exited or been killed, we are done. */ - cstat->type = CMD_WSTATUS; - cstat->val = status; - sudo_ev_del(evbase, signal_event); - sudo_ev_loopexit(evbase); + ec->child = -1; + ec->cstat->type = CMD_WSTATUS; + ec->cstat->val = status; + sudo_ev_del(ec->evbase, signal_event); + sudo_ev_loopexit(ec->evbase); goto done; } } } else { /* Send signal to child. */ if (signo == SIGALRM) { - terminate_command(child, false); - } else if (kill(child, signo) != 0) { - sudo_warn("kill(%d, SIG%s)", (int)child, signame); + terminate_command(ec->child, false); + } else if (kill(ec->child, signo) != 0) { + sudo_warn("kill(%d, SIG%s)", (int)ec->child, signame); } } - rc = 0; done: - debug_return_int(rc); + debug_return; } /* - * Forward a signal to the monitor (pty version). + * Forward a signal to the monitor (pty version) or handle + * changes to the monitors's status (SIGCHLD). */ -static int -dispatch_signal_pty(struct sudo_event_base *evbase, pid_t child, - int signo, char *signame, struct command_status *cstat) +static void +dispatch_signal_pty(struct exec_closure *ec, int signo, char *signame) { - int rc = 1; debug_decl(dispatch_signal_pty, SUDO_DEBUG_EXEC) sudo_debug_printf(SUDO_DEBUG_INFO, "%s: evbase %p, child: %d, signo %s(%d), cstat %p", - __func__, evbase, (int)child, signame, signo, cstat); + __func__, ec->evbase, (int)ec->child, signame, signo, ec->cstat); + + if (ec->child == -1) + goto done; if (signo == SIGCHLD) { int n, status; @@ -667,9 +671,9 @@ dispatch_signal_pty(struct sudo_event_base *evbase, pid_t child, * Monitor process was signaled; wait for it as needed. */ do { - pid = waitpid(child, &status, WUNTRACED|WNOHANG); + pid = waitpid(ec->child, &status, WUNTRACED|WNOHANG); } while (pid == -1 && errno == EINTR); - if (pid == child) { + if (pid == ec->child) { /* * If the monitor dies we get notified via backchannel_cb(). * If it was stopped, we should stop too (the command keeps @@ -680,28 +684,29 @@ dispatch_signal_pty(struct sudo_event_base *evbase, pid_t child, "monitor stopped, suspending parent"); n = suspend_parent(WSTOPSIG(status)); kill(pid, SIGCONT); - schedule_signal(evbase, n); + schedule_signal(ec->evbase, n); /* Re-enable I/O events and restart event loop. */ - add_io_events(evbase); - sudo_ev_loopcontinue(evbase); + add_io_events(ec->evbase); + sudo_ev_loopcontinue(ec->evbase); goto done; } else if (WIFSIGNALED(status)) { sudo_debug_printf(SUDO_DEBUG_INFO, "monitor killed, signal %d", WTERMSIG(status)); + ec->child = -1; } else { sudo_debug_printf(SUDO_DEBUG_INFO, "monitor exited, status %d", WEXITSTATUS(status)); + ec->child = -1; } } } else { /* Schedule signo to be forwared to the child. */ - schedule_signal(evbase, signo); + schedule_signal(ec->evbase, signo); /* Restart event loop to service signal immediately. */ - sudo_ev_loopcontinue(evbase); + sudo_ev_loopcontinue(ec->evbase); } - rc = 0; done: - debug_return_int(rc); + debug_return; } /* Signal pipe callback */ @@ -712,9 +717,9 @@ signal_pipe_cb(int fd, int what, void *v) char signame[SIG2STR_MAX]; unsigned char signo; ssize_t nread; - int rc = 0; debug_decl(signal_pipe_cb, SUDO_DEBUG_EXEC) + /* Process received signals until the child dies or the pipe is empty. */ do { /* read signal pipe */ nread = read(fd, &signo, sizeof(signo)); @@ -738,13 +743,11 @@ signal_pipe_cb(int fd, int what, void *v) snprintf(signame, sizeof(signame), "%d", signo); sudo_debug_printf(SUDO_DEBUG_DIAG, "received SIG%s", signame); if (ec->log_io) { - rc = dispatch_signal_pty(ec->evbase, ec->child, signo, signame, - ec->cstat); + dispatch_signal_pty(ec, signo, signame); } else { - rc = dispatch_signal(ec->evbase, ec->child, signo, signame, - ec->cstat); + dispatch_signal(ec, signo, signame); } - } while (rc == 0); + } while (ec->child != -1); debug_return; } diff --git a/src/exec_pty.c b/src/exec_pty.c index fc0f864a4..da7a7cf2a 100644 --- a/src/exec_pty.c +++ b/src/exec_pty.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009-2016 Todd C. Miller + * Copyright (c) 2009-2017 Todd C. Miller * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -510,6 +510,10 @@ terminate_command(pid_t pid, bool use_pgrp) { debug_decl(terminate_command, SUDO_DEBUG_EXEC); + /* Avoid killing more than a single process or process group. */ + if (pid <= 0) + debug_return; + /* * Note that SIGCHLD will interrupt the sleep() */ @@ -1069,6 +1073,10 @@ deliver_signal(pid_t pid, int signo, bool from_parent) int status; debug_decl(deliver_signal, SUDO_DEBUG_EXEC); + /* Avoid killing more than a single process or process group. */ + if (pid <= 0) + debug_return; + if (signo == SIGCONT_FG) strlcpy(signame, "CONT_FG", sizeof(signame)); else if (signo == SIGCONT_BG) @@ -1138,13 +1146,11 @@ send_status(int fd, struct command_status *cstat) * Wait for command status after receiving SIGCHLD. * If the command was stopped, the status is send back to the parent. * Otherwise, cstat is filled in but not sent. - * Returns true if command is still alive, else false. */ -static bool +static void handle_sigchld(int backchannel, struct command_status *cstat) { char signame[SIG2STR_MAX]; - bool alive = true; int status; pid_t pid; debug_decl(handle_sigchld, SUDO_DEBUG_EXEC); @@ -1161,7 +1167,7 @@ handle_sigchld(int backchannel, struct command_status *cstat) sudo_debug_printf(SUDO_DEBUG_DIAG, "waitpid returned %d, expected pid %d", pid, cmnd_pid); sudo_warn(U_("%s: %s"), __func__, "waitpid"); - debug_return_bool(false); + debug_return; } if (WIFCONTINUED(status)) { @@ -1177,15 +1183,15 @@ handle_sigchld(int backchannel, struct command_status *cstat) snprintf(signame, sizeof(signame), "%d", WTERMSIG(status)); sudo_debug_printf(SUDO_DEBUG_INFO, "%s: command (%d) killed, SIG%s", __func__, cmnd_pid, signame); - alive = false; + cmnd_pid = -1; } else if (WIFEXITED(status)) { sudo_debug_printf(SUDO_DEBUG_INFO, "%s: command (%d) exited: %d", __func__, cmnd_pid, WEXITSTATUS(status)); - alive = false; + cmnd_pid = -1; } else { sudo_debug_printf(SUDO_DEBUG_WARN, "%s: unexpected wait status %d for command (%d)", - __func__, status, cmnd_pid); + __func__, status, (int)cmnd_pid); } /* Don't overwrite execve() failure with child exit status. */ @@ -1206,7 +1212,7 @@ handle_sigchld(int backchannel, struct command_status *cstat) } } - debug_return_bool(alive); + debug_return; } struct monitor_closure { @@ -1216,7 +1222,6 @@ struct monitor_closure { struct sudo_event *signal_pipe_event; struct command_status *cstat; int backchannel; - bool alive; }; static void @@ -1242,8 +1247,8 @@ mon_signal_pipe_cb(int fd, int what, void *v) * directly to the command. */ if (signo == SIGCHLD) { - mc->alive = handle_sigchld(mc->backchannel, mc->cstat); - if (!mc->alive) { + handle_sigchld(mc->backchannel, mc->cstat); + if (cmnd_pid == -1) { /* Remove all but the errpipe event. */ sudo_ev_del(mc->evbase, mc->backchannel_event); sudo_ev_del(mc->evbase, mc->signal_pipe_event); @@ -1492,7 +1497,6 @@ exec_monitor(struct command_details *details, int backchannel) mc.cstat = &cstat; mc.evbase = evbase; mc.backchannel = backchannel; - mc.alive = true; mc.signal_pipe_event = sudo_ev_alloc(signal_pipe[0], SUDO_EV_READ|SUDO_EV_PERSIST, mon_signal_pipe_cb, &mc); @@ -1521,7 +1525,7 @@ exec_monitor(struct command_details *details, int backchannel) * the error pipe is closed. */ (void) sudo_ev_loop(evbase, 0); - if (mc.alive) { + if (cmnd_pid != -1) { /* XXX An error occurred, should send a message back. */ sudo_debug_printf(SUDO_DEBUG_ERROR, "Command still running after event loop exit, sending SIGKILL"); -- 2.40.0