fix(box): harden CRI from adversarial review (security + async-signal-safety + leaks)

Fixes confirmed by a multi-lens adversarial review of this session's CRI work:

HIGH:
- Security: a container image/pod env entry named A3S_SEC_* could spoof the
  runtime security envelope (the guest matches first-wins), escalating
  supplemental groups / unmasking paths / toggling seccomp. CreateContainer now
  strips all caller-supplied A3S_SEC_* before adding the runtime's trusted ones.
- Async-signal-safety: the default seccomp BPF filter was built (heap alloc)
  INSIDE the post-fork pre_exec child — malloc can deadlock there on musl
  (multi-threaded guest). Split into build_default_bpf_filter (pre-fork) +
  install_seccomp_filter (syscalls only); both CRI-exec and box-run paths build
  the filter before fork and only install it in the child.
- Mount leak: MaskedPaths/ReadonlyPaths were re-applied on every exec, stacking
  unbounded mounts on each liveness/readiness probe. apply_container_path_restrictions
  is now idempotent (mountpoint / ST_RDONLY checks).

MEDIUM:
- Graceful shutdown reaping was skipped when the gRPC server returned Err (`?`);
  it now always reaps, then propagates the error.
- Non-Linux build fix: setgroups count cast (size_t vs BSD c_int) -> `as _`.
- Path-traversal hardening: reject MaskedPaths/ReadonlyPaths and sysctl names
  containing `..` / `/` before joining onto the rootfs / /proc/sys path.

LOW:
- Localhost seccomp profiles now warn instead of silently downgrading.
- Corrected the misleading default-seccomp doc comment.
- A3S_SEC_* control vars are no longer leaked into the workload's environment.

Verified on the KVM server: build + clippy clean, 219 cri tests pass, and
seccomp-default / ReadonlyPaths / safe-sysctls / RemoveContainer-running all
still pass (no regression from the refactor).

Author: Roy Lin

src/cri/src/config_mapper.rs

@@ -61,12 +61,32 @@ fn parse_sysctls(config: &PodSandboxConfig) -> Vec<(String, String)> {
     let mut sysctls: Vec<(String, String)> = linux
         .sysctls
         .iter()
+        .filter(|(name, _)| {
+            let safe = is_safe_sysctl_name(name);
+            if !safe {
+                tracing::warn!(sysctl = %name, "Dropping sysctl with an unsafe name");
+            }
+            safe
+        })
         .map(|(name, value)| (name.clone(), value.clone()))
         .collect();
     sysctls.sort();
     sysctls
 }
 
+/// A sysctl name is safe to map onto `/proc/sys/<name with '.'→'/'>` only if it
+/// is a non-empty dot-separated key with no path-traversal characters. Guards
+/// against a crafted name (e.g. `../../proc/sysrq-trigger`) escaping
+/// `/proc/sys` when the guest substitutes `.` for `/`.
+fn is_safe_sysctl_name(name: &str) -> bool {
+    !name.is_empty()
+        && !name.contains('/')
+        && !name.split('.').any(|seg| seg.is_empty() || seg == "..")
+        && name
+            .chars()
+            .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-'))
+}
+
 fn parse_hostname(config: &PodSandboxConfig) -> Result<Option<String>> {
     let hostname = config.hostname.trim();
     if hostname.is_empty() {

src/cri/src/runtime_service/mod.rs

@@ -764,6 +764,12 @@ impl RuntimeService for BoxRuntimeService {
             .map(|image| image.env.as_slice())
             .unwrap_or(&[]);
         let mut env = merge_env(image_env, &config.envs);
+        // A3S_SEC_* is the runtime's TRUSTED security envelope. Strip any
+        // image- or pod-supplied A3S_SEC_* entry first: the guest matches these
+        // keys first-wins, so a caller-controlled value would otherwise
+        // override the runtime's (escalating supplemental groups, unmasking
+        // paths, or toggling seccomp). Only runtime-derived values may set them.
+        env.retain(|(key, _)| !key.starts_with("A3S_SEC_"));
         let working_dir = if config.working_dir.is_empty() {
             image_config
                 .and_then(|image| image.working_dir.clone())
@@ -814,8 +820,20 @@ impl RuntimeService for BoxRuntimeService {
             // unconfined. Localhost profiles are not yet plumbed into the VM.
             if let Some(profile) = sc.seccomp.as_ref() {
                 use crate::cri_api::security_profile::ProfileType;
-                if profile.profile_type() == ProfileType::RuntimeDefault {
-                    env.push(("A3S_SEC_SECCOMP".to_string(), "default".to_string()));
+                match profile.profile_type() {
+                    ProfileType::RuntimeDefault => {
+                        env.push(("A3S_SEC_SECCOMP".to_string(), "default".to_string()));
+                    }
+                    ProfileType::Localhost => {
+                        // Not yet plumbed into the VM — warn rather than
+                        // silently downgrading to unconfined.
+                        tracing::warn!(
+                            container = %metadata.name,
+                            profile = %profile.localhost_ref,
+                            "Localhost seccomp profiles are not yet supported; container runs without the requested profile"
+                        );
+                    }
+                    ProfileType::Unconfined => {}
                 }
             }
         }

src/cri/src/server.rs

@@ -103,16 +103,18 @@ impl CriServer {
         // Keep a handle so we can reap sandbox VMs once the server stops; the
         // service itself is moved into the gRPC server below.
         let shutdown_service = runtime_service.clone();
-        Server::builder()
+        let result = Server::builder()
             .add_service(RuntimeServiceServer::new(runtime_service))
             .add_service(ImageServiceServer::new(image_service))
             .serve_with_incoming_shutdown(uds_stream, shutdown_signal())
-            .await?;
+            .await;
 
-        // The server stopped (SIGTERM/SIGINT): tear down sandbox VMs and unmount
-        // their overlays so they do not orphan across restarts.
+        // The server stopped — on graceful signal (Ok) OR a transport error.
+        // Reap sandbox VMs + unmount overlays unconditionally so they do not
+        // orphan across restarts, then surface any server error.
         tracing::info!("CRI server stopping — reaping sandbox VMs");
         shutdown_service.shutdown_all_sandboxes().await;
+        result?;
 
         Ok(())
     }

src/guest/init/src/exec_server.rs

@@ -445,6 +445,11 @@ fn build_command(
 
     for entry in spec.env {
         if let Some((key, value)) = entry.split_once('=') {
+            // A3S_SEC_* are runtime control vars (consumed below for setgroups /
+            // masked paths / seccomp); don't leak them into the workload's env.
+            if key.starts_with("A3S_SEC_") {
+                continue;
+            }
             command.env(key, value);
         }
     }
@@ -848,6 +853,14 @@ fn configure_child_process(
         .map(|rootfs| CString::new(rootfs.as_bytes()).expect("rootfs path was pre-validated"));
     let workdir = CString::new(workdir.as_bytes()).expect("working directory was pre-validated");
 
+    // Build the seccomp BPF filter BEFORE fork: building allocates, and
+    // allocating in the post-fork child is not async-signal-safe (malloc may
+    // deadlock on musl). The child only installs the prebuilt filter.
+    #[cfg(target_os = "linux")]
+    let seccomp_filter = apply_seccomp.then(crate::namespace::build_default_bpf_filter);
+    #[cfg(not(target_os = "linux"))]
+    let _ = apply_seccomp;
+
     unsafe {
         command.pre_exec(move || {
             if libc::setpgid(0, 0) != 0 {
@@ -865,7 +878,7 @@ fn configure_child_process(
             // needs CAP_SETGID, which user.apply() drops via setuid below.
             if !supplemental_groups.is_empty() {
                 let ret = libc::setgroups(
-                    supplemental_groups.len() as libc::size_t,
+                    supplemental_groups.len() as _,
                     supplemental_groups.as_ptr() as *const libc::gid_t,
                 );
                 if ret != 0 {
@@ -876,14 +889,12 @@ fn configure_child_process(
                 user.apply()?;
             }
             // Install the seccomp filter last — after chroot and the privilege
-            // drop — so it is active across execve. The default filter allows
-            // execve and the syscalls the child still needs.
+            // drop — so it is active across execve. Only the prebuilt filter is
+            // installed here; no allocation happens in the post-fork child.
             #[cfg(target_os = "linux")]
-            if apply_seccomp {
-                crate::namespace::apply_default_seccomp()?;
+            if let Some(filter) = &seccomp_filter {
+                crate::namespace::install_seccomp_filter(filter)?;
             }
-            #[cfg(not(target_os = "linux"))]
-            let _ = apply_seccomp;
             Ok(())
         });
     }
@@ -966,8 +977,36 @@ fn parse_sec_path_list<'a>(env: &'a [String], prefix: &str) -> Vec<&'a str> {
 fn apply_container_path_restrictions(rootfs: &str, masked: &[&str], readonly: &[&str]) {
     use nix::mount::{mount, MsFlags};
 
+    use std::os::unix::fs::MetadataExt;
+
+    // Reject entries that are not safe absolute paths (must be rooted, no `..`
+    // component) so a crafted MaskedPaths/ReadonlyPaths value cannot escape the
+    // container rootfs through the join below.
+    let is_safe = |path: &str| path.starts_with('/') && !path.split('/').any(|c| c == "..");
+
+    // A target is already restricted if it is a distinct mount — its st_dev
+    // differs from its parent directory's. This makes re-application on every
+    // exec idempotent (build_command runs per-exec) instead of stacking mounts.
+    let is_mountpoint = |target: &str| -> bool {
+        let Ok(dev) = std::fs::metadata(target).map(|m| m.dev()) else {
+            return false;
+        };
+        std::path::Path::new(target)
+            .parent()
+            .and_then(|p| std::fs::metadata(p).ok())
+            .map(|pm| pm.dev() != dev)
+            .unwrap_or(false)
+    };
+
     for path in masked {
+        if !is_safe(path) {
+            warn!("Skipping unsafe masked path {path}");
+            continue;
+        }
         let target = format!("{rootfs}{path}");
+        if is_mountpoint(&target) {
+            continue; // already masked
+        }
         match std::fs::metadata(&target) {
             Ok(meta) if meta.is_dir() => {
                 if let Err(e) = mount(
@@ -996,10 +1035,22 @@ fn apply_container_path_restrictions(rootfs: &str, masked: &[&str], readonly: &[
     }
 
     for path in readonly {
+        if !is_safe(path) {
+            warn!("Skipping unsafe readonly path {path}");
+            continue;
+        }
         let target = format!("{rootfs}{path}");
         if !std::path::Path::new(&target).exists() {
             continue;
         }
+        // Idempotent: skip if already read-only — re-binding would stack mounts
+        // on every exec into the container.
+        if nix::sys::statvfs::statvfs(target.as_str())
+            .map(|s| s.flags().contains(nix::sys::statvfs::FsFlags::ST_RDONLY))
+            .unwrap_or(false)
+        {
+            continue;
+        }
         // A fresh bind is read-write regardless of the source, so bind then
         // remount read-only — the order matters.
         if let Err(e) = mount(

src/guest/init/src/namespace.rs

@@ -324,9 +324,19 @@ fn apply_security_before_exec(cmd: &mut Command) -> Result<(), NamespaceError> {
     let seccomp_mode = config.seccomp.clone();
     let cap_drop = config.cap_drop.clone();
 
+    // Build the seccomp BPF filter BEFORE fork. Building allocates, which is
+    // not async-signal-safe in the post-fork child (malloc may deadlock on
+    // musl); the child only installs the prebuilt filter.
+    let seccomp_filter = if matches!(seccomp_mode, SeccompMode::Default) {
+        Some(build_default_bpf_filter())
+    } else {
+        None
+    };
+
     // Use pre_exec to apply security in the child process right before exec
     // SAFETY: pre_exec runs after fork, before exec. We only call
-    // async-signal-safe operations (prctl, seccomp).
+    // async-signal-safe operations (prctl, seccomp) — the seccomp filter is
+    // built above, pre-fork.
     unsafe {
         cmd.pre_exec(move || {
             // 1. Set no-new-privileges
@@ -342,10 +352,12 @@ fn apply_security_before_exec(cmd: &mut Command) -> Result<(), NamespaceError> {
                 drop_capabilities(&cap_drop)?;
             }
 
-            // 3. Apply seccomp filter
+            // 3. Apply seccomp filter (prebuilt before fork)
             match &seccomp_mode {
                 SeccompMode::Default => {
-                    apply_default_seccomp()?;
+                    if let Some(filter) = &seccomp_filter {
+                        install_seccomp_filter(filter)?;
+                    }
                 }
                 SeccompMode::Unconfined => {
                     // No seccomp filter
@@ -496,38 +508,26 @@ fn cap_name_to_number(name: &str) -> Option<i32> {
 ///
 /// Based on Docker's default seccomp profile — blocks syscalls that could
 /// escape the sandbox or compromise the host.
+/// Install a prebuilt seccomp BPF filter on the current thread.
+///
+/// **Async-signal-safe**: performs only `prctl` and the `seccomp` syscall and
+/// reads the caller-owned `filter` slice — it does NOT allocate. It is therefore
+/// safe to call from a post-fork `pre_exec` hook PROVIDED the filter was built
+/// *before* the fork (see [`build_default_bpf_filter`]); building the filter
+/// allocates and must never run in the child of a multi-threaded process.
+///
+/// Sets `PR_SET_NO_NEW_PRIVS` (required for unprivileged seccomp) then loads the
+/// filter via `SECCOMP_SET_MODE_FILTER`, putting the process in
+/// `SECCOMP_MODE_FILTER` (`/proc/self/status` `Seccomp: 2`). The default filter
+/// returns `EPERM` for the syscalls listed in [`build_default_bpf_filter`].
 #[cfg(target_os = "linux")]
-pub(crate) fn apply_default_seccomp() -> Result<(), std::io::Error> {
-    // Use SECCOMP_SET_MODE_FILTER via prctl
-    // The default profile uses a BPF filter that blocks:
-    // - kexec_load, kexec_file_load (kernel replacement)
-    // - reboot (system reboot)
-    // - mount, umount2 (filesystem manipulation — unless in mount namespace)
-    // - pivot_root, chroot (filesystem escape)
-    // - swapon, swapoff (swap manipulation)
-    // - init_module, finit_module, delete_module (kernel modules)
-    // - acct (process accounting)
-    // - settimeofday, clock_settime (time manipulation)
-    // - personality (execution domain change)
-    // - keyctl (kernel keyring)
-    // - ptrace (process tracing — unless CAP_SYS_PTRACE)
-    // - userfaultfd (memory manipulation)
-    // - perf_event_open (performance monitoring)
-    // - bpf (eBPF programs)
-    // - unshare (namespace creation — already in namespace)
-    // - setns (namespace switching)
-
-    // Build BPF filter program
-    let filter = build_default_bpf_filter();
-
-    // Install the filter via prctl + seccomp
-    // First, ensure no-new-privs is set (required for unprivileged seccomp)
+pub(crate) fn install_seccomp_filter(filter: &[libc::sock_filter]) -> Result<(), std::io::Error> {
+    // First, ensure no-new-privs is set (required for unprivileged seccomp).
     let ret = unsafe { libc::prctl(libc::PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) };
     if ret != 0 {
         return Err(std::io::Error::last_os_error());
     }
 
-    // SECCOMP_SET_MODE_FILTER = 1, SECCOMP_FILTER_FLAG_TSYNC = 1
     let prog = libc::sock_fprog {
         len: filter.len() as u16,
         filter: filter.as_ptr() as *mut libc::sock_filter,
@@ -547,7 +547,7 @@ pub(crate) fn apply_default_seccomp() -> Result<(), std::io::Error> {
 /// Returns SECCOMP_RET_ERRNO(EPERM) for blocked syscalls,
 /// SECCOMP_RET_ALLOW for everything else.
 #[cfg(target_os = "linux")]
-fn build_default_bpf_filter() -> Vec<libc::sock_filter> {
+pub(crate) fn build_default_bpf_filter() -> Vec<libc::sock_filter> {
     // BPF constants
     const BPF_LD: u16 = 0x00;
     const BPF_W: u16 = 0x00;