Skip to content

Bind bwrap grants from descriptors, closing the check-then-mount gap - #160

Open
M3gA-Mind wants to merge 3 commits into
tinyhumansai:mainfrom
M3gA-Mind:feat/p7a-bwrap-fd-binds
Open

M3gA-Mind wants to merge 3 commits into
tinyhumansai:mainfrom
M3gA-Mind:feat/p7a-bwrap-fd-binds

Conversation

@M3gA-Mind

Copy link
Copy Markdown
Collaborator

Summary

Closes item 1 of #104: the check-then-mount gap on nested grants.

reject_symlinked_grants checks that a grant nested in a writable directory doesn't reach its target through a symlink. Then bwrap --bind PATH PATH resolved the path again. A program with write access to the parent, already running in another sandbox, could swap the directory for a symlink between the two steps.

Where bwrap supports it, each grant is now opened once as an O_PATH descriptor and bound with --bind-fd/--ro-bind-fd, so what was checked is what is mounted.

  • A grant inside another grant is opened beneath its parent's descriptor with RESOLVE_NO_SYMLINKS | RESOLVE_BENEATH: no symlink on that stretch is followed.
  • A top-level grant is opened by its declared path, since its ancestors belong to the host.
  • A missing read-only grant is skipped, as --ro-bind-try skips it.
  • Baseline system paths stay bound by path.

The descriptors are mapped onto 10, 11, … only in the child after fork, using the command-fds crate, so they are never inheritable in the host process, where another thread could spawn at the same time. The launcher keeps them open and still closes every other inherited descriptor.

Fd binds need bwrap listing --bind-fd (bubblewrap 0.9.0 does; the code comment claiming 0.10+ was wrong) and openat2 (Linux 5.6+). Without them grants are bound by path as before. BubblewrapBackend::binds_by_descriptor() and a debug log report which mode is in effect, and the module docs describe the remaining gap for that case.

Related issue

Part of #104 (item 1).

API or behavior changes

  • New BubblewrapBackend::binds_by_descriptor().
  • New Linux dependencies in tinybox-jail:
    • command-fds =0.3.3 (Apache-2.0, by Google), which does the dup2 in a pre_exec hook without allocating, so the workspace keeps forbidding unsafe;
    • nix 0.31 (fs), already in the lockfile, for a safe openat2.
  • A nested grant whose path inside its parent grant is a symlink at spawn time is refused with ELOOP, where before it was mounted. Before, the earlier check refused it only if the link was already there.

Validation

On our Linux CI runner (bubblewrap 0.9.0), at 30c2f2ddf5. The head differs only by moving the path checks at the top of jail_args_with_fds, unchanged, into check_paths, to keep it under the line limit. On the head, cargo clippy --all-targets --all-features -- -D warnings reports 0 errors, the same as main.

  • cargo build --locked --all-targets --all-features: ok. The new lockfile entry is exactly what cargo resolves.
  • cargo deny --locked check: the new dependencies add only a duplicate-version warning for nix (0.27 is already in the tree). No licence or ban findings.
  • cargo fmt --all -- --check and RUSTDOCFLAGS="-D warnings" cargo doc: ok.
  • TINYBOX_REQUIRE_BWRAP=1 cargo test -p tinybox-jail bwrap: 51 passed. The live run reports binds_by_descriptor=true.
  • cargo test --workspace --no-fail-fast: 1582 passed, 0 failed, with no failures unique to this branch.
  • Red controls, each restored afterwards:
    • following symlinks again: the swap test fails ("a symlinked nested grant was opened");
    • closing the grant descriptors in the launcher: the launcher test fails (fd 10 closed);
    • forcing path binds: the detection test fails while the other 50 pass, which proves the fallback.

Tests

  • Argv: grants appear as --bind-fd 10 <root> and --ro-bind-fd 11 <nested> (nested after its parent), never also by path; baseline paths are still bound by path; without support everything is bound by path.
  • a_symlink_swapped_in_after_the_check_is_not_followed: a nested grant replaced by a link out after the root was opened is refused.
  • a_symlink_above_a_grant_is_left_to_the_host: a top-level grant spelled through a host symlink still opens.
  • a_missing_read_only_grant_is_skipped_as_ro_bind_try_skips_it.
  • Launcher: the mapped descriptors reach bwrap, and an inheritable stray descriptor is still closed.
  • Live (bwrap_isolation): binds_by_descriptor() matches whether bwrap --help lists --bind-fd. With 0.9.0 every live nested-grant test runs through descriptors.
  • Red controls:
    • following symlinks again fails the swap test;
    • a launcher that closes the grant descriptors fails the launcher test;
    • forcing path binds fails the detection test while the other live tests still pass, which proves the fallback.

Documentation

The known-gaps section in tinybox-jail/src/bwrap/mod.rs, and rustdoc on the new accessor and the builder.

Checklist

  • The change is focused on one logical change
  • No new #[allow(...)], #[ignore], or relaxed lints
  • No secrets, tokens, or .env contents in the diff or the description

A nested grant's symlink check ran before bwrap re-resolved the grant's path,
so a program with write access to the parent could swap in a link between
them. Where bwrap lists --bind-fd and the kernel has openat2, each grant is
now opened once as an O_PATH descriptor, a nested one beneath its parent with
RESOLVE_NO_SYMLINKS | RESOLVE_BENEATH, and bound with --bind-fd. Descriptors
are mapped only in the child after fork (command-fds), never inheritable in
the host. Elsewhere grants stay bound by path, as before, and
BubblewrapBackend::binds_by_descriptor reports which is in effect.
@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a0e8ed90-7e18-41b6-a74a-cedcefd8aad6
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@tinysweeper

tinysweeper Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Reviewed head: 43057ccd29358b66c17cb9525feccdea6e340a34

Verdict: Ready for maintainer review · Priority: none
Coverage: Reviewed 8 of 9 files; not reviewed: Cargo.lock.
Pull request purpose (title): Bind bwrap grants fr…

Diagnostics and supporting details

Full summary

Pull request purpose (title): Bind bwrap grants from descriptors, closing the check-then-mount gap. The change closes item 1 of #104: `reject_symlinked_grants` validated a grant nested in a writable directory, but `bwrap --bind PATH` re-resolved that path, leaving a window in which a program with write access to the parent could swap a directory for a symlink between the check and the mount. Where `bwrap` lists `--bind-fd` and the kernel provides `openat2` (Linux 5.6+), each grant is opened once as an `O_PATH` descriptor and bound with `--bind-fd`/`--ro-bind-fd`, so what was checked is what is mounted; otherwise grants fall back to path binds and the documented gap remains. The latest-push-delta section is unavailable: the review state has the prior reviewed head and rendered evidence but no trusted base-to-head diff, so no push delta is reported here.

Unreviewed paths

  • Cargo.lock

Review snapshot

Change surface Files Review signal Count
Production 3 Active findings 0
Tests 4 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 1 Pending checks/questions 0

Completeness: Complete
Test assessment: Test coverage is assessed from changed tests and lane evidence; execution is not claimed without trusted check data.

What changed

Grant binding moves from path re-resolution to descriptors opened at spawn time in `crates/tinybox-jail/src/bwrap/args.rs`: each jail grant is opened as an `O_PATH | O_CLOEXEC` descriptor, nested grants relative to their parent's descriptor with `RESOLVE_NO_SYMLINKS | RESOLVE_BENEATH`, top-level grants by their declared path, and a missing read-only grant is skipped as `--ro-bind-try` would skip it. Emitted argv uses `--bind-fd`/`--ro-bind-fd` with the child fd number for grants while baseline system paths stay path-bound. Descriptors are mapped onto 10, 11, … via `command_fds::CommandFdExt::fd_mappings` only in the child after fork; the shell launcher keeps fds 10 through 10+k open and still closes every other inherited descriptor. `BubblewrapBackend::binds_by_descriptor()` and a debug log report which mode is active, gated on `bwrap --help` listing `--bind-fd` plus a working `openat2`, and the module's known-gaps documentation is updated to scope the remaining check-then-use window to builds and kernels without that support. `crates/tinybox-jail/Cargo.toml` gains `command-fds` (pinned =0.3.3) and `nix` 0.31 with the `fs` feature.

Features

  • Added — Descriptor-based grant binding via --bind-fd/--ro-bind-fd: Closes the check-then-mount window: the grant mounted is the object that was opened and checked, not a path re-resolved by bwrap. Baseline system paths continue to be bound by path, so only the jail's own grants change behavior. (crates/tinybox-jail/src/bwrap/args.rs pub(super) fn jail_args(, crates/tinybox-jail/src/bwrap/args.rs)
  • Modified — Nested-grant opening with RESOLVE_NO_SYMLINKS | RESOLVE_BENEATH: A grant inside an already-opened grant is opened relative to the parent descriptor and refused with ELOOP if a symlink was planted in between, where previously it would have been mounted; a symlink above a top-level grant is still left to the host, and a missing read-only grant is skipped rather than erroring. (crates/tinybox-jail/src/bwrap/args.rs, crates/tinybox-jail/src/bwrap/args.rs pub(super) fn jail_args()
  • Modified — Launcher preserves grant descriptors 10..10+k only: The shell launcher keeps exactly the grant descriptors the child needs and still closes every other inherited descriptor above 2, so no unrelated descriptor leaks into bwrap. (crates/tinybox-jail/src/bwrap/launcher.rs for fd in /proc/self/fd/*; do, crates/tinybox-jail/src/bwrap/launcher.rs pub(super) fn command()
  • Added — Post-fork fd mapping through command-fds: Descriptors are mapped onto 10, 11, … only in the child after fork, so they are never inheritable in the host process where another thread could spawn concurrently. (crates/tinybox-jail/src/bwrap/mod.rs fn prepare_command(verified: &Verified, jail: &Jail, cmd: &Command) -> io::Resul, crates/tinybox-jail/Cargo.toml)
  • Internal refactor — Path validation extracted into check_paths: The absolute/clean path checks and `reject_symlinked_grants` now have a single definition used by both the path-only and descriptor paths, with no behavior change. (crates/tinybox-jail/src/bwrap/args.rs, crates/tinybox-jail/src/bwrap/args.rs pub(super) fn jail_args()
  • Modified — Module documentation of the remaining known gap: The known-gaps section now scopes the check-then-use window to bwrap builds lacking `--bind-fd` or kernels before Linux 5.6, and describes the descriptor path otherwise. (crates/tinybox-jail/src/bwrap/mod.rs)

Tests

  • unit — Symlink swap, host-symlink, and missing-grant cases: a nested grant replaced by a link out after the root was opened fails with an error naming the path and ELOOP/symbolic links; a top-level grant spelled through a host symlink still opens; a missing read-only grant is skipped and never reaches the argv.: These exercise the real filesystem paths of the new opening logic, including the ELOOP and ENOENT branches. Not executed during this review. (crates/tinybox-jail/src/bwrap/args_tests.rs, crates/tinybox-jail/src/bwrap/launcher_tests.rs fn an_args_directory_reached_through_a_symlink_into_a_grant_is_refused() -> io::, crates/tinybox-jail/src/bwrap/args_tests.rs fn a_denial_through_a_baseline_symlink_also_covers_the_link_target() -> io::Resu)
  • integration — The live `bwrap_isolation` test checks that `backend.binds_by_descriptor()` matches whether `bwrap --help` lists `--bind-fd`, on the assumption that CI kernels provide `openat2`; the nested read-only grant test is cited alongside.: This is environment-dependent and asserts against the host bwrap rather than forcing each mode; not executed during this review. (crates/tinybox-jail/tests/bwrap_isolation.rs fn a_read_only_grant_nested_in_the_root_stays_read_only() -> io::Result<()> {)

Findings

No active actionable findings.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change threads a grant-descriptor count through the launcher so the shell script keeps fds 10..10+k open for `bwrap --bind-fd`, and the sibling tests are updated consistently (every call site passes 0 except the new test, which passes 2 and corrects the old `command(shell, program, ...)` argument order to `command(shell, &bwrap, &file, ...)`). The script's range check, the two existing callers in `mod.rs` and the new test all line up; I found no defect in these two files. _The code index is behind this pull request (indexed at `b23e4ec813aa`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change threads a grant-descriptor count into the launcher script so the descriptors mapped onto 10 and up survive while every other inheritable descriptor is still closed, with a test covering both. The shell arithmetic and quoting are unchanged in kind from the existing script, the count is derived from the repository's own opened grant list rather than external input, and the callers in `mod.rs` were updated to pass it. No reportable defect found in these two files. _The code index is behind this pull request (indexed at `b23e4ec813aa`), so retrieved context may be out of date._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: This turn's commit adds only the launcher-side half of the descriptor plumbing: `SCRIPT` now takes the grant count as `$2` and keeps fds 10..10+k instead of closing them, `command()` passes that count, and `the_grant_descriptors_reach_bwrap_and_nothing_else_does` pins both directions — the two mapped grants reach bwrap on 10 and 11 in order, and an unrelated inheritable descriptor is still closed. The test skips gracefully when bash is absent, which is the same convention the neighbouring `sh`-based tests use. I found no defect introduced by this revision. _The code index is behind this pull request (indexed at `b23e4ec813aa`), so retrieved context may be out of date._

commits

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The commits lane reported no sensitive content in committed files.
  • Lane summary: No sensitive content in committed files. _The code index is behind this pull request (indexed at `b23e4ec813aa`), so retrieved context may be out of date._

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The description lane found no defects, stating the description accurately covers the descriptor-count plumbing, the fd-keeping change, and the new behaviour for symlinked nested grants and dependencies.
  • Lane summary: This revision wires the grant-descriptor count through the launcher, keeps fds 10..10+k open in the cleanup loop, and adds a test proving the mapped descriptors reach the stand-in and a stray one is closed. The description accurately covers the change, including the behaviour change for symlinked nested grants and the new dependencies. No defects found. _The code index is behind this pull request (indexed at `b23e4ec813aa`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.003515
  • Tokens: 385365 input · 6041 output · 132864 cached · 839 embedding
  • Updated: 2026-11-12T07:30:13Z
  • Continuity: summary cache chain restarted at the storage ceiling.
Head State Pass summary
3d6e9e7828ee ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-11-11T12:31:50Z)
e7839006828e ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-11-11T14:17:28Z)
43057ccd2935 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-11-12T07:30:13Z)

tinysweeper 0.1.2

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

Diagnostics

Total spend: $0.0472

@tinysweeper tinysweeper Bot added priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later. and removed priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. labels Nov 11, 2026
# Conflicts:
#	crates/tinybox-jail/src/bwrap/launcher_tests.rs

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

Diagnostics

Total spend: $0.0035

@tinysweeper tinysweeper Bot added priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. and removed priority: p2 Soon. Real but survivable — a rough edge, a gap, a thing that will bite later. labels Nov 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant