Fix --preserve-fds, eliminate stray FD being passed into container

crates/libcontainer/src/process/container_init_process.rs

@@ -508,6 +508,9 @@ pub fn container_init_process(
     // will be closed after execve into the container payload. We can't close the
     // fds immediately since we at least still need it for the pipe used to wait on
     // starting the container.
+    //
+    // Note: this should happen very late, in order to avoid accidentally leaking FDs
+    // Please refer to https://github.com/opencontainers/runc/security/advisories/GHSA-xr7r-f8xq-vfvv for more details.
     syscall.close_range(preserve_fds).map_err(|err| {
         tracing::error!(?err, "failed to cleanup extra fds");
         InitProcessError::SyscallOther(err)

crates/libcontainer/src/process/container_main_process.rs

@@ -75,15 +75,6 @@ pub fn container_main_process(container_args: &ContainerArgs) -> Result<(Pid, bo
         })
     };
 
-    // Before starting the intermediate process, mark all non-stdio open files as O_CLOEXEC
-    // to ensure we don't leak any file descriptors to the intermediate process.
-    // Please refer to https://github.com/opencontainers/runc/security/advisories/GHSA-xr7r-f8xq-vfvv for more details.
-    let syscall = container_args.syscall.create_syscall();
-    syscall.close_range(0).map_err(|err| {
-        tracing::error!(?err, "failed to cleanup extra fds");
-        ProcessError::SyscallOther(err)
-    })?;
-
     let intermediate_pid = fork::container_clone(cb).map_err(|err| {
         tracing::error!("failed to fork intermediate process: {}", err);
         ProcessError::IntermediateProcessFailed(err)

crates/libcontainer/src/syscall/linux.rs

@@ -17,7 +17,7 @@ use nix::fcntl::{open, OFlag};
 use nix::mount::{mount, umount2, MntFlags, MsFlags};
 use nix::sched::{unshare, CloneFlags};
 use nix::sys::stat::{mknod, Mode, SFlag};
-use nix::unistd::{chown, chroot, fchdir, pivot_root, sethostname, Gid, Uid};
+use nix::unistd::{chown, chroot, close, fchdir, pivot_root, sethostname, Gid, Uid};
 use oci_spec::runtime::PosixRlimit;
 
 use super::{Result, Syscall, SyscallError};
@@ -232,11 +232,15 @@ impl Syscall for LinuxSyscall {
     /// Function to set given path as root path inside process
     fn pivot_rootfs(&self, path: &Path) -> Result<()> {
         // open the path as directory and read only
-        let newroot =
-            open(path, OFlag::O_DIRECTORY | OFlag::O_RDONLY, Mode::empty()).map_err(|errno| {
-                tracing::error!(?errno, ?path, "failed to open the new root for pivot root");
-                errno
-            })?;
+        let newroot = open(
+            path,
+            OFlag::O_DIRECTORY | OFlag::O_RDONLY | OFlag::O_CLOEXEC,
+            Mode::empty(),
+        )
+        .map_err(|errno| {
+            tracing::error!(?errno, ?path, "failed to open the new root for pivot root");
+            errno
+        })?;
 
         // make the given path as the root directory for the container
         // see https://man7.org/linux/man-pages/man2/pivot_root.2.html, specially the notes
@@ -279,6 +283,11 @@ impl Syscall for LinuxSyscall {
             errno
         })?;
 
+        close(newroot).map_err(|errno| {
+            tracing::error!(?errno, ?newroot, "failed to close new root directory");
+            errno
+        })?;
+
         Ok(())
     }
 

tests/contest/contest/src/main.rs

@@ -12,6 +12,7 @@ use tests::cgroups;
 use crate::tests::devices::get_devices_test;
 use crate::tests::domainname::get_domainname_tests;
 use crate::tests::example::get_example_test;
+use crate::tests::fd_control::get_fd_control_test;
 use crate::tests::hooks::get_hooks_tests;
 use crate::tests::hostname::get_hostname_test;
 use crate::tests::intel_rdt::get_intel_rdt_test;
@@ -113,6 +114,7 @@ fn main() -> Result<()> {
     let scheduler = get_scheduler_test();
     let io_priority_test = get_io_priority_test();
     let devices = get_devices_test();
+    let fd_control = get_fd_control_test();
 
     tm.add_test_group(Box::new(cl));
     tm.add_test_group(Box::new(cc));
@@ -136,6 +138,7 @@ fn main() -> Result<()> {
     tm.add_test_group(Box::new(sysctl));
     tm.add_test_group(Box::new(scheduler));
     tm.add_test_group(Box::new(devices));
+    tm.add_test_group(Box::new(fd_control));
 
     tm.add_test_group(Box::new(io_priority_test));
     tm.add_cleanup(Box::new(cgroups::cleanup_v1));

tests/contest/contest/src/tests/fd_control/mod.rs

@@ -0,0 +1,112 @@
+use std::fs;
+use std::os::fd::{AsRawFd, RawFd};
+
+use anyhow::{anyhow, Context, Result};
+use oci_spec::runtime::{ProcessBuilder, Spec, SpecBuilder};
+use test_framework::{test_result, ConditionalTest, Test, TestGroup, TestResult};
+
+use crate::utils::{is_runtime_runc, test_inside_container_create_args};
+
+fn create_spec() -> Result<Spec> {
+    SpecBuilder::default()
+        .process(
+            ProcessBuilder::default()
+                .args(
+                    ["runtimetest", "fd_control"]
+                        .iter()
+                        .map(|s| s.to_string())
+                        .collect::<Vec<String>>(),
+                )
+                .build()?,
+        )
+        .build()
+        .context("failed to create spec")
+}
+
+fn open_devnull_no_cloexec() -> Result<(fs::File, RawFd)> {
+    // Rust std by default sets cloexec, so we undo it
+    let devnull = fs::File::open("/dev/null")?;
+    let devnull_fd = devnull.as_raw_fd();
+    let flags = nix::fcntl::fcntl(devnull_fd, nix::fcntl::FcntlArg::F_GETFD)?;
+    let mut flags = nix::fcntl::FdFlag::from_bits_retain(flags);
+    flags.remove(nix::fcntl::FdFlag::FD_CLOEXEC);
+    nix::fcntl::fcntl(devnull_fd, nix::fcntl::FcntlArg::F_SETFD(flags))?;
+    Ok((devnull, devnull_fd))
+}
+
+// If not opening any other FDs, verify youki itself doesnt open anything that gets
+// leaked in if passing --preserve-fds with a large number
+// NOTE: this will also fail if the test harness itself starts leaking FDs
+fn only_stdio_test() -> TestResult {
+    let spec = test_result!(create_spec());
+    test_inside_container_create_args(
+        spec,
+        &|bundle_path| {
+            fs::write(bundle_path.join("num-fds"), "0".as_bytes())?;
+            Ok(())
+        },
+        &["--preserve-fds".as_ref(), "100".as_ref()],
+    )
+}
+
+// If we know we have an open FD without cloexec, it should be closed if preserve-fds
+// is 0 (the default)
+fn closes_fd_test() -> TestResult {
+    // Open this before the setup function so it's kept alive for the container lifetime
+    let (_devnull, _devnull_fd) = match open_devnull_no_cloexec() {
+        Ok(v) => v,
+        Err(e) => return TestResult::Failed(anyhow!("failed to open dev null: {}", e)),
+    };
+
+    let spec = test_result!(create_spec());
+    test_inside_container_create_args(
+        spec,
+        &|bundle_path| {
+            fs::write(bundle_path.join("num-fds"), "0".as_bytes())?;
+            Ok(())
+        },
+        &["--preserve-fds".as_ref(), "0".as_ref()],
+    )
+}
+
+// Given an open FD, verify it can be passed down with preserve-fds
+fn pass_single_fd_test() -> TestResult {
+    // Open this before the setup function so it's kept alive for the container lifetime
+    let (_devnull, devnull_fd) = match open_devnull_no_cloexec() {
+        Ok(v) => v,
+        Err(e) => return TestResult::Failed(anyhow!("failed to open dev null: {}", e)),
+    };
+
+    let spec = test_result!(create_spec());
+    test_inside_container_create_args(
+        spec,
+        &|bundle_path| {
+            fs::write(bundle_path.join("num-fds"), "1".as_bytes())?;
+            Ok(())
+        },
+        &[
+            "--preserve-fds".as_ref(),
+            (devnull_fd - 2).to_string().as_ref(), // relative to stdio
+        ],
+    )
+}
+
+pub fn get_fd_control_test() -> TestGroup {
+    let mut test_group = TestGroup::new("fd_control");
+    test_group.set_nonparallel(); // fds are process-wide state
+    let test_only_stdio = ConditionalTest::new(
+        "only_stdio",
+        // runc errors if any of the N passed FDs via preserve-fd are not currently open
+        Box::new(|| !is_runtime_runc()),
+        Box::new(only_stdio_test),
+    );
+    let test_closes_fd = Test::new("closes_fd", Box::new(closes_fd_test));
+    let test_pass_single_fd = Test::new("pass_single_fd", Box::new(pass_single_fd_test));
+    test_group.add(vec![Box::new(test_only_stdio)]);
+    test_group.add(vec![
+        Box::new(test_closes_fd),
+        Box::new(test_pass_single_fd),
+    ]);
+
+    test_group
+}

tests/contest/contest/src/tests/hooks/invoke.rs

@@ -71,7 +71,10 @@ fn get_test(test_name: &'static str) -> Test {
             let id_str = id.to_string();
             let bundle = prepare_bundle().unwrap();
             set_config(&bundle, &spec).unwrap();
-            create_container(&id_str, &bundle).unwrap().wait().unwrap();
+            create_container(&id_str, &bundle, &[])
+                .unwrap()
+                .wait()
+                .unwrap();
             start_container(&id_str, &bundle).unwrap().wait().unwrap();
             delete_container(&id_str, &bundle).unwrap().wait().unwrap();
             let log = {

tests/contest/contest/src/tests/lifecycle/container_create.rs

@@ -82,6 +82,10 @@ impl TestableGroup for ContainerCreate {
         "create"
     }
 
+    fn parallel(&self) -> bool {
+        true
+    }
+
     fn run_all(&self) -> Vec<(&'static str, TestResult)> {
         vec![
             ("empty_id", self.create_empty_id()),

tests/contest/contest/src/tests/lifecycle/container_lifecycle.rs

@@ -93,6 +93,10 @@ impl TestableGroup for ContainerLifecycle {
         "lifecycle"
     }
 
+    fn parallel(&self) -> bool {
+        true
+    }
+
     fn run_all(&self) -> Vec<(&'static str, TestResult)> {
         vec![
             ("create", self.create()),

tests/contest/contest/src/tests/mod.rs

@@ -2,6 +2,7 @@ pub mod cgroups;
 pub mod devices;
 pub mod domainname;
 pub mod example;
+pub mod fd_control;
 pub mod hooks;
 pub mod hostname;
 pub mod intel_rdt;

tests/contest/contest/src/tests/pidfile/pidfile_test.rs

@@ -1,12 +1,11 @@
 use std::fs::File;
-use std::process::{Command, Stdio};
 
 use anyhow::anyhow;
 use test_framework::{Test, TestGroup, TestResult};
 use uuid::Uuid;
 
 use crate::utils::{
-    delete_container, generate_uuid, get_runtime_path, get_state, kill_container, prepare_bundle,
+    create_container, delete_container, generate_uuid, get_state, kill_container, prepare_bundle,
     State,
 };
 
@@ -19,6 +18,7 @@ fn cleanup(id: &Uuid, bundle: &tempfile::TempDir) {
 
 // here we have to manually create and manage the container
 // as the test_inside container does not provide a way to set the pid file argument
+// TODO: this comment is now out of date, the test just needs updating
 fn test_pidfile() -> TestResult {
     // create id for the container and pidfile
     let container_id = generate_uuid();
@@ -30,22 +30,14 @@ fn test_pidfile() -> TestResult {
     let _ = File::create(&pidfile_path).unwrap();
 
     // start the container
-    Command::new(get_runtime_path())
-        .stdin(Stdio::null())
-        .stdout(Stdio::null())
-        .stderr(Stdio::null())
-        .arg("--root")
-        .arg(bundle.as_ref().join("runtime"))
-        .arg("create")
-        .arg(container_id.to_string())
-        .arg("--bundle")
-        .arg(bundle.as_ref().join("bundle"))
-        .arg("--pid-file")
-        .arg(pidfile_path)
-        .spawn()
-        .unwrap()
-        .wait()
-        .unwrap();
+    create_container(
+        &container_id.to_string(),
+        &bundle,
+        &["--pid-file".as_ref(), pidfile_path.as_ref()],
+    )
+    .unwrap()
+    .wait()
+    .unwrap();
 
     let (out, err) = get_state(&container_id.to_string(), &bundle).unwrap();
 

tests/contest/contest/src/utils/mod.rs

@@ -7,5 +7,5 @@ pub use support::{
 };
 pub use test_utils::{
     create_container, delete_container, get_state, kill_container, test_inside_container,
-    test_outside_container, State,
+    test_inside_container_create_args, test_outside_container, State,
 };

tests/contest/contest/src/utils/test_utils.rs

@@ -1,6 +1,7 @@
 //! Contains utility functions for testing
 //! Similar to https://github.com/opencontainers/runtime-tools/blob/master/validation/util/test.go
 use std::collections::HashMap;
+use std::ffi::OsStr;
 use std::path::{Path, PathBuf};
 use std::process::{Child, Command, ExitStatus, Stdio};
 use std::thread::sleep;
@@ -43,7 +44,7 @@ pub struct ContainerData {
 }
 
 /// Starts the runtime with given directory as root directory
-pub fn create_container<P: AsRef<Path>>(id: &str, dir: P) -> Result<Child> {
+pub fn create_container<P: AsRef<Path>>(id: &str, dir: P, extra_args: &[&OsStr]) -> Result<Child> {
     let res = Command::new(get_runtime_path())
         // set stdio so that we can get o/p of runtimetest
         // in test_inside_container function
@@ -55,6 +56,7 @@ pub fn create_container<P: AsRef<Path>>(id: &str, dir: P) -> Result<Child> {
         .arg(id)
         .arg("--bundle")
         .arg(dir.as_ref().join("bundle"))
+        .args(extra_args)
         .spawn()
         .context("could not create container")?;
     Ok(res)
@@ -121,7 +123,7 @@ pub fn test_outside_container(
     let id_str = id.to_string();
     let bundle = prepare_bundle().unwrap();
     set_config(&bundle, &spec).unwrap();
-    let create_result = create_container(&id_str, &bundle).unwrap().wait();
+    let create_result = create_container(&id_str, &bundle, &[]).unwrap().wait();
     let (out, err) = get_state(&id_str, &bundle).unwrap();
     let state: Option<State> = match serde_json::from_str(&out) {
         Ok(v) => Some(v),
@@ -143,6 +145,14 @@ pub fn test_outside_container(
 pub fn test_inside_container(
     spec: Spec,
     setup_for_test: &dyn Fn(&Path) -> Result<()>,
+) -> TestResult {
+    test_inside_container_create_args(spec, setup_for_test, &[])
+}
+
+pub fn test_inside_container_create_args(
+    spec: Spec,
+    setup_for_test: &dyn Fn(&Path) -> Result<()>,
+    create_args: &[&OsStr],
 ) -> TestResult {
     let id = generate_uuid();
     let id_str = id.to_string();
@@ -176,7 +186,7 @@ pub fn test_inside_container(
             .join("runtimetest"),
     )
     .unwrap();
-    let create_process = create_container(&id_str, &bundle).unwrap();
+    let create_process = create_container(&id_str, &bundle, create_args).unwrap();
     // here we do not wait for the process by calling wait() as in the test_outside_container
     // function because we need the output of the runtimetest. If we call wait, it will return
     // and we won't have an easy way of getting the stdio of the runtimetest.

tests/contest/runtimetest/src/main.rs

@@ -44,6 +44,7 @@ fn main() {
         "io_priority_class_be" => tests::test_io_priority_class(&spec, IoprioClassBe),
         "io_priority_class_idle" => tests::test_io_priority_class(&spec, IoprioClassIdle),
         "devices" => tests::validate_devices(&spec),
+        "fd_control" => tests::validate_fd_control(&spec),
         _ => eprintln!("error due to unexpected execute test name: {execute_test}"),
     }
 }