fix(sandbox): pin the Landlock thread so a sandboxed MCP server cannot exec unconfined - #932
Merged
Merged
Conversation
…cape 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.
Deploying mcpproxy-docs with
|
| Latest commit: |
0378688
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5d0b65d0.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-landlock-per-thread-esca.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 30521245482 --repo smart-mcp-proxy/mcpproxy-go
|
…s broken 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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arming auto-merge: security fix for a real Landlock per-thread escape. Codex cross-model review CLEAN; regression tests mutation-verified on real Linux (guard deleted AND guard moved below execve both fail the new test); sandbox integration ran the escape test 30x under full CPU load; the E2E job that was persistently red is green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security impact
An untrusted stdio MCP server could run completely unconfined while mcpproxy logged
sandbox: Landlock enforced (ABI 7), 1 rlimit(s) set. The sandbox reported success, the filesystem allowlist was never in force for the spawned process, and nothing in the logs distinguished a confined run from an escaped one. Only the rlimits (which are per-process) still applied.This affects
docker_isolation.mode = "sandbox"on Linux — the path intended for hosts where Docker is unavailable or broken (snap-Docker under AppArmor).Mechanism
Landlock domains are per-thread.
landlock_restrict_self(2)commits the new credentials oncurrent— the calling thread — and nothing else.internal/sandbox/sandbox_linux.gocalledlandlockRestrictSelfon whatever OS thread the goroutine happened to occupy at that moment.internal/sandbox/runchild_unix.gothen did three things beforesyscall.Exec:fmt.Fprintf(diag, ...)),dropPrivilegesBestEffort(),os.Environ().Each is a chance for the goroutine to park. On a loaded machine sysmon retakes the P during the blocking syscall, the M cannot reacquire one at
exitsyscall, and the goroutine is handed to a different M — a thread that never entered the Landlock domain. Theexecvefrom that thread runs the untrusted command with no confinement at all. There was noruntime.LockOSThreadanywhere in the repo.This is also exactly why the CI failure printed
Landlock enforced (ABI 7)and64(rlimit applied, because rlimits are per-process) andWROTE_OUTSIDEin the same run — a combination only explicable by a thread split.Fix
Applynow callsruntime.LockOSThread()as its first statement, deliberately never unlocked (the intended caller execs immediately, and the confinement is irreversible regardless). It lives inApplyrather than onlyRunChildso every caller of the public API is covered.Applyrecords the confined tid onReport.LandlockTID.RunChildre-checks it immediately beforesyscall.Execand, on mismatch, writes a diagnostic and returns exit code 4 instead of exec'ing — fail-closed. A server that does not start is recoverable; one that starts outside its sandbox is not. (Exit codes in this file: 2 bad invocation, 3 confinement unavailable, 4 thread lost, 126 exec failed, 127 not on PATH.)sandbox.go,sandbox_linux.goandwrap.goall claimed Apply "confines the current process", which is what made the hazard invisible to review. They now state that Apply confines a thread, that only the re-exec-then-exec shape is sound, and that an in-processApplycannot confine a multithreaded Go process.Empirical evidence
Root-cause measurements (4-vCPU CI-like conditions): idle 0/100 escapes; with a 2 ms reschedule before exec 58/100 escapes; with
runtime.LockOSThread()inApply0/100.Independently re-verified for this PR on a real Linux 6.12 kernel (14 CPUs, aarch64 container), using the new
TestApplyPinsThreadAcrossReschedule:The regression test is a genuine negative control — it was confirmed to fail on a binary built with the pin removed, and pass with it.
One finding worth recording: the provocation shape matters. A plain
time.SleepbetweenApplyandexecvemoved the goroutine 0/50 times — the runtime hands a sleeping goroutine back to the M it parked on. A blocking syscall with every P saturated moved it 50/50. That is the production shape (the blocking diag write), and it is what the test uses; a sleep-based version of this test would have been a vacuous guard.Tests and CI
internal/sandbox/threadpin_linux_test.go:TestApplyPinsThreadAcrossReschedulere-execs a child that applies confinement, deliberately provokes a thread migration, then execs a shell writing outside the allowlist — tolerance zero. PlusTestApplyReportsRestrictedThreadcovering the tid the guard depends on. Both skip cleanly off Linux viasandbox.Available().internal/sandbox/runchild_unix_test.go:TestThreadLostpins the fail-closed guard's decision table (including "no domain enforced → nothing to verify"), and runs on macOS too.internal/upstream/core/sandbox_linux_test.gonow asserts theoutsidedir is not under the read-write allowlist and not under/dev, so a future temp-layout change cannot silently turn the denial check into a vacuous pass..github/workflows/sandbox-integration.ymlgains a "Sandbox enforcement under scheduler load" step. The dedicated sandbox job runs on an idle runner and passed even with the bug present, so it could not discriminate; the new step saturates every CPU before running the enforcement tests. Cleanup is viatrap ... EXIT INT TERMplus atimeout-minutes, so the busy loops cannot outlive the step or hang the job (verified by running the step logic against a deliberately failing command).This also fixes the persistent
TestSandboxWrapper_EndToEndfailure in the loaded "End-to-End Tests (ubuntu-latest, 1.25)" job that is currently blocking #928 — that job runs dozens of race-instrumented packages on a 4-vCPU runner, which is what made the thread split likely there but not in the unloaded dedicated sandbox job on the same runner image and kernel.