sandbox: mirror the dynamic loader into the namespaces sandbox - #796
Conversation
|
Preview removed (PR closed). |
Avocet review statusState: Completed The formal GitHub review is the authoritative result. This comment only reports operational status. |
…he test on linux Binding the loader file itself broke FHS distros: the loader lives inside a /lib* directory that bindPlan already mirrors read-only, so creating the bind target file fails with EPERM. The directory bind covers the loader and its sibling libc either way.
avocet-bot
left a comment
avocet-bot
left a comment
There was a problem hiding this comment.
Review model: Claude Opus
Reviewed base: 438b1bcd9ddd9ac1cb6ecb6abdb6667f6b035363
Reviewed commit: bebacb9e7ea7cdd56147d320235eaae35d4125cc
Current PR base: 438b1bcd9ddd9ac1cb6ecb6abdb6667f6b035363
Current PR head: c76295d0a6de4ff5d0c5cd4dbc4467d5a4304d9a
Verdict for the reviewed commit: REQUEST_CHANGES (informational only)
This review applies only to the reviewed comparison above.
Newer commits and their integration with the current base were not reviewed.
Re-request review to review the current PR revision.
stage1_linux.go:269,311-314,321-332,353 — On standard FHS distros the new loader binds break namespaces-sandbox startup for dynamically-linked (cgo/non-Go) plugins, a regression from the prior working behavior.
Mechanism: for a typical glibc binary the ELF interpreter is /lib64/ld-linux-x86-64.so.2 (confirmed for the test helper on this repo). loaderBinds emits two binds — the interpreter file and its directory /lib64. dedupeBinds collapses only exact src strings, so the loader directory /lib64 is deduped against the FHS entry (line 257) but the loader file survives. Because loaderBinds is appended after the FHS list (line 269), the deduped plan binds /lib64 (ro) at index 3 and then the loader file /lib64/ld-linux-x86-64.so.2 afterward. By the time the loader-file bind runs, its parent root/lib64 is already a recursive read-only bind (remountRO applied in the same bindIntoRoot call). bindIntoRoot unconditionally creates the file mountpoint via os.OpenFile(target, O_CREATE|O_WRONLY, 0o644) (line 353); opening for write under a read-only mount returns EROFS, so bindIntoRoot errors, setupNamespaceRoot/stage1Main fail, and Stage1 exits 87 — the plugin never starts. This holds whenever the interpreter lives under any FHS-bound dir (/lib, /lib64, /usr/lib, multiarch subdirs), i.e. all mainstream distros.
I could not exercise the live mount here (unprivileged user namespaces are blocked in this environment, so Probe() returns landlock and TestDynamicBinaryRunsInNamespacesSandbox skips), but the failure follows directly from the bind ordering and standard read-only-filesystem semantics. Note the plan-only TestBindPlanIncludesLoader passes and gives false confidence because it never runs bindIntoRoot.
Required fix: don't emit a redundant loader bind when the interpreter already sits under an FHS-mirrored directory (e.g. skip a bind whose src is contained by an earlier planned bind), or order the loader binds before the FHS dir binds so the loader file's placeholder is created on the writable tmpfs and later shadowed by the dir bind. Extend dedup/containment handling accordingly, and ensure the e2e namespaces test actually runs (not just the plan assertion) on a standard-distro CI runner so this path is covered.
Avocet review statusState: Completed The formal GitHub review is the authoritative result. This comment only reports operational status. |
avocet-bot
left a comment
avocet-bot
left a comment
There was a problem hiding this comment.
Review model: Claude Opus
Model verdict: APPROVE (informational only)
Formal review policy did not authorize an authoritative event (required_checks_missing).
Re-review of the prior finding: FIXED. Previously loaderBinds returned both the interpreter file and its directory; the file bind survived dedup and, being ordered after the read-only /lib64 bind, hit EROFS in bindIntoRoot's OpenFile(O_CREATE|O_WRONLY) placeholder step — breaking dynamically-linked plugins on standard FHS distros. The new loaderBinds (stage1_linux.go:311) returns only filepath.Dir(interp). On standard x86-64 the loader dir is /lib64, an exact FHS entry, so dedupeBinds drops it (confirmed on this host). Directory binds never create a file placeholder — they MkdirAll (a no-op on an already-present dir) then Mount (which does not write to the underlying fs) — so even a multiarch subdir loader (/lib/x86_64-linux-gnu under an already-RO /lib) no longer fails. The NixOS/Guix case now binds the store loader dir and works. Tests pass/skip correctly here (namespaces backend unavailable, so the e2e test skips to landlock); gofmt and vet clean; the added //go:build linux guard is correct.
Non-blocking: on non-FHS distros, mirroring only the loader's directory brings glibc's co-located libs but not shared libraries the binary pulls from other store paths via RPATH; a purely libc-linked plugin (like the test helper) works, but richer non-Go plugins may still miss transitive deps. Not a regression from base and out of scope for this fix.
2 participants
What
Fixes external plugins failing to start on non-FHS distros (NixOS, Guix).
The namespaces sandbox's bind plan only mirrored FHS loader locations
(
/lib,/lib64,/usr/lib*, ...); on NixOS/Guix the dynamic loaderlives under
/nix/store//gnu/store, so exec of any dynamically-linkedplugin failed with
ENOENTandgo-pluginreported a misleading"Unrecognized remote plugin message" handshake error. The probe did not
catch it because it never execs a real binary.
Changes
bindPlannow discovers the plugin binary's dynamic loader from its ELF.interpsection (debug/elf) and mirrors the loader file and itsdirectory (glibc keeps
libc.so.6etc. next to the loader) into thesandbox, deduplicated against the FHS entries. No distro-specific
hardcoding — covers NixOS, Guix, and any future layout; static binaries
are unaffected (no-op).
internal/sandbox/loader_bind_test.go(+testdata/loaderhelper/, a small dynamically-linked helper):bindPlanincludes the binary's exact loader path,namespaces sandbox.
Verification
internal/sandboxtests pass, including-racego test ./cmd/clawpatrol/plugin-spawn tests (TestExampleSocksTunnel,TestExternalCredentialInjectsAuthorizationThroughBuiltInHTTPS, ...)previously failed on NixOS; they now pass (only pre-existing
environmental Landlock failure remains on this host)
gofmt -l .cleanCloses #794