Harden secure path validation

src/main.rs

@@ -1,7 +1,7 @@
 use getopts::Options;
 use nix::sys::resource::{self, Resource, rlim_t};
-use nix::sys::stat::{self, Mode};
-use nix::unistd::{self, Uid, Gid, User};
+use nix::sys::stat::{self, Mode, SFlag};
+use nix::unistd::{self, AccessFlags, Uid, Gid, User};
 use std::{env, process};
 use std::collections::BTreeMap;
 use std::ffi::CString;
@@ -106,33 +106,44 @@ fn parse_header(path: &Path) -> io::Result<(Vec<String>, Vec<String>)> {
 }
 
 fn validate_secure_path(path: &Path) -> io::Result<()> {
-    if !path.exists() {
-        return Err(io::Error::new(io::ErrorKind::NotFound, "No such file or directory"));
-    }
-    let base_path = if path.is_relative() {
-        std::env::current_dir()?
+    unistd::access(path, AccessFlags::X_OK)
+        .map_err(|err| io::Error::from_raw_os_error(err as i32))?;
+
+    let full_path = if path.is_relative() {
+        std::env::current_dir()?.join(path)
     } else {
-        PathBuf::new()
+        path.to_path_buf()
     };
     let mut current_path = PathBuf::new();
-    for component in base_path.components().chain(path.components()) {
+    for component in full_path.components() {
         current_path.push(component);
         let stat = stat::lstat(&current_path)?;
+        let file_type = SFlag::from_bits_truncate(stat.st_mode);
+        let mode = Mode::from_bits_truncate(stat.st_mode);
+        let insecure_directory = file_type.contains(SFlag::S_IFDIR)
+            && mode.contains(Mode::S_IWOTH)
+            && !mode.contains(Mode::S_ISVTX);
         if stat.st_uid != 0 {
-            return Err(io::Error::new(io::ErrorKind::Other, "File not hierarchically owned by root"));
+            return Err(io::Error::new(io::ErrorKind::Other, format!("Path is insecure: {} is not root-owned", current_path.display())));
+        }
+        if insecure_directory {
+            return Err(io::Error::new(io::ErrorKind::Other, format!("Path is insecure: {} is a world-writable non-sticky directory", current_path.display())));
         }
     }
-    if check_path_in_nosuid_mount(path)? {
-        return Err(io::Error::new(io::ErrorKind::Other, "File is in a nosuid mount"));
+
+    let (mount_point, mount_options) = path_mount(path)?;
+    if mount_options.split(',').any(|opt| opt == "nosuid") {
+        return Err(io::Error::new(io::ErrorKind::Other, format!("Path is in a nosuid mount: {mount_point}")));
     }
+
     Ok(())
 }
 
-fn check_path_in_nosuid_mount(path: &Path) -> io::Result<bool> {
+fn path_mount(path: &Path) -> io::Result<(String, String)> {
     let path = path.canonicalize().unwrap();
     let mounts = File::open("/proc/self/mounts")?;
     let reader = io::BufReader::new(mounts);
-    if let Some(mount_options) = reader
+    reader
         .lines()
         .collect::<Result<Vec<_>, _>>()?
         .into_iter()
@@ -142,17 +153,13 @@ fn check_path_in_nosuid_mount(path: &Path) -> io::Result<bool> {
             if parts.len() >= 4 {
                 let (point, options) = (&parts[1], &parts[3]);
                 if path.starts_with(point) {
-                    return Some(options.to_string());
+                    return Some((point.to_string(), options.to_string()));
                 }
             }
             None
         })
         .next()
-    {
-        Ok(mount_options.split(',').any(|opt| opt == "nosuid"))
-    } else {
-        Err(io::Error::new(io::ErrorKind::Other, "File not in any mount"))
-    }
+        .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Path is not in any mount"))
 }
 
 fn build_safe_env(uid: Uid, env_overrides: &[String]) -> Result<Vec<CString>, String> {

tests/conftest.py

@@ -9,7 +9,9 @@
 @pytest.fixture
 def run_program():
     def _run(script, script_permissions=0o4755, **popen_kwargs):
-        if popen_kwargs.get("executable") is not None:
+        if popen_kwargs.get("script_path") is not None:
+            executable_path = str(popen_kwargs.pop("script_path"))
+        elif popen_kwargs.get("executable") is not None:
             executable_path = str(popen_kwargs["executable"])
         else:
             executable_path = str(Path("/tmp") / f"program_{uuid.uuid4().hex}")
@@ -22,16 +24,17 @@ def _run(script, script_permissions=0o4755, **popen_kwargs):
         script_path.chmod(script_permissions)
 
         popen_kwargs["stdout"] = subprocess.PIPE
+        popen_kwargs["stderr"] = subprocess.PIPE
         popen_kwargs["text"] = True
         popen_kwargs.setdefault("executable", executable_path)
         popen_kwargs.setdefault("args", [executable_path])
         popen_kwargs.setdefault("user", 1000)
         popen_kwargs.setdefault("group", 1000)
         try:
             process = subprocess.Popen(**popen_kwargs)
-            stdout, _ = process.communicate()
+            stdout, stderr = process.communicate()
             if process.returncode != 0:
-                raise subprocess.CalledProcessError(process.returncode, popen_kwargs["args"], output=stdout)
+                raise subprocess.CalledProcessError(process.returncode, popen_kwargs["args"], output=stdout, stderr=stderr)
         finally:
             script_path.unlink(missing_ok=True)
 

tests/test_secure_path.py

@@ -0,0 +1,67 @@
+import shutil
+import subprocess
+import uuid
+from pathlib import Path
+
+import pytest
+
+
+def test_insecure_world_writable_directory_rejected(run_program):
+    directory = Path("/tmp") / f"exec_suid_insecure_{uuid.uuid4().hex}"
+    directory.mkdir()
+    directory.chmod(0o777)
+    try:
+        script = directory / "program"
+        with pytest.raises(subprocess.CalledProcessError) as error:
+            run_program(
+                """
+                #!/usr/bin/exec-suid -- /bin/bash -p
+
+                printf 'ok\\n'
+                """,
+                executable=str(script),
+            )
+        assert "Path is insecure:" in error.value.stderr
+        assert "is a world-writable non-sticky directory" in error.value.stderr
+    finally:
+        shutil.rmtree(directory, ignore_errors=True)
+
+
+def test_sticky_world_writable_directory_allowed(run_program):
+    directory = Path("/tmp") / f"exec_suid_sticky_{uuid.uuid4().hex}"
+    directory.mkdir()
+    directory.chmod(0o1777)
+    try:
+        script = directory / "program"
+        assert run_program(
+            """
+            #!/usr/bin/exec-suid -- /bin/bash -p
+
+            printf 'ok\\n'
+            """,
+            executable=str(script),
+        ) == "ok"
+    finally:
+        shutil.rmtree(directory, ignore_errors=True)
+
+
+def test_direct_invocation_requires_script_execute_permission(run_program):
+    directory = Path("/tmp") / f"exec_suid_noexec_{uuid.uuid4().hex}"
+    directory.mkdir()
+    try:
+        script = directory / "program"
+        with pytest.raises(subprocess.CalledProcessError) as error:
+            run_program(
+                """
+                #!/usr/bin/exec-suid -- /bin/bash -p
+
+                printf 'ok\\n'
+                """,
+                script_permissions=0o4700,
+                script_path=str(script),
+                executable="/usr/bin/exec-suid",
+                args=["/usr/bin/exec-suid", str(script)],
+            )
+        assert "Permission denied" in error.value.stderr
+    finally:
+        shutil.rmtree(directory, ignore_errors=True)