Repository: memorysafety/sudo-rs
Pull request #156 (2023-03-28 09:04:09)

Resolve path implementation and split up common code


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 4c85f33f..7865066a 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -482,7 +482,6 @@ dependencies = [
  "sudo-pam",
  "sudo-system",
  "thiserror",
- "which",
 ]
 
 [[package]]
diff --git a/Cargo.toml b/Cargo.toml
index 164cb590..8512b239 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -27,7 +27,6 @@ publish = true
 clap = "4.0.32"
 libc = "0.2.139"
 thiserror = "1.0.38"
-which = "4.4.0"
 rpassword = "7.2.0"
 glob = "0.3.1"
 sha2 = "0.10.6"
diff --git a/lib/sudo-common/Cargo.toml b/lib/sudo-common/Cargo.toml
index 9d60824f..faad9aeb 100644
--- a/lib/sudo-common/Cargo.toml
+++ b/lib/sudo-common/Cargo.toml
@@ -8,9 +8,8 @@ homepage.workspace = true
 publish.workspace = true
 
 [dependencies]
+libc.workspace = true
 sudo-cli.workspace = true
 sudo-pam.workspace = true
 sudo-system.workspace = true
-libc.workspace = true
 thiserror.workspace = true
-which.workspace = true
diff --git a/lib/sudo-common/src/command.rs b/lib/sudo-common/src/command.rs
new file mode 100644
index 00000000..591e0d7e
--- /dev/null
+++ b/lib/sudo-common/src/command.rs
@@ -0,0 +1,30 @@
+use std::path::PathBuf;
+
+use crate::{resolve::resolve_path, Error};
+
+#[derive(Debug)]
+pub struct CommandAndArguments<'a> {
+    pub command: PathBuf,
+    pub arguments: Vec<&'a str>,
+}
+
+impl<'a> CommandAndArguments<'a> {
+    pub fn try_from_args(external_args: &'a [String], path: &str) -> Result<Self, Error> {
+        let mut iter = external_args.iter();
+        let command = iter.next().ok_or(Error::InvalidCommand)?.to_string();
+
+        // resolve the binary if the path is not absolute
+        let command = if command.starts_with('/') {
+            PathBuf::from(command)
+        } else {
+            // TODO: use value of secure_path setting to possible override current path
+            // FIXME: propagating the error is a possible security problem since it leaks information before any permission check is done
+            resolve_path(&PathBuf::from(command), path).ok_or_else(|| Error::InvalidCommand)?
+        };
+
+        Ok(CommandAndArguments {
+            command,
+            arguments: iter.map(|v| v.as_str()).collect(),
+        })
+    }
+}
diff --git a/lib/sudo-common/src/context.rs b/lib/sudo-common/src/context.rs
index 53582485..a9bba77d 100644
--- a/lib/sudo-common/src/context.rs
+++ b/lib/sudo-common/src/context.rs
@@ -1,59 +1,12 @@
-use std::{collections::HashMap, path::PathBuf, str::FromStr};
+use std::path::PathBuf;
 use sudo_cli::SudoOptions;
 use sudo_system::{hostname, Group, User};
 
-use crate::error::Error;
-
-pub type Environment = HashMap<String, String>;
-
-#[derive(Debug)]
-pub struct CommandAndArguments<'a> {
-    pub command: PathBuf,
-    pub arguments: Vec<&'a str>,
-}
-
-impl<'a> TryFrom<&'a [String]> for CommandAndArguments<'a> {
-    type Error = Error;
-
-    fn try_from(external_args: &'a [String]) -> Result<Self, Self::Error> {
-        let mut iter = external_args.iter();
-
-        let command = iter.next().ok_or(Error::InvalidCommand)?.to_string();
-
-        // resolve the binary if the path is not absolute
-        let command = if command.starts_with('/') {
-            PathBuf::from(command)
-        } else {
-            // TODO: we resolve in the context of the current user using the 'which' crate - we want to reconsider this in the future
-            // TODO: use value of secure_path setting to possible override current path
-            // FIXME: propagating the error is a possible security problem since it leaks information before any permission check is done
-            which::which(command).map_err(|_| Error::InvalidCommand)?
-        };
-
-        Ok(CommandAndArguments {
-            command,
-            arguments: iter.map(|v| v.as_str()).collect(),
-        })
-    }
-}
-
-#[derive(PartialEq, Debug)]
-enum NameOrId<'a, T: FromStr> {
-    Name(&'a str),
-    Id(T),
-}
-
-impl<'a, T: FromStr> NameOrId<'a, T> {
-    pub fn parse(input: &'a str) -> Option<Self> {
-        if input.is_empty() {
-            None
-        } else if let Some(stripped) = input.strip_prefix('#') {
-            stripped.parse::<T>().ok().map(|id| Self::Id(id))
-        } else {
-            Some(Self::Name(input))
-        }
-    }
-}
+use crate::{
+    command::CommandAndArguments,
+    resolve::{resolve_current_user, resolve_target_group, resolve_target_user},
+    Error,
+};
 
 /// A Context is based off of global information and is 'non-judgmental'
 #[derive(Debug)]
@@ -73,48 +26,13 @@ pub struct Context<'a> {
     pub pid: i32,
 }
 
-fn resolve_current_user() -> Result<User, Error> {
-    User::real()?.ok_or(Error::UserNotFound("current user".to_string()))
-}
-
-fn resolve_target_user(target_name_or_id: &Option<String>) -> Result<User, Error> {
-    let is_default = target_name_or_id.is_none();
-    let target_name_or_id = target_name_or_id.as_deref().unwrap_or("root");
-
-    let mut user = match NameOrId::parse(target_name_or_id) {
-        Some(NameOrId::Name(name)) => User::from_name(name)?,
-        Some(NameOrId::Id(uid)) => User::from_uid(uid)?,
-        _ => None,
-    }
-    .ok_or_else(|| Error::UserNotFound(target_name_or_id.to_string()))?;
-
-    user.is_default = is_default;
-
-    Ok(user)
-}
-
-fn resolve_target_group(
-    target_name_or_id: &Option<String>,
-    target_user: &User,
-) -> Result<Group, Error> {
-    match target_name_or_id.as_deref() {
-        Some(name_or_id) => match NameOrId::parse(name_or_id) {
-            Some(NameOrId::Name(name)) => Group::from_name(name)?,
-            Some(NameOrId::Id(gid)) => Group::from_gid(gid)?,
-            _ => None,
-        },
-        None => Group::from_gid(target_user.gid)?,
-    }
-    .ok_or(Error::GroupNotFound(
-        target_name_or_id
-            .clone()
-            .unwrap_or_else(|| target_user.gid.to_string()),
-    ))
-}
-
 impl<'a> Context<'a> {
-    pub fn build_from_options(sudo_options: &'a SudoOptions) -> Result<Context<'a>, Error> {
-        let command = CommandAndArguments::try_from(sudo_options.external_args.as_slice())?;
+    pub fn build_from_options(
+        sudo_options: &'a SudoOptions,
+        path: &str,
+    ) -> Result<Context<'a>, Error> {
+        let command =
+            CommandAndArguments::try_from_args(sudo_options.external_args.as_slice(), path)?;
         let hostname = hostname();
         let current_user = resolve_current_user()?.with_groups();
         let target_user = resolve_target_user(&sudo_options.user)?.with_groups();
@@ -140,67 +58,14 @@ impl<'a> Context<'a> {
 mod tests {
     use std::collections::HashMap;
     use sudo_cli::SudoOptions;
-    use sudo_system::User;
-
-    use super::{resolve_target_group, resolve_target_user, Context, NameOrId};
 
-    #[test]
-    fn test_name_or_id() {
-        assert_eq!(NameOrId::<u32>::parse(""), None);
-        assert_eq!(NameOrId::<u32>::parse("mies"), Some(NameOrId::Name("mies")));
-        assert_eq!(NameOrId::<u32>::parse("1337"), Some(NameOrId::Name("1337")));
-        assert_eq!(NameOrId::<u32>::parse("#1337"), Some(NameOrId::Id(1337)));
-        assert_eq!(NameOrId::<u32>::parse("#-1"), None);
-    }
-
-    #[test]
-    fn test_resolve_target_user() {
-        assert_eq!(
-            resolve_target_user(&Some("mies".to_string())).is_err(),
-            true
-        );
-        assert_eq!(resolve_target_user(&Some("root".to_string())).is_ok(), true);
-        assert_eq!(resolve_target_user(&Some("#1".to_string())).is_ok(), true);
-        assert_eq!(resolve_target_user(&Some("#-1".to_string())).is_err(), true);
-        assert_eq!(
-            resolve_target_user(&Some("#1337".to_string())).is_err(),
-            true
-        );
-    }
-
-    #[test]
-    fn test_resolve_target_group() {
-        let current_user = User {
-            uid: 1000,
-            gid: 1000,
-            name: "test".to_string(),
-            gecos: String::new(),
-            home: "/home/test".to_string(),
-            shell: "/bin/sh".to_string(),
-            passwd: String::new(),
-            groups: None,
-            is_default: false,
-        };
-
-        assert_eq!(
-            resolve_target_group(&Some("root".to_string()), &current_user).is_ok(),
-            true
-        );
-        assert_eq!(
-            resolve_target_group(&Some("#1".to_string()), &current_user).is_ok(),
-            true
-        );
-        assert_eq!(
-            resolve_target_group(&Some("#-1".to_string()), &current_user).is_err(),
-            true
-        );
-    }
+    use super::Context;
 
     #[test]
     fn test_build_context() {
         let options = SudoOptions::try_parse_from(["sudo", "echo", "hello"]).unwrap();
-
-        let context = Context::build_from_options(&options).unwrap();
+        let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
+        let context = Context::build_from_options(&options, path).unwrap();
 
         let mut target_environment = HashMap::new();
         target_environment.insert("SUDO_USER".to_string(), context.current_user.name.clone());
diff --git a/lib/sudo-common/src/lib.rs b/lib/sudo-common/src/lib.rs
index b385a90d..cca47814 100644
--- a/lib/sudo-common/src/lib.rs
+++ b/lib/sudo-common/src/lib.rs
@@ -1,4 +1,13 @@
 #![forbid(unsafe_code)]
+use std::collections::HashMap;
 
+pub use command::CommandAndArguments;
+pub use context::Context;
+pub use error::Error;
+
+pub mod command;
 pub mod context;
 pub mod error;
+pub mod resolve;
+
+pub type Environment = HashMap<String, String>;
diff --git a/lib/sudo-common/src/resolve.rs b/lib/sudo-common/src/resolve.rs
new file mode 100644
index 00000000..2e034ce0
--- /dev/null
+++ b/lib/sudo-common/src/resolve.rs
@@ -0,0 +1,208 @@
+use std::{
+    env, fs,
+    os::unix::prelude::MetadataExt,
+    path::{Path, PathBuf},
+    str::FromStr,
+};
+use sudo_system::{Group, User};
+
+use crate::Error;
+
+#[derive(PartialEq, Debug)]
+enum NameOrId<'a, T: FromStr> {
+    Name(&'a str),
+    Id(T),
+}
+
+impl<'a, T: FromStr> NameOrId<'a, T> {
+    pub fn parse(input: &'a str) -> Option<Self> {
+        if input.is_empty() {
+            None
+        } else if let Some(stripped) = input.strip_prefix('#') {
+            stripped.parse::<T>().ok().map(|id| Self::Id(id))
+        } else {
+            Some(Self::Name(input))
+        }
+    }
+}
+
+pub(crate) fn resolve_current_user() -> Result<User, Error> {
+    User::real()?.ok_or(Error::UserNotFound("current user".to_string()))
+}
+
+pub(crate) fn resolve_target_user(target_name_or_id: &Option<String>) -> Result<User, Error> {
+    let is_default = target_name_or_id.is_none();
+    let target_name_or_id = target_name_or_id.as_deref().unwrap_or("root");
+
+    let mut user = match NameOrId::parse(target_name_or_id) {
+        Some(NameOrId::Name(name)) => User::from_name(name)?,
+        Some(NameOrId::Id(uid)) => User::from_uid(uid)?,
+        _ => None,
+    }
+    .ok_or_else(|| Error::UserNotFound(target_name_or_id.to_string()))?;
+
+    user.is_default = is_default;
+
+    Ok(user)
+}
+
+pub(crate) fn resolve_target_group(
+    target_name_or_id: &Option<String>,
+    target_user: &User,
+) -> Result<Group, Error> {
+    match target_name_or_id.as_deref() {
+        Some(name_or_id) => match NameOrId::parse(name_or_id) {
+            Some(NameOrId::Name(name)) => Group::from_name(name)?,
+            Some(NameOrId::Id(gid)) => Group::from_gid(gid)?,
+            _ => None,
+        },
+        None => Group::from_gid(target_user.gid)?,
+    }
+    .ok_or(Error::GroupNotFound(
+        target_name_or_id
+            .clone()
+            .unwrap_or_else(|| target_user.gid.to_string()),
+    ))
+}
+
+/// Check whether a path points to a regular file and any executable flag is set
+fn is_valid_executable(path: &PathBuf) -> bool {
+    if path.is_file() {
+        match fs::metadata(path) {
+            Ok(meta) => meta.mode() & 0o111 != 0,
+            _ => false,
+        }
+    } else {
+        false
+    }
+}
+
+/// Resolve a executable name based in the PATH environment variable
+/// When resolving a path, this code checks whether the target file is
+/// a regular file and has any executable bits set. It does not specifically
+/// check for user, group, or others' executable bit.
+pub(crate) fn resolve_path(command: &Path, path: &str) -> Option<PathBuf> {
+    // To prevent command spoofing, sudo checks "." and "" (both denoting current directory)
+    // last when searching for a command in the user's PATH (if one or both are in the PATH).
+    // Depending on the security policy, the user's PATH environment variable may be modified,
+    // replaced, or passed unchanged to the program that sudo executes.
+    let mut resolve_current_path = false;
+
+    path.split(':')
+        // register whether to look in the current directory, but first check the other PATH segments
+        .filter(|&path| {
+            if path.is_empty() || path == "." {
+                resolve_current_path = true;
+
+                false
+            } else {
+                true
+            }
+        })
+        // construct a possible executable absolute path candidate
+        .map(|path| PathBuf::from(path).join(command))
+        // check whether the candidate is a regular file and any executable flag is set
+        .find(is_valid_executable)
+        // if no no executable could be resolved try the current directory
+        // if it was present in the PATH
+        .or_else(|| {
+            if resolve_current_path {
+                env::current_dir()
+                    .ok()
+                    .map(|dir| dir.join(command))
+                    .and_then(|path| {
+                        if is_valid_executable(&path) {
+                            Some(path)
+                        } else {
+                            None
+                        }
+                    })
+            } else {
+                None
+            }
+        })
+}
+
+#[cfg(test)]
+mod tests {
+    use std::path::PathBuf;
+    use sudo_system::User;
+
+    use super::{resolve_target_group, resolve_target_user, NameOrId};
+    use crate::resolve::resolve_path;
+
+    // this test is platform specific -> should be changed when targetting different platforms
+    #[test]
+    fn test_resolve_path() {
+        let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
+        assert_eq!(
+            resolve_path(&PathBuf::from("sh"), path),
+            Some(PathBuf::from("/usr/bin/sh"))
+        );
+        assert_eq!(
+            resolve_path(&PathBuf::from("sudo"), path),
+            Some(PathBuf::from("/usr/bin/sudo"))
+        );
+        assert_eq!(
+            resolve_path(&PathBuf::from("env"), path),
+            Some(PathBuf::from("/usr/bin/env"))
+        );
+        assert_eq!(
+            resolve_path(&PathBuf::from("thisisnotonyourfs"), path),
+            None
+        );
+        assert_eq!(resolve_path(&PathBuf::from("thisisnotonyourfs"), "."), None);
+    }
+
+    #[test]
+    fn test_name_or_id() {
+        assert_eq!(NameOrId::<u32>::parse(""), None);
+        assert_eq!(NameOrId::<u32>::parse("mies"), Some(NameOrId::Name("mies")));
+        assert_eq!(NameOrId::<u32>::parse("1337"), Some(NameOrId::Name("1337")));
+        assert_eq!(NameOrId::<u32>::parse("#1337"), Some(NameOrId::Id(1337)));
+        assert_eq!(NameOrId::<u32>::parse("#-1"), None);
+    }
+
+    #[test]
+    fn test_resolve_target_user() {
+        assert_eq!(
+            resolve_target_user(&Some("mies".to_string())).is_err(),
+            true
+        );
+        assert_eq!(resolve_target_user(&Some("root".to_string())).is_ok(), true);
+        assert_eq!(resolve_target_user(&Some("#1".to_string())).is_ok(), true);
+        assert_eq!(resolve_target_user(&Some("#-1".to_string())).is_err(), true);
+        assert_eq!(
+            resolve_target_user(&Some("#1337".to_string())).is_err(),
+            true
+        );
+    }
+
+    #[test]
+    fn test_resolve_target_group() {
+        let current_user = User {
+            uid: 1000,
+            gid: 1000,
+            name: "test".to_string(),
+            gecos: String::new(),
+            home: "/home/test".to_string(),
+            shell: "/bin/sh".to_string(),
+            passwd: String::new(),
+            groups: None,
+            is_default: false,
+        };
+
+        assert_eq!(
+            resolve_target_group(&Some("root".to_string()), &current_user).is_ok(),
+            true
+        );
+        assert_eq!(
+            resolve_target_group(&Some("#1".to_string()), &current_user).is_ok(),
+            true
+        );
+        assert_eq!(
+            resolve_target_group(&Some("#-1".to_string()), &current_user).is_err(),
+            true
+        );
+    }
+}
diff --git a/lib/sudo-env/src/environment.rs b/lib/sudo-env/src/environment.rs
index 652d0f8b..6e535379 100644
--- a/lib/sudo-env/src/environment.rs
+++ b/lib/sudo-env/src/environment.rs
@@ -1,6 +1,6 @@
 use std::collections::HashSet;
 
-use sudo_common::context::{CommandAndArguments, Context, Environment};
+use sudo_common::{CommandAndArguments, Context, Environment};
 use sudo_system::PATH_MAX;
 use sudoers::Policy;
 
diff --git a/lib/sudo-exec/src/lib.rs b/lib/sudo-exec/src/lib.rs
index 4f173afe..9f54aff0 100644
--- a/lib/sudo-exec/src/lib.rs
+++ b/lib/sudo-exec/src/lib.rs
@@ -15,7 +15,7 @@ use signal_hook::{
         siginfo::{Cause, Process, Sent},
     },
 };
-use sudo_common::context::{Context, Environment};
+use sudo_common::{Context, Environment};
 use sudo_system::{getpgid, kill, set_target_user};
 
 /// We only handle the signals that ogsudo handles.
diff --git a/lib/sudo-pam/src/converse.rs b/lib/sudo-pam/src/converse.rs
index c8c5ddae..9a3e380f 100644
--- a/lib/sudo-pam/src/converse.rs
+++ b/lib/sudo-pam/src/converse.rs
@@ -6,7 +6,6 @@ use crate::{error::PamResult, PamErrorType};
 
 /// Each message in a PAM conversation will have a message style. Each of these
 /// styles must be handled separately.
-#[derive(Clone, Copy)]
 pub enum PamMessageStyle {
     /// Prompt for input using a message. The input should considered secret
     /// and should be hidden from view.
@@ -167,6 +166,17 @@ pub(crate) struct ConverserData<C> {
     pub(crate) panicked: bool,
 }
 
+impl<C: Converser> ConverserData<C> {
+    /// This function creates a pam_conv struct with the converse function for
+    /// the specific converser.
+    pub(crate) unsafe fn create_pam_conv(self: std::pin::Pin<&mut Self>) -> pam_conv {
+        pam_conv {
+            conv: Some(converse::<C>),
+            appdata_ptr: self.get_unchecked_mut() as *mut ConverserData<C> as *mut libc::c_void,
+        }
+    }
+}
+
 /// This function implements the conversation function of `pam_conv`.
 ///
 /// This function should always be called with an appdata_ptr that implements
@@ -193,7 +203,7 @@ pub(crate) extern "C" fn converse<C: Converser>(
             messages: Vec::with_capacity(num_msg as usize),
         };
         for i in 0..num_msg as isize {
-            let message: &pam_message = unsafe { &**msg.offset(i) };
+            let message: &pam_message = unsafe { &*((*msg).offset(i)) };
 
             let msg = unsafe { sudo_cutils::string_from_ptr(message.msg) };
             let style = if let Some(style) = PamMessageStyle::from_int(message.msg_style) {
@@ -263,167 +273,3 @@ pub(crate) extern "C" fn converse<C: Converser>(
     };
     res.as_int()
 }
-
-#[cfg(test)]
-mod test {
-    use super::*;
-    use std::pin::Pin;
-    use PamMessageStyle::*;
-
-    impl SequentialConverser for String {
-        fn handle_normal_prompt(&self, msg: &str) -> PamResult<String> {
-            Ok(format!("{self} says {msg}"))
-        }
-
-        fn handle_hidden_prompt(&self, msg: &str) -> PamResult<String> {
-            Ok(format!("{self}s secret is {msg}"))
-        }
-
-        fn handle_error(&self, msg: &str) -> PamResult<()> {
-            panic!("{msg}")
-        }
-
-        fn handle_info(&self, _msg: &str) -> PamResult<()> {
-            Ok(())
-        }
-    }
-
-    // essentially do the inverse of the "conversation function"
-    fn dummy_pam(msgs: &[PamMessage], talkie: &pam_conv) -> Vec<Option<String>> {
-        let pam_msgs = msgs
-            .iter()
-            .map(|PamMessage { msg, style, .. }| pam_message {
-                msg: unsafe { sudo_cutils::into_leaky_cstring(msg) },
-                msg_style: *style as i32,
-            })
-            .rev()
-            .collect::<Vec<pam_message>>();
-        let mut ptrs = pam_msgs
-            .iter()
-            .map(|x| x as *const pam_message)
-            .rev()
-            .collect::<Vec<*const pam_message>>();
-
-        let mut raw_response = std::ptr::null_mut::<pam_response>();
-        let conv_err = unsafe {
-            talkie.conv.expect("non-null fn ptr")(
-                ptrs.len() as i32,
-                ptrs.as_mut_ptr(),
-                &mut raw_response,
-                talkie.appdata_ptr,
-            )
-        };
-
-        // deallocate the leaky strings
-        for rec in ptrs {
-            unsafe { libc::free((*rec).msg as *mut _) }
-        }
-        if conv_err != 0 {
-            return vec![];
-        }
-
-        let result = msgs
-            .iter()
-            .enumerate()
-            .map(|(i, _)| unsafe {
-                let ptr = raw_response.add(i);
-                if (*ptr).resp.is_null() {
-                    None
-                } else {
-                    // "The resp_retcode member of this struct is unused and should be set to zero."
-                    assert_eq!((*ptr).resp_retcode, 0);
-                    let response = sudo_cutils::string_from_ptr((*ptr).resp);
-                    libc::free((*ptr).resp as *mut _);
-                    Some(response)
-                }
-            })
-            .collect();
-
-        unsafe { libc::free(raw_response as *mut _) };
-        result
-    }
-
-    fn msg(style: PamMessageStyle, msg: &str) -> PamMessage {
-        let msg = msg.to_string();
-        PamMessage {
-            style,
-            msg,
-            response: None,
-        }
-    }
-
-    // sanity check on the test cases; lib.rs is expected to manage the lifetime of the pointer
-    // inside the pam_conv object explicitly.
-
-    use std::marker::PhantomData;
-    struct PamConvBorrow<'a> {
-        pam_conv: pam_conv,
-        _marker: std::marker::PhantomData<&'a ()>,
-    }
-
-    impl<'a> PamConvBorrow<'a> {
-        fn new<C: Converser>(data: Pin<&'a mut ConverserData<C>>) -> PamConvBorrow<'a> {
-            let appdata_ptr =
-                unsafe { data.get_unchecked_mut() as *mut ConverserData<C> as *mut libc::c_void };
-            PamConvBorrow {
-                pam_conv: pam_conv {
-                    conv: Some(converse::<C>),
-                    appdata_ptr,
-                },
-                _marker: PhantomData,
-            }
-        }
-
-        fn borrow(&self) -> &pam_conv {
-            &self.pam_conv
-        }
-    }
-
-    #[test]
-    fn pam_gpt() {
-        let mut hello = Box::pin(ConverserData {
-            converser: "tux".to_string(),
-            panicked: false,
-        });
-        let cookie = PamConvBorrow::new(hello.as_mut());
-        let pam_conv = cookie.borrow();
-
-        assert_eq!(dummy_pam(&[], pam_conv), vec![]);
-
-        assert_eq!(
-            dummy_pam(&[msg(PromptEchoOn, "hello")], pam_conv),
-            vec![Some("tux says hello".to_string())]
-        );
-
-        assert_eq!(
-            dummy_pam(&[msg(PromptEchoOff, "fish")], pam_conv),
-            vec![Some("tuxs secret is fish".to_string())]
-        );
-
-        assert_eq!(dummy_pam(&[msg(TextInfo, "mars")], pam_conv), vec![None]);
-
-        assert_eq!(
-            dummy_pam(
-                &[
-                    msg(PromptEchoOff, "banging the rocks together"),
-                    msg(TextInfo, ""),
-                    msg(PromptEchoOn, ""),
-                ],
-                pam_conv
-            ),
-            vec![
-                Some("tuxs secret is banging the rocks together".to_string()),
-                None,
-                Some("tux says ".to_string()),
-            ]
-        );
-
-        //assert!(!hello.panicked); // not allowed by borrow checker
-        let real_hello = unsafe { &mut *(pam_conv.appdata_ptr as *mut ConverserData<String>) };
-        assert!(!real_hello.panicked);
-
-        assert_eq!(dummy_pam(&[msg(ErrorMessage, "oops")], pam_conv), vec![]);
-
-        assert!(hello.panicked); // allowed now
-    }
-}
diff --git a/lib/sudo-pam/src/error.rs b/lib/sudo-pam/src/error.rs
index 4b3b8be9..e509972c 100644
--- a/lib/sudo-pam/src/error.rs
+++ b/lib/sudo-pam/src/error.rs
@@ -180,17 +180,3 @@ pub(crate) fn pam_err(err: libc::c_int, handle: *const pam_handle_t) -> Result<(
         Err(PamError::from_pam(err, handle))
     }
 }
-
-#[cfg(test)]
-mod test {
-    use super::PamErrorType;
-
-    #[test]
-    fn isomorphy() {
-        for i in -100..100 {
-            let pam = PamErrorType::from_int(i);
-            assert_eq!(pam.as_int(), i);
-            assert_eq!(PamErrorType::from_int(pam.as_int()), pam);
-        }
-    }
-}
diff --git a/lib/sudo-pam/src/lib.rs b/lib/sudo-pam/src/lib.rs
index 4ea0f74d..e7b29bfa 100644
--- a/lib/sudo-pam/src/lib.rs
+++ b/lib/sudo-pam/src/lib.rs
@@ -1,5 +1,6 @@
 use std::{
     ffi::{CStr, CString},
+    pin::Pin,
     time::Duration,
 };
 
@@ -15,7 +16,8 @@ mod error;
 pub use converse::CLIConverser;
 
 pub struct PamContext<'a, C: Converser> {
-    data_ptr: *mut ConverserData<C>,
+    data: Pin<Box<ConverserData<C>>>,
+    pam_conv: Option<pam_conv>,
     pamh: Option<&'a mut pam_handle_t>,
     silent: bool,
     allow_null_auth_token: bool,
@@ -64,30 +66,28 @@ impl<C: Converser> PamContextBuilder<C> {
                 None => std::ptr::null(),
             };
 
-            // this will be de-allocated explicitly in this type's drop method
-            let data_ptr = Box::into_raw(Box::new(ConverserData {
+            let data = ConverserData {
                 converser,
                 panicked: false,
-            }));
+            };
 
             let mut context = PamContext {
-                data_ptr,
+                data: Box::pin(data),
+                pam_conv: None,
                 pamh: None,
                 silent: false,
                 allow_null_auth_token: true,
                 last_pam_status: None,
                 session_started: false,
             };
+            context.pam_conv = Some(unsafe { context.data.as_mut().create_pam_conv() });
 
             let mut pamh = std::ptr::null_mut();
             let res = unsafe {
                 pam_start(
                     c_service_name.as_ptr(),
                     c_user_ptr,
-                    &pam_conv {
-                        conv: Some(converse::converse::<C>),
-                        appdata_ptr: data_ptr as *mut libc::c_void,
-                    },
+                    &context.pam_conv.unwrap(),
                     &mut pamh,
                 )
             };
@@ -452,11 +452,6 @@ impl<'a, C: Converser> PamContext<'a, C> {
 
         Ok(res)
     }
-
-    /// Check if anything panicked since the last call.
-    pub fn has_panicked(&self) -> bool {
-        unsafe { (*self.data_ptr).panicked }
-    }
 }
 
 impl<'a> PamContext<'a, CLIConverser> {
@@ -468,8 +463,6 @@ impl<'a> PamContext<'a, CLIConverser> {
 
 impl<'a, C: Converser> Drop for PamContext<'a, C> {
     fn drop(&mut self) {
-        // data_ptr's pointee is de-allocated in this scope
-        let _data = unsafe { Box::from_raw(self.data_ptr) };
         if self.pamh.is_some() {
             if !self.session_started {
                 let _ = self.close_session();
@@ -486,5 +479,6 @@ impl<'a, C: Converser> Drop for PamContext<'a, C> {
                 ) | PAM_DATA_SILENT as i32
             };
         }
+        self.pam_conv = None;
     }
 }
diff --git a/lib/sudoers/src/lib.rs b/lib/sudoers/src/lib.rs
index 7fa2e3c5..e47768f0 100644
--- a/lib/sudoers/src/lib.rs
+++ b/lib/sudoers/src/lib.rs
@@ -44,7 +44,8 @@ pub struct Judgement {
 }
 
 mod policy;
-pub use policy::{Authorization, Policy};
+
+pub use policy::{Authorization, Policy, PreJudgementPolicy};
 
 /// This function takes a file argument for a sudoers file and processes it.
 impl Sudoers {
diff --git a/lib/sudoers/src/policy.rs b/lib/sudoers/src/policy.rs
index edcfc236..6224934c 100644
--- a/lib/sudoers/src/policy.rs
+++ b/lib/sudoers/src/policy.rs
@@ -1,3 +1,5 @@
+use crate::Sudoers;
+
 use super::Judgement;
 /// Data types and traits that represent what the "terms and conditions" are after a succesful
 /// permission check.
@@ -47,3 +49,22 @@ impl Policy for Judgement {
             .expect("env_check missing from settings")
     }
 }
+
+pub trait PreJudgementPolicy {
+    fn secure_path(&self) -> Option<&str>;
+}
+
+impl PreJudgementPolicy for Sudoers {
+    fn secure_path(&self) -> Option<&str> {
+        let path = self
+            .settings
+            .str_value
+            .get("secure_path")
+            .expect("secure_path missing from settings");
+        if path.is_empty() {
+            None
+        } else {
+            Some(path)
+        }
+    }
+}
diff --git a/sudo/src/main.rs b/sudo/src/main.rs
index fbab88b7..c067d4ce 100644
--- a/sudo/src/main.rs
+++ b/sudo/src/main.rs
@@ -1,13 +1,12 @@
 #![forbid(unsafe_code)]
 
+use std::env;
+
 use pam::authenticate;
 use sudo_cli::SudoOptions;
-use sudo_common::{
-    context::{Context, Environment},
-    error::Error,
-};
+use sudo_common::{Context, Environment, Error};
 use sudo_env::environment;
-use sudoers::{Authorization, Policy, Sudoers};
+use sudoers::{Authorization, Policy, PreJudgementPolicy, Sudoers};
 
 mod diagnostic;
 mod pam;
@@ -40,34 +39,46 @@ fn check_sudoers(sudoers: &Sudoers, context: &Context) -> sudoers::Judgement {
     )
 }
 
-fn main() -> Result<(), Error> {
-    // parse cli options
-    let sudo_options = SudoOptions::parse();
+/// Resolve the path to use and build a context object from the options
+fn build_context<'a>(
+    sudo_options: &'a SudoOptions,
+    sudoers: &impl PreJudgementPolicy,
+) -> Result<Context<'a>, Error> {
+    let env_path = env::var("PATH").unwrap_or_default();
+    let path = sudoers.secure_path().unwrap_or(&env_path);
+
+    Context::build_from_options(sudo_options, path)
+}
 
+/// show warning message when SUDO_RS_IS_UNSTABLE is not set to the appropriate value
+fn unstable_warning() {
     let check_var = std::env::var("SUDO_RS_IS_UNSTABLE").unwrap_or_else(|_| "".to_string());
 
     if check_var != "I accept that my system may break unexpectedly" {
-        eprintln!("WARNING!");
-        eprintln!("Sudo-rs is in the early stages of development and could potentially break your system.");
-        eprintln!(
-            "We recommend that you do not run this on any production environment. To turn off this"
-        );
-        eprintln!(
-            "warning and start using sudo-rs set the environment variable SUDO_RS_IS_UNSTABLE to"
-        );
         eprintln!(
-            "the value `I accept that my system may break unexpectedly`. If you are unsure how to"
+            "WARNING!
+Sudo-rs is in the early stages of development and could potentially break your system.
+We recommend that you do not run this on any production environment. To turn off this
+warning and start using sudo-rs set the environment variable SUDO_RS_IS_UNSTABLE to
+the value `I accept that my system may break unexpectedly`. If you are unsure how to
+do this then this software is not suited for you at this time."
         );
-        eprintln!("do this then this software is not suited for you at this time.");
+
         std::process::exit(1);
     }
+}
+
+fn main() -> Result<(), Error> {
+    // parse cli options
+    let sudo_options = SudoOptions::parse();
+
+    unstable_warning();
 
     // parse sudoers file
     let sudoers = parse_sudoers()?;
 
-    // build context and environment
-    let current_env = std::env::vars().collect::<Environment>();
-    let context = Context::build_from_options(&sudo_options)?;
+    // build context given a path
+    let context = build_context(&sudo_options, &sudoers)?;
 
     // check sudoers file for permission
     let policy = check_sudoers(&sudoers, &context);
@@ -82,6 +93,8 @@ fn main() -> Result<(), Error> {
         }
     };
 
+    // build environment
+    let current_env = std::env::vars().collect::<Environment>();
     let target_env = environment::get_target_environment(current_env, &context, &policy);
 
     // run command and return corresponding exit code


Test patch:
diff --git a/lib/sudo-env/tests/env_tests.rs b/lib/sudo-env/tests/env_tests.rs
index 6a5acef4..7c0ecad0 100644
--- a/lib/sudo-env/tests/env_tests.rs
+++ b/lib/sudo-env/tests/env_tests.rs
@@ -1,6 +1,6 @@
 use std::collections::HashSet;
 use sudo_cli::SudoOptions;
-use sudo_common::context::{CommandAndArguments, Context, Environment};
+use sudo_common::{CommandAndArguments, Context, Environment};
 use sudo_env::environment::get_target_environment;
 use sudo_system::{Group, User};
 
@@ -74,7 +74,9 @@ fn parse_env_commands(input: &str) -> Vec<(&str, Environment)> {
 }
 
 fn create_test_context<'a>(sudo_options: &'a SudoOptions) -> Context<'a> {
-    let command = CommandAndArguments::try_from(sudo_options.external_args.as_slice()).unwrap();
+    let path = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin";
+    let command =
+        CommandAndArguments::try_from_args(sudo_options.external_args.as_slice(), path).unwrap();
 
     let current_user = User {
         uid: 1000,