# HG changeset patch # User Paul Fisher # Date 1746312085 14400 # Node ID 9d1160b02d2c44cef1d4fa61fed83d094ff13ea5 # Parent 171fb117105383b6b92418be9138a81ab4a0b557 Safety and doc fixes: - Don't panic when given a string with a null character; instead return `PAM_CONV_ERR`. - Improve pattern matching and use ?s where appropriate. - Format etc. diff -r 171fb1171053 -r 9d1160b02d2c src/conv.rs --- a/src/conv.rs Wed Apr 16 16:55:59 2025 -0400 +++ b/src/conv.rs Sat May 03 18:41:25 2025 -0400 @@ -19,11 +19,6 @@ resp_retcode: libc::c_int, // Unused - always zero } -/// `PamConv` acts as a channel for communicating with user. -/// -/// Communication is mediated by the pam client (the application that invoked -/// pam). Messages sent will be relayed to the user by the client, and response -/// will be relayed back. #[repr(C)] pub struct Inner { conv: extern "C" fn( @@ -35,6 +30,11 @@ appdata_ptr: *const libc::c_void, } +/// A `Conv`ersation channel with the user. +/// +/// Communication is mediated by the PAM client (the application that invoked +/// pam). Messages sent will be relayed to the user by the client, and response +/// will be relayed back. pub struct Conv<'a>(&'a Inner); impl Conv<'_> { diff -r 171fb1171053 -r 9d1160b02d2c src/items.rs --- a/src/items.rs Wed Apr 16 16:55:59 2025 -0400 +++ b/src/items.rs Sat May 03 18:41:25 2025 -0400 @@ -1,3 +1,5 @@ +use std::ffi::CStr; + #[repr(u32)] pub enum ItemType { /// The service name @@ -36,11 +38,11 @@ /// The `ItemType` for this type fn type_id() -> ItemType; - /// The function to convert from the pointer to the C-representation to this safer wrapper type + /// The function to convert from the pointer to the C-representation to this safer wrapper type. /// /// # Safety /// - /// This function can assume the pointer is a valid pointer to a `Self::Raw` instance. + /// This function assumes the pointer is a valid pointer to a `Self::Raw` instance. unsafe fn from_raw(raw: *const Self::Raw) -> Self; /// The function to convert from this wrapper type to a C-compatible pointer. @@ -49,11 +51,12 @@ macro_rules! cstr_item { ($name:ident) => { + ///A `CStr`-based item from a PAM conversation. #[derive(Debug)] - pub struct $name<'s>(pub &'s std::ffi::CStr); + pub struct $name<'s>(pub &'s CStr); impl<'s> std::ops::Deref for $name<'s> { - type Target = &'s std::ffi::CStr; + type Target = &'s CStr; fn deref(&self) -> &Self::Target { &self.0 } diff -r 171fb1171053 -r 9d1160b02d2c src/module.rs --- a/src/module.rs Wed Apr 16 16:55:59 2025 -0400 +++ b/src/module.rs Sat May 03 18:41:25 2025 -0400 @@ -1,11 +1,10 @@ //! Functions for use in pam modules. +use crate::constants::{PamFlag, PamResultCode}; +use crate::items::{Item, ItemType}; use libc::c_char; use std::ffi::{CStr, CString}; -use crate::constants::{PamFlag, PamResultCode}; -use crate::items::ItemType; - /// Opaque type, used as a pointer when making pam API calls. /// /// A module is invoked via an external function such as `pam_sm_authenticate`. @@ -85,21 +84,23 @@ /// /// The data stored under the provided key must be of type `T` otherwise the /// behaviour of this function is undefined. - pub unsafe fn get_data(&self, key: &str) -> PamResult<&T> { - let c_key = CString::new(key).unwrap(); + /// + /// The data, if present, is owned by the current PAM conversation. + pub unsafe fn get_data(&self, key: &str) -> PamResult> { + let c_key = CString::new(key).map_err(|_| PamResultCode::PAM_CONV_ERR)?; let mut ptr: *const libc::c_void = std::ptr::null(); - let res = pam_get_data(self, c_key.as_ptr(), &mut ptr); - if PamResultCode::PAM_SUCCESS == res && !ptr.is_null() { - let typed_ptr = ptr.cast::(); - let data: &T = &*typed_ptr; - Ok(data) - } else { - Err(res) + to_result(pam_get_data(self, c_key.as_ptr(), &mut ptr))?; + match ptr.is_null() { + true => Ok(None), + false => { + let typed_ptr = ptr.cast::(); + Ok(Some(&*typed_ptr)) + } } } - /// Stores a value that can be retrieved later with `get_data`. The value lives - /// as long as the current pam cycle. + /// Stores a value that can be retrieved later with `get_data`. + /// The conversation takes ownership of the data. /// /// See the [`pam_set_data` manual page]( /// https://www.man7.org/linux/man-pages/man3/pam_set_data.3.html). @@ -107,8 +108,8 @@ /// # Errors /// /// Returns an error if the underlying PAM function call fails. - pub fn set_data(&self, key: &str, data: Box) -> PamResult<()> { - let c_key = CString::new(key).unwrap(); + pub fn set_data(&mut self, key: &str, data: Box) -> PamResult<()> { + let c_key = CString::new(key).map_err(|_| PamResultCode::PAM_CONV_ERR)?; let res = unsafe { pam_set_data( self, @@ -120,8 +121,11 @@ to_result(res) } - /// Retrieves a value that has been set, possibly by the pam client. This is - /// particularly useful for getting a `PamConv` reference. + /// Retrieves a value that has been set, possibly by the pam client. + /// This is particularly useful for getting a `PamConv` reference. + /// + /// These items are *references to PAM memory* + /// which are *owned by the conversation*. /// /// See the [`pam_get_item` manual page]( /// https://www.man7.org/linux/man-pages/man3/pam_get_item.3.html). @@ -131,26 +135,19 @@ /// Returns an error if the underlying PAM function call fails. pub fn get_item(&self) -> PamResult> { let mut ptr: *const libc::c_void = std::ptr::null(); - let (res, item) = unsafe { + let out = unsafe { let r = pam_get_item(self, T::type_id(), &mut ptr); + to_result(r)?; let typed_ptr = ptr.cast::(); - let t = if typed_ptr.is_null() { - None - } else { - Some(T::from_raw(typed_ptr)) - }; - (r, t) + match typed_ptr.is_null() { + true => None, + false => Some(T::from_raw(typed_ptr)), + } }; - match res { - PamResultCode::PAM_SUCCESS => Ok(item), - other => Err(other), - } + Ok(out) } - /// Sets a value in the pam context. The value can be retrieved using - /// `get_item`. - /// - /// Note that all items are strings, except `PAM_CONV` and `PAM_FAIL_DELAY`. + /// Sets an item in the pam context. It can be retrieved using `get_item`. /// /// See the [`pam_set_item` manual page]( /// https://www.man7.org/linux/man-pages/man3/pam_set_item.3.html). @@ -158,11 +155,7 @@ /// # Errors /// /// Returns an error if the underlying PAM function call fails. - /// - /// # Panics - /// - /// Panics if the provided item key contains a nul byte. - pub fn set_item_str(&mut self, item: T) -> PamResult<()> { + pub fn set_item(&mut self, item: T) -> PamResult<()> { let res = unsafe { pam_set_item(self, T::type_id(), item.into_raw().cast::()) }; to_result(res) @@ -178,21 +171,10 @@ /// # Errors /// /// Returns an error if the underlying PAM function call fails. - /// - /// # Panics - /// - /// Panics if the provided prompt string contains a nul byte. pub fn get_user(&self, prompt: Option<&str>) -> PamResult { - let prompt_string; - let c_prompt = match prompt { - Some(p) => { - prompt_string = CString::new(p).unwrap(); - prompt_string.as_ptr() - } - None => std::ptr::null(), - }; + let prompt = option_cstr(prompt)?; let output: *mut c_char = std::ptr::null_mut(); - let res = unsafe { pam_get_user(self, &output, c_prompt) }; + let res = unsafe { pam_get_user(self, &output, prompt_ptr(prompt.as_ref())) }; match res { PamResultCode::PAM_SUCCESS => copy_pam_string(output), otherwise => Err(otherwise), @@ -209,25 +191,35 @@ /// # Errors /// /// Returns an error if the underlying PAM function call fails. - /// - /// # Panics - /// - /// Panics if the provided prompt string contains a nul byte. pub fn get_authtok(&self, prompt: Option<&str>) -> PamResult { - let prompt_string; - let c_prompt = match prompt { - Some(p) => { - prompt_string = CString::new(p).unwrap(); - prompt_string.as_ptr() - } - None => std::ptr::null(), + let prompt = option_cstr(prompt)?; + let output: *mut c_char = std::ptr::null_mut(); + let res = unsafe { + pam_get_authtok( + self, + ItemType::AuthTok, + &output, + prompt_ptr(prompt.as_ref()), + ) }; - let output: *mut c_char = std::ptr::null_mut(); - let res = unsafe { pam_get_authtok(self, ItemType::AuthTok, &output, c_prompt) }; - match res { - PamResultCode::PAM_SUCCESS => copy_pam_string(output), - otherwise => Err(otherwise), - } + to_result(res)?; + copy_pam_string(output) + } +} + +/// Safely converts a `&str` option to a `CString` option. +fn option_cstr(prompt: Option<&str>) -> PamResult> { + prompt + .map(CString::new) + .transpose() + .map_err(|_| PamResultCode::PAM_CONV_ERR) +} + +/// The pointer to the prompt CString, or null if absent. +fn prompt_ptr(prompt: Option<&CString>) -> *const c_char { + match prompt { + Some(c_str) => c_str.as_ptr(), + None => std::ptr::null(), } } @@ -236,10 +228,13 @@ fn copy_pam_string(result_ptr: *const c_char) -> PamResult { // We really shouldn't get a null pointer back here, but if we do, return nothing. if result_ptr.is_null() { - return Ok(String::from("")); + return Ok(String::new()); } - let bytes = unsafe { CStr::from_ptr(result_ptr).to_bytes() }; - String::from_utf8(bytes.to_vec()).map_err(|_| PamResultCode::PAM_CONV_ERR) + let bytes = unsafe { CStr::from_ptr(result_ptr) }; + Ok(bytes + .to_str() + .map_err(|_| PamResultCode::PAM_CONV_ERR)? + .into()) } /// Convenience to transform a `PamResultCode` into a unit `PamResult`.