fix: exec the OCI runtime instead of spawning it

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## v0.3.0
+
+- The OCI runtime is now `exec`'d rather than spawned as a child process. containerd's shim invokes the runtime through go-runc, which builds commands with Go's `exec.CommandContext`; a cancelled or timed-out call delivers SIGKILL to the direct child only, so the interceptor died and the real runtime was orphaned beneath it. For a `runc delete` that left the task undeleted and its `containerd-shim-runc-v2` resident indefinitely, at roughly 5 MiB each until the host was rebooted.
+- A runtime killed by a signal is now reported as signal death. Previously it was mapped to exit code -1, which `process::exit` truncates to 255 — indistinguishable from a runtime that genuinely exited 255.
+- **Deployment change required for the above to take effect.** Docker generates its own non-`exec` `/bin/sh` wrapper for any runtime whose `runtimeArgs` is non-empty, which re-inserts exactly the process layer this release removes. Pass the interceptor's flags from a wrapper script of your own that `exec`s, name that script as the runtime's `path`, and leave `runtimeArgs` unset. See "Passing flags" in the README.
+- Networking mounts (`/etc/hosts`, `/etc/hostname`, `/etc/resolv.conf`) are now always made read-only. This is a behaviour change for anyone who did not previously pass `--oi-readonly-networking-mounts`, and there is no opt-out. The flag is retained as an accepted no-op so an existing `daemon.json` that passes it keeps working. The mounts themselves are left in place, so Docker's embedded DNS resolver still works for sibling-container name resolution. Mount options are normalised rather than appended to: a mount that arrives with no options at all, or with a `rw` following a `ro`, now ends up read-only too, because `ro` is always placed last and that is the flag the kernel honours.
+- The container spec is only written back to disk when a modification actually changed it.
+
 ## v0.2.3
 
 - Switched CI from `dtolnay/rust-toolchain` to `actions-rust-lang/setup-rust-toolchain`. CI builds now compile with `RUSTFLAGS=-D warnings` and surface rustc/rustfmt warnings as PR annotations; release builds are exempt so a new stable lint cannot break a tagged release. Dependabot now tracks GitHub Actions pins.

Cargo.toml

@@ -8,7 +8,7 @@ license = "MIT OR Apache-2.0"
 name = "oci-interceptor"
 publish = false
 repository = "https://github.com/picoCTF/oci-interceptor"
-version = "0.2.3"
+version = "0.3.0"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 

README.md

@@ -38,7 +38,7 @@ Options:
       --oi-runtime-path <runtime-path>
           Path to OCI runtime. [default: runc]
       --oi-readonly-networking-mounts
-          Mount networking files as readonly
+          Deprecated, and now a no-op: networking mounts are always readonly
       --oi-write-debug-output
           Write debug output
       --oi-debug-output-dir <debug-output-dir>
@@ -70,10 +70,7 @@ option, as it defaults to `runc`, the default runtime bundled with Docker.
     "default-runtime": "oci-interceptor",
     "runtimes": {
         "oci-interceptor": {
-            "path": "/usr/local/bin/oci-interceptor",
-            "runtimeArgs": [
-                "--oi-readonly-networking-mounts"
-            ]
+            "path": "/usr/local/bin/oci-interceptor"
         }
     }
 }
@@ -82,7 +79,40 @@ The Docker daemon must be restarted (`systemctl restart docker.service`) in orde
 
 Note that if you set `oci-interceptor` as the default runtime, you can still bypass it for a specific container by specifying `docker run --runtime=runc`.
 
-While it is not possible to override `runtimeArgs` with a `docker run` option, you could specify multiple interceptor "runtimes" (with different flags) and switch between them using `docker run --runtime=<name>`.
+#### Passing flags: use a wrapper script, not `runtimeArgs`
+
+**Do not put the interceptor's flags in `runtimeArgs`.** Docker generates its own wrapper
+script for any runtime whose `runtimeArgs` is non-empty, and that generated wrapper does
+not `exec`:
+
+```sh
+#!/bin/sh
+/usr/local/bin/oci-interceptor --flags $@
+```
+
+It therefore stays in the process tree between the containerd shim and the real runtime.
+containerd invokes the runtime through go-runc, which builds commands with Go's
+`exec.CommandContext`; a cancelled or timed-out call delivers SIGKILL to the direct child
+only — that shell — orphaning `runc` beneath it. For a `runc delete` the task is then never
+deleted and its `containerd-shim-runc-v2` never shuts down, leaking roughly 5 MiB per
+occurrence until the host is rebooted. That defeats the point of the interceptor exec'ing
+the runtime at all.
+
+If you have no flags to pass, you need no wrapper at all: name the binary itself as `path`,
+as in the example above. Otherwise put the flags in a wrapper script of your own that
+`exec`s, and name that script as `path`:
+
+```sh
+#!/bin/sh
+exec /usr/local/bin/oci-interceptor --oi-env HTTP_PROXY=http://proxy.example:3128 "$@"
+```
+
+Keep `"$@"` quoted — Docker's generated wrapper uses a bare `$@`, which word-splits and
+glob-expands its arguments. An absent or empty `runtimeArgs` generates no wrapper.
+
+You can still run several interceptor "runtimes" with different flags and switch between
+them using `docker run --runtime=<name>`; each one needs its own wrapper script, named by
+its own `path`.
 
 ## Supported Customizations
 
@@ -100,12 +130,14 @@ users to fill the host storage volume.
 
 This can usually only be circumvented by manually creating read-only bind mounts over these paths (in which case Docker can no longer manage the container's DNS configuration) or by making the entire rootfs read-only (which severely constrains the workloads possible inside the container).
 
-To avoid this issue, specify the `--oi-readonly-networking-mounts` flag. This modifies these mounts to be read-only, preventing writes from inside the container.
+These mounts are always modified to be read-only, preventing writes from inside the container. The mounts themselves are left in place: Docker points a container on a user-defined network at its embedded DNS resolver by writing `nameserver 127.0.0.11` into the bind-mounted `/etc/resolv.conf`, so removing the mount would break resolution of sibling containers by name. Only write access is taken away.
+
+The `--oi-readonly-networking-mounts` flag is retained as an accepted no-op so that an existing `daemon.json` passing it keeps working; it no longer has any effect.
 
 #### Related issues
 
 - Workaround for [moby#13152](https://github.com/moby/moby/issues/41991), [moby#41991](https://github.com/moby/moby/issues/41991) (without custom bind mounts or making entire rootfs readonly)
-- Optionally reverts [moby#5129](https://github.com/moby/moby/pull/5129)
+- Reverts [moby#5129](https://github.com/moby/moby/pull/5129)
 
 ### Overriding environment variables
 
@@ -127,15 +159,15 @@ Unit tests run with `cargo test`. End-to-end integration tests live in `tests/in
 To run the integration tests locally on a Linux host with Docker:
 
 1. Build and install the binary: `cargo build --release && sudo install -m 0755 target/release/oci-interceptor /usr/local/bin/oci-interceptor`
-2. Configure `/etc/docker/daemon.json` with the named runtimes listed in the module docs of [tests/integration.rs](tests/integration.rs). The CI workflow (`.github/workflows/CI.yml`, `integration` job) is the canonical reference for the required daemon.json.
+2. Configure `/etc/docker/daemon.json` with the named runtimes listed in the module docs of [tests/integration.rs](tests/integration.rs). The CI workflow (`.github/workflows/CI.yml`, `integration` job) shows the exact set the suite expects. Note that it is a test harness, not a reference deployment: it uses `runtimeArgs` because that is the only native per-runtime flag mechanism and the flag assertions need it. For a real host, follow [Passing flags](#passing-flags-use-a-wrapper-script-not-runtimeargs) instead.
 3. `sudo systemctl restart docker`
 4. `OCI_INTERCEPTOR_INTEGRATION=1 cargo test --test integration -- --test-threads=1`
 
 CI runs the same suite on every push and pull request, which provides a regression check against the Docker and runc versions shipped on `ubuntu-latest` runners.
 
 ### Debug output
 
-Specify the `--oi-write-debug-output` flag to write original, parsed, and modified container configs to the directory specified as `--oi-debug-output-dir` (default `/var/log/oci-interceptor`).
+Specify the `--oi-write-debug-output` flag to write original, parsed, and modified container configs to the directory specified as `--oi-debug-output-dir` (default `/var/log/oci-interceptor`). As with every other flag, pass these from an exec'ing wrapper script rather than `runtimeArgs` — see [Passing flags](#passing-flags-use-a-wrapper-script-not-runtimeargs).
 
 The resulting files will be named:
 - `<container_hostname>_original.json` (the original config)

src/main.rs

@@ -8,7 +8,7 @@ use clap::{
 use env_vars::{EnvVar, EnvVarOverride, modify_env_vars, parse_env_var};
 use networking_mounts::modify_networking_mounts;
 use oci_spec::runtime::Spec;
-use std::{fs, io::Write, path::PathBuf, process};
+use std::{fs, io::Write, os::unix::process::CommandExt, path::PathBuf, process};
 
 fn main() -> Result<()> {
     let matches = clap::Command::new(crate_name!())
@@ -26,10 +26,13 @@ fn main() -> Result<()> {
                 .help("Path to OCI runtime."),
         )
         .arg(
+            // Retained as an accepted no-op so a daemon.json still passing it keeps
+            // working: rejecting it would fail every container launch on any host
+            // upgraded before its runtime config was updated.
             Arg::new("readonly-networking-mounts")
                 .long("oi-readonly-networking-mounts")
                 .action(ArgAction::SetTrue)
-                .help("Mount networking files as readonly"),
+                .help("Deprecated, and now a no-op: networking mounts are always readonly"),
         )
         .arg(
             Arg::new("write-debug-output")
@@ -137,11 +140,20 @@ fn main() -> Result<()> {
             serde_json::to_writer_pretty(&parsed_file, &spec)?;
         }
 
-        // Make any enabled modifications
-        if matches.get_flag("readonly-networking-mounts") {
-            modify_networking_mounts(&mut spec);
-            spec_modified = true;
-        }
+        // Make any enabled modifications.
+        //
+        // Networking mounts are always made read-only. Docker bind-mounts these three
+        // files from outside the container's writable layer, so where XFS project quotas
+        // restrict that layer they are an escape hatch for filling the host volume -- see
+        // "Read-only networking mounts" in the README. On a shared host that affects every
+        // other container, which is what makes it worth enforcing unconditionally.
+        //
+        // Note this is not a barrier between containers: each gets its own copy of these
+        // files, so tampering only ever affects the container doing it, and a process with
+        // code execution can bypass them anyway (HOSTALIASES, RES_OPTIONS, LD_PRELOAD, or
+        // simply querying a resolver directly). Treat it as quota enforcement, with
+        // in-container tamper resistance as a side effect.
+        spec_modified |= modify_networking_mounts(&mut spec);
         if !env_var_overrides.is_empty() {
             modify_env_vars(&mut spec, env_var_overrides);
             spec_modified = true;
@@ -176,7 +188,8 @@ fn main() -> Result<()> {
             .write_all(format!("{} {}\n", runtime_path, runtime_options.join(" ")).as_bytes())?;
         runtime_calls.flush()?;
     }
-    std::process::exit(call_oci_runtime(runtime_path, runtime_options)?);
+    // On success this replaces the current process, so nothing below runs.
+    Err(call_oci_runtime(runtime_path, runtime_options))
 }
 
 /// Extracts the container bundle path from the trailing runtime options, if present.
@@ -194,19 +207,29 @@ fn get_bundle_path(options: &mut [String]) -> Option<PathBuf> {
     None
 }
 
-/// Calls the actual OCI runtime, passing along any runtime options.
-fn call_oci_runtime(runtime_path: &str, options: Vec<String>) -> Result<i32> {
-    let mut child = process::Command::new(runtime_path)
+/// Replaces this process with the actual OCI runtime, passing along any runtime options.
+///
+/// This execs rather than spawning a child and waiting on it, so that the runtime inherits
+/// our PID and we leave the process tree entirely. A wrapper that spawns and waits is
+/// wrong for an OCI runtime in two ways:
+///
+/// - containerd's shim invokes the runtime through go-runc, which builds commands with
+///   Go's `exec.CommandContext`. On a cancelled or timed-out call that delivers SIGKILL to
+///   the direct child only. With a wrapper in between, the wrapper dies and the real
+///   runtime is orphaned, while the shim believes the call was cancelled. For a
+///   `runc delete` that leaves the task undeleted and its shim never shut down.
+/// - A runtime killed by a signal was reported to the caller as exit code -1, which
+///   `process::exit` truncates to 255. Callers that distinguish signal death from a 255
+///   exit status saw the wrong thing. Exec'ing reports the real wait status.
+///
+/// Returns only on failure to exec.
+fn call_oci_runtime(runtime_path: &str, options: Vec<String>) -> anyhow::Error {
+    let err = process::Command::new(runtime_path)
         .args(options.as_slice())
-        .spawn()
-        .with_context(|| "Failed to execute underlying OCI runtime")?;
-    let status = child
-        .wait()
-        .with_context(|| "Failed to wait on OCI runtime process")?;
-    match status.code() {
-        Some(code) => Ok(code),
-        None => Ok(-1), // child process was killed by a signal
-    }
+        .exec();
+    anyhow::Error::new(err).context(format!(
+        "Failed to execute underlying OCI runtime: {runtime_path}"
+    ))
 }
 
 #[cfg(test)]

src/networking_mounts.rs

@@ -7,24 +7,104 @@ use oci_spec::runtime::Spec;
 /// - /etc/resolv.conf
 ///
 /// in the container config, making them read-only.
-pub(crate) fn modify_networking_mounts(spec: &mut Spec) {
-    if let Some(mounts) = spec.mounts() {
-        let mut mounts = mounts.clone();
-        for mount in mounts.iter_mut() {
-            match mount.destination().to_str() {
-                Some("/etc/hosts") | Some("/etc/hostname") | Some("/etc/resolv.conf") => {
-                    if let Some(options) = mount.options()
-                        && !options.contains(&"ro".into())
-                    {
-                        let mut options = options.clone();
-                        options.push("ro".into());
-                        mount.set_options(Some(options));
-                    }
+///
+/// The mounts themselves are left in place. Docker points a container on a user-defined
+/// network at its embedded DNS resolver by writing `nameserver 127.0.0.11` into the
+/// bind-mounted /etc/resolv.conf, so removing that mount would break resolution of
+/// sibling containers by name. Only write access is taken away.
+///
+/// Returns whether any mount was actually changed.
+pub(crate) fn modify_networking_mounts(spec: &mut Spec) -> bool {
+    let Some(mounts) = spec.mounts() else {
+        return false;
+    };
+    let mut mounts = mounts.clone();
+    let mut changed = false;
+    for mount in mounts.iter_mut() {
+        match mount.destination().to_str() {
+            Some("/etc/hosts") | Some("/etc/hostname") | Some("/etc/resolv.conf") => {
+                // Absent options are still writable, and the kernel takes the last of a
+                // conflicting pair, so an existing "ro" earlier in the list does not
+                // survive a later "rw". Drop both and re-append "ro" so it always wins.
+                let current = mount.options().clone().unwrap_or_default();
+                let mut wanted: Vec<String> = current
+                    .iter()
+                    .filter(|option| *option != "ro" && *option != "rw")
+                    .cloned()
+                    .collect();
+                wanted.push(String::from("ro"));
+                if wanted != current {
+                    mount.set_options(Some(wanted));
+                    changed = true;
                 }
-                Some(_) => {}
-                None => {}
             }
+            _ => {}
         }
+    }
+    if changed {
         spec.set_mounts(Some(mounts));
     }
+    changed
+}
+
+#[cfg(test)]
+mod tests {
+    use super::modify_networking_mounts;
+    use oci_spec::runtime::Spec;
+
+    fn spec_with(mounts: &str) -> Spec {
+        serde_json::from_str(&format!("{{\"ociVersion\":\"1.0.0\",\"mounts\":{mounts}}}"))
+            .expect("test spec should parse")
+    }
+
+    fn options_of(spec: &Spec, destination: &str) -> Vec<String> {
+        spec.mounts()
+            .as_ref()
+            .expect("spec should have mounts")
+            .iter()
+            .find(|mount| mount.destination().to_str() == Some(destination))
+            .expect("mount should be present")
+            .options()
+            .clone()
+            .unwrap_or_default()
+    }
+
+    #[test]
+    fn a_mount_without_options_still_becomes_readonly() {
+        let mut spec = spec_with("[{\"destination\":\"/etc/hosts\"}]");
+        assert!(modify_networking_mounts(&mut spec));
+        assert_eq!(options_of(&spec, "/etc/hosts"), vec!["ro"]);
+    }
+
+    #[test]
+    fn a_writable_mount_keeps_its_other_options() {
+        let mut spec =
+            spec_with("[{\"destination\":\"/etc/hosts\",\"options\":[\"rbind\",\"rw\"]}]");
+        assert!(modify_networking_mounts(&mut spec));
+        assert_eq!(options_of(&spec, "/etc/hosts"), vec!["rbind", "ro"]);
+    }
+
+    #[test]
+    fn a_later_rw_does_not_survive_an_earlier_ro() {
+        // The kernel takes the last of a conflicting pair, so this mount was writable.
+        let mut spec =
+            spec_with("[{\"destination\":\"/etc/resolv.conf\",\"options\":[\"ro\",\"rw\"]}]");
+        assert!(modify_networking_mounts(&mut spec));
+        assert_eq!(options_of(&spec, "/etc/resolv.conf"), vec!["ro"]);
+    }
+
+    #[test]
+    fn an_already_readonly_mount_reports_no_change() {
+        let mut spec =
+            spec_with("[{\"destination\":\"/etc/hostname\",\"options\":[\"rbind\",\"ro\"]}]");
+        assert!(!modify_networking_mounts(&mut spec));
+        assert_eq!(options_of(&spec, "/etc/hostname"), vec!["rbind", "ro"]);
+    }
+
+    #[test]
+    fn unrelated_mounts_are_left_writable() {
+        let mut spec = spec_with("[{\"destination\":\"/tmp\",\"options\":[\"rw\"]}]");
+        assert!(!modify_networking_mounts(&mut spec));
+        assert_eq!(options_of(&spec, "/tmp"), vec!["rw"]);
+    }
 }

tests/integration.rs

@@ -90,16 +90,16 @@ fn passthrough_propagates_nonzero_exit_code() {
 }
 
 #[test]
-fn networking_mounts_writable_without_flag() {
-    if !check_enabled("networking_mounts_writable_without_flag") {
+fn networking_mounts_readonly_without_any_flag() {
+    if !check_enabled("networking_mounts_readonly_without_any_flag") {
         return;
     }
     for target in ["/etc/hosts", "/etc/hostname", "/etc/resolv.conf"] {
         let opts = mount_options("oi-default", target);
         let opts: Vec<&str> = opts.split(',').collect();
         assert!(
-            opts.contains(&"rw"),
-            "{target} expected to be rw by default, got: {opts:?}"
+            opts.contains(&"ro"),
+            "{target} expected to be ro with no flags at all, got: {opts:?}"
         );
     }
 }