Skip to content

contain: fail closed on incomplete file descriptor cleanup - #328

Open
canbolayir wants to merge 3 commits into
google:masterfrom
canbolayir:fix/high-fd-inheritance
Open

canbolayir wants to merge 3 commits into
google:masterfrom
canbolayir:fix/high-fd-inheritance

Conversation

@canbolayir

Copy link
Copy Markdown

Summary

Ensure that file descriptor cleanup cannot silently leave high-numbered inherited descriptors open across execve().

The existing fallback in containMakeFdsCOENaive() only scans descriptors 0 through 1023. If close_range(CLOSE_RANGE_CLOEXEC) fails and /proc/self/fd is unavailable after mount/chroot setup, an inheritable descriptor numbered 1024 or higher can therefore survive into the jailed process.

This change:

  • opens /proc/self/fd before mount/chroot setup changes the filesystem view;
  • retains that directory descriptor until final FD cleanup;
  • uses it to enumerate all open descriptors if close_range() fails;
  • removes the bounded 0..1023 fallback; and
  • fails closed if neither complete cleanup mechanism is available.

Security impact

A high-numbered inherited host descriptor can expose resources that were not explicitly passed with --pass_fd, including files, sockets, pipes, or directory descriptors.

Lowering RLIMIT_NOFILE does not address this because descriptors opened before the limit is lowered remain valid.

Reproduction

The issue was reproduced by:

  1. opening a host-only file as FD 2048;
  2. forcing close_range() to return ENOSYS;
  3. using --disable_proc with a minimal chroot; and
  4. executing a static program in the jail that reads FD 2048.

Before this change, the jailed program can read the host-only descriptor.

With this change, the same descriptor does not survive execve().

Relation to #297

This is separate from #297.

The issue here is not primarily support for older kernels. The current containment path already attempts fallback cleanup when close_range() fails. The problem is that the final fallback can report success while only checking descriptors below 1024.

close_range() can also be unavailable to the process because of an outer syscall policy or similar execution environment, even on a kernel that implements it.

Validation

  • reproduced against f100fd917c6d3ed0fd15d868528b735cb5fb1a1a;
  • patched behavior verified with the same high-FD reproducer;
  • git apply --check passed;
  • git diff --check passed;
  • clean Ubuntu 24.04 build with make clean && make -j2 passed.

@zerox80

zerox80 commented Oct 9, 2026

Copy link
Copy Markdown

One data point on the "close_range is available since 5.9" argument from #294/#297: close_range() itself landed in 5.9, but the CLOSE_RANGE_CLOEXEC flag that util::makeRangeCOE() passes was only added in 5.11. On 5.9/5.10 the call fails with EINVAL (unknown flag), so containMakeFdsCOE() falls through to /proc/self/fd and then to the 0..1023 loop on any 5.10 kernel. The path this PR fixes is therefore reachable on a still widely deployed LTS series, not only on pre-5.9 kernels or under a restrictive outer seccomp policy.

I reproduced it on 4ff54a6 with --disable_proc and close_range(..., CLOSE_RANGE_CLOEXEC) forced to return EINVAL via seccomp (emulating 5.10): a non-CLOEXEC descriptor at fd 1500 in the parent is readable inside the jail. Same result with ENOSYS.

In MODE_LISTEN_TCP this goes beyond stray parent fds. The per-session connection socket and stdio pipes are created without SOCK_CLOEXEC/O_CLOEXEC (acceptConn() in net.cc, listenMode() in nsjail.cc), and nsjail raises its own RLIMIT_NOFILE to 8192. Once enough sessions are active, their descriptors are numbered ≥ 1024 and survive into jails started later. Creating them with CLOEXEC (cf. #349 for the accept side) would be a cheap complement to this PR.

@elnadym165

Copy link
Copy Markdown

Thanks for working on this FD-sanitization issue.

I independently reproduced the high-numbered file-descriptor inheritance problem against NsJail commit 4ff54a6e0d5b65a0e1633d6e2fd1425ba6c882ce and validated a fail-closed mitigation in an isolated Linux environment.

I also have regression evidence covering NsJail's TCP listening mode and normal execution when descriptor cleanup succeeds.

Would an automated regression test covering high-numbered FD inheritance and cleanup-failure behavior be useful for this PR? I would be happy to contribute focused test coverage without duplicating the existing fix.

Thanks!

@canbolayir

Copy link
Copy Markdown
Author

Added automated regression coverage for high-numbered FD inheritance. The tests pass on Ubuntu 24.04, covering the close_range() ENOSYS fallback, FD 2048 isolation, and explicit --pass_fd behavior.

The CLA and zizmor checks are also green. The remaining upstream workflows require maintainer approval.

Could a maintainer review the fix and regression coverage when convenient? Thanks!

@elnadym165

Copy link
Copy Markdown

Hi @canbolayir,

Thanks for adding automated high-FD regression coverage to #328.

I've prepared a small, test-only extension to your existing tests that adds EPERM and EINVAL failure modes alongside the current ENOSYS case.

The patch changes only tests/high_fd_cleanup_helper.c and tests/high_fd_cleanup_test.sh (58 additions, 7 deletions). It also verifies the actual injected syscall errno before launching NsJail.

Local validation results:

  • Original upstream regression: 2/2 PASS
  • Expanded regression: 6/6 PASS
  • Vulnerable-versus-patched differential: 12/12 scenarios, 60/60 assertions PASS
  • Original vulnerable commit 4ff54a6: unauthorized FD 2048 survives under all three injected errors
  • PR contain: fail closed on incomplete file descriptor cleanup #328: unauthorized FD 2048 is closed, while explicit --pass_fd 2048 remains functional
  • Patch applies cleanly to c01f6da09659b2c09ef9edf5fbe075b29beda238

No production code changes are involved.

Would you be interested in incorporating this additional regression coverage? I can share the patch or submit a focused contribution using whichever workflow you prefer.

Thanks!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants