From a2d01a957d31e133c37d77ae149527f9483e4f19 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Sun, 28 Apr 2024 10:28:32 -0600 Subject: [PATCH] Avoid using ioctl(TIOCNOTTY) in the monitor. We don't need to revoke the terminal in the monitor, just signal the foreground process group. This is more portable and has the same effect as ioctl(TIOCNOTTY) would on Linux. Since we now signal the command from the monitor, there is no reason to forward SIGHUP from the kernel. GitHub issue #367. Reference:https://github.com/sudo-project/sudo/commit/a2d01a957d31e133c37d77ae149527f9483e4f19 Conflict:NA --- src/exec_monitor.c | 84 +++++++++++++++++++++++++--------------------- src/exec_pty.c | 31 ++++++++++------- src/sudo.h | 2 +- 3 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/exec_monitor.c b/src/exec_monitor.c index c570b5d86..524f5c8c9 100644 --- a/src/exec_monitor.c +++ b/src/exec_monitor.c @@ -1,7 +1,7 @@ /* * SPDX-License-Identifier: ISC * - * Copyright (c) 2009-2023 Todd C. Miller + * Copyright (c) 2009-2024 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 @@ -310,6 +310,48 @@ mon_errsock_cb(int fd, int what, void *v) debug_return; } +/* + * Called when the user's terminal has gone away but before our pty is + * actually revoked. We simulate the effect of ioctl(TIOCNOTTY) on Linux + * by sending SIGHUP and SIGCONT to the foreground process group. + */ +static void +mon_handle_revoke(int fd, pid_t cmnd_pid, struct command_status *cstat) +{ + debug_decl(mon_handle_revoke, SUDO_DEBUG_EXEC); + + /* + * Signal the foreground process group and the command's process group + * (if different). We must do this before the pty is revoked be the + * main sudo process so we can determine the foreground process group. + * Otherwise, if the foreground process group is different from the + * command's process group it will not be signaled. + */ + if (io_fds[SFD_FOLLOWER] != -1) { + const pid_t pgrp = tcgetpgrp(io_fds[SFD_FOLLOWER]); + if (pgrp != -1 && pgrp != cmnd_pid) { + sudo_debug_printf(SUDO_DEBUG_NOTICE, "%s: killpg(%d, SIGHUP)", + __func__, pgrp); + killpg(pgrp, SIGHUP); + sudo_debug_printf(SUDO_DEBUG_NOTICE, "%s: killpg(%d, SIGCONT)", + __func__, pgrp); + killpg(pgrp, SIGCONT); + } + } + sudo_debug_printf(SUDO_DEBUG_NOTICE, "%s: killpg(%d, SIGHUP)", + __func__, cmnd_pid); + killpg(cmnd_pid, SIGHUP); + sudo_debug_printf(SUDO_DEBUG_NOTICE, "%s: killpg(%d, SIGCONT)", + __func__, cmnd_pid); + killpg(cmnd_pid, SIGCONT); + + /* + * Now that the running command as been signaled, tell the + * parent it is OK to close the pty leader, revoking the pty. + */ + send_status(fd, cstat); +} + static void mon_backchannel_cb(int fd, int what, void *v) { @@ -337,44 +379,8 @@ mon_backchannel_cb(int fd, int what, void *v) sudo_ev_loopbreak(mc->evbase); } else { switch (cstmp.type) { - case CMD_IOCTL: - if (cstmp.val != TIOCNOTTY) { - sudo_warnx(U_("unexpected ioctl on backchannel: %d"), - cstmp.val); - } else if (io_fds[SFD_FOLLOWER] != -1) { - int result, ttyfd; - - /* - * Parent asks us to revoke the terminal when the - * user's terminal goes away. Doing this in the - * monitor allows the foreground command to receive - * SIGHUP before the terminal is revoked. - */ - result = ioctl(io_fds[SFD_FOLLOWER], TIOCNOTTY, NULL); - if (result == -1) { - sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, - "%s: unable to revoke follower pty", __func__); - ttyfd = open(_PATH_TTY, O_RDWR); - if (ttyfd != -1) { - result = ioctl(ttyfd, TIOCNOTTY, NULL); - if (result == -1) { - sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, - "%s: unable to revoke controlling tty", - __func__); - } - close(ttyfd); - } else { - sudo_debug_printf(SUDO_DEBUG_ERROR|SUDO_DEBUG_ERRNO, - "%s: unable to open %s", __func__, _PATH_TTY); - } - } - if (result == 0) { - sudo_debug_printf(SUDO_DEBUG_INFO, - "%s: revoked controlling tty for session", __func__); - } - /* Now tell the parent to close the pty leader. */ - send_status(fd, &cstmp); - } + case CMD_REVOKE: + mon_handle_revoke(fd, mc->cmnd_pid, &cstmp); break; case CMD_SIGNO: deliver_signal(mc, cstmp.val, true); diff --git a/src/exec_pty.c b/src/exec_pty.c index fff9b8f1e..4dd5915ed 100644 --- a/src/exec_pty.c +++ b/src/exec_pty.c @@ -1,7 +1,7 @@ /* * SPDX-License-Identifier: ISC * - * Copyright (c) 2009-2023 Todd C. Miller + * Copyright (c) 2009-2024 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 @@ -385,13 +385,13 @@ read_callback(int fd, int what, void *v) /* If writer already consumed the buffer, close it too. */ if (iob->wevent != NULL && iob->off == iob->len) { /* - * Don't close the pty leader, it will invalidate the pty. - * We ask the monitor to revoke the pty nicely using TIOCNOTTY. + * Don't close the pty leader yet, it will invalidate the pty. + * We ask the monitor to signal the running process first. */ const int wfd = sudo_ev_get_fd(iob->wevent); if (wfd == io_fds[SFD_LEADER]) { sudo_debug_printf(SUDO_DEBUG_NOTICE, "user's tty revoked"); - send_command_status(iob->ec, CMD_IOCTL, TIOCNOTTY); + send_command_status(iob->ec, CMD_REVOKE, 0); } else { safe_close(wfd); } @@ -474,12 +474,12 @@ write_callback(int fd, int what, void *v) if (iob->revent != NULL) { /* * Don't close the pty leader, it will invalidate the pty. - * We ask the monitor to revoke the pty nicely using TIOCNOTTY. + * We ask the monitor to signal the running process first. */ const int rfd = sudo_ev_get_fd(iob->revent); if (rfd == io_fds[SFD_LEADER]) { sudo_debug_printf(SUDO_DEBUG_NOTICE, "user's tty revoked"); - send_command_status(iob->ec, CMD_IOCTL, TIOCNOTTY); + send_command_status(iob->ec, CMD_REVOKE, 0); } else { safe_close(rfd); } @@ -684,15 +684,11 @@ backchannel_cb(int fd, int what, void *v) sudo_ev_loopbreak(ec->evbase); *ec->cstat = cstat; break; - case CMD_IOCTL: - if (cstat.val != TIOCNOTTY) { - sudo_warnx(U_("unexpected ioctl on backchannel: %d"), - cstat.val); - } else if (io_fds[SFD_LEADER] != -1) { + case CMD_REVOKE: + if (io_fds[SFD_LEADER] != -1) { /* * Monitor requests that we revoke the user's terminal. - * This must happen after the monitor has used TIOCNOTTY - * to invalidate the session and gracefully kill the + * This must happen after the monitor has signaled the * controlling terminal's process group. */ close(io_fds[SFD_LEADER]); @@ -855,6 +851,15 @@ signal_cb_pty(int signo, int what, void *v) case SIGWINCH: sync_ttysize(ec); break; + case SIGHUP: + /* + * Avoid forwarding SIGHUP sent by the kernel, it probably means + * that the user's terminal was revoked. When we detect that the + * terminal has been revoked, the monitor will send SIGHUP itself. + */ + if (!USER_SIGNALED(sc->siginfo)) + break; + FALLTHROUGH; default: /* * Do not forward signals sent by the command itself or a member of the diff --git a/src/sudo.h b/src/sudo.h index ca245ca68..d3122ef4e 100644 --- a/src/sudo.h +++ b/src/sudo.h @@ -225,7 +225,7 @@ struct command_status { #define CMD_WSTATUS 2 #define CMD_SIGNO 3 #define CMD_PID 4 -#define CMD_IOCTL 5 +#define CMD_REVOKE 5 int type; int val; }; -- 2.33.0