Check if stdio tty pgrp matches for exec_bg

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,34 +120,31 @@ 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());
     }
 
-    // If there is another process later in the pipeline, don't interfere
-    // with its access to the Tty
-    if io::stdout().is_pipe() {
-        foreground = false;
-    }
-
     // Copy terminal settings from `/dev/tty` to the pty.
     if let Err(err) = user_tty.copy_to(&pty.follower) {
         dev_error!("cannot copy terminal settings to pty: {err}");
         foreground = false;
     }
 
     // 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 +182,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 +231,7 @@ pub(in crate::exec) fn exec_pty(
         tty_size,
         foreground,
         term_raw,
+        preserve_oflag,
         &mut registry,
     )?;
 
@@ -299,6 +299,7 @@ struct ParentClosure {
     tty_size: TermSize,
     foreground: bool,
     term_raw: bool,
+    preserve_oflag: bool,
     backchannel: ParentBackchannel,
     message_queue: VecDeque<MonitorMessage>,
     backchannel_write_handle: EventHandle,
@@ -322,6 +323,7 @@ impl ParentClosure {
         tty_size: TermSize,
         foreground: bool,
         term_raw: bool,
+        preserve_oflag: bool,
         registry: &mut EventRegistry<Self>,
     ) -> io::Result<Self> {
         // Enable nonblocking assertions as we will poll this inside the event loop.
@@ -349,6 +351,7 @@ impl ParentClosure {
             tty_size,
             foreground,
             term_raw,
+            preserve_oflag,
             backchannel,
             message_queue: VecDeque::new(),
             backchannel_write_handle,
@@ -530,7 +533,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 +621,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 {

src/system/term/mod.rs

@@ -165,19 +165,48 @@ mod sealed {
     pub(crate) trait Sealed {}
 
     impl<F: AsFd> 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<T: SafeTty> 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<ProcessId>;
-    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<ProcessId>
+    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<OsString>;
-    fn is_terminal(&self) -> bool;
     fn is_pipe(&self) -> bool;
-    fn tcgetsid(&self) -> io::Result<ProcessId>;
+    fn tcgetsid(&self) -> io::Result<ProcessId>
+    where
+        Self: sealed::SafeTty;
 }
 
 impl<F: AsFd> 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<ProcessId> {
         // SAFETY: tcgetpgrp cannot cause UB
@@ -212,11 +241,6 @@ impl<F: AsFd> 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());

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,14 +252,21 @@ 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;
         }
 
+        // FIXME preserve c_oflag if sudo output is piped
+        // https://github.com/sudo-project/sudo/commit/78b712101e17883b6d916495ef2b9bb0e34d3ca1
+
         // SAFETY: `fd` is a valid file descriptor for the tty; for `term`: same as above.
         unsafe { tcsetattr_nobg(fd, TCSADRAIN, &term) }?;
 
@@ -287,7 +294,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))
     }
 }
 

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");
+}

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

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

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

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