From 95dc04d64ead2b0396b6974a1e9302cb8018a92f Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 11:43:39 +0100 Subject: [PATCH 1/7] Ensure the Converser can't swap out the Conversation with a differently sized one Doing so would cause UB. None of the existing Converser impls do this and it is also hard to accidentally do, but it can't hurt to be a bit more strict either. --- src/pam/converse.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 6518b0831..a93351e3c 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -56,15 +56,15 @@ impl PamMessage { /// Note that generally there will only be one message in each conversation /// because of historical reasons, and instead multiple conversations will /// be started for individual messages. -pub struct Conversation { - messages: Vec, +pub struct Conversation<'a> { + messages: &'a mut [PamMessage], } -impl Conversation { +impl<'a> Conversation<'a> { /// Get a mutable iterator of the messages in this conversation. /// /// This can be used to add the resulting values to the messages. - pub fn messages_mut(&mut self) -> impl Iterator { + pub fn messages_mut(self) -> impl Iterator { self.messages.iter_mut() } } @@ -72,7 +72,7 @@ impl Conversation { pub trait Converser { /// Handle all the message in the given conversation. They may all be /// handled in sequence or at the same time if possible. - fn handle_conversation(&self, conversation: &mut Conversation) -> PamResult<()>; + fn handle_conversation(&self, conversation: Conversation<'_>) -> PamResult<()>; } pub trait SequentialConverser: Converser { @@ -97,7 +97,7 @@ impl Converser for T where T: SequentialConverser, { - fn handle_conversation(&self, conversation: &mut Conversation) -> PamResult<()> { + fn handle_conversation(&self, conversation: Conversation<'_>) -> PamResult<()> { use PamMessageStyle::*; for msg in conversation.messages_mut() { @@ -204,9 +204,7 @@ pub(super) unsafe extern "C" fn converse( ) -> libc::c_int { let result = std::panic::catch_unwind(|| { // convert the input messages to Rust types - let mut conversation = Conversation { - messages: Vec::with_capacity(num_msg as usize), - }; + let mut messages = Vec::with_capacity(num_msg as usize); for i in 0..num_msg as isize { // SAFETY: the PAM contract ensures that `num_msg` does not exceed the amount // of messages presented to this function in `msg`, and that it is not being @@ -223,7 +221,7 @@ pub(super) unsafe extern "C" fn converse( return PamErrorType::ConversationError; }; - conversation.messages.push(PamMessage { + messages.push(PamMessage { msg, style, response: None, @@ -235,7 +233,9 @@ pub(super) unsafe extern "C" fn converse( let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; if app_data .converser - .handle_conversation(&mut conversation) + .handle_conversation(Conversation { + messages: messages.as_mut_slice(), + }) .is_err() { return PamErrorType::ConversationError; @@ -256,7 +256,7 @@ pub(super) unsafe extern "C" fn converse( } // Store the responses - for (i, msg) in conversation.messages.into_iter().enumerate() { + for (i, msg) in messages.into_iter().enumerate() { // SAFETY: `i` will not exceed `num_msg` by the way `conversation_messages` // is constructed, so `temp_resp` will have allocated-and-initialized data at // the required offset that only we have a writable pointer to. From e321385ab1d8225b0a2db05d5734be1089f20171 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 13:56:09 +0100 Subject: [PATCH 2/7] Pass a single PamMessage to Converser at a time A Converser doesn't need to distinguish between a single conversation with multiple messages and multiple conversations with a single message each. --- src/pam/converse.rs | 70 ++++++++++++++------------------------------- 1 file changed, 22 insertions(+), 48 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index a93351e3c..3c6c96462 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -50,29 +50,9 @@ impl PamMessage { } } -/// Contains the conversation messages and allows setting responses to -/// each of these messages. -/// -/// Note that generally there will only be one message in each conversation -/// because of historical reasons, and instead multiple conversations will -/// be started for individual messages. -pub struct Conversation<'a> { - messages: &'a mut [PamMessage], -} - -impl<'a> Conversation<'a> { - /// Get a mutable iterator of the messages in this conversation. - /// - /// This can be used to add the resulting values to the messages. - pub fn messages_mut(self) -> impl Iterator { - self.messages.iter_mut() - } -} - pub trait Converser { - /// Handle all the message in the given conversation. They may all be - /// handled in sequence or at the same time if possible. - fn handle_conversation(&self, conversation: Conversation<'_>) -> PamResult<()>; + /// Handle a single message. + fn handle_message(&self, msg: &mut PamMessage) -> PamResult<()>; } pub trait SequentialConverser: Converser { @@ -97,23 +77,21 @@ impl Converser for T where T: SequentialConverser, { - fn handle_conversation(&self, conversation: Conversation<'_>) -> PamResult<()> { + fn handle_message(&self, msg: &mut PamMessage) -> PamResult<()> { use PamMessageStyle::*; - for msg in conversation.messages_mut() { - match msg.style { - PromptEchoOn => { - msg.set_response(self.handle_normal_prompt(&msg.msg)?); - } - PromptEchoOff => { - msg.set_response(self.handle_hidden_prompt(&msg.msg)?); - } - ErrorMessage => { - self.handle_error(&msg.msg)?; - } - TextInfo => { - self.handle_info(&msg.msg)?; - } + match msg.style { + PromptEchoOn => { + msg.set_response(self.handle_normal_prompt(&msg.msg)?); + } + PromptEchoOff => { + msg.set_response(self.handle_hidden_prompt(&msg.msg)?); + } + ErrorMessage => { + self.handle_error(&msg.msg)?; + } + TextInfo => { + self.handle_info(&msg.msg)?; } } @@ -228,17 +206,13 @@ pub(super) unsafe extern "C" fn converse( }); } - // send the conversation of to the Rust part - // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM - let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; - if app_data - .converser - .handle_conversation(Conversation { - messages: messages.as_mut_slice(), - }) - .is_err() - { - return PamErrorType::ConversationError; + for message in &mut messages { + // send the conversation of to the Rust part + // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM + let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; + if app_data.converser.handle_message(message).is_err() { + return PamErrorType::ConversationError; + } } // Conversation should now contain response messages From d1b497fd15355235d16645ee675f7c9b45f43964 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 14:06:17 +0100 Subject: [PATCH 3/7] Fuse the three loops in converse This simplifies the code and makes it easier to directly return the response from Converser::handle_message in the future. This will leak some memory when there are multiple messages in a single conversation and handling one of the messages returns an error. This shouldn't matter for sudo-rs however as we would exit shortly after the error anyway. --- src/pam/converse.rs | 48 ++++++++++++++++++++------------------------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 3c6c96462..65f2befac 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -181,14 +181,26 @@ pub(super) unsafe extern "C" fn converse( appdata_ptr: *mut libc::c_void, ) -> libc::c_int { let result = std::panic::catch_unwind(|| { - // convert the input messages to Rust types - let mut messages = Vec::with_capacity(num_msg as usize); - for i in 0..num_msg as isize { + // Allocate enough memory for the responses, which are initialized with zero. + // SAFETY: this will either allocate the required amount of (initialized) bytes, + // or return a null pointer. + let temp_resp = unsafe { + libc::calloc( + num_msg as libc::size_t, + std::mem::size_of::() as libc::size_t, + ) + } as *mut pam_response; + if temp_resp.is_null() { + return PamErrorType::BufferError; + } + + for i in 0..num_msg as usize { + // convert the input messages to Rust types // SAFETY: the PAM contract ensures that `num_msg` does not exceed the amount // of messages presented to this function in `msg`, and that it is not being // written to at the same time as we are reading it. Note that the reference // we create does not escape this loopy body. - let message: &pam_message = unsafe { &**msg.offset(i) }; + let message: &pam_message = unsafe { &**msg.add(i) }; // SAFETY: PAM ensures that the messages passed are properly null-terminated let msg = unsafe { string_from_ptr(message.msg) }; @@ -199,38 +211,20 @@ pub(super) unsafe extern "C" fn converse( return PamErrorType::ConversationError; }; - messages.push(PamMessage { + let mut msg = PamMessage { msg, style, response: None, - }); - } + }; - for message in &mut messages { - // send the conversation of to the Rust part + // send the conversation off to the Rust part // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; - if app_data.converser.handle_message(message).is_err() { + if app_data.converser.handle_message(&mut msg).is_err() { return PamErrorType::ConversationError; } - } - - // Conversation should now contain response messages - // allocate enough memory for the responses, set it to zero - // SAFETY: this will either allocate the required amount of (initialized) bytes, - // or return a null pointer. - let temp_resp = unsafe { - libc::calloc( - num_msg as libc::size_t, - std::mem::size_of::() as libc::size_t, - ) - } as *mut pam_response; - if temp_resp.is_null() { - return PamErrorType::BufferError; - } - // Store the responses - for (i, msg) in messages.into_iter().enumerate() { + // Store the response // SAFETY: `i` will not exceed `num_msg` by the way `conversation_messages` // is constructed, so `temp_resp` will have allocated-and-initialized data at // the required offset that only we have a writable pointer to. From 3f3c6b68cb0e4cd8bcdc1ca61b1327530a312587 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 14:12:44 +0100 Subject: [PATCH 4/7] Return response from Converser::handle_message Rather than storing it in PamMessage. --- src/pam/converse.rs | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 65f2befac..ff2035e33 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -35,24 +35,15 @@ impl PamMessageStyle { } } -/// A PamMessage contains the data in a single message of a pam conversation -/// and contains the response to that message. +/// A PamMessage contains the data in a single message of a pam conversation. pub struct PamMessage { pub msg: String, pub style: PamMessageStyle, - response: Option, -} - -impl PamMessage { - /// Set a response value to the message. - pub fn set_response(&mut self, resp: PamBuffer) { - self.response = Some(resp); - } } pub trait Converser { /// Handle a single message. - fn handle_message(&self, msg: &mut PamMessage) -> PamResult<()>; + fn handle_message(&self, msg: &PamMessage) -> PamResult>; } pub trait SequentialConverser: Converser { @@ -77,25 +68,23 @@ impl Converser for T where T: SequentialConverser, { - fn handle_message(&self, msg: &mut PamMessage) -> PamResult<()> { + fn handle_message(&self, msg: &PamMessage) -> PamResult> { use PamMessageStyle::*; match msg.style { PromptEchoOn => { - msg.set_response(self.handle_normal_prompt(&msg.msg)?); + self.handle_normal_prompt(&msg.msg).map(Some) } PromptEchoOff => { - msg.set_response(self.handle_hidden_prompt(&msg.msg)?); + self.handle_hidden_prompt(&msg.msg).map(Some) } ErrorMessage => { - self.handle_error(&msg.msg)?; + self.handle_error(&msg.msg).map(|()| None) } TextInfo => { - self.handle_info(&msg.msg)?; + self.handle_info(&msg.msg).map(|()| None) } } - - Ok(()) } } @@ -211,18 +200,17 @@ pub(super) unsafe extern "C" fn converse( return PamErrorType::ConversationError; }; - let mut msg = PamMessage { + let msg = PamMessage { msg, style, - response: None, }; // send the conversation off to the Rust part // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; - if app_data.converser.handle_message(&mut msg).is_err() { + let Ok(resp_buf) = app_data.converser.handle_message(&msg) else { return PamErrorType::ConversationError; - } + }; // Store the response // SAFETY: `i` will not exceed `num_msg` by the way `conversation_messages` @@ -230,7 +218,7 @@ pub(super) unsafe extern "C" fn converse( // the required offset that only we have a writable pointer to. let response: &mut pam_response = unsafe { &mut *(temp_resp.add(i)) }; - if let Some(secbuf) = msg.response { + if let Some(secbuf) = resp_buf { response.resp = secbuf.leak().as_ptr().cast(); } } @@ -345,7 +333,6 @@ mod test { PamMessage { style, msg, - response: None, } } From 4afa853ffb3f58caffebebeb7a4e93b61bf033c1 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 14:17:10 +0100 Subject: [PATCH 5/7] Get rid of PamMessage And pass it's fields directly to Converser::handle_message instead. --- src/pam/converse.rs | 47 +++++++++++++++------------------------------ 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index ff2035e33..1d9856468 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -35,15 +35,9 @@ impl PamMessageStyle { } } -/// A PamMessage contains the data in a single message of a pam conversation. -pub struct PamMessage { - pub msg: String, - pub style: PamMessageStyle, -} - pub trait Converser { - /// Handle a single message. - fn handle_message(&self, msg: &PamMessage) -> PamResult>; + /// Handle a single message in a conversation. + fn handle_message(&self, style: PamMessageStyle, msg: &str) -> PamResult>; } pub trait SequentialConverser: Converser { @@ -68,22 +62,14 @@ impl Converser for T where T: SequentialConverser, { - fn handle_message(&self, msg: &PamMessage) -> PamResult> { + fn handle_message(&self, style: PamMessageStyle, msg: &str) -> PamResult> { use PamMessageStyle::*; - match msg.style { - PromptEchoOn => { - self.handle_normal_prompt(&msg.msg).map(Some) - } - PromptEchoOff => { - self.handle_hidden_prompt(&msg.msg).map(Some) - } - ErrorMessage => { - self.handle_error(&msg.msg).map(|()| None) - } - TextInfo => { - self.handle_info(&msg.msg).map(|()| None) - } + match style { + PromptEchoOn => self.handle_normal_prompt(msg).map(Some), + PromptEchoOff => self.handle_hidden_prompt(msg).map(Some), + ErrorMessage => self.handle_error(msg).map(|()| None), + TextInfo => self.handle_info(msg).map(|()| None), } } } @@ -200,15 +186,10 @@ pub(super) unsafe extern "C" fn converse( return PamErrorType::ConversationError; }; - let msg = PamMessage { - msg, - style, - }; - // send the conversation off to the Rust part // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; - let Ok(resp_buf) = app_data.converser.handle_message(&msg) else { + let Ok(resp_buf) = app_data.converser.handle_message(style, &msg) else { return PamErrorType::ConversationError; }; @@ -251,6 +232,11 @@ mod test { use std::pin::Pin; use PamMessageStyle::*; + struct PamMessage { + msg: String, + style: PamMessageStyle, + } + impl SequentialConverser for String { fn handle_normal_prompt(&self, msg: &str) -> PamResult { Ok(PamBuffer::new(format!("{self} says {msg}").into_bytes())) @@ -330,10 +316,7 @@ mod test { fn msg(style: PamMessageStyle, msg: &str) -> PamMessage { let msg = msg.to_string(); - PamMessage { - style, - msg, - } + PamMessage { style, msg } } // sanity check on the test cases; lib.rs is expected to manage the lifetime of the pointer From 3c498f33e4ae8c6ebee20bb134d04a1a84152200 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 14:21:49 +0100 Subject: [PATCH 6/7] Make Converser::handle_message a standalone function And rename SequentialConverser to Converser to reduce the amount of changes in the rest of the code. --- src/pam/converse.rs | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index 1d9856468..aa455e3db 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -36,11 +36,6 @@ impl PamMessageStyle { } pub trait Converser { - /// Handle a single message in a conversation. - fn handle_message(&self, style: PamMessageStyle, msg: &str) -> PamResult>; -} - -pub trait SequentialConverser: Converser { /// Handle a normal prompt, i.e. present some message and ask for a value. /// The value is not considered a secret. fn handle_normal_prompt(&self, msg: &str) -> PamResult; @@ -58,19 +53,19 @@ pub trait SequentialConverser: Converser { fn handle_info(&self, msg: &str) -> PamResult<()>; } -impl Converser for T -where - T: SequentialConverser, -{ - fn handle_message(&self, style: PamMessageStyle, msg: &str) -> PamResult> { - use PamMessageStyle::*; +/// Handle a single message in a conversation. +fn handle_message( + converser: &C, + style: PamMessageStyle, + msg: &str, +) -> PamResult> { + use PamMessageStyle::*; - match style { - PromptEchoOn => self.handle_normal_prompt(msg).map(Some), - PromptEchoOff => self.handle_hidden_prompt(msg).map(Some), - ErrorMessage => self.handle_error(msg).map(|()| None), - TextInfo => self.handle_info(msg).map(|()| None), - } + match style { + PromptEchoOn => converser.handle_normal_prompt(msg).map(Some), + PromptEchoOff => converser.handle_hidden_prompt(msg).map(Some), + ErrorMessage => converser.handle_error(msg).map(|()| None), + TextInfo => converser.handle_info(msg).map(|()| None), } } @@ -95,7 +90,7 @@ impl CLIConverser { } } -impl SequentialConverser for CLIConverser { +impl Converser for CLIConverser { fn handle_normal_prompt(&self, msg: &str) -> PamResult { if self.no_interact { return Err(PamError::InteractionRequired); @@ -189,7 +184,7 @@ pub(super) unsafe extern "C" fn converse( // send the conversation off to the Rust part // SAFETY: appdata_ptr contains the `*mut ConverserData` that is untouched by PAM let app_data = unsafe { &mut *(appdata_ptr as *mut ConverserData) }; - let Ok(resp_buf) = app_data.converser.handle_message(style, &msg) else { + let Ok(resp_buf) = handle_message(&app_data.converser, style, &msg) else { return PamErrorType::ConversationError; }; @@ -237,7 +232,7 @@ mod test { style: PamMessageStyle, } - impl SequentialConverser for String { + impl Converser for String { fn handle_normal_prompt(&self, msg: &str) -> PamResult { Ok(PamBuffer::new(format!("{self} says {msg}").into_bytes())) } From 83db1dbef3ffc675d33cb550e0d4843f52bb6f84 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Mon, 20 Jan 2025 14:42:22 +0100 Subject: [PATCH 7/7] Avoid harmless memory leak in converse Miri complains. --- src/pam/converse.rs | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/pam/converse.rs b/src/pam/converse.rs index aa455e3db..bd327c892 100644 --- a/src/pam/converse.rs +++ b/src/pam/converse.rs @@ -151,19 +151,7 @@ pub(super) unsafe extern "C" fn converse( appdata_ptr: *mut libc::c_void, ) -> libc::c_int { let result = std::panic::catch_unwind(|| { - // Allocate enough memory for the responses, which are initialized with zero. - // SAFETY: this will either allocate the required amount of (initialized) bytes, - // or return a null pointer. - let temp_resp = unsafe { - libc::calloc( - num_msg as libc::size_t, - std::mem::size_of::() as libc::size_t, - ) - } as *mut pam_response; - if temp_resp.is_null() { - return PamErrorType::BufferError; - } - + let mut resp_bufs = Vec::with_capacity(num_msg as usize); for i in 0..num_msg as usize { // convert the input messages to Rust types // SAFETY: the PAM contract ensures that `num_msg` does not exceed the amount @@ -188,7 +176,24 @@ pub(super) unsafe extern "C" fn converse( return PamErrorType::ConversationError; }; - // Store the response + resp_bufs.push(resp_buf); + } + + // Allocate enough memory for the responses, which are initialized with zero. + // SAFETY: this will either allocate the required amount of (initialized) bytes, + // or return a null pointer. + let temp_resp = unsafe { + libc::calloc( + num_msg as libc::size_t, + std::mem::size_of::() as libc::size_t, + ) + } as *mut pam_response; + if temp_resp.is_null() { + return PamErrorType::BufferError; + } + + // Store the responses + for (i, resp_buf) in resp_bufs.into_iter().enumerate() { // SAFETY: `i` will not exceed `num_msg` by the way `conversation_messages` // is constructed, so `temp_resp` will have allocated-and-initialized data at // the required offset that only we have a writable pointer to.