changeset 93:efc2b56c8928

Remove undefined behavior per MIRI. This replaces a bunch of raw pointers with NonNull and removes all the undefined behavior that we can find with MIRI. We also remove the `SecureString` dependency (since it doesn't work with MIRI, and because it's not really necessary).
author Paul Fisher <paul@pfish.zone>
date Mon, 23 Jun 2025 13:02:58 -0400
parents 5ddbcada30f2
children db167f96ba46
files Cargo.toml src/conv.rs src/libpam/answer.rs src/libpam/conversation.rs src/libpam/memory.rs src/libpam/question.rs
diffstat 6 files changed, 91 insertions(+), 92 deletions(-) [+]
line wrap: on
line diff
--- a/Cargo.toml	Sun Jun 22 19:29:32 2025 -0400
+++ b/Cargo.toml	Mon Jun 23 13:02:58 2025 -0400
@@ -23,7 +23,6 @@
 libc = "0.2.97"
 num_enum = "0.7.3"
 regex = "1.11.1"
-secure-string = "0.3.0"
 
 [build-dependencies]
 bindgen = "0.72.0"
--- a/src/conv.rs	Sun Jun 22 19:29:32 2025 -0400
+++ b/src/conv.rs	Mon Jun 23 13:02:58 2025 -0400
@@ -4,9 +4,9 @@
 #![allow(dead_code)]
 
 use crate::constants::{ErrorCode, Result};
-use secure_string::SecureString;
 use std::cell::Cell;
 use std::fmt;
+use std::fmt::Debug;
 use std::result::Result as StdResult;
 
 /// The types of message and request that can be sent to a user.
@@ -122,7 +122,7 @@
     /// A Q&A that asks the user for text and does not show it while typing.
     ///
     /// In other words, a password entry prompt.
-    MaskedQAndA<'a, Q=&'a str, A=SecureString>,
+    MaskedQAndA<'a, Q=&'a str, A=String>,
     Message::MaskedPrompt
 );
 
@@ -284,11 +284,10 @@
 ///
 /// ```
 /// # use nonstick::{Conversation, Result};
-/// # use secure_string::SecureString;
 /// // Bring this trait into scope to get `masked_prompt`, among others.
 /// use nonstick::SimpleConversation;
 ///
-/// fn ask_for_token(convo: &mut impl Conversation) -> Result<SecureString> {
+/// fn ask_for_token(convo: &mut impl Conversation) -> Result<String> {
 ///     convo.masked_prompt("enter your one-time token")
 /// }
 /// ```
@@ -297,7 +296,6 @@
 ///
 /// ```
 /// use nonstick::{Conversation, SimpleConversation};
-/// use secure_string::SecureString;
 /// # use nonstick::{BinaryData, Result};
 /// mod some_library {
 /// #    use nonstick::Conversation;
@@ -314,7 +312,7 @@
 /// #     todo!()
 /// # }
 /// #
-/// # fn masked_prompt(&mut self, request: &str) -> Result<SecureString> {
+/// # fn masked_prompt(&mut self, request: &str) -> Result<String> {
 /// #     todo!()
 /// # }
 /// #
@@ -355,7 +353,7 @@
     /// Prompts the user for something.
     fn prompt(&mut self, request: &str) -> Result<String>;
     /// Prompts the user for something, but hides what the user types.
-    fn masked_prompt(&mut self, request: &str) -> Result<SecureString>;
+    fn masked_prompt(&mut self, request: &str) -> Result<String>;
     /// Alerts the user to an error.
     fn error_msg(&mut self, message: &str);
     /// Sends an informational message to the user.
@@ -403,7 +401,7 @@
 
 impl<C: Conversation> SimpleConversation for C {
     conv_fn!(prompt(message: &str) -> String { QAndA });
-    conv_fn!(masked_prompt(message: &str) -> SecureString { MaskedQAndA } );
+    conv_fn!(masked_prompt(message: &str) -> String { MaskedQAndA } );
     conv_fn!(error_msg(message: &str) { ErrorMsg });
     conv_fn!(info_msg(message: &str) { InfoMsg });
     conv_fn!(radio_prompt(message: &str) -> String { RadioQAndA });
@@ -464,9 +462,9 @@
                     _ => panic!("unexpected prompt!"),
                 }
             }
-            fn masked_prompt(&mut self, request: &str) -> Result<SecureString> {
+            fn masked_prompt(&mut self, request: &str) -> Result<String> {
                 assert_eq!("reveal", request);
-                Ok(SecureString::from("my secrets"))
+                Ok("my secrets".to_owned())
             }
             fn error_msg(&mut self, message: &str) {
                 self.error_ran = true;
@@ -507,7 +505,7 @@
         ]);
 
         assert_eq!("whatwhat", what.answer().unwrap());
-        assert_eq!(SecureString::from("my secrets"), pass.answer().unwrap());
+        assert_eq!("my secrets", pass.answer().unwrap());
         assert_eq!(Ok(()), err.answer());
         assert_eq!(Ok(()), info.answer());
         assert_eq!(ErrorCode::PermissionDenied, has_err.answer().unwrap_err());
@@ -572,10 +570,7 @@
         let mut tester = MuxTester;
 
         assert_eq!("answer", tester.prompt("question").unwrap());
-        assert_eq!(
-            SecureString::from("open sesame"),
-            tester.masked_prompt("password!").unwrap()
-        );
+        assert_eq!("open sesame", tester.masked_prompt("password!").unwrap());
         tester.error_msg("oh no");
         tester.info_msg("let me tell you");
         // Linux-PAM extensions. Always implemented, but separate for clarity.
--- a/src/libpam/answer.rs	Sun Jun 22 19:29:32 2025 -0400
+++ b/src/libpam/answer.rs	Mon Jun 23 13:02:58 2025 -0400
@@ -7,6 +7,7 @@
 use crate::{ErrorCode, Result};
 use std::ffi::CStr;
 use std::ops::{Deref, DerefMut};
+use std::ptr::NonNull;
 use std::{iter, mem, ptr, slice};
 
 /// The corridor via which the answer to Messages navigate through PAM.
@@ -27,7 +28,7 @@
         // all allocated answer memory.
         for (input, output) in iter::zip(value, outputs.iter_mut()) {
             match input {
-                OwnedMessage::MaskedPrompt(p) => TextAnswer::fill(output, p.answer()?.unsecure())?,
+                OwnedMessage::MaskedPrompt(p) => TextAnswer::fill(output, p.answer()?.as_ref())?,
                 OwnedMessage::Prompt(p) => TextAnswer::fill(output, &(p.answer()?))?,
                 OwnedMessage::Error(p) => TextAnswer::fill(output, p.answer().map(|_| "")?)?,
                 OwnedMessage::Info(p) => TextAnswer::fill(output, p.answer().map(|_| "")?)?,
@@ -107,7 +108,7 @@
     fn fill(dest: &mut Answer, text: &str) -> Result<()> {
         let allocated = memory::malloc_str(text)?;
         dest.free_contents();
-        dest.data = allocated.cast();
+        dest.data = allocated.as_ptr().cast();
         Ok(())
     }
 
@@ -168,29 +169,28 @@
     pub fn fill(dest: &mut Answer, data_and_type: (&[u8], u8)) -> Result<()> {
         let allocated = CBinaryData::alloc(data_and_type)?;
         dest.free_contents();
-        dest.data = allocated.cast();
+        dest.data = allocated.as_ptr().cast();
         Ok(())
     }
 
     /// Gets the binary data in this answer.
-    pub fn data(&self) -> Option<&CBinaryData> {
+    pub fn data(&self) -> Option<NonNull<CBinaryData>> {
         // SAFETY: We either got this data from PAM or allocated it ourselves.
         // Either way, we trust that it is either valid data or null.
-        unsafe { self.0.data.cast::<CBinaryData>().as_ref() }
+        NonNull::new(self.0.data.cast::<CBinaryData>())
     }
 
     /// Zeroes out the answer data, frees it, and points our data to `null`.
     ///
-    /// When this `TextAnswer` is part of an [`Answers`],
+    /// When this `BinaryAnswer` is part of an [`Answers`],
     /// this is optional (since that will perform the `free`),
     /// but it will clear potentially sensitive data.
     pub fn zero_contents(&mut self) {
         // SAFETY: We know that our data pointer is either valid or null.
         // Once we're done, it's null and the answer is safe.
         unsafe {
-            let data_ref = self.0.data.cast::<CBinaryData>().as_mut();
-            if let Some(d) = data_ref {
-                d.zero_contents()
+            if let Some(ptr) = NonNull::new(self.0.data) {
+                CBinaryData::zero_contents(ptr.cast())
             }
             memory::free(self.0.data);
             self.0.data = ptr::null_mut()
@@ -277,9 +277,13 @@
 
         if let [bin, radio] = &mut answers[..] {
             let up = unsafe { BinaryAnswer::upcast(bin) };
-            assert_eq!(BinaryData::from((&[1, 2, 3][..], 99)), up.data().into());
+            assert_eq!(BinaryData::from((&[1, 2, 3][..], 99)), unsafe {
+                CBinaryData::as_binary_data(up.data().unwrap())
+            });
             up.zero_contents();
-            assert_eq!(BinaryData::default(), up.data().into());
+            assert_eq!(BinaryData::default(), unsafe {
+                CBinaryData::as_binary_data(up.data().unwrap())
+            });
 
             assert_text_answer("beep boop", radio);
         } else {
@@ -309,7 +313,9 @@
         let real_data = BinaryData::new([1, 2, 3, 4, 5, 6, 7, 8], 9);
         BinaryAnswer::fill(answer, (&real_data).into()).expect("alloc should succeed");
         let bin_answer = unsafe { BinaryAnswer::upcast(answer) };
-        assert_eq!(real_data, bin_answer.data().into());
+        assert_eq!(real_data, unsafe {
+            CBinaryData::as_binary_data(bin_answer.data().unwrap())
+        });
         answer.free_contents();
         answer.free_contents();
         unsafe { memory::free(answer_ptr) }
--- a/src/libpam/conversation.rs	Sun Jun 22 19:29:32 2025 -0400
+++ b/src/libpam/conversation.rs	Mon Jun 23 13:02:58 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::Immovable;
+use crate::libpam::memory::{CBinaryData, Immovable};
 use crate::libpam::pam_ffi::AppData;
 pub use crate::libpam::pam_ffi::LibPamConversation;
 use crate::libpam::question::QuestionsTrait;
@@ -139,7 +139,10 @@
         Message::RadioPrompt(qa) => fill_text!(qa, resp),
         Message::BinaryPrompt(qa) => {
             let bin_resp = unsafe { BinaryAnswer::upcast(resp) };
-            qa.set_answer(Ok(bin_resp.data().into()));
+            qa.set_answer(Ok(bin_resp
+                .data()
+                .map(|d| unsafe { CBinaryData::as_binary_data(d) })
+                .unwrap_or_default()));
             bin_resp.zero_contents()
         }
     }
--- a/src/libpam/memory.rs	Sun Jun 22 19:29:32 2025 -0400
+++ b/src/libpam/memory.rs	Mon Jun 23 13:02:58 2025 -0400
@@ -4,7 +4,9 @@
 use crate::{BinaryData, ErrorCode};
 use std::ffi::{c_char, CStr, CString};
 use std::marker::{PhantomData, PhantomPinned};
-use std::{ptr, slice};
+use std::mem::offset_of;
+use std::ptr::NonNull;
+use std::{mem, ptr, slice};
 
 /// Allocates `count` elements to hold `T`.
 #[inline]
@@ -50,30 +52,22 @@
 /// # Safety
 ///
 /// It's on you to provide a valid string.
-pub unsafe fn copy_pam_string(result_ptr: *const libc::c_char) -> Result<String> {
-    // We really shouldn't get a null pointer back here, but if we do, return nothing.
-    if result_ptr.is_null() {
-        return Ok(String::new());
-    }
-    let bytes = unsafe { CStr::from_ptr(result_ptr) };
-    bytes
-        .to_str()
+pub unsafe fn copy_pam_string(result_ptr: *const c_char) -> Result<String> {
+    Ok(wrap_string(result_ptr)?
         .map(String::from)
-        .map_err(|_| ErrorCode::ConversationError)
+        .unwrap_or_default())
 }
 
 /// Wraps a string returned from PAM as an `Option<&str>`.
-pub unsafe fn wrap_string<'a>(data: *const libc::c_char) -> Result<Option<&'a str>> {
-    let ret = if data.is_null() {
-        None
-    } else {
-        Some(
-            CStr::from_ptr(data)
+pub unsafe fn wrap_string<'a>(data: *const c_char) -> Result<Option<&'a str>> {
+    match NonNull::new(data.cast_mut()) {
+        Some(data) => Ok(Some(
+            CStr::from_ptr(data.as_ptr())
                 .to_str()
                 .map_err(|_| ErrorCode::ConversationError)?,
-        )
-    };
-    Ok(ret)
+        )),
+        None => Ok(None),
+    }
 }
 
 /// Allocates a string with the given contents on the C heap.
@@ -82,7 +76,7 @@
 ///
 /// - it allocates data on the C heap with [`libc::malloc`].
 /// - it doesn't take ownership of the data passed in.
-pub fn malloc_str(text: &str) -> Result<*mut c_char> {
+pub fn malloc_str(text: &str) -> Result<NonNull<c_char>> {
     let data = text.as_bytes();
     if data.contains(&0) {
         return Err(ErrorCode::ConversationError);
@@ -92,8 +86,8 @@
     // SAFETY: we just allocated this and we have enough room.
     unsafe {
         libc::memcpy(data_alloc.cast(), data.as_ptr().cast(), data.len());
+        Ok(NonNull::new_unchecked(data_alloc))
     }
-    Ok(data_alloc)
 }
 
 /// Writes zeroes over the contents of a C string.
@@ -105,7 +99,10 @@
 /// It's up to you to provide a valid C string.
 pub unsafe fn zero_c_string(cstr: *mut c_char) {
     if !cstr.is_null() {
-        libc::memset(cstr.cast(), 0, libc::strlen(cstr.cast()));
+        let len = libc::strlen(cstr.cast());
+        for x in 0..len {
+            ptr::write_volatile(cstr.byte_offset(x as isize), mem::zeroed())
+        }
     }
 }
 
@@ -128,17 +125,21 @@
 
 impl CBinaryData {
     /// Copies the given data to a new BinaryData on the heap.
-    pub fn alloc((data, data_type): (&[u8], u8)) -> Result<*mut CBinaryData> {
+    pub fn alloc((data, data_type): (&[u8], u8)) -> Result<NonNull<CBinaryData>> {
         let buffer_size =
             u32::try_from(data.len() + 5).map_err(|_| ErrorCode::ConversationError)?;
         // SAFETY: We're only allocating here.
         let dest = unsafe {
-            let dest_buffer: *mut CBinaryData = calloc::<u8>(buffer_size as usize).cast();
-            let dest = &mut *dest_buffer;
+            let mut dest_buffer: NonNull<Self> =
+                NonNull::new_unchecked(calloc::<u8>(buffer_size as usize).cast());
+            let dest = dest_buffer.as_mut();
             dest.total_length = buffer_size.to_be_bytes();
             dest.data_type = data_type;
-            let dest = dest.data.as_mut_ptr();
-            libc::memcpy(dest.cast(), data.as_ptr().cast(), data.len());
+            libc::memcpy(
+                Self::data_ptr(dest_buffer).cast(),
+                data.as_ptr().cast(),
+                data.len(),
+            );
             dest_buffer
         };
         Ok(dest)
@@ -148,39 +149,33 @@
         u32::from_be_bytes(self.total_length).saturating_sub(5) as usize
     }
 
-    /// Clears this data and frees it.
-    pub unsafe fn zero_contents(&mut self) {
-        let contents = slice::from_raw_parts_mut(self.data.as_mut_ptr(), self.length());
-        for v in contents {
-            *v = 0
+    fn data_ptr(ptr: NonNull<Self>) -> *mut u8 {
+        unsafe {
+            ptr.as_ptr()
+                .cast::<u8>()
+                .byte_offset(offset_of!(Self, data) as isize)
         }
-        self.data_type = 0;
-        self.total_length = [0; 4];
     }
-}
+
+    unsafe fn data_slice<'a>(ptr: NonNull<Self>) -> &'a mut [u8] {
+        unsafe { slice::from_raw_parts_mut(Self::data_ptr(ptr), ptr.as_ref().length()) }
+    }
 
-impl<'a> From<&'a CBinaryData> for (&'a [u8], u8) {
-    fn from(value: &'a CBinaryData) -> Self {
-        (
-            unsafe { slice::from_raw_parts(value.data.as_ptr(), value.length()) },
-            value.data_type,
-        )
+    pub unsafe fn data<'a>(ptr: NonNull<Self>) -> (&'a [u8], u8) {
+        unsafe { (Self::data_slice(ptr), ptr.as_ref().data_type) }
     }
-}
 
-impl From<&'_ CBinaryData> for BinaryData {
-    fn from(value: &'_ CBinaryData) -> Self {
-        // This is a dumb trick but I like it because it is simply the presence
-        // of `.map(|z: (_, _)| z)` in the middle of this that gives
-        // type inference the hint it needs to make this work.
-        let [ret] = [value].map(Into::into).map(|z: (_, _)| z).map(Into::into);
-        ret
+    pub unsafe fn zero_contents(ptr: NonNull<Self>) {
+        for byte in Self::data_slice(ptr) {
+            ptr::write_volatile(byte as *mut u8, mem::zeroed());
+        }
+        ptr::write_volatile(ptr.as_ptr(), mem::zeroed());
     }
-}
 
-impl From<Option<&'_ CBinaryData>> for BinaryData {
-    fn from(value: Option<&CBinaryData>) -> Self {
-        value.map(Into::into).unwrap_or_default()
+    #[allow(clippy::wrong_self_convention)]
+    pub unsafe fn as_binary_data(ptr: NonNull<Self>) -> BinaryData {
+        let (data, data_type) = unsafe { (CBinaryData::data_slice(ptr), ptr.as_ref().data_type) };
+        (Vec::from(data), data_type).into()
     }
 }
 
@@ -193,6 +188,7 @@
     #[test]
     fn test_strings() {
         let str = malloc_str("hello there").unwrap();
+        let str = str.as_ptr();
         malloc_str("hell\0 there").unwrap_err();
         unsafe {
             let copied = copy_pam_string(str).unwrap();
--- a/src/libpam/question.rs	Sun Jun 22 19:29:32 2025 -0400
+++ b/src/libpam/question.rs	Mon Jun 23 13:02:58 2025 -0400
@@ -11,6 +11,7 @@
 use crate::Result;
 use num_enum::{IntoPrimitive, TryFromPrimitive};
 use std::ffi::{c_void, CStr};
+use std::ptr::NonNull;
 use std::{ptr, slice};
 
 /// Abstraction of a collection of questions to be sent in a PAM conversation.
@@ -214,10 +215,9 @@
 
     /// Gets this message's data pointer as borrowed binary data.
     unsafe fn binary_data(&self) -> (&[u8], u8) {
-        self.data
-            .cast::<CBinaryData>()
-            .as_ref()
-            .map(Into::into)
+        NonNull::new(self.data)
+            .map(|nn| nn.cast())
+            .map(|ptr| CBinaryData::data(ptr))
             .unwrap_or_default()
     }
 }
@@ -227,7 +227,7 @@
     fn try_from(msg: &Message) -> Result<Self> {
         let alloc = |style, text| Ok((style, memory::malloc_str(text)?.cast()));
         // We will only allocate heap data if we have a valid input.
-        let (style, data): (_, *mut c_void) = match *msg {
+        let (style, data): (_, NonNull<c_void>) = match *msg {
             Message::MaskedPrompt(p) => alloc(Style::PromptEchoOff, p.question()),
             Message::Prompt(p) => alloc(Style::PromptEchoOn, p.question()),
             Message::Error(p) => alloc(Style::ErrorMsg, p.question()),
@@ -244,7 +244,7 @@
         }?;
         Ok(Self {
             style: style.into(),
-            data,
+            data: data.as_ptr(),
             _marker: Default::default(),
         })
     }
@@ -261,8 +261,8 @@
                 match style {
                     #[cfg(feature = "linux-pam-extensions")]
                     Style::BinaryPrompt => {
-                        if let Some(d) = self.data.cast::<CBinaryData>().as_mut() {
-                            d.zero_contents()
+                        if let Some(d) = NonNull::new(self.data) {
+                            CBinaryData::zero_contents(d.cast())
                         }
                     }
                     #[cfg(feature = "linux-pam-extensions")]
@@ -367,7 +367,7 @@
                 let indirect = interrogation.ptr();
 
                 let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) };
-                let messages: Vec<OwnedMessage> = unsafe { remade }
+                let messages: Vec<OwnedMessage> = remade
                     .map(TryInto::try_into)
                     .collect::<Result<_>>()
                     .unwrap();