Use drop_privilege wrapper (#1528)

Author: squell

src/pam/askpass.rs

@@ -9,7 +9,7 @@ use libc::O_CLOEXEC;
 use crate::cutils::cerr;
 use crate::log::user_error;
 use crate::system::interface::ProcessId;
-use crate::system::{ForkResult, fork, mark_fds_as_cloexec};
+use crate::system::{ForkResult, audit, fork, mark_fds_as_cloexec};
 
 pub(super) fn spawn_askpass(program: &Path, prompt: &str) -> io::Result<(ProcessId, OwnedFd)> {
     // Create socket
@@ -38,17 +38,15 @@ pub(super) fn spawn_askpass(program: &Path, prompt: &str) -> io::Result<(Process
 }
 
 fn handle_child(program: &Path, prompt: &str, stdout: OwnedFd) -> ! {
-    // Drop root privileges.
-    // SAFETY: setuid does not change any memory and only affects OS state.
-    unsafe {
-        libc::setuid(libc::getuid());
-    }
-
     if let Err(e) = mark_fds_as_cloexec() {
         eprintln_ignore_io_error!("Failed to mark fds as CLOEXEC: {e}");
         process::exit(1);
     };
 
+    // root privileges are dangerous after this point, since we are about to
+    // execute a command under control of the user, so drop them
+    audit::irrevocably_drop_privileges();
+
     // Exec askpass program
     let error = Command::new(program).arg(prompt).stdout(stdout).exec();
     user_error!(

src/sudo/edit.rs

@@ -14,7 +14,7 @@ use crate::exec::ExitReason;
 use crate::log::{user_error, user_info};
 use crate::system::file::{FileLock, create_temporary_dir};
 use crate::system::wait::{Wait, WaitError, WaitOptions};
-use crate::system::{ForkResult, fork, mark_fds_as_cloexec};
+use crate::system::{ForkResult, audit, fork, mark_fds_as_cloexec};
 
 struct ParentFileInfo<'a> {
     path: &'a Path,
@@ -206,11 +206,9 @@ fn handle_child_inner(
 ) -> Result<(), String> {
     mark_fds_as_cloexec().map_err(|e| format!("Failed to mark fds as CLOEXEC: {e}"))?;
 
-    // Drop root privileges.
-    // SAFETY: setuid does not change any memory and only affects OS state.
-    unsafe {
-        libc::setuid(libc::getuid());
-    }
+    // root privileges are dangerous after this point, since we are about to manipulate the
+    // file system and execute a command under control of the user, so drop them
+    audit::irrevocably_drop_privileges();
 
     let tempdir = TempDirDropGuard(
         create_temporary_dir()

src/system/audit.rs

@@ -95,6 +95,24 @@ pub(crate) fn sudo_call<T>(
     Ok(result)
 }
 
+/// Drop root privileges by setting effective user id equal to the real user id.
+/// This routine will panic if the process is not privileged.
+pub fn irrevocably_drop_privileges() {
+    // SAFETY:
+    // - getuid() and geteuid() are always safe to call
+    // - setuid() does not change any memory and only affects OS state.
+    unsafe {
+        let real_uid = libc::getuid();
+        let effective_uid = libc::geteuid();
+        // we never use setuid/setgid/setgroups except in the pre_exec hook before exec'ing,
+        // or in sudo_call (which always resets the user state after the closure finishes).
+        // this extra check is here to punish programming mistakes due to sloppiness.
+        assert_eq!(effective_uid, UserId::ROOT.inner(), "setuid violation");
+
+        cerr(libc::setuid(real_uid)).expect("setuid violation");
+    }
+}
+
 // of course we can also write "file & 0o040 != 0", but this makes the intent explicit
 enum Op {
     Read = 4,