[WIP] Error fixes

Author: bjorn3

src/pam/converse.rs

@@ -128,7 +128,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 {
@@ -157,12 +157,9 @@ impl Converser for CLIConverser {
         } else {
             tty.read_password(self.password_timeout)
         }
-        .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,
         })
     }
 

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/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;
+use crate::pam::{PamError, PamResult};
 use crate::system::time::Duration;
 
 use super::securemem::PamBuffer;
@@ -100,7 +101,7 @@ fn read_unbuffered(
     source: &mut dyn io::Read,
     sink: &mut dyn io::Write,
     hide_input: Hidden<'_>,
-) -> io::Result<PamBuffer> {
+) -> PamResult<PamBuffer> {
     struct ExitGuard<'a> {
         pw_len: usize,
         feedback: bool,
@@ -137,6 +138,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;
             }
@@ -169,10 +174,7 @@ fn read_unbuffered(
                 let _ = state.sink.write(b"*");
             }
         } else {
-            return Err(Error::new(
-                ErrorKind::OutOfMemory,
-                "incorrect password attempt",
-            ));
+            return Err(PamError::IncorrectPasswordAttempt);
         }
     }
 
@@ -259,12 +261,13 @@ 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> {
         Ok(Terminal::Tty(
             fs::OpenOptions::new()
                 .read(true)
                 .write(true)
-                .open("/dev/tty")?,
+                .open("/dev/tty")
+                .map_err(|_| PamError::TtyRequired)?,
         ))
     }
 
@@ -274,7 +277,7 @@ impl Terminal<'_> {
     }
 
     /// Reads input with TTY echo disabled
-    pub fn read_password(&mut self, timeout: Option<Duration>) -> io::Result<PamBuffer> {
+    pub fn read_password(&mut self, timeout: Option<Duration>) -> PamResult<PamBuffer> {
         let hide_input = HiddenInput::new()?;
         self.read_inner(
             timeout,
@@ -286,7 +289,7 @@ impl Terminal<'_> {
     pub fn read_password_with_feedback(
         &mut self,
         timeout: Option<Duration>,
-    ) -> io::Result<PamBuffer> {
+    ) -> PamResult<PamBuffer> {
         let hide_input = HiddenInput::new()?;
         self.read_inner(
             timeout,
@@ -298,15 +301,15 @@ impl Terminal<'_> {
     }
 
     /// Reads input with TTY echo enabled
-    pub fn read_cleartext(&mut self) -> io::Result<PamBuffer> {
+    pub fn read_cleartext(&mut self) -> PamResult<PamBuffer> {
         self.read_inner(None, Hidden::No)
     }
 
     fn read_inner(
         &mut self,
         timeout: Option<Duration>,
         hide_input: Hidden<'_>,
-    ) -> io::Result<PamBuffer> {
+    ) -> PamResult<PamBuffer> {
         match self {
             Terminal::StdIE(stdin, stdout) => {
                 let mut reader = TimeoutRead::new(stdin.as_fd(), timeout);

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