Feature pam tty

lib/sudo-common/src/context.rs

@@ -18,6 +18,7 @@ pub struct Context {
     pub command: CommandAndArguments,
     pub target_user: User,
     pub target_group: Group,
+    pub stdin: bool,
     // system
     pub hostname: String,
     pub path: String,
@@ -52,6 +53,7 @@ impl Context {
             preserve_env_list: sudo_options.preserve_env_list,
             launch,
             chdir: sudo_options.directory,
+            stdin: sudo_options.stdin,
             process: sudo_system::Process::new(),
         })
     }

lib/sudo-env/tests/env_tests.rs

@@ -134,6 +134,7 @@ fn create_test_context<'a>(sudo_options: &'a SudoOptions) -> Context {
         path,
         launch: sudo_common::context::LaunchType::Direct,
         chdir: sudo_options.directory.clone(),
+        stdin: sudo_options.stdin,
         process: Process::new(),
     }
 }

lib/sudo-pam/src/converse.rs

@@ -1,5 +1,3 @@
-use std::io::{BufRead, Write};
-
 use sudo_pam_sys::*;
 
 use crate::{error::PamResult, rpassword, securemem::PamBuffer, PamErrorType};
@@ -88,7 +86,7 @@ pub trait Converser {
 pub trait SequentialConverser: Converser {
     /// Handle a normal prompt, i.e. present some message and ask for a value.
     /// The value is not considered a secret.
-    fn handle_normal_prompt(&self, msg: &str) -> PamResult<Vec<u8>>;
+    fn handle_normal_prompt(&self, msg: &str) -> PamResult<PamBuffer>;
 
     /// Handle a hidden prompt, i.e. present some message and ask for a value.
     /// The value is considered secret and should not be visible.
@@ -113,7 +111,7 @@ where
         for msg in conversation.messages_mut() {
             match msg.style {
                 PromptEchoOn => {
-                    msg.set_response(PamBuffer::new(self.handle_normal_prompt(&msg.msg)?));
+                    msg.set_response(self.handle_normal_prompt(&msg.msg)?);
                 }
                 PromptEchoOff => {
                     msg.set_response(self.handle_hidden_prompt(&msg.msg)?);
@@ -133,38 +131,43 @@ where
 
 /// A converser that uses stdin/stdout/stderr to display messages and to request
 /// input from the user.
-pub struct CLIConverser;
-
-// TODO: all of these functions should communicate via the TTY; refactor
-// some code from the rpassword.rs module into here
-impl SequentialConverser for CLIConverser {
-    fn handle_normal_prompt(&self, msg: &str) -> PamResult<Vec<u8>> {
-        print!("[Sudo: input needed] {msg} ");
-        std::io::stdout().flush().unwrap();
+pub struct CLIConverser {
+    pub(crate) use_stdin: bool,
+}
 
-        let mut s = String::new();
+use rpassword::Terminal;
 
-        std::io::stdin().lock().read_line(&mut s)?;
-        // temporary fix: get rid of the \n that read_line adds
-        s.pop();
+impl CLIConverser {
+    fn open(&self) -> std::io::Result<Terminal> {
+        if self.use_stdin {
+            Terminal::open_stdie()
+        } else {
+            Terminal::open_tty()
+        }
+    }
+}
 
-        Ok(s.into_bytes())
+impl SequentialConverser for CLIConverser {
+    fn handle_normal_prompt(&self, msg: &str) -> PamResult<PamBuffer> {
+        let mut tty = self.open()?;
+        tty.prompt(&format!("[Sudo: input needed] {msg} "))?;
+        Ok(tty.read_cleartext()?)
     }
 
     fn handle_hidden_prompt(&self, msg: &str) -> PamResult<PamBuffer> {
-        Ok(rpassword::prompt_password(format!(
-            "[Sudo: authenticate] {msg}"
-        ))?)
+        let mut tty = self.open()?;
+        tty.prompt(&format!("[Sudo: authenticate] {msg}"))?;
+        Ok(tty.read_password()?)
     }
 
     fn handle_error(&self, msg: &str) -> PamResult<()> {
-        eprintln!("[Sudo error] {msg}");
-        Ok(())
+        let mut tty = self.open()?;
+        Ok(tty.prompt(&format!("[Sudo error] {msg}\n"))?)
     }
 
     fn handle_info(&self, msg: &str) -> PamResult<()> {
-        println!("[Sudo] {msg}");
-        Ok(())
+        let mut tty = self.open()?;
+        Ok(tty.prompt(&format!("[Sudo] {msg}\n"))?)
     }
 }
 
@@ -275,8 +278,8 @@ mod test {
     use PamMessageStyle::*;
 
     impl SequentialConverser for String {
-        fn handle_normal_prompt(&self, msg: &str) -> PamResult<Vec<u8>> {
-            Ok(format!("{self} says {msg}").into_bytes())
+        fn handle_normal_prompt(&self, msg: &str) -> PamResult<PamBuffer> {
+            Ok(PamBuffer::new(format!("{self} says {msg}").into_bytes()))
         }
 
         fn handle_hidden_prompt(&self, msg: &str) -> PamResult<PamBuffer> {

lib/sudo-pam/src/lib.rs

@@ -463,8 +463,8 @@ impl<'a, C: Converser> PamContext<'a, C> {
 
 impl<'a> PamContext<'a, CLIConverser> {
     /// Create a builder that uses the CLI conversation function.
-    pub fn builder_cli() -> PamContextBuilder<CLIConverser> {
-        PamContextBuilder::default().converser(CLIConverser)
+    pub fn builder_cli(use_stdin: bool) -> PamContextBuilder<CLIConverser> {
+        PamContextBuilder::default().converser(CLIConverser { use_stdin })
     }
 }
 

lib/sudo-pam/src/rpassword.rs

@@ -2,39 +2,41 @@
 ///
 /// This module contains code that was originally written by Conrad Kleinespel for the rpassword
 /// crate. No copyright notices were found in the original code.
-
+///
 /// See: https://docs.rs/rpassword/latest/rpassword/
 ///
-/// CHANGES TO THE ORIGINAL CODE:
-/// - {prompt,read}_password_from_bufread deleted (we don't need them)
-/// - rtool_box::print_tty was inlined
-/// - SafeString was removed and replaced with more general PamBuffer type (and moved to cutils);
-///   also, the original code actually allowed the string to quickly escape the security net
-/// - instead of String, password are read as [u8]
-/// - this also removes the need for 'fix_line_issues', since we know we read a \n
-/// - replaced 'io_result' with our own 'cerr' function.
-/// - replaced occurences of explicit 'i32' and 'c_int' with RawFd
-/// - unified 'read_password' and 'read_password_from_fd_with_hidden_input' functions
-/// - only open /dev/tty once
-use std::io::{self, Read, Write};
-use std::mem;
-use std::os::fd::{AsRawFd, RawFd};
-
-use libc::{tcsetattr, termios, ECHO, ECHONL, TCSANOW};
+/// Most code was replaced and so is no longer a derived work; work that we kept:
+///
+/// - the "HiddenInput" struct and implementation
+///   * replaced occurences of explicit 'i32' and 'c_int' with RawFd
+///   * make it return an Option ("None" if the given fd is not a terminal)
+/// - the general idea of a "SafeString" type that clears its memory
+///   (although much more robust than in the original code)
+///
+use std::io::{self, Read};
+use std::os::fd::{AsFd, AsRawFd, RawFd};
+use std::{fs, iter, mem};
+
+use libc::{isatty, tcsetattr, termios, ECHO, ECHONL, TCSANOW};
 
 use sudo_cutils::cerr;
 
 use crate::securemem::PamBuffer;
 
-struct HiddenInput {
+pub struct HiddenInput {
     fd: RawFd,
     term_orig: termios,
 }
 
 impl HiddenInput {
-    fn new(tty: &impl AsRawFd) -> io::Result<HiddenInput> {
+    fn new(tty: &impl AsRawFd) -> io::Result<Option<HiddenInput>> {
         let fd = tty.as_raw_fd();
 
+        // If the file descriptor is not a terminal, there is nothing to hide
+        if unsafe { isatty(fd) } == 0 {
+            return Ok(None);
+        }
+
         // Make two copies of the terminal settings. The first one will be modified
         // and the second one will act as a backup for when we want to set the
         // terminal back to its original state.
@@ -50,7 +52,7 @@ impl HiddenInput {
         // Save the settings for now.
         cerr(unsafe { tcsetattr(fd, TCSANOW, &term) })?;
 
-        Ok(HiddenInput { fd, term_orig })
+        Ok(Some(HiddenInput { fd, term_orig }))
     }
 }
 
@@ -70,14 +72,12 @@ fn safe_tcgetattr(fd: RawFd) -> io::Result<termios> {
 }
 
 /// Reads a password from the given file descriptor
-fn read_password(source: &mut (impl io::Read + AsRawFd)) -> io::Result<PamBuffer> {
-    let _hide_input = HiddenInput::new(source)?;
-
+fn read_unbuffered(source: &mut impl io::Read) -> io::Result<PamBuffer> {
     let mut password = PamBuffer::default();
 
     const EOL: u8 = 0x0A;
 
-    for (read_byte, dest) in std::iter::zip(source.bytes(), password.iter_mut()) {
+    for (read_byte, dest) in iter::zip(source.bytes(), password.iter_mut()) {
         match read_byte? {
             EOL => break,
             ch => *dest = ch,
@@ -87,14 +87,94 @@ fn read_password(source: &mut (impl io::Read + AsRawFd)) -> io::Result<PamBuffer
     Ok(password)
 }
 
-/// Prompts on the TTY and then reads a password from TTY
-pub fn prompt_password(prompt: impl ToString) -> io::Result<PamBuffer> {
-    let mut stream = std::fs::OpenOptions::new()
-        .read(true)
-        .write(true)
-        .open("/dev/tty")?;
-    stream
-        .write_all(prompt.to_string().as_str().as_bytes())
-        .and_then(|_| stream.flush())
-        .and_then(|_| read_password(&mut stream))
+/// Write something and immediately flush
+fn write_unbuffered(sink: &mut impl io::Write, text: &str) -> io::Result<()> {
+    sink.write_all(text.as_bytes())?;
+    sink.flush()
+}
+
+/// A data structure representing either /dev/tty or /dev/stdin+stderr
+pub enum Terminal<'a> {
+    Tty(fs::File),
+    StdIE(io::StdinLock<'a>, io::StderrLock<'a>),
+}
+
+impl Terminal<'_> {
+    /// Open the current TTY for user communication
+    pub fn open_tty() -> io::Result<Self> {
+        Ok(Terminal::Tty(
+            fs::OpenOptions::new()
+                .read(true)
+                .write(true)
+                .open("/dev/tty")?,
+        ))
+    }
+
+    /// Open standard input and standard error for user communication
+    pub fn open_stdie() -> io::Result<Self> {
+        Ok(Terminal::StdIE(io::stdin().lock(), io::stderr().lock()))
+    }
+
+    /// Reads input with TTY echo disabled
+    pub fn read_password(&mut self) -> io::Result<PamBuffer> {
+        let mut input = self.source();
+        let _hide_input = HiddenInput::new(&input.as_fd())?;
+        read_unbuffered(&mut input)
+    }
+
+    /// Reads input with TTY echo enabled
+    pub fn read_cleartext(&mut self) -> io::Result<PamBuffer> {
+        read_unbuffered(&mut self.source())
+    }
+
+    /// Display information
+    pub fn prompt(&mut self, text: &str) -> io::Result<()> {
+        write_unbuffered(&mut self.sink(), text)
+    }
+
+    // boilerplate reduction functions
+    fn source(&mut self) -> &mut dyn ReadAsFd {
+        match self {
+            Terminal::StdIE(x, _) => x,
+            Terminal::Tty(x) => x,
+        }
+    }
+
+    fn sink(&mut self) -> &mut dyn io::Write {
+        match self {
+            Terminal::StdIE(_, x) => x,
+            Terminal::Tty(x) => x,
+        }
+    }
+}
+
+trait ReadAsFd: io::Read + AsFd {}
+impl<T: io::Read + AsFd> ReadAsFd for T {}
+
+#[cfg(test)]
+mod test {
+    use super::{read_unbuffered, write_unbuffered};
+
+    #[test]
+    fn miri_test_read() {
+        let mut data = "password123\nhello world".as_bytes();
+        let buf = read_unbuffered(&mut data).unwrap();
+        // check that the \n is not part of input
+        assert_eq!(
+            buf.iter()
+                .map(|&b| b as char)
+                .take_while(|&x| x != '\0')
+                .collect::<String>(),
+            "password123"
+        );
+        // check that the \n is also consumed but the rest of the input is still there
+        assert_eq!(std::str::from_utf8(data).unwrap(), "hello world");
+    }
+
+    #[test]
+    fn miri_test_write() {
+        let mut data = Vec::new();
+        write_unbuffered(&mut data, "prompt").unwrap();
+        assert_eq!(std::str::from_utf8(&data).unwrap(), "prompt");
+    }
 }

lib/sudo-pam/src/securemem.rs

@@ -16,8 +16,9 @@ impl PamBuffer {
         result
     }
 
-    // initialized the buffer with already existing data (otherwise populating it is a bit hairy)
+    // initialize the buffer with already existing data (otherwise populating it is a bit hairy)
     // this is inferior than placing the data into the securebuffer directly
+    #[cfg(test)]
     pub fn new(mut src: impl AsMut<[u8]>) -> Self {
         let mut buffer = PamBuffer::default();
         let src = src.as_mut();
@@ -79,7 +80,7 @@ mod test {
     #[test]
     fn miri_test_leaky_cstring() {
         let test = |text: &str| unsafe {
-            let buf = PamBuffer::new(&mut text.to_string().as_bytes_mut());
+            let buf = PamBuffer::new(text.to_string().as_bytes_mut());
             assert_eq!(&buf[..text.len()], text.as_bytes());
             let ptr = buf.leak();
             let result = sudo_cutils::string_from_ptr(ptr as *mut _);

sudo/src/pam.rs

@@ -80,7 +80,7 @@ pub fn authenticate(context: &Context) -> Result<(), Error> {
     let target_user = context.target_user.uid;
 
     // init pam
-    let mut pam = sudo_pam::PamContext::builder_cli()
+    let mut pam = sudo_pam::PamContext::builder_cli(context.stdin)
         .target_user(authenticate_for)
         .service_name("sudo")
         .build()?;

test-framework/sudo-compliance-tests/src/pass_auth.rs

@@ -7,7 +7,6 @@ use sudo_test::{Command, Env, User};
 
 use crate::{Result, PASSWORD, USERNAME};
 
-#[ignore]
 #[test]
 fn correct_password() -> Result<()> {
     let env = Env(format!("{USERNAME}    ALL=(ALL:ALL) ALL"))

test-framework/sudo-compliance-tests/src/sudoers.rs

@@ -72,7 +72,6 @@ fn cannot_sudo_if_sudoers_file_is_not_owned_by_root() -> Result<()> {
     Ok(())
 }
 
-#[ignore]
 #[test]
 fn user_specifications_evaluated_bottom_to_top() -> Result<()> {
     let env = Env(format!(