diff --git a/src/pam/converse.rs b/src/pam/converse.rs index abad6facb..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, }) } @@ -193,7 +190,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 +240,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 +420,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/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/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)] 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" + ); } }