refactor(seccomp): extract trusted policy substrate - #6119
Conversation
|
Changed-file classification: backend/runtime. Author-reported runtime / visual state
Checkboxes are author attestations. GitHub Actions results remain the execution evidence for CI; this check does not prove that a local command ran. This comment updates automatically when the description or changed files change. |
github-actions
Bot
added
the
needs runtime validation
StressTestor
left a comment
StressTestor
left a comment
There was a problem hiding this comment.
reviewed at 07d58ef5655a6bc0baca1cb8551e2e6c13b1e28a, always against the production build (make -C security/seccomp all, no -DODYSSEUS_LAUNCHER_TESTING). measured on Debian 13 aarch64 with bubblewrap 0.12.0, again on Ubuntu 24.04 aarch64 (kernel 6.8.0, bubblewrap 0.9.0) as an ordinary non-root user, and built plus generator-checked on x86_64 Debian 12.
verdict: request changes, and the argv handling at odysseus-seccomp-launcher.c:993-998 is the only blocking part. everything else below is non-blocking: two coverage gaps, one broken invariant in the test oracles, and four maintenance items left as inline comments.
what holds up
i re-fetched every pin myself. the Moby blob at 35797366d7cdae8d1d84eac06fbb314ccaf3ccaf is sha256 536529b665dd…, exactly policy.json, and the vendored copy is byte-identical modulo the trailing newline _verify_source strips. the bubblewrap v0.11.0 annotated tag object a871b148… dereferences to commit 9ca3b05e…, and the release archive is 988fd6b2…. four for four.
the generator's refusal to flatten argument-sensitive rules is the subtle thing here and it's right: ioctl, clone, clone3, personality, socket, socketpair appear zero times in the generated allowlist, so the conditional rules in build_filter can't be shadowed by a blanket allow. i resolved all 356 x86_64 and 360 aarch64 names through libseccomp and confirmed no collision with the six conditional syscall numbers, so that holds at the number level too.
the masked TIOCSTI deny at odysseus-seccomp-launcher.c:246-253 is a real bug class handled correctly, and tests/seccomp_probe.c carries the high-bit bypass probe. i also tried to break the appended clone rule's tightness: only 0x7e020011 passes, while 0x10000011, 0x10020011 and 0x7e020000 all stay denied. SCMP_CMP_EQ admits exactly one word.
blocking: the argv you validate isn't the argv bwrap receives
odysseus-seccomp-launcher.c:993-998 drops every token equal to --clearenv while copying the caller's option region. validate_bwrap_arguments accepts --setenv NAME VALUE for 13 names with no constraint on VALUE beyond length (:713-724), so --setenv TERM --clearenv is a well-formed three-token option to the validator. emission then deletes the middle token and --setenv TERM swallows whatever comes next. one token removed, one absorbed, so everything downstream realigns and bwrap parses the rest normally.
put the pair mid-region and it eats the next zero-argument flag. seven of the eight contract flags go this way, one per pair, using a different allowed name each time. launcher rc=0, payload netns equals the launcher's own, the payload sees lo and eth0 where the control sees only lo, and the payload environment reads TERM=--unshare-net. that last part is the bug in one string.
put the pair last and it reaches your own trailer instead. --setenv TERM swallows the --seccomp you append at :999, 3 becomes bwrap's command, and since PATH is an allowed --setenv name a file named 3 in the workspace bind is what runs:
odysseus-seccomp-launcher /usr/bin/bwrap <full fixed contract> \
--setenv PATH /work --setenv TERM --clearenv -- /work/report
emitted tail: [--setenv] [TERM] [--seccomp] [3] [--ro-bind] [/dev/null] [/usr/bin/bwrap] [--] [/work/report]
| Debian 13 aarch64, privileged | control | attack |
|---|---|---|
| launcher exit | 0 | 0 |
Seccomp: |
2 | 0 |
Seccomp_filters: |
1 | 0 |
/usr/bin/bwrap in sandbox |
0 bytes, character special | 133432 bytes, regular file |
| bpf / keyctl / ptrace | EPERM | not denied |
so the inner default-deny filter and the /dev/null mask over /usr/bin/bwrap are both absent while validation returns success. that's the whole substrate this PR exists to establish.
this is not a root-only or container-only artifact. same result on Ubuntu 24.04, kernel 6.8.0, bwrap 0.9.0, running as an ordinary non-root user against the real binary: control Seccomp: 2 with bwrap masked to a 0-byte character special, attack Seccomp: 0 with bwrap a 67816-byte regular file, launcher rc=0 in both. the mid-region variant strips --unshare-net there too, payload netns equal to the launcher's.
what doesn't break, so this isn't overstated: --cap-drop ALL, --proc /proc, --dev /dev and --chdir all fail closed, because stripping a two-token option orphans its argument and bwrap dies exec'ing it. without an executable named 3 on PATH it also fails closed with execvp 3: No such file or directory.
nothing in the tree constructs the launcher's argv at this ref, and Makefile:32 installs it non-setuid 0755, so it grants no privilege a caller lacked and anyone who can call it can already run /usr/bin/bwrap directly. this is a guardrail that fails open. it still matters, because the guardrail is the deliverable.
the shape of the fix: emit from the parse you already did rather than re-scanning strings, since validate_bwrap_arguments already walks every token by index. one caution if you instead prepend --clearenv unconditionally and tolerate a duplicate: bwrap applies these in order, so --setenv TERM xterm --clearenv ends with TERM unset.
the coverage gap that hid it
the PR is green, and the green doesn't cover the runtime matrix.
dev bc7514fa run 32845804877 : 5798 passed, 4 skipped
PR 07d58ef run 32873082100 : 5809 passed, 66 skipped (job 97884506481)
+11 passed, +62 skipped. every runtime test is gated on skipif(not BWRAP.is_file()); bubblewrap isn't in the ubuntu-24.04 runner image and no workflow installs it. i reproduced both halves: with bwrap removed and a python shim present the three new files give exactly 11 passed / 62 skipped; with bwrap present they give 73 collected / 0 skipped, and the launcher file alone is 64 passed. so the 28-case syscall matrix, the TIOCSTI pair, the direct_bwrap masking check, the environment scrubbing test and every mount-profile acceptance test never execute. two launcher tests survive in CI.
structurally: test_launcher_requires_every_fixed_isolation_option does arguments.remove(option) and asserts rejection, so everything asserts on what the validator rejects and nothing on what bwrap receives. a test_emitted_argv_matches_validated_intent alongside it, diffing bwrap's actual argv against the validator's input, would have caught this.
installing bubblewrap in the pytest job is the cheap fix, but pin it: ubuntu-24.04 ships 0.9.0, policy.json pins 0.11.0, and #6121's boot self-test compares against the literal string bubblewrap 0.11.0. a bare apt-get install -y bubblewrap would test a version the project rejects.
the tests exercise a different binary
tests/test_seccomp_launcher.py:30 builds the test-launcher target, compiled -DODYSSEUS_LAUNCHER_TESTING. that macro guards five sites, and two of them weaken validate_ro_bind: it accepts --ro-bind /proc /proc (:551-559) and accepts any nlink==1 regular file at /run/odysseus/command.sh (:586-590) instead of the production rule at :592-607.
the production binary rejects the base profile the whole suite is built on, rc=65, so most of the suite runs a mount profile the shipped binary refuses. the two paths that exist only in the production build, the command.sh naming rule and that base-profile rejection, have no automated coverage on any machine. the rest of the validator is covered, just through a binary that differs from the shipped one at five sites.
eight of the deny probes pass with no filter loaded
worth fixing alongside the blocker: even once CI runs the matrix, eight deny checks can't distinguish the inner filter from an independently produced EPERM.
using the crafted vector above as a free negative control, the payload runs with Seccomp: 0 and no
filter at all. re-running the matrix in that state, eight probes still report success:
clone_namespace, unshare, mount, umount2, pivot_root, open_by_handle_at, af_packet and
userfaultfd. all eight are denied independently of seccomp, by the dropped
capabilities or by host policy. userfaultfd is the one that has nothing to do with capabilities:
vm.unprivileged_userfaultfd is 0 on both hosts i measured, so it returns EPERM to any
unprivileged process whether or not the inner filter exists. the other fifteen do discriminate: bpf, perf_event_open, clone3, setns, ptrace,
process_vm_readv, process_vm_writev, keyctl, af_alg, af_vsock, io_uring_setup,
socketpair_inet_denied, personality_denied, tiocsti and tiocsti_high_bits all flip to
not-denied once the filter is gone.
measured identically in three environments, including that real kernel as a non-root user. so eight of the 23 seccomp-denial checks would stay green if the inner filter were absent. (direct_bwrap is excluded from that population on purpose: it tests the /dev/null mask rather than the filter.) the fix is a
negative control in the suite: load the payload without the filter and assert the deny probes fail.
the allowlist can lose a real syscall on a re-pin, silently
generate.py:189-199 resolves 9 names; add_allowlist (:175-177) silently skips anything
libseccomp can't resolve. a hand edit to moby-default.json alone is caught by _verify_source,
and a hand edit to the generated header is caught by --check (which make all depends on). but a
re-pin refreshes the digest as part of the same operation, and then nothing catches it.
renamed read to read_typo_probe in the vendored profile and refreshed upstream_sha256 the way
a Moby bump would:
generate.py (regenerate) rc=0
generate.py --check --verify-arches rc=0
make all rc=0
"read" in the built allowlist: 0 occurrences
read() is now default-denied and every gate is green. it surfaces at runtime as
/bin/sh: error while loading shared libraries: libc.so.6: cannot read file data: Operation not permitted, which is a long way from the cause. adding the runtime-essential names like read to required is the cheap half. failing on every negative resolution would be wrong, since legitimate arch-specific names resolve negative by design (60 of 356 on x86_64, 106 of 360 on aarch64 on my hosts); pinning the expected-unresolved set per target arch and diffing against it is the version that holds.
checked and dropped
i thought --ro-bind /dev/null <dest> short-circuiting the broker and venv destination checks at :560-562 was a security issue. it isn't: /dev/null places no host content at the destination, omitting the broker mount entirely is already an accepted contract state, and bwrap rejects both orderings of a real mount plus a /dev/null mount at the same path. it reads oddly but grants nothing.
|
|
||
| `generate.py` removes capability-dependent and argument-dependent rules, then adds the reviewed payload constraints recorded in `policy.json`. It emits the C allowlist consumed by the trusted launcher and the outer OCI profile used only by the Odysseus Compose service. Run `python3 generate.py --check --verify-arches` to verify provenance, deterministic output, and syscall resolution for x86_64 and ARM64. | ||
|
|
||
| The outer bootstrap trace is tied to the upstream Bubblewrap v0.11.0 release commit and annotated-tag object, its release-archive digest, and Debian package `0.11.0-2+deb13u1`. The image build rejects a different package or reported Bubblewrap version until the exact namespace and mount trace is reviewed and the policy is deliberately regenerated. |
There was a problem hiding this comment.
non-blocking: this sentence promises a gate that isn't in this slice. #6121 does implement it exactly (BWRAP_POLICY_VERSION=0.11.0 / BWRAP_POLICY_PACKAGE=0.11.0-2+deb13u1 checked against bwrap --version and dpkg-query in the Dockerfile, plus the boot self-test comparing against the literal bubblewrap 0.11.0). inside these 12 files nothing reads bubblewrap_release_sha256, bubblewrap_tag_object, bubblewrap_commit or shipped_package_basis outside policy.json and tests/test_seccomp_policy.py:132-143, which asserts those literals against a hardcoded copy of themselves. i ran this slice on bubblewrap 0.12.0 against the pinned 0.11.0 and generate.py --check --verify-arches still exits 0. reading #6119 alone the README makes a promise the slice can't keep, so either move the sentence to #6121 or mark it forward-looking. the Moby digest IS genuinely enforced here, by _verify_source.
| }, | ||
| { | ||
| "names": [ | ||
| "mount", |
There was a problem hiding this comment.
non-blocking: this second appended rule is the largest security delta in the PR and worth calling out so the file isn't read as reviewed-and-cleared. it promotes mount/pivot_root/umount2 to unconditional allow, and pivot_root appears in no Moby block at all, so it goes from default-deny to allow. measured with --security-opt seccomp=<profile> and default caps: under Moby's profile clone(0x7e020011) is denied; under this one it's allowed and the child lands in a fresh user+mount+pid+net namespace with CapEff: 000001ffffffffff. seccomp profiles are per-container, so this reaches every process in the container. README paragraph 6 does disclose that the outer profile isn't an isolation boundary for the service process, and the trio genuinely can't be narrowed since seccomp can't dereference mount's pointer args. the clone rule itself is tight: only 0x7e020011 passes, 0x10000011 / 0x10020011 / 0x7e020000 all stay denied.
| return true; | ||
| } | ||
|
|
||
| static bool sensitive_mount_source(const char *path) |
There was a problem hiding this comment.
non-blocking: sensitive_mount_source only inspects the named operand, never the subtree, and that holds in both mount profiles. --ro-bind /root/.ssh /run/x is correctly rejected rc=65, but with the ordinary sandbox profile and a workspace that contains .ssh, --bind /work /work hands the payload the key: with /work/.ssh/id_ed25519 present, cat on it inside the sandbox succeeds, rc=0. worth deciding explicitly whether this list is a control or a reminder, because a reader will assume the former.
|
|
||
| def test_generated_policy_matches_pinned_source_for_both_architectures(): | ||
| completed = subprocess.run( | ||
| ["python", "generate.py", "--check", "--verify-arches"], |
There was a problem hiding this comment.
non-blocking: this shells out to python while security/seccomp/Makefile:18 uses python3. on stock Debian 13, the platform the README's native make path targets, there's no python and the test errors with FileNotFoundError: [Errno 2] No such file or directory: 'python'. reproduced. CI hides it because actions/setup-python provides a shim.
| bool unshare_cgroup; | ||
| bool die_with_parent; | ||
| bool new_session; | ||
| bool clearenv; |
There was a problem hiding this comment.
nit: contract.clearenv is written at odysseus-seccomp-launcher.c:703 and never read. it isn't required by fixed_isolation, so the flag is accepted-and-deduplicated but means nothing.
| static bool load_symbol(void *handle, const char *name, void *destination, size_t size) | ||
| { | ||
| void *symbol = dlsym(handle, name); | ||
| if (symbol == NULL || size != sizeof(symbol)) { |
There was a problem hiding this comment.
nit: size != sizeof(symbol) compares two pointer sizes that are equal on x86_64 and aarch64, the only two the launcher builds for, so this guard can't fire. harmless, but it reads like a real check in the one place that loads the security-critical ABI.
| contract.writable_root, | ||
| contract.broker_runtime_source | ||
| ))); | ||
| return fixed_isolation && mount_profile && separated_runtime_mounts |
There was a problem hiding this comment.
nit: this mixes && with ?: unparenthesised in the expression that decides whether the whole sandbox contract holds. the precedence works out to what you want, but this is the last line in the file i'd want a reader to have to squint at.
| @@ -0,0 +1,35 @@ | |||
| CC ?= cc | |||
| CPPFLAGS := -D_FORTIFY_SOURCE=3 | |||
| CFLAGS := -std=c11 -O2 -fPIE -fstack-protector-strong -Wall -Wextra -Wpedantic -Werror -Wformat=2 -Werror=format-security | |||
There was a problem hiding this comment.
nit: CFLAGS := overwrites environment-supplied hardening flags for odysseus-seccomp-launcher, which the README tells native installs to build and make install as root. a command-line make CFLAGS=... still wins, so this only bites the environment path, which is the one dpkg-buildflags uses.
2 participants
Summary
This draft extracts the trusted seccomp substrate from the cumulative sandbox work in #5818 into an independently reviewable change. It adds deterministic Moby-derived policy generation with pinned provenance, generated inner and outer artifacts, the hardened fixed Bubblewrap launcher, and focused generator, policy, and launcher tests. Dockerfile, Compose, AppArmor, boot self-test, and process-routing integration remain outside this slice.
The scope is exactly these 12 paths:
docker/seccomp/odysseus-bubblewrap.jsonsecurity/seccomp/Makefilesecurity/seccomp/README.mdsecurity/seccomp/generate.pysecurity/seccomp/generated_inner_policy.hsecurity/seccomp/moby-default.jsonsecurity/seccomp/odysseus-seccomp-launcher.csecurity/seccomp/policy.jsontests/seccomp_probe.ctests/test_seccomp_generator.pytests/test_seccomp_launcher.pytests/test_seccomp_policy.pyThe generated outer profile
docker/seccomp/odysseus-bubblewrap.jsonis canonically owned by this PR. PR #6121 consumes this generated profile for Docker/Compose integration and must not duplicate or regenerate it.Target branch
dev, notmain. All PRs land indev;mainis curated by the maintainer at each release. If your PR is onmainby accident, click "Edit" on this PR and change the base.Linked Issue
Fixes #6114
Part of #6091
Part of #5815
Related to #5818; this supersedes the corresponding seccomp-substrate slice of that cumulative PR.
Related to #4754, the broader sandbox request.
Type of Change
Checklist
devdocker compose uporuvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
All commands below were run in a secretless review environment against this exact 12-path branch.
python3 security/seccomp/generate.py --check --verify-archesto verify pinned Moby provenance, deterministic generated artifacts, and x86_64/ARM64 resolution; it passed.make -C security/seccomp check-generated; it passed. Runpython3 -m py_compile security/seccomp/generate.py; it passed.python3 -m pytest -q tests/test_seccomp_generator.py tests/test_seccomp_policy.py; 7 tests passed. Runpython3 -m pytest -q tests/test_seccomp_launcher.py; 16 tests passed.Visual / UI changes — REQUIRED if you touched anything that renders
Anything that changes what the UI looks like — buttons, icons, padding, colors, fonts, spacing, layout, CSS, HTML, SVG, or any
static/js/module that draws to the DOM — needs all of the following. PRs that change rendering without these WILL be closed.This slice does not touch UI-rendering files.
--red,--fg,--bg,--card,--border, etc.) — do not introduce new color values, font sizes, or spacing units.static/index.html) or plain text.Fira Code) for primary UI text. Don't override.Screenshots / clips