Propagate all error kinds from pam::converse not just timeouts (#1346)

Author: bjorn3

src/pam/converse.rs

@@ -1,5 +1,4 @@
 use std::ffi::{c_int, c_void};
-use std::io;
 use std::time::Duration;
 
 use crate::cutils::string_from_ptr;
@@ -12,7 +11,7 @@ use super::{error::PamResult, rpassword, securemem::PamBuffer, PamError, PamErro
 
 /// Each message in a PAM conversation will have a message style. Each of these
 /// styles must be handled separately.
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Eq, PartialEq)]
 pub enum PamMessageStyle {
     /// Prompt for input using a message. The input should considered secret
     /// and should be hidden from view.
@@ -128,7 +127,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 {
@@ -142,11 +141,11 @@ impl CLIConverser {
 impl Converser for CLIConverser {
     fn handle_normal_prompt(&self, msg: &str) -> PamResult<PamBuffer> {
         let (mut tty, _guard) = self.open()?;
-        Ok(tty.read_input(
+        tty.read_input(
             &format!("[{}: input needed] {msg} ", self.name),
             None,
             Hidden::No,
-        )?)
+        )
     }
 
     fn handle_hidden_prompt(&self, msg: &str) -> PamResult<PamBuffer> {
@@ -163,13 +162,6 @@ impl Converser for CLIConverser {
                 Hidden::Yes(())
             },
         )
-        .map_err(|err| {
-            if let io::ErrorKind::TimedOut = err.kind() {
-                PamError::TimedOut
-            } else {
-                PamError::IoError(err)
-            }
-        })
     }
 
     fn handle_error(&self, msg: &str) -> PamResult<()> {
@@ -192,7 +184,7 @@ pub(super) struct ConverserData<C> {
     // 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<PamError>,
     pub(super) panicked: bool,
 }
 
@@ -238,15 +230,22 @@ pub(super) unsafe extern "C" fn converse<C: Converser>(
             // send the conversation off to the Rust part
             // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM
             let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData<C>) };
+
+            if app_data.error.is_some()
+                && (style == PamMessageStyle::PromptEchoOff
+                    || style == PamMessageStyle::PromptEchoOn)
+            {
+                return PamErrorType::ConversationError;
+            }
+
             match handle_message(app_data, style, &msg) {
                 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,
             }
         }
 
@@ -423,7 +422,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());

src/pam/error.rs

@@ -175,8 +175,11 @@ pub enum PamError {
     Utf8Error(Utf8Error),
     Pam(PamErrorType),
     IoError(std::io::Error),
+    TtyRequired,
     EnvListFailure,
     InteractionRequired,
+    NoPasswordProvided,
+    IncorrectPasswordAttempt,
     TimedOut,
     InvalidUser(String, String),
 }
@@ -218,13 +221,16 @@ 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 authenticate"),
             PamError::EnvListFailure => {
                 write!(
                     f,
                     "It was not possible to get a list of environment variables"
                 )
             }
             PamError::InteractionRequired => write!(f, "Interaction is required"),
+            PamError::NoPasswordProvided => write!(f, "Authentication required but not attempted"),
+            PamError::IncorrectPasswordAttempt => write!(f, "Incorrect authentication attempt"),
             PamError::TimedOut => write!(f, "timed out"),
             PamError::InvalidUser(username, other_user) => {
                 write!(

src/pam/mod.rs

@@ -78,7 +78,7 @@ impl PamContext {
             converser_name: converser_name.to_owned(),
             no_interact,
             auth_prompt: Some("authenticate".to_owned()),
-            timed_out: false,
+            error: None,
             panicked: false,
         }));
 
@@ -174,8 +174,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)]

src/pam/rpassword.rs

@@ -14,14 +14,15 @@
 //!   (although much more robust than in the original code)
 
 use std::ffi::c_void;
-use std::io::{self, Error, ErrorKind, Read};
+use std::io::{self, ErrorKind, Read};
 use std::os::fd::{AsFd, AsRawFd, BorrowedFd};
 use std::time::{Duration, Instant};
 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 super::securemem::PamBuffer;
 
@@ -93,7 +94,7 @@ fn read_unbuffered(
     source: &mut dyn io::Read,
     sink: &mut dyn io::Write,
     hide_input: &Hidden<HiddenInput>,
-) -> io::Result<PamBuffer> {
+) -> PamResult<PamBuffer> {
     struct ExitGuard<'a> {
         pw_len: usize,
         feedback: bool,
@@ -121,15 +122,17 @@ fn read_unbuffered(
     // with the amount of asterisks on the terminal (both tracked in `pw_len`)
     #[allow(clippy::unbuffered_bytes)]
     for read_byte in source.bytes() {
-        let read_byte = read_byte?;
+        let read_byte = read_byte.map_err(|err| match err {
+            err if err.kind() == io::ErrorKind::TimedOut => PamError::TimedOut,
+            err => PamError::IoError(err),
+        })?;
 
         if read_byte == b'\n' || read_byte == b'\r' {
-            break;
+            return Ok(password);
         }
 
         if let Hidden::Yes(input) | Hidden::WithFeedback(input) = hide_input {
             if read_byte == input.term_orig.c_cc[VEOF] {
-                password.fill(0);
                 break;
             }
 
@@ -161,14 +164,17 @@ fn read_unbuffered(
                 let _ = state.sink.write(b"*");
             }
         } else {
-            return Err(Error::new(
-                ErrorKind::OutOfMemory,
-                "incorrect password attempt",
-            ));
+            return Err(PamError::IncorrectPasswordAttempt);
         }
     }
 
-    Ok(password)
+    if state.pw_len == 0 {
+        // In case of EOF or Ctrl-D we don't want to ask for a password a second
+        // time, so return an error.
+        Err(PamError::NoPasswordProvided)
+    } else {
+        Ok(password)
+    }
 }
 
 /// Write something and immediately flush
@@ -251,14 +257,15 @@ pub enum Terminal<'a> {
 
 impl Terminal<'_> {
     /// Open the current TTY for user communication
-    pub fn open_tty() -> io::Result<Self> {
+    pub fn open_tty() -> PamResult<Self> {
         // 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 +280,7 @@ impl Terminal<'_> {
         prompt: &str,
         timeout: Option<Duration>,
         hidden: Hidden<()>,
-    ) -> io::Result<PamBuffer> {
+    ) -> PamResult<PamBuffer> {
         fn do_hide_input(
             hidden: Hidden<()>,
             input: BorrowedFd,

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

@@ -98,7 +98,7 @@ fn required_password_is_target_users_fail() {
     let diagnostic = if sudo_test::is_original_sudo() {
         "Authentication failure"
     } else {
-        "Maximum 3 incorrect authentication attempts"
+        "Authentication failed, try again."
     };
     assert_contains!(output.stderr(), diagnostic);
 }
@@ -131,7 +131,7 @@ fn password_is_required_when_target_user_is_self() {
     let diagnostic = if sudo_test::is_original_sudo() {
         "Authentication failure"
     } else {
-        "Maximum 3 incorrect authentication attempts"
+        "Authentication required but not attempted"
     };
     assert_contains!(output.stderr(), diagnostic);
 }

test-framework/sudo-compliance-tests/src/su/syslog.rs

@@ -85,19 +85,16 @@ fn logs_every_failed_authentication_attempt() {
 
     eprintln!("\n--- /var/log/auth.log ---\n{auth_log}\n--- /var/log/auth.log ---\n");
 
-    if sudo_test::is_original_sudo() {
-        assert_contains!(
-            auth_log,
-            format!("su: pam_unix(su:auth): auth could not identify password for [{target_user}]")
-        );
+    assert_contains!(
+        auth_log,
+        format!("su: pam_unix(su:auth): auth could not identify password for [{target_user}]")
+    );
 
+    if sudo_test::is_original_sudo() {
         let tty = "none";
         assert_contains!(
             auth_log,
             format!("FAILED SU (to {target_user}) {invoking_user} on {tty}")
         );
-    } else {
-        let tty = "";
-        assert_contains!(auth_log, format!("su: pam_unix(su:auth): authentication failure; logname= uid={invoking_userid} euid=0 tty={tty} ruser={invoking_user} rhost=  user={target_user}"));
     }
 }

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 authenticate"
     };
     assert_contains!(output.stderr(), diagnostic);
 }

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

@@ -141,9 +141,6 @@ fn v_flag_without_pwd_fails_if_nopasswd_is_not_set_for_all_users_entries() {
             );
         }
     } else {
-        assert_contains!(
-            stderr,
-            "[sudo: authenticate] Password: \nsudo: Authentication failed, try again.\n[sudo: authenticate] Password: \nsudo: Authentication failed, try again.\n[sudo: authenticate] Password: \nsudo: Maximum 3 incorrect authentication attempts"
-        );
+        assert_contains!(stderr, "Authentication required but not attempted");
     }
 }

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

@@ -34,7 +34,7 @@ fn incorrect_password() {
     let diagnostic = if sudo_test::is_original_sudo() {
         "incorrect password attempt"
     } else {
-        "incorrect authentication attempt"
+        "Authentication failed, try again."
     };
     assert_contains!(output.stderr(), diagnostic);
 }
@@ -54,7 +54,7 @@ fn no_password() {
     let diagnostic = if sudo_test::is_original_sudo() {
         "no password was provided"
     } else {
-        "incorrect authentication attempt"
+        "Authentication required but not attempted"
     };
     assert_contains!(output.stderr(), diagnostic);
 }
@@ -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 authentication 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 authentication attempt");
         }
     }
 }

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 authenticate"
     };
     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 authentication attempt"
         };
         assert_contains!(stderr, diagnostic);
     }