Merge pull request #63 from bleggett/bleggett/fix-gidset

Must change GID *before* changing UID, to avoid permission errors

Author: kaniini

src/caps.rs

@@ -219,6 +219,7 @@ impl AsRef<str> for CapabilityBit {
 
 pub const PR_SET_SECUREBITS: i32 = 28;
 pub const SECBIT_KEEP_CAPS: i32 = 16;
+pub const SECBIT_NO_SETUID_FIXUP: i32 = 4;
 pub const SECBIT_KEEP_CAPS_LOCKED: i32 = 32;
 
 /* from <unistd.h> */
@@ -418,10 +419,10 @@ pub fn set_caps(caps: CapResult) -> anyhow::Result<()> {
 }
 
 pub fn set_keep_caps() -> anyhow::Result<()> {
-    let ret = unsafe { libc::prctl(PR_SET_SECUREBITS, SECBIT_KEEP_CAPS) };
+    let ret = unsafe { libc::prctl(PR_SET_SECUREBITS, SECBIT_NO_SETUID_FIXUP) };
     if ret < 0 {
         Err(anyhow!(
-            "failed to set SECBIT_KEEP_CAPS: {}",
+            "failed to set SECBIT_NO_SETUID_FIXUP: {}",
             io::Error::last_os_error()
         ))
     } else {

src/wrap.rs

@@ -11,7 +11,7 @@ use std::path::PathBuf;
 use std::process;
 use std::ptr;
 
-use crate::caps::{CapabilityBit, get_caps, set_caps};
+use crate::caps::{CapabilityBit, get_caps, set_caps, set_keep_caps};
 use crate::cgroup::CGroup;
 use crate::config::{
     AttachRequest, Capabilities, CreateDirMutation, CreateRequest, ExecutableSpec, IdMapping,
@@ -508,8 +508,6 @@ impl Wrappable for CreateRequest {
             unshare(&vec![Namespace::User])?;
         }
 
-        apply_capabilities(self.capabilities.as_ref())?;
-
         debug!("signalling supervisor to do configuration");
         pef.write_all(&2_u64.to_ne_bytes())?;
         pef.flush()?;
@@ -520,6 +518,17 @@ impl Wrappable for CreateRequest {
         let mut buf = [0u8; 8];
         cef.read_exact(&mut buf)?;
 
+        // We need to toggle SECBIT before we change UID/GID,
+        // or else changing UID/GID may cause us to lose the capabilities
+        // we need to explicitly drop capabilities later on.
+        set_keep_caps()?;
+        // Set these *first*, before we exec. Otherwise
+        // we may not be able to switch after dropping caps.
+        apply_gid_uid(self.exec.gid, self.exec.uid)?;
+        // Now, we can synchronize effective/inherited/permitted caps
+        // as a final step.
+        apply_capabilities(self.capabilities.as_ref())?;
+
         debug!("ready to launch workload");
         self.exec.execute()
     }
@@ -556,26 +565,6 @@ impl ExecutableSpec {
         let mut env_charptrs: Vec<_> = env_cstrings.iter().map(|arg| arg.as_ptr()).collect();
         env_charptrs.push(ptr::null());
 
-        if let Some(target_uid) = self.uid {
-            unsafe {
-                // Check this to avoid a spurious log if we don't need to change,
-                // because we are already running as the target UID.
-                if libc::getuid() != target_uid && libc::setuid(target_uid as libc::uid_t) < 0 {
-                    warn!("unable to set target UID: {:?}", Error::last_os_error());
-                }
-            }
-        }
-
-        if let Some(target_gid) = self.gid {
-            unsafe {
-                // Check this to avoid a spurious log if we don't need to change,
-                // because we are already running as the target GID.
-                if libc::getgid() != target_gid && libc::setgid(target_gid as libc::gid_t) < 0 {
-                    warn!("unable to set target GID: {:?}", Error::last_os_error());
-                }
-            }
-        }
-
         if let Some(wd) = &self.working_directory {
             env::set_current_dir(wd.clone())?;
         }
@@ -718,6 +707,32 @@ impl Mutatable for CreateDirMutation {
     }
 }
 
+fn apply_gid_uid(gid: Option<u32>, uid: Option<u32>) -> Result<()> {
+    // NOTE - order is important here - must change GID *before* changing UID, to avoid
+    // locking oneself out of the GID change with an "operation not permitted" error
+    if let Some(target_gid) = gid {
+        unsafe {
+            // Check this to avoid a spurious log if we don't need to change,
+            // because we are already running as the target GID.
+            if libc::getgid() != target_gid && libc::setgid(target_gid as libc::gid_t) < 0 {
+                warn!("unable to set target GID: {:?}", Error::last_os_error());
+            }
+        }
+    }
+
+    if let Some(target_uid) = uid {
+        unsafe {
+            // Check this to avoid a spurious log if we don't need to change,
+            // because we are already running as the target UID.
+            if libc::getuid() != target_uid && libc::setuid(target_uid as libc::uid_t) < 0 {
+                warn!("unable to set target UID: {:?}", Error::last_os_error());
+            }
+        }
+    }
+
+    Ok(())
+}
+
 fn apply_capabilities(capabilities: Option<&Capabilities>) -> Result<()> {
     let Some(caps) = capabilities else {
         return Ok(());