changeset 101:94b51fa4f797

Fix memory soundness issues: - Ensure Questions are pinned in memory when sending them through PAM. - Hold on to the PAM conversation struct after we build it. (Linux-PAM is leninent about this and copies the pam_conv structure.)
author Paul Fisher <paul@pfish.zone>
date Tue, 24 Jun 2025 17:54:33 -0400
parents 3f11b8d30f63
children 94eb11cb1798
files src/libpam/answer.rs src/libpam/conversation.rs src/libpam/handle.rs src/libpam/pam_ffi.rs src/libpam/question.rs
diffstat 5 files changed, 32 insertions(+), 27 deletions(-) [+]
line wrap: on
line diff
--- a/src/libpam/answer.rs	Tue Jun 24 17:08:01 2025 -0400
+++ b/src/libpam/answer.rs	Tue Jun 24 17:54:33 2025 -0400
@@ -6,10 +6,10 @@
 pub use crate::libpam::pam_ffi::Answer;
 use crate::{ErrorCode, Result};
 use std::ffi::CStr;
+use std::mem::ManuallyDrop;
 use std::ops::{Deref, DerefMut};
 use std::ptr::NonNull;
-use std::{iter, mem, ptr, slice};
-use std::mem::ManuallyDrop;
+use std::{iter, ptr, slice};
 
 /// The corridor via which the answer to Messages navigate through PAM.
 #[derive(Debug)]
--- a/src/libpam/conversation.rs	Tue Jun 24 17:08:01 2025 -0400
+++ b/src/libpam/conversation.rs	Tue Jun 24 17:54:33 2025 -0400
@@ -2,7 +2,7 @@
 use crate::conv::{Conversation, ErrorMsg, InfoMsg, MaskedQAndA, Message, QAndA};
 use crate::libpam::answer::BinaryAnswer;
 use crate::libpam::answer::{Answer, Answers, TextAnswer};
-use crate::libpam::memory::{CBinaryData, Immovable};
+use crate::libpam::memory::CBinaryData;
 use crate::libpam::pam_ffi::AppData;
 pub use crate::libpam::pam_ffi::LibPamConversation;
 use crate::libpam::question::QuestionsTrait;
@@ -21,7 +21,6 @@
             callback: Self::wrapper_callback::<C>,
             appdata: (conv as *const C).cast(),
             life: PhantomData,
-            _marker: Immovable(PhantomData),
         }
     }
 
@@ -66,13 +65,13 @@
 impl Conversation for LibPamConversation<'_> {
     fn communicate(&self, messages: &[Message]) {
         let internal = || {
-            let questions = Questions::new(messages)?;
+            let questions = Box::pin(Questions::new(messages)?);
             let mut response_pointer = std::ptr::null_mut();
             // SAFETY: We're calling into PAM with valid everything.
             let result = unsafe {
                 (self.callback)(
                     messages.len() as c_int,
-                    questions.ptr(),
+                    questions.as_ref().ptr(),
                     &mut response_pointer,
                     self.appdata,
                 )
--- a/src/libpam/handle.rs	Tue Jun 24 17:08:01 2025 -0400
+++ b/src/libpam/handle.rs	Tue Jun 24 17:54:33 2025 -0400
@@ -11,7 +11,6 @@
 use num_enum::{IntoPrimitive, TryFromPrimitive};
 use std::cell::Cell;
 use std::ffi::{c_char, c_int, CString};
-use std::marker::PhantomData;
 use std::ops::{Deref, DerefMut};
 use std::ptr;
 
@@ -33,9 +32,12 @@
 
 /// An owned PAM handle.
 pub struct OwnedLibPamHandle<'a> {
+    /// The handle itself.
     handle: HandleWrap,
+    /// The last return value from the handle.
     last_return: Cell<Result<()>>,
-    _conversation_lifetime: PhantomData<&'a mut ()>,
+    /// If set, the Conversation that this PAM handle owns.
+    conversation: Option<Box<LibPamConversation<'a>>>,
 }
 
 #[derive(Debug, PartialEq)]
@@ -77,20 +79,26 @@
         username: Option<String>,
         conversation: &impl Conversation,
     ) -> Result<Self> {
-        let conv = LibPamConversation::wrap(conversation);
+        let conv = Box::new(LibPamConversation::wrap(conversation));
         let service_cstr = CString::new(service_name).map_err(|_| ErrorCode::ConversationError)?;
         let username_cstr = memory::prompt_ptr(memory::option_cstr(username.as_deref())?.as_ref());
 
         let mut handle: *mut LibPamHandle = ptr::null_mut();
         // SAFETY: We've set everything up properly to call `pam_start`.
         // The returned value will be a valid pointer provided the result is OK.
-        let result =
-            unsafe { pam_ffi::pam_start(service_cstr.as_ptr(), username_cstr, &conv, &mut handle) };
+        let result = unsafe {
+            pam_ffi::pam_start(
+                service_cstr.as_ptr(),
+                username_cstr,
+                conv.as_ref(),
+                &mut handle,
+            )
+        };
         ErrorCode::result_from(result)?;
         Ok(Self {
             handle: HandleWrap(handle),
             last_return: Cell::new(Ok(())),
-            _conversation_lifetime: Default::default(),
+            conversation: Some(conv),
         })
     }
 }
--- a/src/libpam/pam_ffi.rs	Tue Jun 24 17:08:01 2025 -0400
+++ b/src/libpam/pam_ffi.rs	Tue Jun 24 17:54:33 2025 -0400
@@ -57,7 +57,6 @@
     /// but for requests with style `PAM_BINARY_PROMPT`,
     /// this will be `CBinaryData` (a Linux-PAM extension).
     pub data: Option<CHeapBox<c_void>>,
-    pub _marker: Immovable,
 }
 
 /// The callback that PAM uses to get information in a conversation.
@@ -86,8 +85,9 @@
     /// The pointer that will be passed as the last parameter
     /// to the conversation callback.
     pub appdata: *const AppData,
+    /// Marker to associate the lifetime of this with the conversation
+    /// that was passed in.
     pub life: PhantomData<&'a mut ()>,
-    pub _marker: Immovable,
 }
 
 /// Gets a string version of an error message.
--- a/src/libpam/question.rs	Tue Jun 24 17:08:01 2025 -0400
+++ b/src/libpam/question.rs	Tue Jun 24 17:54:33 2025 -0400
@@ -11,6 +11,7 @@
 use crate::Result;
 use num_enum::{IntoPrimitive, TryFromPrimitive};
 use std::ffi::{c_void, CStr};
+use std::pin::Pin;
 use std::slice;
 
 /// Abstraction of a collection of questions to be sent in a PAM conversation.
@@ -64,7 +65,7 @@
         Self: Sized;
 
     /// Gets the pointer that is passed .
-    fn ptr(&self) -> *const *const Question;
+    fn ptr(self: Pin<&Self>) -> *const *const Question;
 
     /// Converts a pointer into a borrowed list of Questions.
     ///
@@ -115,7 +116,7 @@
         })
     }
 
-    fn ptr(&self) -> *const *const Question {
+    fn ptr(self: Pin<&Self>) -> *const *const Question {
         &self.pointer as *const *const Question
     }
 
@@ -131,10 +132,9 @@
 #[derive(Debug)]
 #[repr(C)]
 pub struct LinuxPamQuestions {
-    #[allow(clippy::vec_box)] // we need to do this.
+    #[allow(clippy::vec_box)] // we need to box vec items.
     /// The place where the questions are.
     questions: Vec<Box<Question>>,
-    _marker: Immovable,
 }
 
 impl LinuxPamQuestions {
@@ -155,11 +155,10 @@
             .collect();
         Ok(Self {
             questions: questions?,
-            _marker: Default::default(),
         })
     }
 
-    fn ptr(&self) -> *const *const Question {
+    fn ptr(self: Pin<&Self>) -> *const *const Question {
         self.questions.as_ptr().cast()
     }
 
@@ -246,7 +245,6 @@
         Ok(Self {
             style: style.into(),
             data: Some(data),
-            _marker: Default::default(),
         })
     }
 }
@@ -328,15 +326,15 @@
             use super::super::*;
             #[test]
             fn standard() {
-                let interrogation = <$typ>::new(&[
+                let interrogation = Box::pin(<$typ>::new(&[
                     MaskedQAndA::new("hocus pocus").message(),
                     QAndA::new("what").message(),
                     QAndA::new("who").message(),
                     InfoMsg::new("hey").message(),
                     ErrorMsg::new("gasp").message(),
                 ])
-                .unwrap();
-                let indirect = interrogation.ptr();
+                .unwrap());
+                let indirect = interrogation.as_ref().ptr();
 
                 let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) };
                 let messages: Vec<OwnedMessage> = remade
@@ -364,11 +362,11 @@
             #[test]
             #[cfg(feature = "linux-pam-extensions")]
             fn linux_extensions() {
-                let interrogation = <$typ>::new(&[
+                let interrogation = Box::pin(<$typ>::new(&[
                     BinaryQAndA::new((&[5, 4, 3, 2, 1], 66)).message(),
                     RadioQAndA::new("you must choose").message(),
-                ]).unwrap();
-                let indirect = interrogation.ptr();
+                ]).unwrap());
+                let indirect = interrogation.as_ref().ptr();
 
                 let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) };
                 let messages: Vec<OwnedMessage> = remade