contain: fail closed on incomplete file descriptor cleanup

.github/workflows/high-fd-cleanup.yml

@@ -0,0 +1,37 @@
+name: High FD cleanup regression
+
+on:
+  push:
+    paths:
+      - contain.cc
+      - Makefile
+      - tests/high_fd_cleanup_*
+      - .github/workflows/high-fd-cleanup.yml
+  pull_request:
+    paths:
+      - contain.cc
+      - Makefile
+      - tests/high_fd_cleanup_*
+      - .github/workflows/high-fd-cleanup.yml
+
+permissions:
+  contents: read
+
+jobs:
+  high-fd-regression:
+    runs-on: ubuntu-24.04
+    timeout-minutes: 15
+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+        with:
+          submodules: recursive
+      - name: Install build dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y --no-install-recommends \
+            build-essential libprotobuf-dev protobuf-compiler pkg-config \
+            libnl-3-dev libnl-route-3-dev flex bison
+      - name: Build nsjail
+        run: make -j2
+      - name: High FD cleanup regression
+        run: sudo make test-high-fd

Makefile

@@ -159,8 +159,11 @@ test-cmdline: $(BIN)
 	$(call run_test, ./nsjail --rlimit_cpu 1.5 --help > /dev/null, 255)
 	$(call run_test, ./nsjail --time_limit 0x10 --cgroup_mem_swap_max=-1 --user 1000:1000:1 --help > /dev/null, 0)
 
-.PHONY: test
-test: $(BIN) $(TEST_BINS) test-cmdline
+.PHONY: test test-high-fd
+test-high-fd: $(BIN)
+	sh tests/high_fd_cleanup_test.sh
+
+test: $(BIN) $(TEST_BINS) test-cmdline test-high-fd
 	$(call run_test, ./tests/nstun_buffer_budget_test, 0)
 	$(call run_test, ./tests/nstun_policy_test, 0)
 	$(call run_test, ./tests/nstun_ip_test, 0)

contain.cc

@@ -333,30 +333,16 @@ static bool containMakeFdsCOECloseRange(nsj_t* nsj) {
 	return true;
 }
 
-static bool containMakeFdsCOENaive(nsj_t* nsj) {
-	/*
-	 * Don't use getrlimit(RLIMIT_NOFILE) here, as it can return an artifically small value
-	 * (e.g. 32), which could be smaller than a maximum assigned number to file-descriptors
-	 * in this process. Just use some reasonably sane value (e.g. 1024)
-	 */
-	for (unsigned fd = 0; fd < 1024; fd++) {
-		RETURN_ON_FAILURE(containMakeFdCOE(fd, containPassFd(nsj, fd)));
-	}
-	return true;
-}
-
-static bool containMakeFdsCOEProc(nsj_t* nsj) {
-	int dirfd = open("/proc/self/fd", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
-	if (dirfd == -1) {
-		PLOG_D("open('/proc/self/fd', O_DIRECTORY|O_RDONLY|O_CLOEXEC)");
+static bool containMakeFdsCOEProc(nsj_t* nsj, int* proc_fd) {
+	if (*proc_fd == -1) {
 		return false;
 	}
-	DIR* dir = fdopendir(dirfd);
+	DIR* dir = fdopendir(*proc_fd);
 	if (dir == nullptr) {
-		PLOG_W("fdopendir(fd=%d)", dirfd);
-		close(dirfd);
+		PLOG_W("fdopendir(fd=%d)", *proc_fd);
 		return false;
 	}
+	*proc_fd = -1; /* fdopendir() owns the descriptor now. */
 	/* Make all fds above stderr close-on-exec */
 	for (;;) {
 		errno = 0;
@@ -393,18 +379,15 @@ static bool containMakeFdsCOEProc(nsj_t* nsj) {
 	return true;
 }
 
-static bool containMakeFdsCOE(nsj_t* nsj) {
+static bool containMakeFdsCOE(nsj_t* nsj, int* proc_fd) {
 	RETURN_ON_FAILURE(containValidateDefaultStdioFds(nsj));
 	if (containMakeFdsCOECloseRange(nsj)) {
 		return true;
 	}
-	if (containMakeFdsCOEProc(nsj)) {
+	if (containMakeFdsCOEProc(nsj, proc_fd)) {
 		return true;
 	}
-	if (containMakeFdsCOENaive(nsj)) {
-		return true;
-	}
-	LOG_E("Couldn't mark relevant file-descriptors as close-on-exec with any known method");
+	LOG_E("Couldn't safely mark all file descriptors as close-on-exec");
 	return false;
 }
 
@@ -440,6 +423,16 @@ bool setupFD(nsj_t* nsj, int fd_in, int fd_out, int fd_err) {
 }
 
 bool containProc(nsj_t* nsj, int parent_fd, pid_t expected_parent) {
+	/* Keep a view of all descriptors available after mount/chroot setup removes /proc. */
+	int proc_fd = open("/proc/self/fd", O_DIRECTORY | O_RDONLY | O_CLOEXEC);
+	if (proc_fd == -1) {
+		PLOG_D("open('/proc/self/fd', O_DIRECTORY|O_RDONLY|O_CLOEXEC)");
+	}
+	defer {
+		if (proc_fd != -1) {
+			close(proc_fd);
+		}
+	};
 	RETURN_ON_FAILURE(containUserNs(nsj));
 	RETURN_ON_FAILURE(containInitPidNs(nsj));
 	RETURN_ON_FAILURE(containInitMountNs(nsj));
@@ -455,7 +448,7 @@ bool containProc(nsj_t* nsj, int parent_fd, pid_t expected_parent) {
 	RETURN_ON_FAILURE(containTSC(nsj));
 	RETURN_ON_FAILURE(containSetLimits(nsj));
 	RETURN_ON_FAILURE(containPrepareEnv(nsj, parent_fd, expected_parent));
-	RETURN_ON_FAILURE(containMakeFdsCOE(nsj));
+	RETURN_ON_FAILURE(containMakeFdsCOE(nsj, &proc_fd));
 
 	return true;
 }

tests/high_fd_cleanup_helper.c

@@ -0,0 +1,118 @@
+// Regression harness for inherited high-numbered file descriptors.
+// Built statically so the probe can run in a minimal chroot without /proc.
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <linux/filter.h>
+#include <linux/seccomp.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <sys/resource.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+#define TEST_FD 2048
+#define MARKER "nsjail-high-fd-regression\n"
+
+static void fail(const char* what) {
+	perror(what);
+	exit(1);
+}
+
+static void block_close_range(void) {
+	struct sock_filter filter[] = {
+	    BPF_STMT(BPF_LD | BPF_W | BPF_ABS, offsetof(struct seccomp_data, nr)),
+	    BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, __NR_close_range, 0, 1),
+	    BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ERRNO | ENOSYS),
+	    BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog prog = {
+	    .len = (unsigned short)(sizeof(filter) / sizeof(filter[0])),
+	    .filter = filter,
+	};
+	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) == -1) {
+		fail("prctl(PR_SET_NO_NEW_PRIVS)");
+	}
+	if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &prog) == -1) {
+		fail("prctl(PR_SET_SECCOMP)");
+	}
+}
+
+static void ensure_fd_limit(void) {
+	struct rlimit lim;
+	if (getrlimit(RLIMIT_NOFILE, &lim) == -1) {
+		fail("getrlimit(RLIMIT_NOFILE)");
+	}
+	if (lim.rlim_cur <= TEST_FD) {
+		if (lim.rlim_max <= TEST_FD) {
+			fprintf(stderr, "RLIMIT_NOFILE hard limit cannot accommodate fd %d\n", TEST_FD);
+			exit(1);
+		}
+		lim.rlim_cur = TEST_FD + 1;
+		if (setrlimit(RLIMIT_NOFILE, &lim) == -1) {
+			fail("setrlimit(RLIMIT_NOFILE)");
+		}
+	}
+}
+
+static void launch(int argc, char** argv) {
+	// Syntax: helper launch <secret-file> <nsjail> [nsjail arguments ...].
+	if (argc < 5) {
+		fprintf(stderr, "usage: %s launch <secret-file> <nsjail> [args ...]\n", argv[0]);
+		exit(2);
+	}
+	ensure_fd_limit();
+	int source = open(argv[2], O_RDONLY | O_CLOEXEC);
+	if (source == -1) {
+		fail("open(secret)");
+	}
+	if (dup2(source, TEST_FD) == -1) {
+		fail("dup2(high fd)");
+	}
+	if (source != TEST_FD) {
+		close(source);
+	}
+	// Deliberately make it inheritable. Only nsjail should sanitize it.
+	if (fcntl(TEST_FD, F_SETFD, 0) == -1) {
+		fail("fcntl(F_SETFD)");
+	}
+	block_close_range();
+	execvp(argv[3], &argv[3]);
+	fail("execvp(nsjail)");
+}
+
+static int probe(const char* expected) {
+	if (strcmp(expected, "closed") == 0) {
+		errno = 0;
+		if (fcntl(TEST_FD, F_GETFD) == -1 && errno == EBADF) {
+			return 0;
+		}
+		fprintf(stderr, "FAIL: inherited fd %d survived into the jail\n", TEST_FD);
+		return 1;
+	}
+	if (strcmp(expected, "open") == 0) {
+		char buf[sizeof(MARKER)] = {0};
+		ssize_t n = read(TEST_FD, buf, sizeof(MARKER) - 1);
+		if (n == (ssize_t)(sizeof(MARKER) - 1) && memcmp(buf, MARKER, sizeof(MARKER) - 1) == 0) {
+			return 0;
+		}
+		fprintf(stderr, "FAIL: explicit --pass_fd did not preserve fd %d\n", TEST_FD);
+		return 1;
+	}
+	fprintf(stderr, "unknown expectation: %s\n", expected);
+	return 2;
+}
+
+int main(int argc, char** argv) {
+	if (argc >= 2 && strcmp(argv[1], "launch") == 0) {
+		launch(argc, argv);
+	}
+	if (argc == 3 && strcmp(argv[1], "probe") == 0) {
+		return probe(argv[2]);
+	}
+	fprintf(stderr, "usage: %s {launch|probe} ...\n", argv[0]);
+	return 2;
+}

tests/high_fd_cleanup_test.sh

@@ -0,0 +1,36 @@
+#!/bin/sh
+# Integration regression test for nsjail's high-FD close-on-exec fallback.
+set -eu
+
+cd "$(dirname "$0")/.."
+nsjail="$(pwd)/nsjail"
+if [ ! -x "$nsjail" ]; then
+    echo "FAIL: build nsjail before running this test" >&2
+    exit 1
+fi
+
+tmp=$(mktemp -d)
+trap 'rm -rf "$tmp"' EXIT HUP INT TERM
+mkdir -p "$tmp/jail"
+cc -std=gnu11 -O2 -Wall -Wextra -Werror -static \
+    -o "$tmp/probe" tests/high_fd_cleanup_helper.c
+cp "$tmp/probe" "$tmp/jail/probe"
+printf 'nsjail-high-fd-regression\n' > "$tmp/host-secret"
+
+# No /proc is present in the chroot; unlike the original fallback, the fixed
+# code retains the /proc/self/fd directory opened before entering the chroot.
+# The launcher forces close_range to return ENOSYS using an inherited seccomp
+# filter, even on kernels where CLOSE_RANGE_CLOEXEC is supported.
+# Avoid depending on namespace creation privileges in CI. With no mount
+# namespace, nsjail uses chroot(2), which requires root (run with sudo).
+base_args="--mode o --quiet --disable_proc --disable_clone_newuser --disable_clone_newnet --disable_clone_newcgroup --disable_clone_newns --disable_clone_newpid --disable_clone_newipc --disable_clone_newuts"
+
+echo "[high-fd] inherited fd 2048 must not escape into proc-less jail"
+"$tmp/probe" launch "$tmp/host-secret" "$nsjail" $base_args \
+    --chroot "$tmp/jail" -- /probe probe closed
+
+echo "[high-fd] explicit --pass_fd 2048 must remain supported"
+"$tmp/probe" launch "$tmp/host-secret" "$nsjail" $base_args \
+    --chroot "$tmp/jail" --pass_fd 2048 -- /probe probe open
+
+echo "[high-fd] PASS"