fix(security): verify Docker container ownership before scanning it - #1298
Merged
Merged
Conversation
findServerContainer selected the first `docker ps --filter name=` match, but that filter is a substring match, not anchored: servers whose sanitized names collide as a prefix (e.g. "a" vs "a-b"/"a_b"/"a/b") could resolve to a different server's container. That container ID then flowed unchecked into `docker exec`/`cp`/`diff`, so a scan could exec into, copy files from, and report findings against the wrong server's container. Re-verify every name-filtered candidate against the com.mcpproxy.server ownership label (set at container creation) via a new pure selectOwnedContainer helper, and surface the verified owner as ContainerOwner / container_owner in scan output for auditability. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
881b3fc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8ca4a82e.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-blissful-vaughan-76fa.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35303925633 --repo smart-mcp-proxy/mcpproxy-go
|
…tainer ownership check
Cross-review (opencode gpt-5.6-sol) of the initial ownership fix found two
real gaps:
1. Re-verifying docker-ps candidates by rendering `{{.Label
"com.mcpproxy.server"}}` into tab/newline-delimited --format text and
parsing it back is itself exploitable — mcpproxy server names permit
embedded newlines/tabs (config validation only rejects empty and ':'), so
a crafted name forges an extra fabricated "record" that resolves to an
attacker-chosen container ID. Verified against a real Docker daemon.
2. Exact label equality alone doesn't establish ownership by the CURRENT
mcpproxy instance: two processes sharing a Docker daemon with a same-named
server could select each other's container.
Fixed by dropping the intermediate text-rendering step entirely: ownership is
now established purely through `docker ps --filter label=key=value`, which
Docker matches against raw label bytes server-side with no text format for a
crafted label to escape. Added an optional instance-id filter (SetInstanceID,
injected by internal/server since internal/upstream/core cannot be imported
directly from this package without an import cycle) and shape-validation on
the returned container ID.
New regression tests reproduce both findings against a real Docker daemon
(skipped when unavailable).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iner ownership check Round-2 opencode gpt-5.6-sol re-review confirmed the label-injection fix is sound, but flagged that core.GetInstanceID() persists to a single shared file under os.TempDir() — every mcpproxy process on the same host/user currently gets the SAME instance ID, so the new instance-scoped filter does not actually distinguish two mcpproxy processes running side by side on one machine (only genuinely distinct instance IDs, e.g. separate hosts sharing one remote Docker daemon, are protected). That's a pre-existing property of GetInstanceID(), already relied on with the same gap by internal/upstream/manager.go's container cleanup — fixing it requires changing what gets written onto the label at container creation and every reader of that label, out of scope for this scanner-focused fix. Filed as a separate follow-up task rather than expanding this PR's blast radius. Corrected the doc comments to state the actual guarantee instead of an aspirational one, and fixed a stale regex-length claim in a comment (containerIDPattern accepts 12-64 hex chars, not exactly 12 or 64). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6 tasks
Dumbris
added a commit
that referenced
this pull request
Sep 18, 2026
…p file (#1300) * fix(docker): scope instance ID to data dir instead of a host-wide temp file GetInstanceID() persisted to a single filepath.Join(os.TempDir(), ...) file shared by every process a user runs on a host, so any two concurrent mcpproxy processes (a scratch dev instance next to the main app, two separate installs) silently got the SAME instance ID. That defeats com.mcpproxy.instance container-ownership labels used by manager.go's cleanup and by the ownership filter added in PR #1298 for same-host instances (cross-host-shared-daemon cases still work). Scope the ID to the process's data dir instead (cfg.DataDir, set once via core.SetInstanceDataDir from upstream.NewManager, the common construction point for every entry point) since concurrent instances on one host already require distinct data dirs -- BBolt locks config.db. Fall back to a fresh per-process UUID when no data dir is known yet. On first run under a data dir, adopt-then-retire the legacy shared file so pre-upgrade containers stay cleanable by whichever instance starts first, without letting every future data dir keep re-adopting the same shared ID. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docker): close legacy-instance-id adoption race found by cross-review codex gpt-5.6-sol review of the instance-ID fix (PR #1300) found that adoptLegacyInstanceID's read-then-remove let two concurrent processes both read the same legacy id before either deleted the file, recreating the very host-wide-shared-id bug the fix targets. Claim the legacy file via os.Rename to a process-unique path instead: rename atomically fails once another process has already claimed the source, so exactly one process adopts a given legacy id. Also fixes a test bug the same review caught: the subprocess helper tests never overrode legacyInstanceIDPath in the child process, so they could have adopted-and-deleted a real machine's actual pre-upgrade legacy file. Threads a scratch path through an env var instead. Adds a goroutine-race test (50x under -race) proving the claim is exclusive. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): make legacy-id claim path unique per call, not just per PID Windows CI failed TestAdoptLegacyInstanceIDConcurrentClaimIsExclusive (#1300 windows-amd64 build): all 8 concurrent goroutines got 0 adopters instead of exactly 1. The test spawns racers as goroutines in one OS process, so they all share one os.Getpid() and therefore compute the identical claim destination -- a collision that can't happen in real usage (real racers are always separate processes with distinct PIDs). On Windows that collision made every racer's rename fail. Add an atomic per-call sequence number alongside the PID in the claim path, so it can never collide even when called concurrently within one process. No behavior change for the real (cross-process) case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docker): use a random claim path and preserve claims on save failure Second codex gpt-5.6-sol review round on PR #1300. opencode's Copilot quota was still exhausted, so codex gpt-5.6-sol (the documented fallback) reviewed again. Findings: 1. adoptLegacyInstanceID removed the claimed legacy file unconditionally, even when the subsequent save to the new per-data-dir location failed -- silently losing the id on the next restart. Now the claim file is only removed after a successful save; on failure it's left in place so it isn't silently destroyed. 2. The PID+sequence claim-path suffix (from the previous round's Windows fix) doesn't guarantee cross-restart uniqueness -- PIDs get reused, and the review argued a stale claim file from a crash could collide with a later process's first claim. Go's os.Rename on Windows actually uses MOVEFILE_REPLACE_EXISTING (verified against the Go 1.26 stdlib source), so this wouldn't have caused a failure the way the review described, but using a random UUID instead of PID+sequence removes any theoretical collision outright and is simpler. 3. Test coverage: TestGetInstanceIDDistinctAcrossConcurrentProcesses ran its two subprocesses sequentially despite its name; and there was no test of the ACTUAL cross-process legacy-claim race (only an in-process goroutine simulation, which artificially shares one PID across racers). Added TestGetInstanceIDCrossProcessLegacyClaimIsExclusive (two real subprocesses racing over one seeded legacy file) and TestAdoptLegacyInstanceIDPreservesClaimWhenSaveFails, and made the existing "concurrent processes" test actually start both processes concurrently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(test): match TestLoadConfig_ListenFlag to loadConfig's 3-value signature CI on PR #1300 failed to build cmd/mcpproxy on every platform: "assignment mismatch: 2 variables but loadConfig returns 3 values". Unrelated to this PR's instance-ID changes -- main itself is currently broken this way (#1299 changed loadConfig to also return a *serveConfigSaver, and the listen_flag_test.go added by #1301 wasn't updated for it). Discard the unused saver return to match the real signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
findServerContainerselected the firstdocker ps --filter name=mcpproxy-<sanitized>-match, but that filter is a substring match, not anchored — servers whose sanitized names collide as a prefix (e.g. serveravsa-b/a_b/a/b, which all sanitize toward a colliding token) could resolve to a different server's running container.docker exec/cp/diffinextractFromContainer/extractFullFromContainer, so a scan of serveracould exec into, copy files from, and report supply-chain findings against servera-b's container instead.com.mcpproxy.serverownership label (set at container creation ininternal/upstream/core/instance.go) via a new pureselectOwnedContainerhelper — only an exact label match is trusted.ContainerOwner/container_ownertoResolvedSource/ScanContextso scan output records which server actually owns the container that was inspected, for auditability.Test plan
internal/security/scanner/container_ownership_test.gocover the prefix collision (avsa-b), exact-match, underscore/slash variants, and malformeddocker psoutput — written test-first and confirmed to fail (undefined symbol) before the fix.go build ./...go test ./internal/security/scanner/...(full package, plus-raceon the new tests)golangci-lint run --config .github/.golangci.yml ./internal/security/scanner/...— 0 issues🤖 Generated with Claude Code