Canonicalize paths in the reference frame of the target user (#1234)

Author: squell

src/common/context.rs

@@ -5,7 +5,7 @@ use crate::exec::{RunOptions, Umask};
 #[cfg_attr(not(feature = "sudoedit"), allow(unused_imports))]
 use crate::sudo::{SudoEditOptions, SudoListOptions, SudoRunOptions, SudoValidateOptions};
 use crate::sudoers::Sudoers;
-use crate::system::{Group, Hostname, Process, User};
+use crate::system::{audit::sudo_call, Group, Hostname, Process, User};
 
 use super::{
     command::CommandAndArguments,
@@ -81,7 +81,9 @@ impl Context {
                 system_path.as_ref()
             };
 
-            CommandAndArguments::build_from_args(shell, sudo_options.positional_args, path)
+            sudo_call(&target_user, &target_group, || {
+                CommandAndArguments::build_from_args(shell, sudo_options.positional_args, path)
+            })?
         };
 
         Ok(Context {
@@ -115,18 +117,20 @@ impl Context {
             resolve_target_user_and_group(&sudo_options.user, &sudo_options.group, &current_user)?;
 
         // resolve file arguments; if something can't be resolved, don't add it to the "edit" list
-        let resolved_args = sudo_options.positional_args.iter().map(|arg| {
-            let path = Path::new(arg);
-            let absolute_path;
-            crate::common::resolve::canonicalize_newfile(if path.is_absolute() {
-                path
-            } else {
-                absolute_path = Path::new(".").join(path);
-                &absolute_path
+        let resolved_args = sudo_call(&target_user, &target_group, || {
+            sudo_options.positional_args.iter().map(|arg| {
+                let path = Path::new(arg);
+                let absolute_path;
+                crate::common::resolve::canonicalize_newfile(if path.is_absolute() {
+                    path
+                } else {
+                    absolute_path = Path::new(".").join(path);
+                    &absolute_path
+                })
+                .map_err(|_| arg)
+                .and_then(|path| path.into_os_string().into_string().map_err(|_| arg))
             })
-            .map_err(|_| arg)
-            .and_then(|path| path.into_os_string().into_string().map_err(|_| arg))
-        });
+        })?;
 
         let files_to_edit = resolved_args
             .clone()
@@ -223,7 +227,9 @@ impl Context {
                 system_path.as_ref()
             };
 
-            CommandAndArguments::build_from_args(None, sudo_options.positional_args, path)
+            sudo_call(&target_user, &target_group, || {
+                CommandAndArguments::build_from_args(None, sudo_options.positional_args, path)
+            })?
         };
 
         Ok(Context {
@@ -270,21 +276,21 @@ impl Context {
 
 #[cfg(test)]
 mod tests {
-    use crate::{
-        sudo::SudoAction,
-        system::{interface::UserId, Hostname},
-    };
+    use crate::{common::resolve::CurrentUser, sudo::SudoAction, system::Hostname};
 
     use super::Context;
 
     #[test]
     fn test_build_run_context() {
-        let options = SudoAction::try_parse_from(["sudo", "echo", "hello"])
+        let mut options = SudoAction::try_parse_from(["sudo", "echo", "hello"])
             .unwrap()
             .try_into_run()
             .ok()
             .unwrap();
 
+        let current_user = CurrentUser::resolve().unwrap();
+        options.user = Some(current_user.name.clone());
+
         let context = Context::from_run_opts(options, &mut Default::default()).unwrap();
 
         if cfg!(target_os = "linux") {
@@ -295,6 +301,6 @@ mod tests {
         }
         assert_eq!(context.command.arguments, ["hello"]);
         assert_eq!(context.hostname, Hostname::resolve());
-        assert_eq!(context.target_user.uid, UserId::ROOT);
+        assert_eq!(context.target_user.uid, current_user.uid);
     }
 }

src/system/audit.rs

@@ -17,14 +17,18 @@ use crate::common::resolve::CurrentUser;
 
 /// Temporary change privileges --- essentially a 'mini sudo'
 /// This is only used for sudoedit.
-fn sudo_call<T>(
+pub(crate) fn sudo_call<T>(
     target_user: &User,
     target_group: &Group,
     operation: impl FnOnce() -> T,
 ) -> io::Result<T> {
     const KEEP_UID: libc::uid_t = -1i32 as libc::uid_t;
     const KEEP_GID: libc::gid_t = -1i32 as libc::gid_t;
 
+    // SAFETY: these libc functions are always safe to call
+    let (cur_user_id, cur_group_id) =
+        unsafe { (UserId::new(libc::geteuid()), GroupId::new(libc::getegid())) };
+
     let cur_groups = {
         // SAFETY: calling with size 0 does not modify through the pointer, and is
         // a documented way of getting the length needed.
@@ -43,6 +47,19 @@ fn sudo_call<T>(
     let mut target_groups = target_user.groups.clone();
     inject_group(target_group.gid, &mut target_groups);
 
+    if cfg!(test)
+        && target_user.uid == cur_user_id
+        && target_group.gid == cur_group_id
+        && target_groups
+            .iter()
+            .filter(|x| **x != target_group.gid)
+            .eq(cur_groups.iter().filter(|x| **x != cur_group_id))
+    {
+        // we are not actually switching users, simply run the closure
+        // (this would also be safe in production mode, but it is a needless check)
+        return Ok(operation());
+    }
+
     struct ResetUserGuard(UserId, GroupId, Vec<GroupId>);
 
     impl Drop for ResetUserGuard {
@@ -59,14 +76,7 @@ fn sudo_call<T>(
         }
     }
 
-    // SAFETY: these libc functions are always safe to call
-    let guard = unsafe {
-        ResetUserGuard(
-            UserId::new(libc::geteuid()),
-            GroupId::new(libc::getegid()),
-            cur_groups,
-        )
-    };
+    let guard = ResetUserGuard(cur_user_id, cur_group_id, cur_groups);
 
     set_supplementary_groups(&target_groups)?;
     // SAFETY: this function is always safe to call