Repository navigation
Conversation
…he mask gap The 2.1 native resource jail masked a missing deny_read path with --ro-bind /dev/null, which makes bwrap create an empty file at that path in the caller's tree; and a path created later was not covered anyway. As the bwrap jail does since tinyhumansai#107, a missing denial inside a mount is now refused and one outside every mount is skipped. The docs said missing denials are ignored, or retained to cover later creation, which bubblewrap does neither. They also did not say that a mask lasts only as long as the path it covers: a host that replaces a denied path inside a grant (an atomic rename) exposes the new file to a running jail, shown on bubblewrap 0.9.0 and Linux 7.0. That is now a documented known gap with what a host should do instead. Part of tinyhumansai#104
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Tiny Sweeper reviewReviewed head: Findings
Resolved this pass
Verdict: Changes requested · Priority: high Diagnostics and supporting detailsFull summaryPull request purpose (title): Refuse a missing deny_read inside a 2.1 jail's mounts, and document the mask gap. This pull request closes item 6 of #104 for the 2.1 native resource jail: a missing `deny_read` path inside a mount is now refused before launch instead of being masked with `--ro-bind /dev/null`, which made bubblewrap create an empty file on the host, and a missing denial outside every mount is skipped when the argv is built. The bwrap backend's known-gaps documentation now records that a mask lasts only as long as the path it covers, and the `deny_read` docs in `tinybox-bus` and `tinybox-jail` are corrected to describe per-backend handling. One blocking finding remains open: the new test builds `NativeJailCommand` without its `disable_userns` field, so the tinybox-linux test target will not compile and the new behaviour is never exercised. The latest-push-delta section is unavailable; the whole-pull-request diff is described here, not a push delta. Review snapshot
Completeness: Complete What changedThe native jail argv builder runs a preflight check, `refuse_missing_denials_inside_mounts`, that returns `NotFound` when a `deny_read` path does not exist and lies inside the root, a read-only grant, or a read-write grant. The argv construction loop filters out denials whose `symlink_metadata` fails, so a missing denial outside every mount is never masked. Documentation in `NativeJailConfig::deny_read`, `Jail::deny_read`, and the bwrap known-gaps list is updated to state that bubblewrap refuses a missing denial inside a mount, that a mask lasts only until the host replaces the path, and what a host should do instead. A new Linux-only test covers both branches, but it omits the `disable_userns` field of `NativeJailCommand`, which prevents the tinybox-linux test target from compiling. Features
TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Before merge
Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
tinysweeper
Bot
added
the
priority: p3
# Conflicts: # crates/tinybox-bus/src/jail.rs
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
| read_write: Vec::new(), | ||
| deny_read, | ||
| allow_net: false, | ||
| allow_subprocess: true, |
There was a problem hiding this comment.
Add the missing disable_userns field to the test request
`NativeJailCommand` declares a `disable_userns` field that `native_bwrap_argv` reads (`if request.disable_userns {`), and the only in-repo constructor in `crates/tinybox-module/src/tinybus_module/native_jail.rs` sets it (`disable_userns: bwrap_disables_userns()`). The struct is a plain `#[derive(Debug, Clone)]` type with no `Default`, so a struct literal must name every field: this literal in `a_missing_denial_inside_a_mount_is_refused_and_one_outside_is_skipped` omits `disable_userns`, and `cargo test -p tinybox-linux` fails with E0063 (missing field `disable_userns` in initializer of `NativeJailCommand`). Add the field so the file compiles.
[RULE] missing-struct-field ·
tinysweeper
Bot
added
priority: p1
Summary
Item 6 of #104 asks about
deny_readpaths created after the jail starts.The 2.1 native resource jail. Since #107, the bubblewrap jail refuses a missing
deny_readinside a mount, because a path created later would not be covered. The 2.1 native resource jail had no such check. It masked a missing path with--ro-bind /dev/null <path>, and bubblewrap then creates that path on the host: an empty, read-only file in the caller's tree. This was shown on bubblewrap 0.9.0. The 2.1 path now does what the bwrap jail does:NotFound, the same message);A documented gap. A mask lasts only as long as the path it covers. A denial is a mount over that path's directory entry. When a host process replaces the path while the jail runs (an atomic rename, as credential helpers and editors write, or delete and recreate), the kernel drops that mount from the jail's namespace, and the jail sees the new file. This was shown on bubblewrap 0.9.0 and Linux 7.0 for both a file and a directory mask. bubblewrap can't prevent it.
The bwrap backend's known-gaps docs now say so, with what a host should do instead: deny a credential store outside every grant, so it's never mounted at all, or deny its parent directory.
The
deny_readdocs are also corrected. They said missing paths are "ignored" (bus) or "retained so backend rules can cover later creation" (jail), and bubblewrap does neither.Related issue
Part of #104 (item 6). Items 1-5 are #160 and #167; item 6's seccomp filter is #161. The last part, resource limits on legacy bwrap jails, follows once #159 lands.
API or behavior changes
StartJailedExecWithResourceswith a missingdeny_readpath inside the root or a grant is now refused before launch. Before, it was accepted and an empty file was created on the host.Validation
On our Linux CI runner (bubblewrap 0.9.0), at 780e281695. The head differs only by applying
cargo fmt's patch:cargo clippy --all-targets --all-features -- -D warnings: 0 errors, same as main.cargo buildandcargo doc: ok.cargo test -p tinybox-linux sandbox: 41 passed.cargo test -p tinybox-module native_jail: 21 passed, all live tests run (none skipped).cargo test --workspace --no-fail-fast: no failures unique to this branch.a_missing_denial_inside_a_mount_is_refused_and_one_outside_is_skippedfails (no refusal).--ro-bind /dev/nullmask and a--tmpfsmask both stop covering their path once the host replaces it;Tests
a_missing_denial_inside_a_mount_is_refused_and_one_outside_is_skipped: inside the root, refused withNotFound, and nothing is created on the host; outside every mount, the build succeeds and the path isn't in the argv.Documentation
The known gaps in
tinybox-jail/src/bwrap/mod.rs(the mask lifetime),NativeJailConfig::deny_readintinybox-bus, andJail::deny_readintinybox-jail.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description