isa/sudo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Set the child pid to -1 after we've waited for it and take care to

Browse Source 
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.
This commit is contained in:
Todd C. Miller
2017-02-24 15:14:56 -07:00
parent 05e52c7b82
commit 13fcdb4f5f
2 changed files with 65 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

91
src/exec.c
View File

@@ -1,5 +1,5 @@
/* /*
* Copyright (c) 2009-2016 Todd C. Miller <Todd.Miller@courtesan.com> * Copyright (c) 2009-2017 Todd C. Miller <Todd.Miller@courtesan.com>
* *
* Permission to use, copy, modify, and distribute this software for any * Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above * 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: case CMD_PID:
/* /*
* Once we know the command's pid we can unblock * 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 * avoids a race between exec of the command and
* receipt of a fatal signal from it. * 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)) { if (sudo_ev_got_break(evbase)) {
/* error from callback */ /* error from callback */
sudo_debug_printf(SUDO_DEBUG_ERROR, "event loop exited prematurely"); sudo_debug_printf(SUDO_DEBUG_ERROR, "event loop exited prematurely");
/* kill command if still running and not I/O logging */ /* kill command if not I/O logging */
if (!log_io && kill(child, 0) == 0) if (!log_io)
terminate_command(child, true); terminate_command(ec.child, true);
} }
if (log_io) { 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 static void
dispatch_signal(struct sudo_event_base *evbase, pid_t child, dispatch_signal(struct exec_closure *ec, int signo, char *signame)
int signo, char *signame, struct command_status *cstat)
{ {
int rc = 1;
debug_decl(dispatch_signal, SUDO_DEBUG_EXEC) debug_decl(dispatch_signal, SUDO_DEBUG_EXEC)
sudo_debug_printf(SUDO_DEBUG_INFO, sudo_debug_printf(SUDO_DEBUG_INFO,
"%s: evbase %p, child: %d, signo %s(%d), cstat %p", "%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) { if (signo == SIGCHLD) {
pid_t pid; pid_t pid;
@@ -550,9 +552,9 @@ dispatch_signal(struct sudo_event_base *evbase, pid_t child,
* The command stopped or exited. * The command stopped or exited.
*/ */
do { do {
pid = waitpid(child, &status, WUNTRACED|WNOHANG); pid = waitpid(ec->child, &status, WUNTRACED|WNOHANG);
} while (pid == -1 && errno == EINTR); } while (pid == -1 && errno == EINTR);
if (pid == child) { if (pid == ec->child) {
if (WIFSTOPPED(status)) { if (WIFSTOPPED(status)) {
/* /*
* Save the controlling terminal's process group * 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 (signo == SIGTTOU || signo == SIGTTIN) {
if (saved_pgrp == ppgrp) { if (saved_pgrp == ppgrp) {
pid_t child_pgrp = getpgid(child); pid_t child_pgrp = getpgid(ec->child);
if (child_pgrp != ppgrp) { if (child_pgrp != ppgrp) {
if (tcsetpgrp(fd, child_pgrp) == 0) { if (tcsetpgrp(fd, child_pgrp) == 0) {
if (killpg(child_pgrp, SIGCONT) != 0) { if (killpg(child_pgrp, SIGCONT) != 0) {
@@ -626,39 +628,41 @@ dispatch_signal(struct sudo_event_base *evbase, pid_t child,
} }
} else { } else {
/* Child has exited or been killed, we are done. */ /* Child has exited or been killed, we are done. */
cstat->type = CMD_WSTATUS; ec->child = -1;
cstat->val = status; ec->cstat->type = CMD_WSTATUS;
sudo_ev_del(evbase, signal_event); ec->cstat->val = status;
sudo_ev_loopexit(evbase); sudo_ev_del(ec->evbase, signal_event);
sudo_ev_loopexit(ec->evbase);
goto done; goto done;
} }
} }
} else { } else {
/* Send signal to child. */ /* Send signal to child. */
if (signo == SIGALRM) { if (signo == SIGALRM) {
terminate_command(child, false); terminate_command(ec->child, false);
} else if (kill(child, signo) != 0) { } else if (kill(ec->child, signo) != 0) {
sudo_warn("kill(%d, SIG%s)", (int)child, signame); sudo_warn("kill(%d, SIG%s)", (int)ec->child, signame);
} }
} }
rc = 0;
done: 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 static void
dispatch_signal_pty(struct sudo_event_base *evbase, pid_t child, dispatch_signal_pty(struct exec_closure *ec, int signo, char *signame)
int signo, char *signame, struct command_status *cstat)
{ {
int rc = 1;
debug_decl(dispatch_signal_pty, SUDO_DEBUG_EXEC) debug_decl(dispatch_signal_pty, SUDO_DEBUG_EXEC)
sudo_debug_printf(SUDO_DEBUG_INFO, sudo_debug_printf(SUDO_DEBUG_INFO,
"%s: evbase %p, child: %d, signo %s(%d), cstat %p", "%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) { if (signo == SIGCHLD) {
int n, status; 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. * Monitor process was signaled; wait for it as needed.
*/ */
do { do {
pid = waitpid(child, &status, WUNTRACED|WNOHANG); pid = waitpid(ec->child, &status, WUNTRACED|WNOHANG);
} while (pid == -1 && errno == EINTR); } while (pid == -1 && errno == EINTR);
if (pid == child) { if (pid == ec->child) {
/* /*
* If the monitor dies we get notified via backchannel_cb(). * If the monitor dies we get notified via backchannel_cb().
* If it was stopped, we should stop too (the command keeps * 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"); "monitor stopped, suspending parent");
n = suspend_parent(WSTOPSIG(status)); n = suspend_parent(WSTOPSIG(status));
kill(pid, SIGCONT); kill(pid, SIGCONT);
schedule_signal(evbase, n); schedule_signal(ec->evbase, n);
/* Re-enable I/O events and restart event loop. */ /* Re-enable I/O events and restart event loop. */
add_io_events(evbase); add_io_events(ec->evbase);
sudo_ev_loopcontinue(evbase); sudo_ev_loopcontinue(ec->evbase);
goto done; goto done;
} else if (WIFSIGNALED(status)) { } else if (WIFSIGNALED(status)) {
sudo_debug_printf(SUDO_DEBUG_INFO, sudo_debug_printf(SUDO_DEBUG_INFO,
"monitor killed, signal %d", WTERMSIG(status)); "monitor killed, signal %d", WTERMSIG(status));
ec->child = -1;
} else { } else {
sudo_debug_printf(SUDO_DEBUG_INFO, sudo_debug_printf(SUDO_DEBUG_INFO,
"monitor exited, status %d", WEXITSTATUS(status)); "monitor exited, status %d", WEXITSTATUS(status));
ec->child = -1;
} }
} }
} else { } else {
/* Schedule signo to be forwared to the child. */ /* Schedule signo to be forwared to the child. */
schedule_signal(evbase, signo); schedule_signal(ec->evbase, signo);
/* Restart event loop to service signal immediately. */ /* Restart event loop to service signal immediately. */
sudo_ev_loopcontinue(evbase); sudo_ev_loopcontinue(ec->evbase);
} }
rc = 0;
done: done:
debug_return_int(rc); debug_return;
} }
/* Signal pipe callback */ /* Signal pipe callback */
@@ -712,9 +717,9 @@ signal_pipe_cb(int fd, int what, void *v)
char signame[SIG2STR_MAX]; char signame[SIG2STR_MAX];
unsigned char signo; unsigned char signo;
ssize_t nread; ssize_t nread;
int rc = 0;
debug_decl(signal_pipe_cb, SUDO_DEBUG_EXEC) debug_decl(signal_pipe_cb, SUDO_DEBUG_EXEC)
/* Process received signals until the child dies or the pipe is empty. */
do { do {
/* read signal pipe */ /* read signal pipe */
nread = read(fd, &signo, sizeof(signo)); 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); snprintf(signame, sizeof(signame), "%d", signo);
sudo_debug_printf(SUDO_DEBUG_DIAG, "received SIG%s", signame); sudo_debug_printf(SUDO_DEBUG_DIAG, "received SIG%s", signame);
if (ec->log_io) { if (ec->log_io) {
rc = dispatch_signal_pty(ec->evbase, ec->child, signo, signame, dispatch_signal_pty(ec, signo, signame);
ec->cstat);
} else { } else {
rc = dispatch_signal(ec->evbase, ec->child, signo, signame, dispatch_signal(ec, signo, signame);
ec->cstat);
} }
} while (rc == 0); } while (ec->child != -1);
debug_return; debug_return;
} }

32
src/exec_pty.c
View File

@@ -1,5 +1,5 @@
/* /*
* Copyright (c) 2009-2016 Todd C. Miller <Todd.Miller@courtesan.com> * Copyright (c) 2009-2017 Todd C. Miller <Todd.Miller@courtesan.com>
* *
* Permission to use, copy, modify, and distribute this software for any * Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above * 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); 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() * Note that SIGCHLD will interrupt the sleep()
*/ */
@@ -1069,6 +1073,10 @@ deliver_signal(pid_t pid, int signo, bool from_parent)
int status; int status;
debug_decl(deliver_signal, SUDO_DEBUG_EXEC); 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) if (signo == SIGCONT_FG)
strlcpy(signame, "CONT_FG", sizeof(signame)); strlcpy(signame, "CONT_FG", sizeof(signame));
else if (signo == SIGCONT_BG) else if (signo == SIGCONT_BG)
@@ -1138,13 +1146,11 @@ send_status(int fd, struct command_status *cstat)
* Wait for command status after receiving SIGCHLD. * Wait for command status after receiving SIGCHLD.
* If the command was stopped, the status is send back to the parent. * If the command was stopped, the status is send back to the parent.
* Otherwise, cstat is filled in but not sent. * 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) handle_sigchld(int backchannel, struct command_status *cstat)
{ {
char signame[SIG2STR_MAX]; char signame[SIG2STR_MAX];
bool alive = true;
int status; int status;
pid_t pid; pid_t pid;
debug_decl(handle_sigchld, SUDO_DEBUG_EXEC); 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, sudo_debug_printf(SUDO_DEBUG_DIAG,
"waitpid returned %d, expected pid %d", pid, cmnd_pid); "waitpid returned %d, expected pid %d", pid, cmnd_pid);
sudo_warn(U_("%s: %s"), __func__, "waitpid"); sudo_warn(U_("%s: %s"), __func__, "waitpid");
debug_return_bool(false); debug_return;
} }
if (WIFCONTINUED(status)) { if (WIFCONTINUED(status)) {
@@ -1177,15 +1183,15 @@ handle_sigchld(int backchannel, struct command_status *cstat)
snprintf(signame, sizeof(signame), "%d", WTERMSIG(status)); snprintf(signame, sizeof(signame), "%d", WTERMSIG(status));
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: command (%d) killed, SIG%s", sudo_debug_printf(SUDO_DEBUG_INFO, "%s: command (%d) killed, SIG%s",
__func__, cmnd_pid, signame); __func__, cmnd_pid, signame);
alive = false; cmnd_pid = -1;
} else if (WIFEXITED(status)) { } else if (WIFEXITED(status)) {
sudo_debug_printf(SUDO_DEBUG_INFO, "%s: command (%d) exited: %d", sudo_debug_printf(SUDO_DEBUG_INFO, "%s: command (%d) exited: %d",
__func__, cmnd_pid, WEXITSTATUS(status)); __func__, cmnd_pid, WEXITSTATUS(status));
alive = false; cmnd_pid = -1;
} else { } else {
sudo_debug_printf(SUDO_DEBUG_WARN, sudo_debug_printf(SUDO_DEBUG_WARN,
"%s: unexpected wait status %d for command (%d)", "%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. */ /* 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 { struct monitor_closure {
@@ -1216,7 +1222,6 @@ struct monitor_closure {
struct sudo_event *signal_pipe_event; struct sudo_event *signal_pipe_event;
struct command_status *cstat; struct command_status *cstat;
int backchannel; int backchannel;
bool alive;
}; };
static void static void
@@ -1242,8 +1247,8 @@ mon_signal_pipe_cb(int fd, int what, void *v)
* directly to the command. * directly to the command.
*/ */
if (signo == SIGCHLD) { if (signo == SIGCHLD) {
mc->alive = handle_sigchld(mc->backchannel, mc->cstat); handle_sigchld(mc->backchannel, mc->cstat);
if (!mc->alive) { if (cmnd_pid == -1) {
/* Remove all but the errpipe event. */ /* Remove all but the errpipe event. */
sudo_ev_del(mc->evbase, mc->backchannel_event); sudo_ev_del(mc->evbase, mc->backchannel_event);
sudo_ev_del(mc->evbase, mc->signal_pipe_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.cstat = &cstat;
mc.evbase = evbase; mc.evbase = evbase;
mc.backchannel = backchannel; mc.backchannel = backchannel;
mc.alive = true;
mc.signal_pipe_event = sudo_ev_alloc(signal_pipe[0], mc.signal_pipe_event = sudo_ev_alloc(signal_pipe[0],
SUDO_EV_READ|SUDO_EV_PERSIST, mon_signal_pipe_cb, &mc); 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. * the error pipe is closed.
*/ */
(void) sudo_ev_loop(evbase, 0); (void) sudo_ev_loop(evbase, 0);
if (mc.alive) { if (cmnd_pid != -1) {
/* XXX An error occurred, should send a message back. */ /* XXX An error occurred, should send a message back. */
sudo_debug_printf(SUDO_DEBUG_ERROR, sudo_debug_printf(SUDO_DEBUG_ERROR,
"Command still running after event loop exit, sending SIGKILL"); "Command still running after event loop exit, sending SIGKILL");