# HG changeset patch # User Paul Fisher # Date 1750698178 14400 # Node ID efc2b56c89284b28c6e20327e91836fb57ab1415 # Parent 5ddbcada30f27820137b10d5b0b89a159e6f746b 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). diff -r 5ddbcada30f2 -r efc2b56c8928 Cargo.toml --- 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" diff -r 5ddbcada30f2 -r efc2b56c8928 src/conv.rs --- 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 { +/// fn ask_for_token(convo: &mut impl Conversation) -> Result { /// 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 { +/// # fn masked_prompt(&mut self, request: &str) -> Result { /// # todo!() /// # } /// # @@ -355,7 +353,7 @@ /// Prompts the user for something. fn prompt(&mut self, request: &str) -> Result; /// Prompts the user for something, but hides what the user types. - fn masked_prompt(&mut self, request: &str) -> Result; + fn masked_prompt(&mut self, request: &str) -> Result; /// 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 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 { + fn masked_prompt(&mut self, request: &str) -> Result { 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. diff -r 5ddbcada30f2 -r efc2b56c8928 src/libpam/answer.rs --- 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> { // 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::().as_ref() } + NonNull::new(self.0.data.cast::()) } /// 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::().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) } diff -r 5ddbcada30f2 -r efc2b56c8928 src/libpam/conversation.rs --- 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() } } diff -r 5ddbcada30f2 -r efc2b56c8928 src/libpam/memory.rs --- 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 { - // 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 { + 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> { - let ret = if data.is_null() { - None - } else { - Some( - CStr::from_ptr(data) +pub unsafe fn wrap_string<'a>(data: *const c_char) -> Result> { + 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> { 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> { 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::(buffer_size as usize).cast(); - let dest = &mut *dest_buffer; + let mut dest_buffer: NonNull = + NonNull::new_unchecked(calloc::(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) -> *mut u8 { + unsafe { + ptr.as_ptr() + .cast::() + .byte_offset(offset_of!(Self, data) as isize) } - self.data_type = 0; - self.total_length = [0; 4]; } -} + + unsafe fn data_slice<'a>(ptr: NonNull) -> &'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) -> (&'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) { + 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> 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) -> 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(); diff -r 5ddbcada30f2 -r efc2b56c8928 src/libpam/question.rs --- 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::() - .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 { 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) = 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::().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 = unsafe { remade } + let messages: Vec = remade .map(TryInto::try_into) .collect::>() .unwrap();