diff --git a/src/exec/use_pty/parent.rs b/src/exec/use_pty/parent.rs index 4e515b16d..09d7d3f6a 100644 --- a/src/exec/use_pty/parent.rs +++ b/src/exec/use_pty/parent.rs @@ -94,20 +94,22 @@ pub(in crate::exec) fn exec_pty( // FIXME: maybe all these boolean flags should be on a dedicated type. - // Whether we're running on a pipeline - let mut pipeline = false; // Whether the command should be executed in the background (this is not the `-b` flag) let mut exec_bg = false; // Whether the user's terminal is in raw mode or not. let mut term_raw = false; + // Whether to preserve oflag for the terminal + let mut preserve_oflag = false; // Check if we are part of a pipeline. // FIXME: Here's where we should intercept the IO streams if we want to implement IO logging. // FIXME: ogsudo creates pipes for the IO streams and uses events to read from the strams to // the pipes. Investigate why. - if !io::stdin().is_terminal() { + if !io::stdin().is_terminal_for_pgrp(parent_pgrp) { dev_info!("stdin is not a terminal, command will inherit it"); - pipeline = true; + if io::stdin().is_pipe() { + exec_bg = true; + } command.stdin(Stdio::inherit()); if foreground && parent_pgrp != sudo_pid { @@ -118,13 +120,16 @@ pub(in crate::exec) fn exec_pty( } } - if !io::stdout().is_terminal() { + if !io::stdout().is_terminal_for_pgrp(parent_pgrp) { dev_info!("stdout is not a terminal, command will inherit it"); - pipeline = true; + if io::stdout().is_pipe() { + exec_bg = true; + preserve_oflag = true; + } command.stdout(Stdio::inherit()); } - if !io::stderr().is_terminal() { + if !io::stderr().is_terminal_for_pgrp(parent_pgrp) { dev_info!("stderr is not a terminal, command will inherit it"); command.stderr(Stdio::inherit()); } @@ -142,10 +147,10 @@ pub(in crate::exec) fn exec_pty( } // Start in raw mode unless we're part of a pipeline or backgrounded. - if foreground && !pipeline && !exec_bg { + if foreground && !exec_bg { // Clearer this way that set_raw_mode only conditionally runs #[allow(clippy::collapsible_if)] - if user_tty.set_raw_mode(false).is_ok() { + if user_tty.set_raw_mode(false, preserve_oflag).is_ok() { term_raw = true; } } @@ -183,7 +188,7 @@ pub(in crate::exec) fn exec_pty( match exec_monitor( pty.follower, command, - foreground && !pipeline && !exec_bg, + foreground && !exec_bg, &mut backchannels.monitor, original_set, ) { @@ -232,6 +237,7 @@ pub(in crate::exec) fn exec_pty( tty_size, foreground, term_raw, + preserve_oflag, &mut registry, )?; @@ -299,6 +305,7 @@ struct ParentClosure { tty_size: TermSize, foreground: bool, term_raw: bool, + preserve_oflag: bool, backchannel: ParentBackchannel, message_queue: VecDeque, backchannel_write_handle: EventHandle, @@ -322,6 +329,7 @@ impl ParentClosure { tty_size: TermSize, foreground: bool, term_raw: bool, + preserve_oflag: bool, registry: &mut EventRegistry, ) -> io::Result { // Enable nonblocking assertions as we will poll this inside the event loop. @@ -349,6 +357,7 @@ impl ParentClosure { tty_size, foreground, term_raw, + preserve_oflag, backchannel, message_queue: VecDeque::new(), backchannel_write_handle, @@ -530,7 +539,12 @@ impl ParentClosure { signal_fmt(signal) ); if !self.term_raw { - if self.tty_pipe.left_mut().set_raw_mode(false).is_ok() { + if self + .tty_pipe + .left_mut() + .set_raw_mode(false, self.preserve_oflag) + .is_ok() + { self.term_raw = true; } // Resume command in the foreground @@ -613,7 +627,12 @@ impl ParentClosure { if self.foreground { // We're in the foreground, set tty to raw mode. - if self.tty_pipe.left_mut().set_raw_mode(false).is_ok() { + if self + .tty_pipe + .left_mut() + .set_raw_mode(false, self.preserve_oflag) + .is_ok() + { self.term_raw = true; } } else { diff --git a/src/system/term/mod.rs b/src/system/term/mod.rs index 81d2f789e..67e1836b2 100644 --- a/src/system/term/mod.rs +++ b/src/system/term/mod.rs @@ -165,19 +165,48 @@ mod sealed { pub(crate) trait Sealed {} impl Sealed for F {} + + /// This is known to be a real TTY. No need to use `safe_isatty` before calling any ioctl. + pub(crate) trait SafeTty {} + + impl SafeTty for &mut T {} + impl SafeTty for super::UserTerm {} + impl SafeTty for super::PtyLeader {} + impl SafeTty for super::PtyFollower {} } pub(crate) trait Terminal: sealed::Sealed { - fn tcgetpgrp(&self) -> io::Result; - fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()>; - fn make_controlling_terminal(&self) -> io::Result<()>; + fn is_terminal_for_pgrp(&self, pgrp: ProcessId) -> bool; + fn tcgetpgrp(&self) -> io::Result + where + Self: sealed::SafeTty; + fn tcsetpgrp(&self, pgrp: ProcessId) -> io::Result<()> + where + Self: sealed::SafeTty; + fn make_controlling_terminal(&self) -> io::Result<()> + where + Self: sealed::SafeTty; fn ttyname(&self) -> io::Result; - fn is_terminal(&self) -> bool; fn is_pipe(&self) -> bool; - fn tcgetsid(&self) -> io::Result; + fn tcgetsid(&self) -> io::Result + where + Self: sealed::SafeTty; } impl Terminal for F { + /// Check if the foreground process group ID associated with this terminal is `pgrp`. + /// Returns false if this is not actually a terminal. + fn is_terminal_for_pgrp(&self, pgrp: ProcessId) -> bool { + if !safe_isatty(self.as_fd()) { + return false; + } + + // SAFETY: tcgetpgrp cannot cause UB + let Ok(id) = cerr(unsafe { libc::tcgetpgrp(self.as_fd().as_raw_fd()) }) else { + return false; + }; + ProcessId::new(id) == pgrp + } /// Get the foreground process group ID associated with this terminal. fn tcgetpgrp(&self) -> io::Result { // SAFETY: tcgetpgrp cannot cause UB @@ -212,11 +241,6 @@ impl Terminal for F { Ok(unsafe { os_string_from_ptr(buf.as_ptr()) }) } - /// Rust standard library "IsTerminal" is not secure for setuid programs (CVE-2023-2002) - fn is_terminal(&self) -> bool { - safe_isatty(self.as_fd()) - } - fn is_pipe(&self) -> bool { is_fifo(self.as_fd()) } @@ -266,8 +290,8 @@ mod tests { #[test] fn open_pty() { let pty = Pty::open().unwrap(); - assert!(pty.leader.file.is_terminal()); - assert!(pty.follower.file.is_terminal()); + assert!(safe_isatty(pty.leader.file.as_fd())); + assert!(safe_isatty(pty.follower.file.as_fd())); let path = PathBuf::from(OsString::from_vec(pty.path.into_bytes())); assert!(path.try_exists().unwrap()); diff --git a/src/system/term/user_term.rs b/src/system/term/user_term.rs index dc768f085..19bfe7c10 100644 --- a/src/system/term/user_term.rs +++ b/src/system/term/user_term.rs @@ -235,7 +235,7 @@ impl UserTerm { /// Set the user's terminal to raw mode. Enable terminal signals if `with_signals` is set to /// `true`. - pub fn set_raw_mode(&mut self, with_signals: bool) -> io::Result<()> { + pub fn set_raw_mode(&mut self, with_signals: bool, preserve_oflag: bool) -> io::Result<()> { let fd = self.tty.as_raw_fd(); // Retrieve the original terminal (if we haven't done so already) @@ -252,9 +252,13 @@ impl UserTerm { }; // Set terminal to raw mode. + let oflag = term.c_oflag; // SAFETY: `term` is a valid, initialized struct of type `termios`, which // was previously obtained through `tcgetattr`. unsafe { cfmakeraw(&mut term) }; + if preserve_oflag { + term.c_oflag = oflag; + } // Enable terminal signals. if with_signals { term.c_cflag |= ISIG; @@ -287,7 +291,7 @@ impl UserTerm { // This function is based around the fact that we receive `SIGTTOU` if we call `tcsetpgrp` and // we are not in the foreground process group. - catching_sigttou(|| self.tty.tcsetpgrp(pgrp)) + catching_sigttou(|| self.tcsetpgrp(pgrp)) } } diff --git a/test-framework/sudo-compliance-tests/src/sudo/use_pty.rs b/test-framework/sudo-compliance-tests/src/sudo/use_pty.rs index 8cad4c223..44f80d8ab 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/use_pty.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/use_pty.rs @@ -172,3 +172,42 @@ fn stderr_pipe() { assert_eq!(stdout, "hello world"); } + +#[test] +fn stdout_foreign_pty() { + let env = Env([SUDOERS_ALL_ALL_NOPASSWD, "Defaults use_pty"]).build(); + + // Everything is put in a single command with separators to keep the pts numbers predictable + let output = Command::new("sh") + .args([ + "-c", + "socat - SYSTEM:'ls -l /proc/self/fd; echo @@@@; sudo ls -l /proc/self/fd',pty,setsid,ctty; + echo ====; + socat - SYSTEM:'ls -l /proc/self/fd; echo @@@@; sudo ls -l /proc/self/fd',pty", + ]) + .tty(true) + .output(&env); + + let stdout = output.stdout(); + let (own_term, foreign_term) = stdout.split_once("====").unwrap(); + + let (own_term_in, own_term_sudo) = own_term.split_once("@@@@").unwrap(); + assert_contains!(own_term_in, " 0 -> /dev/pts/1"); + assert_contains!(own_term_in, " 1 -> /dev/pts/1"); + assert_contains!(own_term_in, " 2 -> /dev/pts/0"); + // pts/1 is our controlling tty, so it gets proxied. + // pts/0 is a foreign pty, so it gets inherited + assert_contains!(own_term_sudo, " 0 -> /dev/pts/2"); + assert_contains!(own_term_sudo, " 1 -> /dev/pts/2"); + assert_contains!(own_term_sudo, " 2 -> /dev/pts/0"); + + let (foreign_term_in, foreign_term_sudo) = foreign_term.split_once("@@@@").unwrap(); + assert_contains!(foreign_term_in, " 0 -> /dev/pts/1"); + assert_contains!(foreign_term_in, " 1 -> /dev/pts/1"); + assert_contains!(foreign_term_in, " 2 -> /dev/pts/0"); + // pts/1 is not our controlling tty, so it gets inherited. + // pts/0 is our controlling tty, so it gets proxied + assert_contains!(foreign_term_sudo, " 0 -> /dev/pts/1"); + assert_contains!(foreign_term_sudo, " 1 -> /dev/pts/1"); + assert_contains!(foreign_term_sudo, " 2 -> /dev/pts/2"); +} diff --git a/test-framework/sudo-test/src/ours.freebsd.Dockerfile b/test-framework/sudo-test/src/ours.freebsd.Dockerfile index b4c0a6d17..c0da1b296 100644 --- a/test-framework/sudo-test/src/ours.freebsd.Dockerfile +++ b/test-framework/sudo-test/src/ours.freebsd.Dockerfile @@ -1,5 +1,5 @@ FROM dougrabson/freebsd14.1-small:latest -RUN IGNORE_OSVERSION=yes pkg install -y sshpass rsyslog bash vim pidof dash +RUN IGNORE_OSVERSION=yes pkg install -y sshpass rsyslog bash vim pidof dash socat WORKDIR /usr/src/sudo COPY target/build build # set setuid on install diff --git a/test-framework/sudo-test/src/ours.linux.Dockerfile b/test-framework/sudo-test/src/ours.linux.Dockerfile index 23b9529c9..0cc0aeb99 100644 --- a/test-framework/sudo-test/src/ours.linux.Dockerfile +++ b/test-framework/sudo-test/src/ours.linux.Dockerfile @@ -1,6 +1,6 @@ FROM rust:1-slim-trixie RUN apt-get update && \ - apt-get install -y --no-install-recommends apparmor libpam0g-dev libapparmor1 procps sshpass rsyslog ca-certificates tzdata + apt-get install -y --no-install-recommends apparmor libpam0g-dev libapparmor1 procps sshpass rsyslog ca-certificates tzdata socat # cache the crates.io index in the image for faster local testing RUN cargo search sudo WORKDIR /usr/src/sudo diff --git a/test-framework/sudo-test/src/theirs.freebsd.Dockerfile b/test-framework/sudo-test/src/theirs.freebsd.Dockerfile index cda989b9b..5015d27ed 100644 --- a/test-framework/sudo-test/src/theirs.freebsd.Dockerfile +++ b/test-framework/sudo-test/src/theirs.freebsd.Dockerfile @@ -1,5 +1,5 @@ FROM dougrabson/freebsd14.1-small:latest -RUN IGNORE_OSVERSION=yes pkg install -y sudo pidof sshpass rsyslog bash vim dash FreeBSD-libbsm && \ +RUN IGNORE_OSVERSION=yes pkg install -y sudo pidof sshpass rsyslog bash vim dash FreeBSD-libbsm socat && \ rm /usr/local/etc/sudoers # Ensure we use the same shell across OSes RUN chsh -s /bin/sh diff --git a/test-framework/sudo-test/src/theirs.linux.Dockerfile b/test-framework/sudo-test/src/theirs.linux.Dockerfile index fc49437e1..12675f26d 100644 --- a/test-framework/sudo-test/src/theirs.linux.Dockerfile +++ b/test-framework/sudo-test/src/theirs.linux.Dockerfile @@ -1,6 +1,6 @@ FROM debian:trixie-slim RUN apt-get update && \ - apt-get install -y --no-install-recommends sudo procps sshpass rsyslog && \ + apt-get install -y --no-install-recommends sudo procps sshpass rsyslog socat && \ rm /etc/sudoers # Ensure we use the same shell across OSes RUN chsh -s /bin/sh