From 341ccf28f1cb8d9fa9c4d74e7d85410f7c864a37 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 30 Jul 2026 09:36:16 +0300 Subject: [PATCH 1/2] fix(sandbox): pin the Landlock thread so the sandboxed exec cannot escape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Landlock domains are per-THREAD: landlock_restrict_self(2) commits the new credentials on the calling thread only. Apply ran on whatever OS thread the goroutine happened to occupy, and RunChild then did a blocking diag write, a best-effort privilege drop and os.Environ() before syscall.Exec. Each of those can park the goroutine; on a loaded machine sysmon retakes the P and the goroutine resumes on a different M. The execve then ran the untrusted MCP server from a thread that never entered the domain — completely unconfined, while the log still said "Landlock enforced". Pin the goroutine to its thread at the top of Apply (deliberately never unlocked; the intended caller execs immediately and the confinement is irreversible anyway), and record the confined tid on the Report so RunChild can re-check it immediately before execve and refuse (exit 4) rather than exec unconfined if the pin ever fails. The package docs asserted the wrong premise — that Apply confines the *process* — which is what made the per-thread hazard invisible. Correct them: Apply confines a thread, only the re-exec-then-exec shape is sound, and an in-process Apply cannot confine a multithreaded Go process. Measured on a 14-CPU Linux 6.12 kernel with the new regression test: 20/20 exec'd commands escaped the domain without the pin, 0/100 with it. --- .github/workflows/sandbox-integration.yml | 24 +++ internal/sandbox/runchild_unix.go | 26 +++ internal/sandbox/runchild_unix_test.go | 50 +++++ internal/sandbox/sandbox.go | 23 ++- internal/sandbox/sandbox_linux.go | 34 +++- internal/sandbox/sandbox_linux_test.go | 9 +- internal/sandbox/sandbox_other.go | 4 + internal/sandbox/threadpin_linux_test.go | 203 +++++++++++++++++++ internal/sandbox/wrap.go | 9 +- internal/upstream/core/sandbox_linux_test.go | 31 +++ 10 files changed, 397 insertions(+), 16 deletions(-) create mode 100644 internal/sandbox/runchild_unix_test.go create mode 100644 internal/sandbox/threadpin_linux_test.go diff --git a/.github/workflows/sandbox-integration.yml b/.github/workflows/sandbox-integration.yml index 5bbe367f..367df9bb 100644 --- a/.github/workflows/sandbox-integration.yml +++ b/.github/workflows/sandbox-integration.yml @@ -80,6 +80,30 @@ jobs: ./internal/upstream/core/... \ ./internal/security/scanner/... + # 4b. The escape this suite exists to catch is scheduler-dependent: the + # Landlock domain is per-thread, so the bug only shows when the Go + # runtime moves the goroutine off the confined thread between Apply and + # execve. On an idle runner that never happens and every step above + # passes even with the bug present. Saturate the CPUs first so the + # enforcement tests are actually discriminating. + - name: Sandbox enforcement under scheduler load + timeout-minutes: 10 + run: | + set -euo pipefail + set +m # no job-control messages for the load pack + pids="" + # shellcheck disable=SC2034 + for _ in $(seq 1 "$(nproc)"); do + ( while :; do :; done ) & + pids="$pids $!" + done + # Kill the load pack however this step exits (pass, fail, or timeout) + # so a stuck busy loop can never outlive the step and hang the job. + # shellcheck disable=SC2064 + trap "kill $pids 2>/dev/null || true" EXIT INT TERM + go test -race -count=30 -run 'TestSandboxWrapper_EndToEnd' ./internal/upstream/core/ + go test -race -count=10 -run 'TestApplyPinsThreadAcrossReschedule|TestLandlockEnforcesFilesystemAllowlist' ./internal/sandbox/ + # 5. Build the binary (proves sandbox code compiles on linux/amd64). - name: Build mcpproxy binary run: go build -v -o mcpproxy ./cmd/mcpproxy diff --git a/internal/sandbox/runchild_unix.go b/internal/sandbox/runchild_unix.go index 5e98b9fd..c3202905 100644 --- a/internal/sandbox/runchild_unix.go +++ b/internal/sandbox/runchild_unix.go @@ -22,6 +22,9 @@ import ( // On success it never returns (execve replaces the image). It returns a non-zero // exit code on any failure; diag receives human-readable confinement notes and // errors (os.Stderr in production, so they land in the per-server upstream log). +// Exit codes: 2 bad invocation or missing spec, 3 confinement unavailable while +// fail-closed, 4 the Landlock domain's thread was lost before execve, 126 execve +// failed, 127 target not found on PATH. func RunChild(argv []string, diag io.Writer) int { if diag == nil { diag = io.Discard @@ -65,6 +68,19 @@ func RunChild(argv []string, diag io.Writer) int { dropPrivilegesBestEffort(diag) + // Everything between Apply and here — the diag write above, the privilege + // drop, os.Environ() — can park this goroutine. Apply pins it to the thread + // that carries the Landlock domain, so a mismatch here means that pin failed + // and an execve now would run the untrusted command unconfined. Refuse + // instead: an MCP server that does not start is recoverable, one that starts + // outside its sandbox is not. + if now := currentTID(); threadLost(rep, now) { + fmt.Fprintf(diag, "sandbox: refusing to exec %q — thread changed after confinement "+ + "(Landlock domain is on tid %d, now running on tid %d); the command would run UNCONFINED\n", + target, rep.LandlockTID, now) + return 4 + } + if err := syscall.Exec(target, argv, os.Environ()); err != nil { fmt.Fprintf(diag, "sandbox: exec %q: %v\n", target, err) return 126 @@ -72,6 +88,16 @@ func RunChild(argv []string, diag io.Writer) int { return 0 // unreachable: Exec replaced the image on success. } +// threadLost reports whether the Landlock domain Apply committed lives on a +// different thread than the one now about to execve — in which case the exec +// would run the untrusted command outside the domain entirely. A zero +// LandlockTID means no domain was enforced (rlimits-only, or Landlock +// unavailable under BestEffort), so there is nothing to verify and nothing to +// lose by exec'ing. +func threadLost(rep Report, nowTID int) bool { + return rep.LandlockTID != 0 && nowTID != rep.LandlockTID +} + // describeReport renders a one-line honest summary of what Apply enforced. func describeReport(rep Report) string { switch { diff --git a/internal/sandbox/runchild_unix_test.go b/internal/sandbox/runchild_unix_test.go new file mode 100644 index 00000000..76ef10a4 --- /dev/null +++ b/internal/sandbox/runchild_unix_test.go @@ -0,0 +1,50 @@ +//go:build unix + +package sandbox + +import "testing" + +// TestThreadLost pins the fail-closed guard's decision table. RunChild consults +// it immediately before execve: a true verdict must abort the exec, because +// exec'ing from a thread outside the Landlock domain runs the untrusted command +// with no confinement at all. +func TestThreadLost(t *testing.T) { + cases := []struct { + name string + rep Report + nowTID int + want bool + }{ + { + name: "same thread — safe to exec", + rep: Report{LandlockABI: 5, LandlockTID: 4242}, + nowTID: 4242, + want: false, + }, + { + name: "thread changed after confinement — must refuse", + rep: Report{LandlockABI: 5, LandlockTID: 4242}, + nowTID: 4243, + want: true, + }, + { + name: "no domain enforced (rlimits only) — nothing to verify", + rep: Report{LandlockTID: 0}, + nowTID: 4243, + want: false, + }, + { + name: "Landlock unavailable under BestEffort — nothing to verify", + rep: Report{LandlockABI: -1, LandlockTID: 0}, + nowTID: 99, + want: false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := threadLost(c.rep, c.nowTID); got != c.want { + t.Errorf("threadLost(%+v, %d) = %v, want %v", c.rep, c.nowTID, got, c.want) + } + }) + } +} diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index a353dd90..c4f5b719 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -12,11 +12,19 @@ // recommendation and the honest limits (no uid/gid separation without // privilege, filesystem-allowlist + rlimits only). // -// This package is intentionally minimal: it confines the *current* process and, -// because Landlock domains are inherited across execve, every child it then -// execs (the npx/uvx server and its descendants). The intended integration is a -// tiny re-exec wrapper that calls Apply and then execs the untrusted command; -// the package test exercises exactly that shape. +// This package is intentionally minimal: it confines the *current thread* and, +// because a Landlock domain is inherited across execve, every process that +// thread then execs (the npx/uvx server and its descendants). +// +// The per-thread scope is load-bearing, not a footnote: landlock_restrict_self(2) +// commits the new credentials on the calling thread only. A multithreaded Go +// process therefore CANNOT be confined by calling Apply in-process — the other +// threads keep full filesystem access, and even the calling goroutine may be +// rescheduled onto an unrestricted thread. The only sound shape is the one this +// package implements: a tiny re-exec wrapper that calls Apply and immediately +// execs the untrusted command from the very thread Apply restricted. Apply pins +// that thread (runtime.LockOSThread) and RunChild verifies the thread identity +// again just before execve; the package test exercises exactly that shape. package sandbox import "errors" @@ -74,6 +82,11 @@ type Report struct { // NoNewPrivs reports whether PR_SET_NO_NEW_PRIVS was set (always true when // Landlock is enforced; Landlock requires it). NoNewPrivs bool + // LandlockTID is the OS thread id (Linux tid) the Landlock domain was + // committed on, or 0 when no domain was enforced. A caller about to execve + // must confirm it is still running on this thread: an execve from any other + // thread runs the target unconfined. + LandlockTID int } // wantsLandlock reports whether the spec asks for any filesystem confinement. diff --git a/internal/sandbox/sandbox_linux.go b/internal/sandbox/sandbox_linux.go index 897fa5ff..c68705a7 100644 --- a/internal/sandbox/sandbox_linux.go +++ b/internal/sandbox/sandbox_linux.go @@ -4,6 +4,7 @@ package sandbox import ( "fmt" + "runtime" "strings" "unsafe" @@ -63,13 +64,27 @@ func Available() bool { return err == nil && abi >= 1 } -// Apply confines the current process per spec. On success the calling process -// — and every process it subsequently execs — can only touch the filesystem -// subtrees in the allowlist, under the supplied rlimits. The restriction is -// irreversible for the lifetime of the process, which is why the intended -// caller is a short-lived re-exec wrapper that calls Apply and immediately -// execs the untrusted command. +// Apply confines the calling THREAD per spec (rlimits, being per-process, apply +// process-wide). On success that thread — and every process it subsequently +// execs — can only touch the filesystem subtrees in the allowlist. The +// restriction is irreversible for the lifetime of the thread. +// +// Apply cannot confine a multithreaded Go process: landlock_restrict_self(2) +// commits credentials on the calling thread alone, so every other thread stays +// unrestricted. The only sound caller is a short-lived re-exec wrapper that +// calls Apply and immediately execs the untrusted command from this same +// thread — see RunChild, which re-checks the thread identity before execve. func Apply(spec Spec) (Report, error) { + // Landlock domains are per-THREAD (landlock_restrict_self(2) enforces on the + // calling thread). The Go runtime is free to move this goroutine to another + // OS thread at any preemption point, and an execve issued from a thread that + // never entered the domain would run the untrusted command completely + // unconfined — while the caller logs "Landlock enforced". Pin the goroutine + // to its thread for the rest of its life; deliberately never unlocked, + // because the intended caller execs immediately and the confinement is + // irreversible anyway. + runtime.LockOSThread() + var rep Report // Resource limits first — cheap, and independent of Landlock availability. @@ -136,6 +151,9 @@ func Apply(spec Spec) (Report, error) { if err := landlockRestrictSelf(rulesetFD); err != nil { return rep, fmt.Errorf("sandbox: landlock_restrict_self: %w", err) } + // Record which thread now carries the domain so the caller can fail closed + // if it somehow finds itself elsewhere before execve. + rep.LandlockTID = currentTID() rep.LandlockABI = abi if len(missing) > 0 { @@ -167,6 +185,10 @@ func addPathRule(rulesetFD int, path string, access uint64) (bool, error) { return true, nil } +// currentTID returns the caller's OS thread id — the identity a Landlock domain +// is attached to, and therefore the thing a caller must re-check before execve. +func currentTID() int { return unix.Gettid() } + // --- raw syscall wrappers (x/sys/unix v0.46 ships the numbers/types but not // high-level helpers for these three syscalls) ----------------------------- diff --git a/internal/sandbox/sandbox_linux_test.go b/internal/sandbox/sandbox_linux_test.go index 3ebfaab2..046d3c4d 100644 --- a/internal/sandbox/sandbox_linux_test.go +++ b/internal/sandbox/sandbox_linux_test.go @@ -25,8 +25,15 @@ const ( ) func TestMain(m *testing.M) { - if os.Getenv(envChild) == "1" { + // Landlock confinement is irreversible, so every child mode below must run + // in a throwaway re-exec of this binary rather than the test process. + switch { + case os.Getenv(envChild) == "1": os.Exit(sandboxChild()) + case os.Getenv(envEscapeChild) == "1": + os.Exit(escapeChild()) + case os.Getenv(envTIDChild) == "1": + os.Exit(tidChild()) } os.Exit(m.Run()) } diff --git a/internal/sandbox/sandbox_other.go b/internal/sandbox/sandbox_other.go index 8865040c..b0492c1b 100644 --- a/internal/sandbox/sandbox_other.go +++ b/internal/sandbox/sandbox_other.go @@ -14,6 +14,10 @@ package sandbox // always false off Linux: Landlock is a Linux-only LSM. func Available() bool { return false } +// currentTID has no meaningful answer off Linux, where no thread-scoped +// confinement domain exists. RunChild treats 0 as "nothing to verify". +func currentTID() int { return 0 } + func Apply(spec Spec) (Report, error) { // No filesystem allowlist requested → nothing to enforce, same as Linux. if !spec.wantsLandlock() { diff --git a/internal/sandbox/threadpin_linux_test.go b/internal/sandbox/threadpin_linux_test.go new file mode 100644 index 00000000..b3058bca --- /dev/null +++ b/internal/sandbox/threadpin_linux_test.go @@ -0,0 +1,203 @@ +//go:build linux + +package sandbox + +import ( + "bytes" + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync/atomic" + "syscall" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// Child-process protocol for the thread-pinning regression (see escapeChild). +const ( + envEscapeChild = "MCPPROXY_SANDBOX_TEST_ESCAPE_CHILD" + envTIDChild = "MCPPROXY_SANDBOX_TEST_TID_CHILD" + envOutsideDir = "MCPPROXY_SANDBOX_TEST_OUTSIDE" +) + +// escapeChild reproduces, deliberately, the shape that let an untrusted command +// run UNCONFINED even though Apply reported "Landlock enforced": +// +// Apply → (goroutine parks and may resume on a different OS thread) → execve +// +// Landlock domains are per-THREAD: landlock_restrict_self(2) commits the new +// credentials on `current` only. The production wrapper does a blocking diag +// write, a privilege drop and os.Environ() between Apply and syscall.Exec, each +// of which can park the goroutine; on a loaded machine the Go scheduler then +// resumes it on an OS thread that never entered the Landlock domain, and the +// execve from that thread runs the untrusted command with no confinement at all. +// +// Here we reproduce that window deliberately (see provokeThreadMigration), +// turning a load-dependent race into a near-certain one. Apply must pin the +// thread so the exec is unconditionally confined. +func escapeChild() int { + rwDir := os.Getenv(envRWDir) + outside := os.Getenv(envOutsideDir) + + spec := Spec{ + // RO "/" so /bin/sh and the loader stay reachable; the only writable + // subtrees are rwDir and /dev. outside is under neither. + ReadOnlyPaths: []string{"/"}, + ReadWritePaths: []string{rwDir, "/dev"}, + } + rep, err := Apply(spec) + if err != nil { + os.Stderr.WriteString("escape-child: Apply failed: " + err.Error() + "\n") + return 12 + } + if rep.LandlockABI < 1 { + os.Stderr.WriteString("escape-child: Landlock not enforced: " + rep.LandlockNote + "\n") + return 12 + } + + provokeThreadMigration() + + // Exec directly rather than through RunChild: this test must exercise the + // thread pin itself, not the fail-closed tid guard that backstops it. + script := "echo escaped > " + shellQuoteTest(filepath.Join(outside, "escaped.txt")) + err = syscall.Exec("/bin/sh", []string{"/bin/sh", "-c", script}, os.Environ()) + os.Stderr.WriteString("escape-child: exec failed: " + err.Error() + "\n") + return 126 +} + +// TestApplyPinsThreadAcrossReschedule is the regression guard for the per-thread +// Landlock escape. Each iteration re-execs this test binary into escapeChild, +// which confines itself, deliberately invites a thread migration, and then execs +// a shell that writes OUTSIDE the allowlist. A single surviving write is a +// security failure, so the tolerance is zero. +func TestApplyPinsThreadAcrossReschedule(t *testing.T) { + if !Available() { + t.Skip("Landlock unavailable on this kernel (needs 5.13+ with Landlock LSM enabled)") + } + + const iterations = 20 + escapes := 0 + for i := 0; i < iterations; i++ { + rwDir := t.TempDir() + outside := t.TempDir() + + cmd := exec.Command(os.Args[0]) //nolint:gosec // re-exec of this test binary by design + cmd.Env = append(os.Environ(), + envEscapeChild+"=1", + envRWDir+"="+rwDir, + envOutsideDir+"="+outside, + ) + var errb bytes.Buffer + cmd.Stderr = &errb + runErr := cmd.Run() + + // exit 12 means the child could not confine itself at all — that is a + // broken test environment, not a pass. + var exitErr *exec.ExitError + if errors.As(runErr, &exitErr) && exitErr.ExitCode() == 12 { + t.Fatalf("iteration %d: child could not enforce Landlock:\n%s", i, errb.String()) + } + + escaped := filepath.Join(outside, "escaped.txt") + if _, err := os.Stat(escaped); err == nil { + escapes++ + t.Errorf("iteration %d: command exec'd after Apply wrote OUTSIDE the allowlist (%s) — "+ + "it ran unconfined; child stderr:\n%s", i, escaped, errb.String()) + } + } + if escapes > 0 { + t.Fatalf("%d/%d exec'd commands escaped the Landlock domain; want 0", escapes, iterations) + } +} + +// TestApplyReportsRestrictedThread pins the fail-closed guard's input: Apply +// must report the tid it actually confined, and must still be on that thread +// when it returns. +func TestApplyReportsRestrictedThread(t *testing.T) { + if !Available() { + t.Skip("Landlock unavailable on this kernel (needs 5.13+ with Landlock LSM enabled)") + } + // Apply is irreversible for the calling thread, so this assertion has to run + // in the same throwaway child machinery as the enforcement tests. + rwDir := t.TempDir() + outside := t.TempDir() + cmd := exec.Command(os.Args[0]) //nolint:gosec // re-exec of this test binary by design + cmd.Env = append(os.Environ(), + envTIDChild+"=1", + envRWDir+"="+rwDir, + envOutsideDir+"="+outside, + ) + var errb bytes.Buffer + cmd.Stderr = &errb + if err := cmd.Run(); err != nil { + t.Fatalf("tid child failed: %v\nchild stderr:\n%s", err, errb.String()) + } +} + +// tidChild asserts Report.LandlockTID is populated and still matches the running +// thread after a deliberate reschedule window. +func tidChild() int { + rwDir := os.Getenv(envRWDir) + rep, err := Apply(Spec{ReadOnlyPaths: []string{"/"}, ReadWritePaths: []string{rwDir}}) + if err != nil { + os.Stderr.WriteString("tid-child: Apply failed: " + err.Error() + "\n") + return 12 + } + if rep.LandlockTID == 0 { + os.Stderr.WriteString("tid-child: Report.LandlockTID not populated after enforcement\n") + return 11 + } + provokeThreadMigration() + + if now := currentTID(); now != rep.LandlockTID { + os.Stderr.WriteString("tid-child: thread moved after Apply: confined tid differs from current\n") + return 10 + } + return 0 +} + +func shellQuoteTest(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } + +// provokeThreadMigration parks the calling goroutine in a blocking syscall while +// every P is saturated. That is precisely the mechanism by which the production +// wrapper used to lose its Landlock-restricted thread: sysmon retakes the P +// during the blocking call, and on exitsyscall the M cannot reacquire one, so +// the goroutine is handed to a different M — a different OS thread, outside the +// domain. +// +// The syscall shape is load-bearing. Measured on a 14-CPU Linux 6.12 kernel over +// 50 unpinned runs each: a blocking pipe read moved the goroutine 50/50 times, +// while a plain time.Sleep of the same duration moved it 0/50 (the runtime hands +// a sleeping goroutine back to the M it parked on). A sleep-based version of +// this test would be a vacuous guard. +func provokeThreadMigration() { + var fds [2]int + if err := unix.Pipe(fds[:]); err != nil { + return + } + defer unix.Close(fds[0]) + defer unix.Close(fds[1]) + + // A genuine spin (not a yielding loop) is what keeps every P occupied, so + // the blocking read below cannot get its P back on exitsyscall. + var stop atomic.Bool + for i := 0; i < runtime.NumCPU()*4; i++ { + go func() { + for !stop.Load() { //nolint:revive // deliberate busy-wait: saturating every P is the point + } + }() + } + defer stop.Store(true) + + go func() { + time.Sleep(2 * time.Millisecond) + _, _ = unix.Write(fds[1], []byte{'x'}) + }() + buf := make([]byte, 1) + _, _ = syscall.Read(fds[0], buf) +} diff --git a/internal/sandbox/wrap.go b/internal/sandbox/wrap.go index b99f9e20..3755abc2 100644 --- a/internal/sandbox/wrap.go +++ b/internal/sandbox/wrap.go @@ -8,10 +8,11 @@ import ( // Re-exec wrapper protocol (MCP-34.3). // -// Landlock confines the *current* process and every process it then execs, and -// the confinement is irreversible — so it cannot be applied in-process before -// mcp-go spawns an upstream stdio server. The integration is therefore a tiny -// re-exec wrapper: mcpproxy launches itself as +// Landlock confines the *calling thread* and every process that thread then +// execs, and the confinement is irreversible — so it cannot be applied +// in-process before mcp-go spawns an upstream stdio server (mcpproxy is +// multithreaded; the other threads would stay unrestricted). The integration is +// therefore a tiny re-exec wrapper: mcpproxy launches itself as // // mcpproxy __sandbox_exec -- [args...] // diff --git a/internal/upstream/core/sandbox_linux_test.go b/internal/upstream/core/sandbox_linux_test.go index 7b10ce4c..1e23fd10 100644 --- a/internal/upstream/core/sandbox_linux_test.go +++ b/internal/upstream/core/sandbox_linux_test.go @@ -41,6 +41,16 @@ func TestSandboxWrapper_EndToEnd(t *testing.T) { BestEffort: false, // fail-closed: this test requires real enforcement } + // Assertion (3b) below is only meaningful if `outside` really is outside the + // write allowlist. t.TempDir() layouts are not contractual, so pin it here: + // a future change that put both dirs under one parent would silently turn + // the denial check into a vacuous pass. + for _, allowed := range spec.ReadWritePaths { + if isUnder(outside, allowed) { + t.Fatalf("test setup is vacuous: outside dir %q lies under the read-write allowlist entry %q", outside, allowed) + } + } + // Script: echo stdin back (passthrough), report the fd limit, write inside // the allowlist, then try to write OUTSIDE it (must fail). script := fmt.Sprintf(` @@ -119,6 +129,27 @@ func TestSandboxWrapper_FailClosed(t *testing.T) { func shellQuote(s string) string { return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } +// isUnder reports whether path is parent itself or lives beneath it, comparing +// symlink-resolved forms — Landlock rules apply to the resolved subtree, so a +// purely lexical comparison could miss an overlap (e.g. /tmp → /private/tmp). +func isUnder(path, parent string) bool { + resolve := func(p string) string { + if r, err := filepath.EvalSymlinks(p); err == nil { + return filepath.Clean(r) + } + return filepath.Clean(p) + } + path, parent = resolve(path), resolve(parent) + if path == parent { + return true + } + rel, err := filepath.Rel(parent, path) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + func scrubEnv(env []string, key string) []string { out := env[:0:0] for _, e := range env { From 0378688884440ae0e4586f0b72ca17270b79cf75 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Thu, 30 Jul 2026 09:56:57 +0300 Subject: [PATCH 2/2] test(sandbox): make the Landlock regression tests fail when the fix is broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both guards for the per-thread Landlock escape passed for reasons other than the fix working. TestApplyPinsThreadAcrossReschedule concluded "no escape" from the absence of escaped.txt, so an exec failure, a missing shell or a child that died for any unrelated reason all read as a pass. The exec'd shell now brackets the outside-write attempt with two markers written inside the read-write allowlist, and every iteration demands both: the absence of escaped.txt only means "Landlock denied it" once the child is proven to have run and to have tried. TestThreadLost only exercised the helper, so deleting the guard call from RunChild — or moving it below syscall.Exec — left the suite green. RunChild's Apply step is now indirected through an unexported package var so a test can hand it a Report whose LandlockTID cannot be the running thread, and assert RunChild returns 4 without exec'ing. The target is an absolute path that does not exist, so a RunChild that reached execve comes back 126 instead of replacing the test binary's image; the companion test pins the other direction, that a held thread still execs. Verified on real Linux (kernel 6.12, Landlock ABI enforced, not skipped): both mutations of the guard turn the new test red, and breaking the escape child's exec turns the pin test red. --- internal/sandbox/runchild_unix.go | 11 +++- internal/sandbox/runchild_unix_test.go | 78 +++++++++++++++++++++++- internal/sandbox/threadpin_linux_test.go | 34 ++++++++++- 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/internal/sandbox/runchild_unix.go b/internal/sandbox/runchild_unix.go index c3202905..3c282786 100644 --- a/internal/sandbox/runchild_unix.go +++ b/internal/sandbox/runchild_unix.go @@ -58,7 +58,7 @@ func RunChild(argv []string, diag io.Writer) int { target = resolved } - rep, err := Apply(spec) + rep, err := applyConfinement(spec) if err != nil { // fail-closed: BestEffort was false and the primitive is unavailable. fmt.Fprintf(diag, "sandbox: confinement unavailable and fail-closed: %v\n", err) @@ -88,6 +88,15 @@ func RunChild(argv []string, diag io.Writer) int { return 0 // unreachable: Exec replaced the image on success. } +// applyConfinement indirects Apply for RunChild. Production never reassigns it; +// it exists so the test suite can drive RunChild's fail-closed thread guard, +// whose refusal path is otherwise untestable: Apply is irreversible for the +// calling thread (so the test process cannot call it for real) and a genuine +// thread migration is a race that cannot be provoked on demand. Without the +// seam, deleting the guard from RunChild — or moving it after syscall.Exec — +// would leave every test still green. +var applyConfinement = Apply + // threadLost reports whether the Landlock domain Apply committed lives on a // different thread than the one now about to execve — in which case the exec // would run the untrusted command outside the domain entirely. A zero diff --git a/internal/sandbox/runchild_unix_test.go b/internal/sandbox/runchild_unix_test.go index 76ef10a4..1f736acf 100644 --- a/internal/sandbox/runchild_unix_test.go +++ b/internal/sandbox/runchild_unix_test.go @@ -2,7 +2,13 @@ package sandbox -import "testing" +import ( + "bytes" + "path/filepath" + "runtime" + "strings" + "testing" +) // TestThreadLost pins the fail-closed guard's decision table. RunChild consults // it immediately before execve: a true verdict must abort the exec, because @@ -48,3 +54,73 @@ func TestThreadLost(t *testing.T) { }) } } + +// fakeConfinement makes RunChild's Apply step return rep, so the fail-closed +// guard can be driven from a test process that must not actually confine itself. +func fakeConfinement(t *testing.T, rep Report) { + t.Helper() + prev := applyConfinement + applyConfinement = func(Spec) (Report, error) { return rep, nil } + t.Cleanup(func() { applyConfinement = prev }) +} + +// unreachableTarget is an absolute path that does not exist. Absolute so +// RunChild skips the PATH lookup and gets as far as the guard; non-existent so +// that a RunChild which reached syscall.Exec fails with ENOENT (126) instead of +// replacing this test binary's process image. The two failure modes are +// therefore distinguishable: 4 means the guard refused before exec, 126 means it +// did not. +func unreachableTarget(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "never-exec-me") +} + +// TestRunChildRefusesExecWhenThreadLost covers the guard where it actually +// matters — inside RunChild, before execve. TestThreadLost alone would stay +// green if the call were deleted from RunChild or moved below syscall.Exec, and +// either edit re-opens the escape this whole change exists to close. +func TestRunChildRefusesExecWhenThreadLost(t *testing.T) { + t.Setenv(EnvSpec, `{}`) + // currentTID()+1 is never the running thread, on Linux (real tid) or + // elsewhere (currentTID is a constant 0 off Linux). + fakeConfinement(t, Report{LandlockABI: 5, LandlockTID: currentTID() + 1}) + + target := unreachableTarget(t) + var diag bytes.Buffer + code := RunChild([]string{target}, &diag) + + if code != 4 { + t.Fatalf("RunChild with a lost Landlock thread = %d, want 4 (refuse before execve); diag:\n%s", + code, diag.String()) + } + if strings.Contains(diag.String(), "sandbox: exec ") { + t.Errorf("RunChild attempted the execve despite the lost thread; diag:\n%s", diag.String()) + } + if !strings.Contains(diag.String(), "refusing to exec") || !strings.Contains(diag.String(), "UNCONFINED") { + t.Errorf("refusal must say what went wrong and why it matters; diag:\n%s", diag.String()) + } +} + +// TestRunChildExecsWhenThreadHeld is the other half of the pin: the guard must +// not refuse when the domain is still on this thread, or it would block every +// legitimate launch. Without this, "return 4 unconditionally" would pass the +// test above. +func TestRunChildExecsWhenThreadHeld(t *testing.T) { + t.Setenv(EnvSpec, `{}`) + // Hold the thread so the tid recorded here is still the tid RunChild sees. + runtime.LockOSThread() + defer runtime.UnlockOSThread() + fakeConfinement(t, Report{LandlockABI: 5, LandlockTID: currentTID()}) + + target := unreachableTarget(t) + var diag bytes.Buffer + code := RunChild([]string{target}, &diag) + + if code != 126 { + t.Fatalf("RunChild on the confined thread = %d, want 126 (execve reached, target absent); diag:\n%s", + code, diag.String()) + } + if !strings.Contains(diag.String(), "sandbox: exec ") { + t.Errorf("expected the diag to show execve was attempted; diag:\n%s", diag.String()) + } +} diff --git a/internal/sandbox/threadpin_linux_test.go b/internal/sandbox/threadpin_linux_test.go index b3058bca..24f270d6 100644 --- a/internal/sandbox/threadpin_linux_test.go +++ b/internal/sandbox/threadpin_linux_test.go @@ -64,17 +64,37 @@ func escapeChild() int { // Exec directly rather than through RunChild: this test must exercise the // thread pin itself, not the fail-closed tid guard that backstops it. - script := "echo escaped > " + shellQuoteTest(filepath.Join(outside, "escaped.txt")) + // + // The exec'd shell brackets the escape attempt with two markers written to + // rwDir, which IS inside the allowlist and so is writable whether or not the + // pin held. reachedMarker proves the shell actually started; doneMarker + // proves it got all the way past the outside-write attempt. Without them the + // parent could only observe the absence of escaped.txt, which any unrelated + // failure (exec error, missing shell, child killed) would also produce — a + // broken pin and a broken test environment would look identical. + script := "echo ran > " + shellQuoteTest(filepath.Join(rwDir, reachedMarker)) + "\n" + + "echo escaped > " + shellQuoteTest(filepath.Join(outside, "escaped.txt")) + "\n" + + "echo done > " + shellQuoteTest(filepath.Join(rwDir, doneMarker)) + "\n" err = syscall.Exec("/bin/sh", []string{"/bin/sh", "-c", script}, os.Environ()) os.Stderr.WriteString("escape-child: exec failed: " + err.Error() + "\n") return 126 } +// Markers the exec'd shell writes inside the read-write allowlist, so the parent +// can tell "confined, as intended" apart from "never ran". +const ( + reachedMarker = "child-ran.txt" + doneMarker = "child-done.txt" +) + // TestApplyPinsThreadAcrossReschedule is the regression guard for the per-thread // Landlock escape. Each iteration re-execs this test binary into escapeChild, // which confines itself, deliberately invites a thread migration, and then execs // a shell that writes OUTSIDE the allowlist. A single surviving write is a // security failure, so the tolerance is zero. +// +// Every iteration also demands the two in-allowlist markers the shell writes +// around that attempt, so an iteration cannot pass by simply failing early. func TestApplyPinsThreadAcrossReschedule(t *testing.T) { if !Available() { t.Skip("Landlock unavailable on this kernel (needs 5.13+ with Landlock LSM enabled)") @@ -103,6 +123,18 @@ func TestApplyPinsThreadAcrossReschedule(t *testing.T) { t.Fatalf("iteration %d: child could not enforce Landlock:\n%s", i, errb.String()) } + // Positive proof first: the absence of escaped.txt below only means + // "Landlock denied the write" if the shell really ran and really tried. + // Both markers live inside the allowlist, so a working pin does not stop + // them; only a child that never got there does. + for _, marker := range []string{reachedMarker, doneMarker} { + if _, err := os.Stat(filepath.Join(rwDir, marker)); err != nil { + t.Fatalf("iteration %d: exec'd shell never wrote %s inside the allowlist (%v) — "+ + "the child did not reach the escape attempt, so this iteration proves nothing; "+ + "child exit: %v; child stderr:\n%s", i, marker, err, runErr, errb.String()) + } + } + escaped := filepath.Join(outside, "escaped.txt") if _, err := os.Stat(escaped); err == nil { escapes++