changeset 69:8f3ae0c7ab92

Rework conversation data types and make safe wrappers. This removes the old `Conversation` type and reworks the FFI types used for PAM conversations. This creates safe `TestResponse` and `BinaryResponse` structures in `conv`, providing a safe way to pass response messages to PAM Conversations. The internals of these types are allocated on the C heap, as required by PAM. We also remove the Conversation struct, which was specific to the real PAM implementation so that we can introduce a better abstraction. Also splits a new `PamApplicationHandle` trait from `PamHandle`, for the parts of a PAM handle that are specific to the application side of a PAM transaction.
author Paul Fisher <paul@pfish.zone>
date Sun, 01 Jun 2025 01:15:04 -0400
parents e4e7d68234d0
children 9f8381a1c09c
files src/conv.rs src/handle.rs src/lib.rs src/pam_ffi.rs
diffstat 4 files changed, 391 insertions(+), 125 deletions(-) [+]
line wrap: on
line diff
--- a/src/conv.rs	Tue May 27 16:40:49 2025 -0400
+++ b/src/conv.rs	Sun Jun 01 01:15:04 2025 -0400
@@ -1,123 +1,81 @@
-//! The [Conversation] struct, for interacting with the user.
-//!
-//! This module is experimental and will probably be rewritten in the future
-//! to improve the interface for both PAM modules and clients.
+//! The PAM conversation and associated Stuff.
 
-use crate::constants::MessageStyle;
-use crate::constants::Result;
-use crate::constants::{ErrorCode, InvalidEnum};
-use crate::items::Item;
-use libc::{c_char, c_int};
-use num_derive::FromPrimitive;
-use std::ffi::{CStr, CString};
-use std::ptr;
+use crate::pam_ffi::{BinaryResponseInner, NulError, TextResponseInner};
+use std::num::TryFromIntError;
+use std::ops::Deref;
+use std::result::Result as StdResult;
 
-/// Styles of message that are shown to the user.
-#[derive(Debug, PartialEq, FromPrimitive)]
-#[non_exhaustive] // non-exhaustive because C might give us back anything!
-pub enum MessageStyle {
-    /// Requests information from the user; will be masked when typing.
-    PromptEchoOff = 1,
-    /// Requests information from the user; will not be masked.
-    PromptEchoOn = 2,
-    /// An error message.
-    ErrorMsg = 3,
-    /// An informational message.
-    TextInfo = 4,
-    /// Yes/No/Maybe conditionals. Linux-PAM specific.
-    RadioType = 5,
-    /// For server–client non-human interaction.
-    /// NOT part of the X/Open PAM specification.
-    BinaryPrompt = 7,
-}
+/// An owned text response to a PAM conversation.
+///
+/// It points to a value on the C heap.
+#[repr(C)]
+struct TextResponse(*mut TextResponseInner);
 
-impl TryFrom<c_int> for MessageStyle {
-    type Error = InvalidEnum<Self>;
-    fn try_from(value: c_int) -> std::result::Result<Self, Self::Error> {
-        Self::from_i32(value).ok_or(value.into())
+impl TextResponse {
+    /// Creates a text response.
+    pub fn new(text: impl AsRef<str>) -> StdResult<Self, NulError> {
+        TextResponseInner::alloc(text).map(Self)
     }
 }
 
-impl From<MessageStyle> for c_int {
-    fn from(val: MessageStyle) -> Self {
-        val as Self
+impl Deref for TextResponse {
+    type Target = TextResponseInner;
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: We allocated this ourselves, or it was provided by PAM.
+        unsafe { &*self.0 }
+    }
+}
+
+impl Drop for TextResponse {
+    /// Frees an owned response.
+    fn drop(&mut self) {
+        // SAFETY: We allocated this ourselves, or it was provided by PAM.
+        unsafe { TextResponseInner::free(self.0) }
     }
 }
 
-#[repr(C)]
-struct Message {
-    msg_style: MessageStyle,
-    msg: *const c_char,
-}
-
+/// An owned binary response to a PAM conversation.
+///
+/// It points to a value on the C heap.
 #[repr(C)]
-struct Response {
-    resp: *const c_char,
-    resp_retcode: libc::c_int, // Unused - always zero
-}
+struct BinaryResponse(*mut BinaryResponseInner);
 
-#[doc(hidden)]
-#[repr(C)]
-pub struct Inner {
-    conv: extern "C" fn(
-        num_msg: c_int,
-        pam_message: &&Message,
-        pam_response: &mut *const Response,
-        appdata_ptr: *const libc::c_void,
-    ) -> c_int,
-    appdata_ptr: *const libc::c_void,
+impl BinaryResponse {
+    /// Creates a binary response with the given data.
+    pub fn new(data: impl AsRef<[u8]>, data_type: u8) -> StdResult<Self, TryFromIntError> {
+        BinaryResponseInner::alloc(data, data_type).map(Self)
+    }
 }
 
-/// A communication channel with the user.
-///
-/// Use this to communicate with the user, if needed, beyond the standard
-/// things you can get/set with `get_user`/`get_authtok` and friends.
-/// The PAM client (i.e., the application that is logging in) will present
-/// the messages you send to the user and ask for responses.
-pub struct Conversation<'a>(&'a Inner);
-
-impl Conversation<'_> {
-    /// Sends a message to the PAM client.
-    ///
-    /// This will typically result in the user seeing a message or a prompt.
-    /// For details, see what [MessageStyle]s are available.
-    ///
-    /// Note that the user experience will depend on how each style
-    /// is implemented by the client, and that not all clients
-    /// will implement all message styles.
-    pub fn send(&self, style: MessageStyle, msg: &str) -> Result<Option<&CStr>> {
-        let mut resp_ptr: *const Response = ptr::null();
-        let msg_cstr = CString::new(msg).unwrap();
-        let msg = Message {
-            msg_style: style,
-            msg: msg_cstr.as_ptr(),
-        };
-        // TODO: These need to be freed!
-        let ret = (self.0.conv)(1, &&msg, &mut resp_ptr, self.0.appdata_ptr);
-        ErrorCode::result_from(ret)?;
-
-        let result = unsafe {
-            match (*resp_ptr).resp {
-                p if p.is_null() => None,
-                p => Some(CStr::from_ptr(p)),
-            }
-        };
-        Ok(result)
+impl Deref for BinaryResponse {
+    type Target = BinaryResponseInner;
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: We allocated this ourselves, or it was provided by PAM.
+        unsafe { &*self.0 }
     }
 }
 
-impl Item for Conversation<'_> {
-    type Raw = Inner;
-
-    fn type_id() -> crate::items::ItemType {
-        crate::items::ItemType::Conversation
-    }
-
-    unsafe fn from_raw(raw: *const Self::Raw) -> Self {
-        Self(&*raw)
-    }
-
-    fn into_raw(self) -> *const Self::Raw {
-        self.0 as _
+impl Drop for BinaryResponse {
+    /// Frees an owned response.
+    fn drop(&mut self) {
+        // SAFETY: We allocated this ourselves, or it was provided by PAM.
+        unsafe { BinaryResponseInner::free(self.0) }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::{BinaryResponse, TextResponse};
+
+    #[test]
+    fn test_text_response() {
+        let resp = TextResponse::new("it's a-me!").unwrap();
+        assert_eq!("it's a-me!", resp.contents().to_str().unwrap());
+    }
+    #[test]
+    fn test_binary_response() {
+        let data = [123, 210, 55];
+        let resp = BinaryResponse::new(&data, 99).unwrap();
+        assert_eq!(&data, resp.contents());
+    }
+}
--- a/src/handle.rs	Tue May 27 16:40:49 2025 -0400
+++ b/src/handle.rs	Sun Jun 01 01:15:04 2025 -0400
@@ -96,7 +96,16 @@
     /// [man]: https://www.man7.org/linux/man-pages/man3/pam_set_item.3.html
     /// [mwg]: https://www.chiark.greenend.org.uk/doc/libpam-doc/html/mwg-expected-by-module-item.html#mwg-pam_set_item
     fn set_item<T: Item>(&mut self, item: T) -> Result<()>;
+}
 
+/// Functionality of a PAM handle that can be expected by a PAM application.
+///
+/// If you are not writing a PAM client application (e.g., you are writing
+/// a module), you should not use the functionality exposed by this trait.
+///
+/// Like [`PamHandle`], this is intended to allow creating mock implementations
+/// of PAM for testing PAM applications.
+pub trait PamApplicationHandle: PamHandle {
     /// Closes the PAM session on an owned PAM handle.
     ///
     /// This should be called with the result of the application's last call
@@ -107,9 +116,9 @@
     /// See the [`pam_end` manual page][man] for more information.
     ///
     /// ```no_run
-    /// # use nonstick::PamHandle;
+    /// # use nonstick::PamApplicationHandle;
     /// # use std::error::Error;
-    /// # fn _doc(handle: impl PamHandle, auth_result: nonstick::Result<()>) -> Result<(), Box<dyn Error>> {
+    /// # fn _doc(handle: impl PamApplicationHandle, auth_result: nonstick::Result<()>) -> Result<(), Box<dyn Error>> {
     /// // Earlier: authentication was performed and the result was stored
     /// // into auth_result.
     /// handle.close(auth_result)?;
@@ -182,6 +191,8 @@
 
 impl Drop for LibPamHandle {
     /// Ends the PAM session with a zero error code.
+    /// You probably want to call [`close`](Self::close) instead of
+    /// letting this drop by itself.
     fn drop(&mut self) {
         unsafe {
             pam_ffi::pam_end(&mut self.0, 0);
@@ -235,7 +246,9 @@
         };
         ErrorCode::result_from(ret)
     }
+}
 
+impl PamApplicationHandle for LibPamHandle {
     fn close(mut self, status: Result<()>) -> Result<()> {
         let result = unsafe { pam_ffi::pam_end(&mut self.0, ErrorCode::result_to_c(status)) };
         // Since we've already `pam_end`ed this session, we don't want it to be
--- a/src/lib.rs	Tue May 27 16:40:49 2025 -0400
+++ b/src/lib.rs	Sun Jun 01 01:15:04 2025 -0400
@@ -35,6 +35,6 @@
 #[doc(inline)]
 pub use crate::{
     constants::{ErrorCode, Flags, Result},
-    handle::{LibPamHandle, PamHandle, PamModuleHandle},
+    handle::{LibPamHandle, PamApplicationHandle, PamHandle, PamModuleHandle},
     module::PamModule,
 };
--- a/src/pam_ffi.rs	Tue May 27 16:40:49 2025 -0400
+++ b/src/pam_ffi.rs	Sun Jun 01 01:15:04 2025 -0400
@@ -1,14 +1,282 @@
-//! FFI to the PAM library.
+//! The PAM library FFI and helpers for managing it.
+//!
+//! This includes the functions provided by PAM and the data structures
+//! used by PAM, as well as a few low-level abstractions for dealing with
+//! those data structures.
+//!
+//! Everything in here is hazmat.
+
+// Temporarily allow dead code.
+#![allow(dead_code)]
 
-use libc::c_char;
-use std::ffi::c_int;
+use crate::constants::InvalidEnum;
+use num_derive::FromPrimitive;
+use num_traits::FromPrimitive;
+use std::ffi::{c_char, c_int, c_void, CStr};
 use std::marker::{PhantomData, PhantomPinned};
+use std::num::TryFromIntError;
+use std::slice;
+use thiserror::Error;
+
+/// Makes whatever it's in not [`Send`], [`Sync`], or [`Unpin`].
+type Immovable = PhantomData<(*mut u8, PhantomPinned)>;
 
 /// An opaque pointer given to us by PAM.
 #[repr(C)]
 pub struct Handle {
     _data: (),
-    _marker: PhantomData<(*mut u8, PhantomPinned)>,
+    _marker: Immovable,
+}
+
+/// Styles of message that are shown to the user.
+#[derive(Debug, PartialEq, FromPrimitive)]
+#[non_exhaustive] // non-exhaustive because C might give us back anything!
+pub enum MessageStyle {
+    /// Requests information from the user; will be masked when typing.
+    PromptEchoOff = 1,
+    /// Requests information from the user; will not be masked.
+    PromptEchoOn = 2,
+    /// An error message.
+    ErrorMsg = 3,
+    /// An informational message.
+    TextInfo = 4,
+    /// Yes/No/Maybe conditionals. Linux-PAM specific.
+    RadioType = 5,
+    /// For server–client non-human interaction.
+    /// NOT part of the X/Open PAM specification.
+    BinaryPrompt = 7,
+}
+
+impl TryFrom<c_int> for MessageStyle {
+    type Error = InvalidEnum<Self>;
+    fn try_from(value: c_int) -> std::result::Result<Self, Self::Error> {
+        Self::from_i32(value).ok_or(value.into())
+    }
+}
+
+impl From<MessageStyle> for c_int {
+    fn from(val: MessageStyle) -> Self {
+        val as Self
+    }
+}
+
+/// A message sent by PAM or a module to an application.
+/// This message, and its internal data, is owned by the creator
+/// (either the module or PAM itself).
+#[repr(C)]
+pub struct Message {
+    /// The style of message to request.
+    style: c_int,
+    /// A description of the data requested.
+    /// For most requests, this will be an owned [`CStr`], but for requests
+    /// with [`MessageStyle::BinaryPrompt`], this will be [`BinaryData`].
+    data: *const c_void,
+}
+
+/// Returned when text that should not have any `\0` bytes in it does.
+/// Analogous to [`std::ffi::NulError`], but the data it was created from
+/// is borrowed.
+#[derive(Debug, Error)]
+#[error("null byte within input at byte {0}")]
+pub struct NulError(usize);
+
+#[repr(transparent)]
+pub struct TextResponseInner(ResponseInner);
+
+impl TextResponseInner {
+    /// Allocates a new text response on the C heap.
+    ///
+    /// Both `self` and its internal pointer are located on the C heap.
+    /// You are responsible for calling [`free`](Self::free)
+    /// on the pointer you get back when you're done with it.
+    pub fn alloc(text: impl AsRef<str>) -> Result<*mut Self, NulError> {
+        let str_data = Self::malloc_str(text)?;
+        let inner = ResponseInner::alloc(str_data);
+        Ok(inner as *mut Self)
+    }
+
+    /// Gets the string stored in this response.
+    pub fn contents(&self) -> &CStr {
+        // SAFETY: This data is either passed from PAM (so we are forced to
+        // trust it) or was created by us in TextResponseInner::alloc.
+        // In either case, it's going to be a valid null-terminated string.
+        unsafe { CStr::from_ptr(self.0.data as *const c_char) }
+    }
+
+    /// Releases memory owned by this response.
+    ///
+    /// # Safety
+    ///
+    /// You are responsible for no longer using this after calling free.
+    pub unsafe fn free(me: *mut Self) {
+        ResponseInner::free(me as *mut ResponseInner)
+    }
+
+    /// Allocates a string with the given contents on the C heap.
+    ///
+    /// This is like [`CString::new`](std::ffi::CString::new), but:
+    ///
+    /// - it allocates data on the C heap with [`libc::malloc`].
+    /// - it doesn't take ownership of the data passed in.
+    fn malloc_str(text: impl AsRef<str>) -> Result<*const c_void, NulError> {
+        let data = text.as_ref().as_bytes();
+        if let Some(nul) = data.iter().position(|x| *x == 0) {
+            return Err(NulError(nul));
+        }
+        unsafe {
+            let data_alloc = libc::calloc(data.len() + 1, 1);
+            libc::memcpy(data_alloc, data.as_ptr() as *const c_void, data.len());
+            Ok(data_alloc as *const c_void)
+        }
+    }
+}
+
+/// A [`ResponseInner`] with [`BinaryData`] in it.
+#[repr(transparent)]
+pub struct BinaryResponseInner(ResponseInner);
+
+impl BinaryResponseInner {
+    /// Allocates a new binary response on the C heap.
+    ///
+    /// The `data_type` is a tag you can use for whatever.
+    /// It is passed through PAM unchanged.
+    ///
+    /// The referenced data is copied to the C heap. We do not take ownership.
+    /// You are responsible for calling [`free`](Self::free)
+    /// on the pointer you get back when you're done with it.
+    pub fn alloc(data: impl AsRef<[u8]>, data_type: u8) -> Result<*mut Self, TryFromIntError> {
+        let bin_data = BinaryData::alloc(data, data_type)?;
+        let inner = ResponseInner::alloc(bin_data as *const c_void);
+        Ok(inner as *mut Self)
+    }
+
+    /// Gets the binary data in this response.
+    pub fn contents(&self) -> &[u8] {
+        let data = self.data();
+        let length = (u32::from_be_bytes(data.total_length) - 5) as usize;
+        unsafe { slice::from_raw_parts(data.data.as_ptr(), length) }
+    }
+
+    /// Gets the `data_type` tag that was embedded with the message.
+    pub fn data_type(&self) -> u8 {
+        self.data().data_type
+    }
+
+    #[inline]
+    fn data(&self) -> &BinaryData {
+        // SAFETY: This was either something we got from PAM (in which case
+        // we trust it), or something that was created with
+        // BinaryResponseInner::alloc. In both cases, it points to valid data.
+        unsafe { &*(self.0.data as *const BinaryData) }
+    }
+
+    /// Releases memory owned by this response.
+    ///
+    /// # Safety
+    ///
+    /// You are responsible for not using this after calling free.
+    pub unsafe fn free(me: *mut Self) {
+        ResponseInner::free(me as *mut ResponseInner)
+    }
+}
+
+#[repr(C)]
+pub struct ResponseInner {
+    /// Pointer to the data returned in a response.
+    /// For most responses, this will be a [`CStr`], but for responses to
+    /// [`MessageStyle::BinaryPrompt`]s, this will be [`BinaryData`]
+    data: *const c_void,
+    /// Unused.
+    return_code: c_int,
+}
+
+impl ResponseInner {
+    /// Allocates a response on the C heap pointing to the given data.
+    fn alloc(data: *const c_void) -> *mut Self {
+        unsafe {
+            let alloc = libc::calloc(1, size_of::<ResponseInner>()) as *mut ResponseInner;
+            (*alloc).data = data;
+            alloc
+        }
+    }
+
+    /// Frees one of these that was created with [`Self::alloc`]
+    /// (or provided by PAM).
+    ///
+    /// # Safety
+    ///
+    /// It's up to you to stop using `me` after calling this.
+    unsafe fn free(me: *mut Self) {
+        libc::free((*me).data as *mut c_void);
+        libc::free(me as *mut c_void)
+    }
+}
+
+/// Binary data used in requests and responses.
+///
+/// This is an unsized data type whose memory goes beyond its data.
+/// This must be allocated on the C heap.
+#[repr(C)]
+struct BinaryData {
+    /// The total length of the structure; a u32 in network byte order (BE).
+    total_length: [u8; 4],
+    /// A tag of undefined meaning.
+    data_type: u8,
+    /// Pointer to an array of length [`length`](Self::length) − 5
+    data: [u8; 0],
+    _marker: Immovable,
+}
+
+impl BinaryData {
+    fn alloc(
+        source: impl AsRef<[u8]>,
+        data_type: u8,
+    ) -> Result<*const BinaryData, TryFromIntError> {
+        let source = source.as_ref();
+        let buffer_size = u32::try_from(source.len() + 5)?;
+        let data = unsafe {
+            let dest_buffer = libc::malloc(buffer_size as usize) as *mut BinaryData;
+            let data = &mut *dest_buffer;
+            data.total_length = buffer_size.to_be_bytes();
+            data.data_type = data_type;
+            let dest = data.data.as_mut_ptr();
+            libc::memcpy(
+                dest as *mut c_void,
+                source.as_ptr() as *const c_void,
+                source.len(),
+            );
+            dest_buffer
+        };
+        Ok(data)
+    }
+}
+
+/// An opaque pointer we provide to PAM for callbacks.
+#[repr(C)]
+pub struct AppData {
+    _data: (),
+    _marker: Immovable,
+}
+
+/// The callback that PAM uses to get information in a conversation.
+///
+/// - `num_msg` is the number of messages in the `pam_message` array.
+/// - `messages` is a pointer to an array of pointers to [`Message`]s.
+/// - `responses` is a pointer to an array of [`ResponseInner`]s,
+///   which PAM sets in response to a module's request.
+/// - `appdata` is the `appdata` field of the [`Conversation`] we were passed.
+pub type ConversationCallback = extern "C" fn(
+    num_msg: c_int,
+    messages: *const *const Message,
+    responses: &mut *const ResponseInner,
+    appdata: *const AppData,
+) -> c_int;
+
+/// A callback and the associated [`AppData`] pointer that needs to be passed back to it.
+#[repr(C)]
+pub struct Conversation {
+    callback: ConversationCallback,
+    appdata: *const AppData,
 }
 
 #[link(name = "pam")]
@@ -16,27 +284,19 @@
     pub fn pam_get_data(
         pamh: *const Handle,
         module_data_name: *const c_char,
-        data: &mut *const libc::c_void,
+        data: &mut *const c_void,
     ) -> c_int;
 
     pub fn pam_set_data(
         pamh: *mut Handle,
         module_data_name: *const c_char,
-        data: *const libc::c_void,
-        cleanup: extern "C" fn(
-            pamh: *const libc::c_void,
-            data: *mut libc::c_void,
-            error_status: c_int,
-        ),
+        data: *const c_void,
+        cleanup: extern "C" fn(pamh: *const c_void, data: *mut c_void, error_status: c_int),
     ) -> c_int;
 
-    pub fn pam_get_item(
-        pamh: *const Handle,
-        item_type: c_int,
-        item: &mut *const libc::c_void,
-    ) -> c_int;
+    pub fn pam_get_item(pamh: *const Handle, item_type: c_int, item: &mut *const c_void) -> c_int;
 
-    pub fn pam_set_item(pamh: *mut Handle, item_type: c_int, item: *const libc::c_void) -> c_int;
+    pub fn pam_set_item(pamh: *mut Handle, item_type: c_int, item: *const c_void) -> c_int;
 
     pub fn pam_get_user(
         pamh: *const Handle,
@@ -53,3 +313,38 @@
 
     pub fn pam_end(pamh: *mut Handle, status: c_int) -> c_int;
 }
+
+#[cfg(test)]
+mod test {
+    use super::{BinaryResponseInner, TextResponseInner};
+
+    #[test]
+    fn test_text_response() {
+        let resp = TextResponseInner::alloc("hello").expect("alloc should succeed");
+        let borrow_resp = unsafe { &*resp };
+        let data = borrow_resp.contents().to_str().expect("valid");
+        assert_eq!("hello", data);
+        unsafe {
+            TextResponseInner::free(resp);
+        }
+        TextResponseInner::alloc("hell\0o").expect_err("should error; contains nul");
+    }
+
+    #[test]
+    fn test_binary_response() {
+        let real_data = [1, 2, 3, 4, 5, 6, 7, 8];
+        let resp = BinaryResponseInner::alloc(&real_data, 7).expect("alloc should succeed");
+        let borrow_resp = unsafe { &*resp };
+        let data = borrow_resp.contents();
+        assert_eq!(&real_data, data);
+        assert_eq!(7, borrow_resp.data_type());
+        unsafe { BinaryResponseInner::free(resp) };
+    }
+
+    #[test]
+    #[ignore]
+    fn test_binary_response_too_big() {
+        let big_data: Vec<u8> = vec![0xFFu8; 10_000_000_000];
+        BinaryResponseInner::alloc(&big_data, 0).expect_err("this is too big!");
+    }
+}