From 2deb25d5984f903657bd5ccdc3598e29dd384bfb Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 6 Jan 2025 10:10:04 +0100 Subject: [PATCH 1/6] Improve IO safety of safe_tcgetattr --- src/pam/rpassword.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 6325ee44b..e75fc44f2 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::{AsRawFd, RawFd}; +use std::os::fd::{AsFd, AsRawFd}; use std::{fs, mem}; use libc::{tcsetattr, termios, ECHO, ECHONL, TCSANOW}; @@ -36,13 +36,12 @@ impl HiddenInput { // if we have nothing to show, we have nothing to hide return Ok(None); }; - let fd = tty.as_raw_fd(); // 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(fd)?; - let term_orig = safe_tcgetattr(fd)?; + 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; @@ -52,7 +51,7 @@ impl HiddenInput { // Save the settings for now. // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct - cerr(unsafe { tcsetattr(fd, TCSANOW, &term) })?; + cerr(unsafe { tcsetattr(tty.as_raw_fd(), TCSANOW, &term) })?; Ok(Some(HiddenInput { tty, term_orig })) } @@ -68,10 +67,10 @@ impl Drop for HiddenInput { } } -fn safe_tcgetattr(fd: RawFd) -> io::Result { +fn safe_tcgetattr(tty: impl AsFd) -> io::Result { let mut term = mem::MaybeUninit::::uninit(); // SAFETY: we are passing tcgetattr a pointer to valid memory - cerr(unsafe { ::libc::tcgetattr(fd, term.as_mut_ptr()) })?; + cerr(unsafe { ::libc::tcgetattr(tty.as_fd().as_raw_fd(), term.as_mut_ptr()) })?; // SAFETY: if the previous call was a success, `tcgetattr` has initialized `term` Ok(unsafe { term.assume_init() }) } From 3fb5f352890d6585e9a149a528c3b36d875f2131 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 6 Jan 2025 10:16:08 +0100 Subject: [PATCH 2/6] Pass dyn Read/Write to read/write_unbuffered --- src/pam/rpassword.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index e75fc44f2..1a2a0e60d 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -76,7 +76,7 @@ fn safe_tcgetattr(tty: impl AsFd) -> io::Result { } /// Reads a password from the given file descriptor -fn read_unbuffered(source: &mut impl io::Read) -> io::Result { +fn read_unbuffered(source: &mut dyn io::Read) -> io::Result { let mut password = PamBuffer::default(); let mut pwd_iter = password.iter_mut(); @@ -98,7 +98,7 @@ fn read_unbuffered(source: &mut impl io::Read) -> io::Result { } /// Write something and immediately flush -fn write_unbuffered(sink: &mut impl io::Write, text: &str) -> io::Result<()> { +fn write_unbuffered(sink: &mut dyn io::Write, text: &str) -> io::Result<()> { sink.write_all(text.as_bytes())?; sink.flush() } @@ -127,19 +127,19 @@ impl Terminal<'_> { /// Reads input with TTY echo disabled pub fn read_password(&mut self) -> io::Result { - let mut input = self.source(); + let input = self.source(); let _hide_input = HiddenInput::new()?; - read_unbuffered(&mut input) + read_unbuffered(input) } /// Reads input with TTY echo enabled pub fn read_cleartext(&mut self) -> io::Result { - read_unbuffered(&mut self.source()) + read_unbuffered(self.source()) } /// Display information pub fn prompt(&mut self, text: &str) -> io::Result<()> { - write_unbuffered(&mut self.sink(), text) + write_unbuffered(self.sink(), text) } // boilerplate reduction functions From f9e913a17b976e53b8b7e0a57003468f1554a14d Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 6 Jan 2025 11:52:24 +0100 Subject: [PATCH 3/6] Implement pwfeedback --- src/common/context.rs | 2 + src/defaults/mod.rs | 1 + src/pam/converse.rs | 7 ++- src/pam/mod.rs | 2 + src/pam/rpassword.rs | 98 +++++++++++++++++++++++++++++++++++-- src/su/mod.rs | 2 +- src/sudo/env/environment.rs | 4 ++ src/sudo/env/tests.rs | 1 + src/sudo/pam.rs | 4 +- src/sudo/pipeline.rs | 4 ++ src/sudoers/policy.rs | 5 ++ 11 files changed, 123 insertions(+), 7 deletions(-) diff --git a/src/common/context.rs b/src/common/context.rs index ec34c096d..0bf5ab71c 100644 --- a/src/common/context.rs +++ b/src/common/context.rs @@ -46,6 +46,7 @@ pub struct Context { pub process: Process, // policy pub use_pty: bool, + pub password_feedback: bool, } #[derive(Debug, PartialEq, Eq)] @@ -89,6 +90,7 @@ impl Context { non_interactive: sudo_options.non_interactive, process: Process::new(), use_pty: true, + password_feedback: false, }) } } diff --git a/src/defaults/mod.rs b/src/defaults/mod.rs index d9f0c48a8..3bbe63150 100644 --- a/src/defaults/mod.rs +++ b/src/defaults/mod.rs @@ -29,6 +29,7 @@ defaults! { match_group_by_gid = false #ignored use_pty = true visiblepw = false #ignored + pwfeedback = false env_editor = true passwd_tries = 3 [0..=1000] diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 4a9b2be92..6518b0831 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -127,6 +127,7 @@ pub struct CLIConverser { pub(super) name: String, pub(super) use_stdin: bool, pub(super) no_interact: bool, + pub(super) password_feedback: bool, } use rpassword::Terminal; @@ -157,7 +158,11 @@ impl SequentialConverser for CLIConverser { } let mut tty = self.open()?; tty.prompt(&format!("[{}: authenticate] {msg}", self.name))?; - Ok(tty.read_password()?) + if self.password_feedback { + Ok(tty.read_password_with_feedback()?) + } else { + Ok(tty.read_password()?) + } } fn handle_error(&self, msg: &str) -> PamResult<()> { diff --git a/src/pam/mod.rs b/src/pam/mod.rs index e650162d4..f495238de 100644 --- a/src/pam/mod.rs +++ b/src/pam/mod.rs @@ -378,11 +378,13 @@ impl PamContext { name: &str, use_stdin: bool, no_interact: bool, + password_feedback: bool, ) -> PamContextBuilder { PamContextBuilder::default().converser(CLIConverser { name: name.to_owned(), use_stdin, no_interact, + password_feedback, }) } } diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 1a2a0e60d..b5e64ac9e 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -17,19 +17,19 @@ use std::io::{self, Error, ErrorKind, Read}; use std::os::fd::{AsFd, AsRawFd}; use std::{fs, mem}; -use libc::{tcsetattr, termios, ECHO, ECHONL, TCSANOW}; +use libc::{tcsetattr, termios, ECHO, ECHONL, ICANON, TCSANOW, VEOF, VERASE, VKILL}; use crate::cutils::cerr; use super::securemem::PamBuffer; -pub struct HiddenInput { +struct HiddenInput { tty: fs::File, term_orig: termios, } impl HiddenInput { - fn new() -> io::Result> { + fn new(feedback: bool) -> 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 { @@ -49,6 +49,11 @@ impl HiddenInput { // But don't hide the NL character when the user hits ENTER. term.c_lflag |= ECHONL; + if feedback { + // Disable canonical mode to read character by character when pwfeedback is enabled. + term.c_lflag &= !ICANON; + } + // Save the settings for now. // SAFETY: we are passing tcsetattr a valid file descriptor and pointer-to-struct cerr(unsafe { tcsetattr(tty.as_raw_fd(), TCSANOW, &term) })?; @@ -97,6 +102,74 @@ fn read_unbuffered(source: &mut dyn io::Read) -> io::Result { Ok(password) } +const BACKSPACE: u8 = 8; + +fn erase_feedback(sink: &mut dyn io::Write, i: usize) { + for _ in 0..i { + if sink.write(&[BACKSPACE, b' ', BACKSPACE]).is_err() { + return; + } + } +} + +/// Reads a password from the given file descriptor while showing feedback to the user. +fn read_unbuffered_with_feedback( + source: &mut dyn io::Read, + sink: &mut dyn io::Write, + hide_input: &HiddenInput, +) -> io::Result { + let mut password = PamBuffer::default(); + let mut i = 0; + + for read_byte in source.bytes() { + let read_byte = read_byte?; + + if read_byte == b'\n' || read_byte == b'\r' { + erase_feedback(sink, i); + sink.write(&[b'\n'])?; + break; + } + + if read_byte == hide_input.term_orig.c_cc[VEOF] { + while i > 0 { + password[i - 1] = 0; + i -= 1; + let _ = sink.write(&[BACKSPACE, b' ', BACKSPACE]); + } + break; + } + + if read_byte == hide_input.term_orig.c_cc[VERASE] { + if i > 0 { + password[i - 1] = 0; + i -= 1; + let _ = sink.write(&[BACKSPACE, b' ', BACKSPACE]); + } + } else if read_byte == hide_input.term_orig.c_cc[VKILL] { + erase_feedback(sink, i); + while i > 0 { + password[i - 1] = 0; + i -= 1; + } + } else { + if let Some(dest) = password.get_mut(i) { + *dest = read_byte; + i += 1; + let _ = sink.write(&[b'*']); + } else { + erase_feedback(sink, i); + + return Err(Error::new( + ErrorKind::OutOfMemory, + "incorrect password attempt", + )); + } + } + } + + Ok(password) +} + /// Write something and immediately flush fn write_unbuffered(sink: &mut dyn io::Write, text: &str) -> io::Result<()> { sink.write_all(text.as_bytes())?; @@ -128,10 +201,27 @@ impl Terminal<'_> { /// Reads input with TTY echo disabled pub fn read_password(&mut self) -> io::Result { let input = self.source(); - let _hide_input = HiddenInput::new()?; + let _hide_input = HiddenInput::new(false)?; read_unbuffered(input) } + /// Reads input with TTY echo disabled, but do provide visual feedback while typing. + pub fn read_password_with_feedback(&mut self) -> io::Result { + let (source, sink) = match self { + Terminal::StdIE(x, y) => (x as &mut dyn io::Read, y as &mut dyn io::Write), + Terminal::Tty(x) => ( + &mut &*x as &mut dyn io::Read, + &mut &*x as &mut dyn io::Write, + ), + }; + + if let Some(hide_input) = HiddenInput::new(true)? { + read_unbuffered_with_feedback(source, sink, &hide_input) + } else { + read_unbuffered(self.source()) + } + } + /// Reads input with TTY echo enabled pub fn read_cleartext(&mut self) -> io::Result { read_unbuffered(self.source()) diff --git a/src/su/mod.rs b/src/su/mod.rs index a830fcb5c..4d81e5476 100644 --- a/src/su/mod.rs +++ b/src/su/mod.rs @@ -31,7 +31,7 @@ fn authenticate( "su" }; let use_stdin = true; - let mut pam = PamContext::builder_cli("su", use_stdin, false) + let mut pam = PamContext::builder_cli("su", use_stdin, false, false) .target_user(user) .service_name(context) .build()?; diff --git a/src/sudo/env/environment.rs b/src/sudo/env/environment.rs index 8796a47b9..21c54c6c5 100644 --- a/src/sudo/env/environment.rs +++ b/src/sudo/env/environment.rs @@ -279,6 +279,10 @@ mod tests { fn use_pty(&self) -> bool { true } + + fn pwfeedback(&self) -> bool { + false + } } impl TestConfiguration { diff --git a/src/sudo/env/tests.rs b/src/sudo/env/tests.rs index 79738de8a..74861342b 100644 --- a/src/sudo/env/tests.rs +++ b/src/sudo/env/tests.rs @@ -136,6 +136,7 @@ fn create_test_context(sudo_options: &SudoRunOptions) -> Context { process: Process::new(), use_session_records: false, use_pty: true, + password_feedback: false, } } diff --git a/src/sudo/pam.rs b/src/sudo/pam.rs index 42edb4421..4632f325a 100644 --- a/src/sudo/pam.rs +++ b/src/sudo/pam.rs @@ -34,6 +34,7 @@ impl PamAuthenticator { matches!(context.launch, LaunchType::Shell), context.stdin, context.non_interactive, + context.password_feedback, &context.current_user.name, &context.current_user.name, ) @@ -107,6 +108,7 @@ pub fn init_pam( is_shell: bool, use_stdin: bool, non_interactive: bool, + password_feedback: bool, auth_user: &str, requesting_user: &str, ) -> PamResult> { @@ -116,7 +118,7 @@ pub fn init_pam( } else { "sudo" }; - let mut pam = PamContext::builder_cli("sudo", use_stdin, non_interactive) + let mut pam = PamContext::builder_cli("sudo", use_stdin, non_interactive, password_feedback) .service_name(service_name) .build()?; pam.mark_silent(!is_shell && !is_login_shell); diff --git a/src/sudo/pipeline.rs b/src/sudo/pipeline.rs index 4c5c4b4ec..216b9d234 100644 --- a/src/sudo/pipeline.rs +++ b/src/sudo/pipeline.rs @@ -197,6 +197,10 @@ impl Pipeline { context.use_pty = false } + if policy.pwfeedback() { + context.password_feedback = true; + } + Ok(()) } } diff --git a/src/sudoers/policy.rs b/src/sudoers/policy.rs index 73af14ea9..84604697d 100644 --- a/src/sudoers/policy.rs +++ b/src/sudoers/policy.rs @@ -25,6 +25,7 @@ pub trait Policy { fn secure_path(&self) -> Option; fn use_pty(&self) -> bool; + fn pwfeedback(&self) -> bool; } #[must_use] @@ -90,6 +91,10 @@ impl Policy for Judgement { fn use_pty(&self) -> bool { self.settings.use_pty() } + + fn pwfeedback(&self) -> bool { + self.settings.pwfeedback() + } } pub trait PreJudgementPolicy { From 6862c08c12e283636577d605df0bfa5d32011e75 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 6 Jan 2025 12:02:51 +0100 Subject: [PATCH 4/6] Make clippy happy --- src/pam/rpassword.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index b5e64ac9e..68e7073a1 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -126,7 +126,7 @@ fn read_unbuffered_with_feedback( if read_byte == b'\n' || read_byte == b'\r' { erase_feedback(sink, i); - sink.write(&[b'\n'])?; + let _ = sink.write(b"\n"); break; } @@ -152,10 +152,11 @@ fn read_unbuffered_with_feedback( i -= 1; } } else { + #[allow(clippy::collapsible_else_if)] if let Some(dest) = password.get_mut(i) { *dest = read_byte; i += 1; - let _ = sink.write(&[b'*']); + let _ = sink.write(b"*"); } else { erase_feedback(sink, i); @@ -167,6 +168,7 @@ fn read_unbuffered_with_feedback( } } + Ok(password) } From eaa4bb845d385cb70ffcbd54a4a22f687279ec75 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 6 Jan 2025 12:05:06 +0100 Subject: [PATCH 5/6] Fix borrowck error on older rustc versions --- src/pam/rpassword.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 68e7073a1..18586e16b 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -168,7 +168,6 @@ fn read_unbuffered_with_feedback( } } - Ok(password) } @@ -209,16 +208,11 @@ impl Terminal<'_> { /// Reads input with TTY echo disabled, but do provide visual feedback while typing. pub fn read_password_with_feedback(&mut self) -> io::Result { - let (source, sink) = match self { - Terminal::StdIE(x, y) => (x as &mut dyn io::Read, y as &mut dyn io::Write), - Terminal::Tty(x) => ( - &mut &*x as &mut dyn io::Read, - &mut &*x as &mut dyn io::Write, - ), - }; - if let Some(hide_input) = HiddenInput::new(true)? { - read_unbuffered_with_feedback(source, sink, &hide_input) + match self { + Terminal::StdIE(x, y) => read_unbuffered_with_feedback(x, y, &hide_input), + Terminal::Tty(x) => read_unbuffered_with_feedback(&mut &*x, &mut &*x, &hide_input), + } } else { read_unbuffered(self.source()) } From 1e8ed5dd2d86e46bdc10b4c50cc799ab6d39c087 Mon Sep 17 00:00:00 2001 From: Marc Schoolderman Date: Tue, 7 Jan 2025 13:28:54 +0100 Subject: [PATCH 6/6] improvide readability by removing explicit loops with function re-use --- src/pam/rpassword.rs | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 18586e16b..85239528b 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -102,9 +102,8 @@ fn read_unbuffered(source: &mut dyn io::Read) -> io::Result { Ok(password) } -const BACKSPACE: u8 = 8; - fn erase_feedback(sink: &mut dyn io::Write, i: usize) { + const BACKSPACE: u8 = 0x08; for _ in 0..i { if sink.write(&[BACKSPACE, b' ', BACKSPACE]).is_err() { return; @@ -119,46 +118,43 @@ fn read_unbuffered_with_feedback( hide_input: &HiddenInput, ) -> io::Result { let mut password = PamBuffer::default(); - let mut i = 0; + let mut pw_len = 0; + // invariant: the amount of nonzero-bytes in the buffer correspond + // with the amount of asterisks on the terminal (both tracked in `pw_len`) for read_byte in source.bytes() { let read_byte = read_byte?; if read_byte == b'\n' || read_byte == b'\r' { - erase_feedback(sink, i); + erase_feedback(sink, pw_len); let _ = sink.write(b"\n"); break; } if read_byte == hide_input.term_orig.c_cc[VEOF] { - while i > 0 { - password[i - 1] = 0; - i -= 1; - let _ = sink.write(&[BACKSPACE, b' ', BACKSPACE]); - } + erase_feedback(sink, pw_len); + password.fill(0); break; } if read_byte == hide_input.term_orig.c_cc[VERASE] { - if i > 0 { - password[i - 1] = 0; - i -= 1; - let _ = sink.write(&[BACKSPACE, b' ', BACKSPACE]); + if pw_len > 0 { + erase_feedback(sink, 1); + password[pw_len - 1] = 0; + pw_len -= 1; } } else if read_byte == hide_input.term_orig.c_cc[VKILL] { - erase_feedback(sink, i); - while i > 0 { - password[i - 1] = 0; - i -= 1; - } + erase_feedback(sink, pw_len); + password.fill(0); + pw_len = 0; } else { #[allow(clippy::collapsible_else_if)] - if let Some(dest) = password.get_mut(i) { + if let Some(dest) = password.get_mut(pw_len) { *dest = read_byte; - i += 1; + pw_len += 1; let _ = sink.write(b"*"); } else { - erase_feedback(sink, i); + erase_feedback(sink, pw_len); return Err(Error::new( ErrorKind::OutOfMemory,