Propagate all error kinds from pam::converse not just timeouts

src/pam/converse.rs

@@ -129,7 +129,7 @@
 }
 
 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 {
@@ -143,7 +143,7 @@
 impl Converser for CLIConverser {
     fn handle_normal_prompt(&self, msg: &str) -> PamResult<PamBuffer> {
         let (mut tty, _guard) = self.open()?;
         Ok(tty.read_input(
             &format!("[{}: input needed] {msg} ", self.name),
             None,
             Hidden::No,
@@ -164,12 +164,9 @@
                 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 @@
     // 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,
 }
 
@@ -243,11 +240,10 @@
                 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 @@
             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

@@ -173,8 +173,10 @@ pub enum PamError {
     Utf8Error(Utf8Error),
     Pam(PamErrorType),
     IoError(std::io::Error),
+    TtyRequired,
     EnvListFailure,
     InteractionRequired,
+    IncorrectPasswordAttempt,
     TimedOut,
     InvalidUser(String, String),
 }
@@ -216,13 +218,15 @@ 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,
                     "It was not possible to get a list of environment variables"
                 )
             }
             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!(

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)]

src/pam/rpassword.rs

@@ -13,14 +13,15 @@
 /// - 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};
 
 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<HiddenInput>,
-) -> io::Result<PamBuffer> {
+) -> PamResult<PamBuffer> {
     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<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 +276,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/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);
 }

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");
         }
     }
 }

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);
     }

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);
 }

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);
 }

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);
 }

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);
 }

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);
 }

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"
+        );
     }
 }