From d8ab7705eeaab1c821194490966796492b9840e7 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Wed, 19 Nov 2025 13:31:11 +0100 Subject: [PATCH 1/2] Propagate all error kinds from pam::converse not just timeouts This way they rather than silently discarding the error message and doing another authentication attempt, they properly report the error message and cause sudo to exit. This way for example pam_faillock won't cause a persistent error like incorrect SUDO_ASKPASS value (once implemented) to be treated as multiple successive failed password attempts. --- src/pam/converse.rs | 9 ++++----- src/pam/mod.rs | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index abad6facb..2033da673 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -193,7 +193,7 @@ pub(super) struct ConverserData { // pam_authenticate does not return error codes returned by the conversation // function; these are set by the conversation function instead of returning // multiple error codes. - pub(super) timed_out: bool, + pub(super) error: Option, pub(super) panicked: bool, } @@ -243,11 +243,10 @@ pub(super) unsafe extern "C" fn converse( Ok(resp_buf) => { resp_bufs.push(resp_buf); } - Err(PamError::TimedOut) => { - app_data.timed_out = true; + Err(err) => { + app_data.error = Some(err); return PamErrorType::ConversationError; } - Err(_) => return PamErrorType::ConversationError, } } @@ -424,7 +423,7 @@ mod test { converser_name: "tux".to_string(), no_interact: false, auth_prompt: Some("authenticate".to_owned()), - timed_out: false, + error: None, panicked: false, }); let cookie = PamConvBorrow::new(hello.as_mut()); diff --git a/src/pam/mod.rs b/src/pam/mod.rs index c4e03eb09..2738aee5b 100644 --- a/src/pam/mod.rs +++ b/src/pam/mod.rs @@ -80,7 +80,7 @@ impl PamContext { converser_name: converser_name.to_owned(), no_interact, auth_prompt: Some("authenticate".to_owned()), - timed_out: false, + error: None, panicked: false, })); @@ -176,8 +176,8 @@ impl PamContext { } // SAFETY: self.data_ptr was created by Box::into_raw - if unsafe { (*self.data_ptr).timed_out } { - return Err(PamError::TimedOut); + if let Some(error) = unsafe { (*self.data_ptr).error.take() } { + return Err(error); } #[allow(clippy::question_mark)] From a7d610d6c19d9b5c209567f5433e3a5ff12549bb Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Wed, 19 Nov 2025 14:35:20 +0100 Subject: [PATCH 2/2] [WIP] Error fixes --- src/pam/converse.rs | 11 ++++------ src/pam/error.rs | 4 ++++ src/pam/rpassword.rs | 21 +++++++++++-------- .../src/sudo/flag_list/credential_caching.rs | 2 +- .../src/sudo/pass_auth/stdin.rs | 4 ++-- .../src/sudo/pass_auth/tty.rs | 4 ++-- .../src/sudo/sudoers/timestamp_timeout.rs | 4 ++-- .../src/sudo/timestamp.rs | 4 ++-- .../src/sudo/timestamp/remove.rs | 2 +- .../src/sudo/timestamp/reset.rs | 6 +++--- .../src/sudo/timestamp/validate.rs | 2 +- .../src/sudoedit/sudoers.rs | 5 ++++- 12 files changed, 38 insertions(+), 31 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 2033da673..418a9aee3 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -129,7 +129,7 @@ impl Drop for SignalGuard { } impl CLIConverser { - fn open(&self) -> std::io::Result<(Terminal<'_>, SignalGuard)> { + fn open(&self) -> PamResult<(Terminal<'_>, SignalGuard)> { let term = if self.use_stdin { Terminal::open_stdie()? } else { @@ -164,12 +164,9 @@ impl Converser for CLIConverser { Hidden::Yes(()) }, ) - .map_err(|err| { - if let io::ErrorKind::TimedOut = err.kind() { - PamError::TimedOut - } else { - PamError::IoError(err) - } + .map_err(|err| match err { + PamError::IoError(err) if err.kind() == io::ErrorKind::TimedOut => PamError::TimedOut, + err => err, }) } diff --git a/src/pam/error.rs b/src/pam/error.rs index 47c9c30da..1c979bf38 100644 --- a/src/pam/error.rs +++ b/src/pam/error.rs @@ -173,8 +173,10 @@ pub enum PamError { Utf8Error(Utf8Error), Pam(PamErrorType), IoError(std::io::Error), + TtyRequired, EnvListFailure, InteractionRequired, + IncorrectPasswordAttempt, TimedOut, InvalidUser(String, String), } @@ -216,6 +218,7 @@ impl fmt::Display for PamError { } PamError::Pam(tp) => write!(f, "PAM error: {}", tp.get_err_msg()), PamError::IoError(e) => write!(f, "IO error: {e}"), + PamError::TtyRequired => write!(f, "A terminal is required to read the password"), PamError::EnvListFailure => { write!( f, @@ -223,6 +226,7 @@ impl fmt::Display for PamError { ) } PamError::InteractionRequired => write!(f, "Interaction is required"), + PamError::IncorrectPasswordAttempt => write!(f, "Incorrect password attempt"), PamError::TimedOut => write!(f, "timed out"), PamError::InvalidUser(username, other_user) => { write!( diff --git a/src/pam/rpassword.rs b/src/pam/rpassword.rs index 03cb7c5bb..a768761a8 100644 --- a/src/pam/rpassword.rs +++ b/src/pam/rpassword.rs @@ -13,7 +13,7 @@ /// - the general idea of a "SafeString" type that clears its memory /// (although much more robust than in the original code) /// -use std::io::{self, Error, ErrorKind, Read}; +use std::io::{self, ErrorKind, Read}; use std::os::fd::{AsFd, AsRawFd, BorrowedFd}; use std::time::Instant; use std::{fs, mem}; @@ -21,6 +21,7 @@ use std::{fs, mem}; use libc::{tcsetattr, termios, ECHO, ECHONL, ICANON, TCSANOW, VEOF, VERASE, VKILL}; use crate::cutils::{cerr, safe_isatty}; +use crate::pam::{PamError, PamResult}; use crate::system::time::Duration; use super::securemem::PamBuffer; @@ -93,7 +94,7 @@ fn read_unbuffered( source: &mut dyn io::Read, sink: &mut dyn io::Write, hide_input: &Hidden, -) -> io::Result { +) -> PamResult { struct ExitGuard<'a> { pw_len: usize, feedback: bool, @@ -129,6 +130,10 @@ fn read_unbuffered( if let Hidden::Yes(input) | Hidden::WithFeedback(input) = hide_input { if read_byte == input.term_orig.c_cc[VEOF] { + if state.pw_len == 0 { + //return Err(PamError::NeedsPassword); + } + password.fill(0); break; } @@ -161,10 +166,7 @@ fn read_unbuffered( let _ = state.sink.write(b"*"); } } else { - return Err(Error::new( - ErrorKind::OutOfMemory, - "incorrect password attempt", - )); + return Err(PamError::IncorrectPasswordAttempt); } } @@ -251,14 +253,15 @@ pub enum Terminal<'a> { impl Terminal<'_> { /// Open the current TTY for user communication - pub fn open_tty() -> io::Result { + pub fn open_tty() -> PamResult { // 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) .write(true) - .open("/dev/tty")?, + .open("/dev/tty") + .map_err(|_| PamError::TtyRequired)?, )) } @@ -273,7 +276,7 @@ impl Terminal<'_> { prompt: &str, timeout: Option, hidden: Hidden<()>, - ) -> io::Result { + ) -> PamResult { fn do_hide_input( hidden: Hidden<()>, input: BorrowedFd, diff --git a/test-framework/sudo-compliance-tests/src/sudo/flag_list/credential_caching.rs b/test-framework/sudo-compliance-tests/src/sudo/flag_list/credential_caching.rs index c8d1e1d8c..61788d037 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/flag_list/credential_caching.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/flag_list/credential_caching.rs @@ -87,7 +87,7 @@ fn flag_reset_timestamp() { let diagnostic = if sudo_test::is_original_sudo() { "sudo: a password is required" } else { - "sudo: Authentication failed" + "sudo: A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/stdin.rs b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/stdin.rs index 14e922afa..6de521c2c 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/stdin.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/stdin.rs @@ -96,7 +96,7 @@ fn input_longer_than_max_pam_response_size_is_handled_gracefully() { assert_contains!(stderr, "sudo: 2 incorrect password attempts"); } } else { - assert_contains!(stderr, "incorrect authentication attempt"); + assert_contains!(stderr, "Incorrect password attempt"); assert_not_contains!(stderr, "panic"); } } @@ -124,7 +124,7 @@ fn input_longer_than_password_should_not_be_accepted_as_correct_password() { if sudo_test::is_original_sudo() { assert_contains!(stderr, "sudo: 1 incorrect password attempt"); } else { - assert_contains!(stderr, "incorrect authentication attempt"); + assert_contains!(stderr, "Incorrect password attempt"); } } } diff --git a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs index 19002a11f..4fe22352b 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/pass_auth/tty.rs @@ -52,7 +52,7 @@ fn no_tty() { let diagnostic = if sudo_test::is_original_sudo() { "a terminal is required to read the password" } else { - "Maximum 3 incorrect authentication attempts" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } @@ -96,7 +96,7 @@ fn input_longer_than_password_should_not_be_accepted_as_correct_password() { let diagnostic = if sudo_test::is_original_sudo() { "sudo: 1 incorrect password attempt" } else { - "Authentication failed, try again" + "Incorrect password attempt" }; assert_contains!(stderr, diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/sudoers/timestamp_timeout.rs b/test-framework/sudo-compliance-tests/src/sudo/sudoers/timestamp_timeout.rs index 62bc70125..99ca6bc18 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/sudoers/timestamp_timeout.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/sudoers/timestamp_timeout.rs @@ -46,7 +46,7 @@ Defaults timestamp_timeout=0.1" let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } @@ -73,7 +73,7 @@ Defaults timestamp_timeout=0" let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/timestamp.rs b/test-framework/sudo-compliance-tests/src/sudo/timestamp.rs index 43a5fd591..9d0bbf13b 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/timestamp.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/timestamp.rs @@ -45,7 +45,7 @@ fn by_default_credential_caching_is_local() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } @@ -220,7 +220,7 @@ fn cached_credential_not_shared_between_auth_users() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/timestamp/remove.rs b/test-framework/sudo-compliance-tests/src/sudo/timestamp/remove.rs index bcfa4b295..467b100b6 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/timestamp/remove.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/timestamp/remove.rs @@ -83,7 +83,7 @@ fn also_works_locally() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "Authentication failed" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/timestamp/reset.rs b/test-framework/sudo-compliance-tests/src/sudo/timestamp/reset.rs index f62672c75..827bf6be2 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/timestamp/reset.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/timestamp/reset.rs @@ -24,7 +24,7 @@ fn it_works() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } @@ -72,7 +72,7 @@ fn with_command_prompts_for_password() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } @@ -144,7 +144,7 @@ fn with_command_does_not_cache_credentials() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "Authentication failed" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudo/timestamp/validate.rs b/test-framework/sudo-compliance-tests/src/sudo/timestamp/validate.rs index 8e7a1c224..e49135f2f 100644 --- a/test-framework/sudo-compliance-tests/src/sudo/timestamp/validate.rs +++ b/test-framework/sudo-compliance-tests/src/sudo/timestamp/validate.rs @@ -40,7 +40,7 @@ fn prompts_for_password() { let diagnostic = if sudo_test::is_original_sudo() { "a password is required" } else { - "incorrect authentication attempt" + "A terminal is required to read the password" }; assert_contains!(output.stderr(), diagnostic); } diff --git a/test-framework/sudo-compliance-tests/src/sudoedit/sudoers.rs b/test-framework/sudo-compliance-tests/src/sudoedit/sudoers.rs index 261ea04fd..1dc131ea0 100644 --- a/test-framework/sudo-compliance-tests/src/sudoedit/sudoers.rs +++ b/test-framework/sudo-compliance-tests/src/sudoedit/sudoers.rs @@ -216,6 +216,9 @@ fn passwd() { if sudo_test::is_original_sudo() { assert_contains!(output.stderr(), "a password is required"); } else { - assert_contains!(output.stderr(), "Authentication failed"); + assert_contains!( + output.stderr(), + "A terminal is required to read the password" + ); } }