From 19d6db4486498fd09fb304d0cf8863e0dfda422a Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Tue, 18 Nov 2025 15:11:23 +0100 Subject: [PATCH 1/6] Don't open /dev/tty twice There is no need to do so and this refactor clarifies exactly when HiddenInput::new() returns Some. With this we no longer disable feedback on /dev/tty when we are reading a password from a pipe and we disable feedback on the correct tty if stdin is a different tty than /dev/tty. Once we implement askpass this also prevents interfering with the askpass program if it reads from the tty. --- src/pam/converse.rs | 16 +++++--- src/pam/rpassword.rs | 89 ++++++++++++++++++++------------------------ 2 files changed, 51 insertions(+), 54 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index e2b199151..0e18e5de1 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -1,6 +1,7 @@ use std::io; use crate::cutils::string_from_ptr; +use crate::pam::rpassword::Hidden; use crate::system::{ signal::{self, SignalSet}, time::Duration, @@ -143,7 +144,7 @@ impl Converser for CLIConverser { fn handle_normal_prompt(&self, msg: &str) -> PamResult { let (mut tty, _guard) = self.open()?; tty.prompt(&format!("[{}: input needed] {msg} ", self.name))?; - Ok(tty.read_cleartext()?) + Ok(tty.read_input(None, Hidden::No)?) } fn handle_hidden_prompt(&self, msg: &str) -> PamResult { @@ -152,11 +153,14 @@ impl Converser for CLIConverser { tty.bell()?; } tty.prompt(msg)?; - if self.password_feedback { - tty.read_password_with_feedback(self.password_timeout) - } else { - tty.read_password(self.password_timeout) - } + tty.read_input( + self.password_timeout, + if self.password_feedback { + Hidden::WithFeedback(()) + } else { + Hidden::Yes(()) + }, + ) .map_err(|err| { if let io::ErrorKind::TimedOut = err.kind() { PamError::TimedOut diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 908fdb2fa..51da8f138 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -14,31 +14,24 @@ /// (although much more robust than in the original code) /// use std::io::{self, Error, ErrorKind, Read}; -use std::os::fd::{AsFd, AsRawFd, BorrowedFd}; +use std::os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd}; use std::time::Instant; use std::{fs, mem}; use libc::{tcsetattr, termios, ECHO, ECHONL, ICANON, TCSANOW, VEOF, VERASE, VKILL}; -use crate::cutils::cerr; +use crate::cutils::{cerr, safe_isatty}; use crate::system::time::Duration; use super::securemem::PamBuffer; struct HiddenInput { - tty: fs::File, + tty: OwnedFd, term_orig: termios, } impl HiddenInput { - fn new() -> io::Result> { - // control ourselves that we are really talking to a TTY - // mitigates: https://marc.info/?l=oss-security&m=168164424404224 - let Ok(tty) = fs::File::open("/dev/tty") else { - // if we have nothing to show, we have nothing to hide - return Ok(None); - }; - + fn new(tty: OwnedFd) -> io::Result { // 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. @@ -58,7 +51,7 @@ impl HiddenInput { // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct cerr(unsafe { tcsetattr(tty.as_raw_fd(), TCSANOW, &term) })?; - Ok(Some(HiddenInput { tty, term_orig })) + Ok(HiddenInput { tty, term_orig }) } } @@ -89,17 +82,27 @@ fn erase_feedback(sink: &mut dyn io::Write, i: usize) { } } -enum Hidden<'a> { +pub(super) enum Hidden { No, - Yes(&'a HiddenInput), - WithFeedback(&'a HiddenInput), + Yes(T), + WithFeedback(T), +} + +impl Hidden { + fn as_ref(&self) -> Hidden<&T> { + match self { + Hidden::No => Hidden::No, + Hidden::Yes(x) => Hidden::Yes(x), + Hidden::WithFeedback(x) => Hidden::WithFeedback(x), + } + } } /// Reads a password from the given file descriptor while optionally showing feedback to the user. fn read_unbuffered( source: &mut dyn io::Read, sink: &mut dyn io::Write, - hide_input: Hidden<'_>, + hide_input: Hidden<&HiddenInput>, ) -> io::Result { struct ExitGuard<'a> { pw_len: usize, @@ -260,6 +263,8 @@ pub enum Terminal<'a> { impl Terminal<'_> { /// Open the current TTY for user communication pub fn open_tty() -> io::Result { + // control ourselves that we are really talking to a TTY + // mitigates: https://marc.info/?l=oss-security&m=168164424404224 Ok(Terminal::Tty( fs::OpenOptions::new() .read(true) @@ -273,48 +278,36 @@ impl Terminal<'_> { Ok(Terminal::StdIE(io::stdin().lock(), io::stderr().lock())) } - /// Reads input with TTY echo disabled - pub fn read_password(&mut self, timeout: Option) -> io::Result { - let hide_input = HiddenInput::new()?; - self.read_inner( - timeout, - hide_input.as_ref().map(Hidden::Yes).unwrap_or(Hidden::No), - ) - } - - /// Reads input with TTY echo disabled, but do provide visual feedback while typing. - pub fn read_password_with_feedback( + /// Reads input with TTY echo and visual feedback set according to the `hidden` parameter. + pub(super) fn read_input( &mut self, timeout: Option, + hidden: Hidden<()>, ) -> io::Result { - let hide_input = HiddenInput::new()?; - self.read_inner( - timeout, - hide_input - .as_ref() - .map(Hidden::WithFeedback) - .unwrap_or(Hidden::No), - ) - } - - /// Reads input with TTY echo enabled - pub fn read_cleartext(&mut self) -> io::Result { - self.read_inner(None, Hidden::No) - } + let do_hide_input = |input: BorrowedFd| -> Result, io::Error> { + if !safe_isatty(input) { + // Input is not a tty, so we can't hide feedback. + return Ok(Hidden::No); + } + match hidden { + Hidden::No => Ok(Hidden::No), + Hidden::Yes(()) => Ok(Hidden::Yes(HiddenInput::new(input.try_clone_to_owned()?)?)), + Hidden::WithFeedback(()) => Ok(Hidden::WithFeedback(HiddenInput::new( + input.try_clone_to_owned()?, + )?)), + } + }; - fn read_inner( - &mut self, - timeout: Option, - hide_input: Hidden<'_>, - ) -> io::Result { match self { Terminal::StdIE(stdin, stdout) => { + let hide_input = do_hide_input(stdin.as_fd())?; let mut reader = TimeoutRead::new(stdin.as_fd(), timeout); - read_unbuffered(&mut reader, stdout, hide_input) + read_unbuffered(&mut reader, stdout, hide_input.as_ref()) } Terminal::Tty(file) => { + let hide_input = do_hide_input(file.as_fd())?; let mut reader = TimeoutRead::new(file.as_fd(), timeout); - read_unbuffered(&mut reader, &mut &*file, hide_input) + read_unbuffered(&mut reader, &mut &*file, hide_input.as_ref()) } } } From 295d4f77bb046805db4638e83bdd542efe2cfdca Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Wed, 19 Nov 2025 13:21:23 +0100 Subject: [PATCH 2/6] Pass prompt to Terminal::read_input This is necessary for askpass as with askpass the prompt is passed to the askpass program rather than directly printed to stdout. --- src/pam/converse.rs | 9 ++++++--- src/pam/rpassword.rs | 5 +++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 0e18e5de1..abad6facb 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -143,8 +143,11 @@ impl CLIConverser { impl Converser for CLIConverser { fn handle_normal_prompt(&self, msg: &str) -> PamResult { let (mut tty, _guard) = self.open()?; - tty.prompt(&format!("[{}: input needed] {msg} ", self.name))?; - Ok(tty.read_input(None, Hidden::No)?) + Ok(tty.read_input( + &format!("[{}: input needed] {msg} ", self.name), + None, + Hidden::No, + )?) } fn handle_hidden_prompt(&self, msg: &str) -> PamResult { @@ -152,8 +155,8 @@ impl Converser for CLIConverser { if self.bell && !self.use_stdin { tty.bell()?; } - tty.prompt(msg)?; tty.read_input( + msg, self.password_timeout, if self.password_feedback { Hidden::WithFeedback(()) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 51da8f138..3408ebbe8 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -281,6 +281,7 @@ impl Terminal<'_> { /// Reads input with TTY echo and visual feedback set according to the `hidden` parameter. pub(super) fn read_input( &mut self, + prompt: &str, timeout: Option, hidden: Hidden<()>, ) -> io::Result { @@ -300,11 +301,15 @@ impl Terminal<'_> { match self { Terminal::StdIE(stdin, stdout) => { + write_unbuffered(stdout, prompt.as_bytes())?; + let hide_input = do_hide_input(stdin.as_fd())?; let mut reader = TimeoutRead::new(stdin.as_fd(), timeout); read_unbuffered(&mut reader, stdout, hide_input.as_ref()) } Terminal::Tty(file) => { + write_unbuffered(file, prompt.as_bytes())?; + let hide_input = do_hide_input(file.as_fd())?; let mut reader = TimeoutRead::new(file.as_fd(), timeout); read_unbuffered(&mut reader, &mut &*file, hide_input.as_ref()) From b418575b07873e52abaf89ce238a0388d98443ff Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Wed, 19 Nov 2025 14:19:24 +0100 Subject: [PATCH 3/6] remove try_clone_to_owned --- src/pam/rpassword.rs | 47 ++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 3408ebbe8..bda50b971 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -14,7 +14,7 @@ /// (although much more robust than in the original code) /// use std::io::{self, Error, ErrorKind, Read}; -use std::os::fd::{AsFd, AsRawFd, BorrowedFd, OwnedFd}; +use std::os::fd::{AsFd, AsRawFd, BorrowedFd}; use std::time::Instant; use std::{fs, mem}; @@ -25,18 +25,18 @@ use crate::system::time::Duration; use super::securemem::PamBuffer; -struct HiddenInput { - tty: OwnedFd, +struct HiddenInput<'a> { + tty: BorrowedFd<'a>, term_orig: termios, } -impl HiddenInput { - fn new(tty: OwnedFd) -> io::Result { +impl HiddenInput<'_> { + fn new(tty: BorrowedFd) -> io::Result { // 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. - let mut term = safe_tcgetattr(&tty)?; - let term_orig = safe_tcgetattr(&tty)?; + let mut term = safe_tcgetattr(tty)?; + let term_orig = safe_tcgetattr(tty)?; // Hide the password. This is what makes this function useful. term.c_lflag &= !ECHO; @@ -55,7 +55,7 @@ impl HiddenInput { } } -impl Drop for HiddenInput { +impl Drop for HiddenInput<'_> { fn drop(&mut self) { // Set the the mode back to normal // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct @@ -285,32 +285,33 @@ impl Terminal<'_> { timeout: Option, hidden: Hidden<()>, ) -> io::Result { - let do_hide_input = |input: BorrowedFd| -> Result, io::Error> { - if !safe_isatty(input) { - // Input is not a tty, so we can't hide feedback. - return Ok(Hidden::No); - } - match hidden { - Hidden::No => Ok(Hidden::No), - Hidden::Yes(()) => Ok(Hidden::Yes(HiddenInput::new(input.try_clone_to_owned()?)?)), - Hidden::WithFeedback(()) => Ok(Hidden::WithFeedback(HiddenInput::new( - input.try_clone_to_owned()?, - )?)), - } - }; + //note for the reviewer: these explicit lifetimes are not necessary + fn do_hide_input<'a>( + hidden: Hidden<()>, + input: BorrowedFd<'a>, + ) -> Result>, io::Error> { + Ok(match hidden { + // If input is not a tty, we can't hide feedback. + _ if !safe_isatty(input) => Hidden::No, + + Hidden::No => Hidden::No, + Hidden::Yes(()) => Hidden::Yes(HiddenInput::new(input)?), + Hidden::WithFeedback(()) => Hidden::WithFeedback(HiddenInput::new(input)?), + }) + } match self { Terminal::StdIE(stdin, stdout) => { write_unbuffered(stdout, prompt.as_bytes())?; - let hide_input = do_hide_input(stdin.as_fd())?; + let hide_input = do_hide_input(hidden, stdin.as_fd())?; let mut reader = TimeoutRead::new(stdin.as_fd(), timeout); read_unbuffered(&mut reader, stdout, hide_input.as_ref()) } Terminal::Tty(file) => { write_unbuffered(file, prompt.as_bytes())?; - let hide_input = do_hide_input(file.as_fd())?; + let hide_input = do_hide_input(hidden, file.as_fd())?; let mut reader = TimeoutRead::new(file.as_fd(), timeout); read_unbuffered(&mut reader, &mut &*file, hide_input.as_ref()) } From cffd1aeed35ada2425391345f1bbe968afc28206 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Wed, 19 Nov 2025 14:21:46 +0100 Subject: [PATCH 4/6] remove as_ref --- src/pam/rpassword.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index bda50b971..565a01166 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -88,21 +88,11 @@ pub(super) enum Hidden { WithFeedback(T), } -impl Hidden { - fn as_ref(&self) -> Hidden<&T> { - match self { - Hidden::No => Hidden::No, - Hidden::Yes(x) => Hidden::Yes(x), - Hidden::WithFeedback(x) => Hidden::WithFeedback(x), - } - } -} - /// Reads a password from the given file descriptor while optionally showing feedback to the user. fn read_unbuffered( source: &mut dyn io::Read, sink: &mut dyn io::Write, - hide_input: Hidden<&HiddenInput>, + hide_input: &Hidden, ) -> io::Result { struct ExitGuard<'a> { pw_len: usize, @@ -306,14 +296,14 @@ impl Terminal<'_> { let hide_input = do_hide_input(hidden, stdin.as_fd())?; let mut reader = TimeoutRead::new(stdin.as_fd(), timeout); - read_unbuffered(&mut reader, stdout, hide_input.as_ref()) + read_unbuffered(&mut reader, stdout, &hide_input) } Terminal::Tty(file) => { write_unbuffered(file, prompt.as_bytes())?; let hide_input = do_hide_input(hidden, file.as_fd())?; let mut reader = TimeoutRead::new(file.as_fd(), timeout); - read_unbuffered(&mut reader, &mut &*file, hide_input.as_ref()) + read_unbuffered(&mut reader, &mut &*file, &hide_input) } } } From 759eeff3c8604c7181d843e2e344c9bb54e278f5 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Wed, 19 Nov 2025 14:33:48 +0100 Subject: [PATCH 5/6] remove explicit lifetimes --- src/pam/rpassword.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 565a01166..047464b6a 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -275,11 +275,10 @@ impl Terminal<'_> { timeout: Option, hidden: Hidden<()>, ) -> io::Result { - //note for the reviewer: these explicit lifetimes are not necessary - fn do_hide_input<'a>( + fn do_hide_input( hidden: Hidden<()>, - input: BorrowedFd<'a>, - ) -> Result>, io::Error> { + input: BorrowedFd, + ) -> Result, io::Error> { Ok(match hidden { // If input is not a tty, we can't hide feedback. _ if !safe_isatty(input) => Hidden::No, From ff0dd7804619e623f0aa06cdd3c0e568b73270d4 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Wed, 19 Nov 2025 15:10:52 +0100 Subject: [PATCH 6/6] fix unit tests --- src/pam/rpassword.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 047464b6a..d5cf3de22 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -335,7 +335,7 @@ mod test { fn miri_test_read() { let mut data = "password123\nhello world".as_bytes(); let mut stdout = Vec::new(); - let buf = read_unbuffered(&mut data, &mut stdout, Hidden::No).unwrap(); + let buf = read_unbuffered(&mut data, &mut stdout, &Hidden::No).unwrap(); // check that the \n is not part of input assert_eq!( buf.iter() @@ -351,8 +351,10 @@ mod test { #[test] fn miri_test_longpwd() { let mut stdout = Vec::new(); - assert!(read_unbuffered(&mut "a".repeat(511).as_bytes(), &mut stdout, Hidden::No).is_ok()); - assert!(read_unbuffered(&mut "a".repeat(512).as_bytes(), &mut stdout, Hidden::No).is_err()); + assert!(read_unbuffered(&mut "a".repeat(511).as_bytes(), &mut stdout, &Hidden::No).is_ok()); + assert!( + read_unbuffered(&mut "a".repeat(512).as_bytes(), &mut stdout, &Hidden::No).is_err() + ); } #[test]