From e74611214ad7ce0d06f887471f66abf92e147507 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:17:11 +0300 Subject: [PATCH 1/3] fix(security): verify Docker container ownership before scanning it 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 --- .../scanner/container_ownership_test.go | 63 +++++++++++++++++++ internal/security/scanner/service.go | 2 + internal/security/scanner/source_resolver.go | 60 ++++++++++++++---- internal/security/scanner/types.go | 1 + 4 files changed, 113 insertions(+), 13 deletions(-) create mode 100644 internal/security/scanner/container_ownership_test.go diff --git a/internal/security/scanner/container_ownership_test.go b/internal/security/scanner/container_ownership_test.go new file mode 100644 index 000000000..2f9c90850 --- /dev/null +++ b/internal/security/scanner/container_ownership_test.go @@ -0,0 +1,63 @@ +package scanner + +import "testing" + +// TestSelectOwnedContainerRejectsPrefixCollision reproduces the container +// hijack bug: `docker ps --filter name=mcpproxy-a-` is a substring match, so +// it returns the container for server "a-b" (name "mcpproxy-a-b-") +// when scanning server "a" ever asked for it, because "mcpproxy-a-" is a +// prefix of "mcpproxy-a-b-". The com.mcpproxy.server label carries the +// exact, unsanitized server name, so selecting on label equality must reject +// the colliding candidate instead of returning its container ID. +func TestSelectOwnedContainerRejectsPrefixCollision(t *testing.T) { + psOutput := "deadbeef01\ta-b\n" + if got := selectOwnedContainer(psOutput, "a"); got != "" { + t.Errorf("selectOwnedContainer(%q, %q) = %q, want \"\" (must not select colliding container)", psOutput, "a", got) + } +} + +func TestSelectOwnedContainerMatchesExactOwner(t *testing.T) { + psOutput := "abc123\ta\ndeadbeef01\ta-b\n" + if got := selectOwnedContainer(psOutput, "a"); got != "abc123" { + t.Errorf("selectOwnedContainer(...) = %q, want %q", got, "abc123") + } +} + +// TestSelectOwnedContainerMatchesUnderscoreAndSlashVariants covers the other +// two name-sanitization collisions named in MCP report: "a_b" and "a/b" both +// sanitize toward a token that "a"'s own container name is a prefix of +// ("a-b" for the slash case; the underscore case is a substring match too +// since docker ps --filter name= matches raw container names, not sanitized +// tokens, against the sanitized filter pattern). +func TestSelectOwnedContainerMatchesUnderscoreAndSlashVariants(t *testing.T) { + for _, tc := range []struct { + name string + owner string + }{ + {"underscore", "a_b"}, + {"slash", "a/b"}, + } { + t.Run(tc.name, func(t *testing.T) { + psOutput := "cid1\t" + tc.owner + "\n" + if got := selectOwnedContainer(psOutput, "a"); got != "" { + t.Errorf("selectOwnedContainer matched wrong owner: got %q, want \"\"", got) + } + if got := selectOwnedContainer(psOutput, tc.owner); got != "cid1" { + t.Errorf("selectOwnedContainer failed to match true owner %q: got %q, want %q", tc.owner, got, "cid1") + } + }) + } +} + +func TestSelectOwnedContainerNoCandidates(t *testing.T) { + if got := selectOwnedContainer("", "a"); got != "" { + t.Errorf("selectOwnedContainer(\"\", %q) = %q, want \"\"", "a", got) + } +} + +func TestSelectOwnedContainerSkipsMalformedLines(t *testing.T) { + psOutput := "no-tab-here\ncid2\ta\n" + if got := selectOwnedContainer(psOutput, "a"); got != "cid2" { + t.Errorf("selectOwnedContainer(...) = %q, want %q", got, "cid2") + } +} diff --git a/internal/security/scanner/service.go b/internal/security/scanner/service.go index e1e5be7d8..57899c19c 100644 --- a/internal/security/scanner/service.go +++ b/internal/security/scanner/service.go @@ -1013,6 +1013,7 @@ func (s *Service) StartScan(ctx context.Context, serverName string, dryRun bool, scanCtx.SourcePath = resolved.ServerURL } scanCtx.ContainerID = resolved.ContainerID + scanCtx.ContainerOwner = resolved.ContainerOwner // Docker-image servers (`docker run mcp/fetch`): the scan target is the // image itself, not a source dir. Carry the reference so image-capable // scanners (Trivy) run in image mode, and surface it in the context. @@ -1221,6 +1222,7 @@ func (s *Service) startPass2(serverName string, serverInfo *ServerInfo) { scanCtx.SourcePath = resolved.ServerURL } scanCtx.ContainerID = resolved.ContainerID + scanCtx.ContainerOwner = resolved.ContainerOwner // Docker-image servers: scan the image (Trivy image mode reports OS-package // and bundled-dependency CVEs). No source dir to enrich or export tools into. if resolved.ContainerImage != "" { diff --git a/internal/security/scanner/source_resolver.go b/internal/security/scanner/source_resolver.go index 5a24519ea..0a8e90f6a 100644 --- a/internal/security/scanner/source_resolver.go +++ b/internal/security/scanner/source_resolver.go @@ -98,6 +98,7 @@ type ServerInfo struct { type ResolvedSource struct { SourceDir string // Host directory containing source files ContainerID string // Docker container ID (if applicable) + ContainerOwner string // Server name that owns ContainerID, per the com.mcpproxy.server label (verified, not just name-matched) ContainerImage string // Docker image reference (for "container_image" input) ServerURL string // URL for mcp_connection input (HTTP/SSE servers) Method string // How source was resolved: "docker_extract", "container_image", "working_dir", "local_path", "url", "manual" @@ -158,10 +159,11 @@ func (r *SourceResolver) Resolve(ctx context.Context, info ServerInfo) (*Resolve zap.String("source_dir", sourceDir), ) return &ResolvedSource{ - SourceDir: sourceDir, - ContainerID: containerID, - Method: "docker_extract", - Cleanup: cleanup, + SourceDir: sourceDir, + ContainerID: containerID, + ContainerOwner: info.Name, + Method: "docker_extract", + Cleanup: cleanup, }, nil } r.logger.Warn("Failed to extract from container, trying fallback", @@ -464,11 +466,22 @@ func dirLooksLikeSource(dir string) bool { // (internal/upstream/core), hence the shared dockernaming package — official // registry names like "com.pulsemcp/google-flights" keep their dots and would // otherwise never match (MCP-2123). +// +// The `docker ps --filter name=` clause above is a SUBSTRING match, not an +// anchored one: servers whose sanitized names collide as a prefix (server "a" +// vs "a-b"/"a_b"/"a/b", which all sanitize toward a token "a-b" is a prefix +// of) can make this filter return a container belonging to a different +// server. That container would then be exec'd/copied/diffed and its findings +// reported under the wrong server. To close that gap, every candidate is +// re-verified against the com.mcpproxy.server label — set to the exact, +// unsanitized server name at container creation (internal/upstream/core) — +// via selectOwnedContainer, which rejects any candidate whose label does not +// match serverName exactly. func (r *SourceResolver) findServerContainer(ctx context.Context, serverName string) (string, error) { // Use docker ps with filter to find matching containers cmd := r.dockerCmd(ctx, "ps", "--filter", fmt.Sprintf("name=mcpproxy-%s-", dockernaming.SanitizeServerName(serverName)), - "--format", "{{.ID}}", + "--format", "{{.ID}}\t{{.Label \"com.mcpproxy.server\"}}", "--no-trunc", ) var stdout bytes.Buffer @@ -477,13 +490,33 @@ func (r *SourceResolver) findServerContainer(ctx context.Context, serverName str return "", fmt.Errorf("docker ps failed: %w", err) } - lines := strings.Split(strings.TrimSpace(stdout.String()), "\n") - if len(lines) == 0 || lines[0] == "" { + containerID := selectOwnedContainer(stdout.String(), serverName) + if containerID == "" { return "", fmt.Errorf("no running container found for server %s", serverName) } + return containerID, nil +} - // Return first match - return lines[0], nil +// selectOwnedContainer parses `docker ps --format {{.ID}}\t{{.Label "com.mcpproxy.server"}}` +// output and returns the ID of the first container whose ownership label +// equals serverName exactly. Candidates arrive from a name-prefix substring +// match (see findServerContainer), so this exact-equality check is what +// actually enforces ownership and rejects a same-prefix collision. +func selectOwnedContainer(psOutput, serverName string) string { + for _, line := range strings.Split(strings.TrimSpace(psOutput), "\n") { + if line == "" { + continue + } + parts := strings.SplitN(line, "\t", 2) + if len(parts) != 2 { + continue + } + id, owner := parts[0], parts[1] + if owner == serverName { + return id + } + } + return "" } // extractFromContainer extracts changed files from a running container. @@ -953,10 +986,11 @@ func (r *SourceResolver) ResolveFullSource(ctx context.Context, info ServerInfo) zap.String("source_dir", sourceDir), ) return &ResolvedSource{ - SourceDir: sourceDir, - ContainerID: containerID, - Method: "docker_extract", - Cleanup: cleanup, + SourceDir: sourceDir, + ContainerID: containerID, + ContainerOwner: info.Name, + Method: "docker_extract", + Cleanup: cleanup, }, nil } r.logger.Warn("Failed to extract full source from container, trying fallback", diff --git a/internal/security/scanner/types.go b/internal/security/scanner/types.go index e5c9a4b92..ced385772 100644 --- a/internal/security/scanner/types.go +++ b/internal/security/scanner/types.go @@ -266,6 +266,7 @@ type ScanContext struct { SourcePath string `json:"source_path"` // Actual path/URL that was scanned DockerIsolation bool `json:"docker_isolation"` // Whether server runs in Docker ContainerID string `json:"container_id,omitempty"` // Docker container ID (if applicable) + ContainerOwner string `json:"container_owner,omitempty"` // Server name that owns ContainerID (verified via com.mcpproxy.server label) ContainerImage string `json:"container_image,omitempty"` // Docker image used ServerProtocol string `json:"server_protocol"` // stdio, http, sse ServerCommand string `json:"server_command,omitempty"` // Command used to start server From 6ed63bd173e04dec4bda600a7400ceb441c6cb2c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:32:42 +0300 Subject: [PATCH 2/3] fix(security): close label-injection and instance-scoping gaps in container ownership check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../scanner/container_ownership_test.go | 198 +++++++++++++----- internal/security/scanner/service.go | 7 + internal/security/scanner/source_resolver.go | 116 ++++++---- internal/server/server.go | 1 + 4 files changed, 235 insertions(+), 87 deletions(-) diff --git a/internal/security/scanner/container_ownership_test.go b/internal/security/scanner/container_ownership_test.go index 2f9c90850..2c2f81ff5 100644 --- a/internal/security/scanner/container_ownership_test.go +++ b/internal/security/scanner/container_ownership_test.go @@ -1,63 +1,167 @@ package scanner -import "testing" - -// TestSelectOwnedContainerRejectsPrefixCollision reproduces the container -// hijack bug: `docker ps --filter name=mcpproxy-a-` is a substring match, so -// it returns the container for server "a-b" (name "mcpproxy-a-b-") -// when scanning server "a" ever asked for it, because "mcpproxy-a-" is a -// prefix of "mcpproxy-a-b-". The com.mcpproxy.server label carries the -// exact, unsanitized server name, so selecting on label equality must reject -// the colliding candidate instead of returning its container ID. -func TestSelectOwnedContainerRejectsPrefixCollision(t *testing.T) { - psOutput := "deadbeef01\ta-b\n" - if got := selectOwnedContainer(psOutput, "a"); got != "" { - t.Errorf("selectOwnedContainer(%q, %q) = %q, want \"\" (must not select colliding container)", psOutput, "a", got) - } -} +import ( + "bytes" + "context" + "os/exec" + "strings" + "testing" -func TestSelectOwnedContainerMatchesExactOwner(t *testing.T) { - psOutput := "abc123\ta\ndeadbeef01\ta-b\n" - if got := selectOwnedContainer(psOutput, "a"); got != "abc123" { - t.Errorf("selectOwnedContainer(...) = %q, want %q", got, "abc123") - } -} + "go.uber.org/zap" +) -// TestSelectOwnedContainerMatchesUnderscoreAndSlashVariants covers the other -// two name-sanitization collisions named in MCP report: "a_b" and "a/b" both -// sanitize toward a token that "a"'s own container name is a prefix of -// ("a-b" for the slash case; the underscore case is a substring match too -// since docker ps --filter name= matches raw container names, not sanitized -// tokens, against the sanitized filter pattern). -func TestSelectOwnedContainerMatchesUnderscoreAndSlashVariants(t *testing.T) { - for _, tc := range []struct { - name string - owner string +// TestFirstContainerIDPicksValidHexID covers the ID-shape validation +// firstContainerID performs on `docker ps --format {{.ID}}` output: IDs are +// the one field Docker guarantees is a safe opaque hex token, but the line +// parser still checks the shape rather than trusting the first line blindly. +func TestFirstContainerIDPicksValidHexID(t *testing.T) { + cases := []struct { + name string + in string + want string }{ - {"underscore", "a_b"}, - {"slash", "a/b"}, - } { + {"short id", "deadbeef0123\n", "deadbeef0123"}, + {"full id", strings.Repeat("a", 64) + "\n", strings.Repeat("a", 64)}, + {"multiple lines picks first", "abc123456789\ndef987654321\n", "abc123456789"}, + {"empty input", "", ""}, + {"blank lines only", "\n\n", ""}, + {"rejects too-short garbage", "abc\n", ""}, + {"rejects non-hex garbage", strings.Repeat("g", 12) + "\n", ""}, + {"skips a bad line then picks a good one", "not-an-id\n" + strings.Repeat("b", 12) + "\n", strings.Repeat("b", 12)}, + } + for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - psOutput := "cid1\t" + tc.owner + "\n" - if got := selectOwnedContainer(psOutput, "a"); got != "" { - t.Errorf("selectOwnedContainer matched wrong owner: got %q, want \"\"", got) - } - if got := selectOwnedContainer(psOutput, tc.owner); got != "cid1" { - t.Errorf("selectOwnedContainer failed to match true owner %q: got %q, want %q", tc.owner, got, "cid1") + if got := firstContainerID(tc.in); got != tc.want { + t.Errorf("firstContainerID(%q) = %q, want %q", tc.in, got, tc.want) } }) } } -func TestSelectOwnedContainerNoCandidates(t *testing.T) { - if got := selectOwnedContainer("", "a"); got != "" { - t.Errorf("selectOwnedContainer(\"\", %q) = %q, want \"\"", "a", got) +// dockerAvailableForTest reports whether a real Docker daemon can be reached, +// matching the check DockerRunner.IsDockerAvailable performs elsewhere in this +// package. The regression test below needs a genuine daemon — the +// vulnerability it proves is in how Docker itself renders/matches labels, not +// in a fake shim's approximation of that behavior. +func dockerAvailableForTest(t *testing.T) bool { + t.Helper() + cmd := exec.CommandContext(context.Background(), "docker", "info") + return cmd.Run() == nil +} + +// TestFindServerContainerRejectsLabelInjection is a regression test for the +// vulnerability opencode's cross-review of the original substring-collision +// fix found: re-verifying `docker ps --filter name=` candidates by rendering +// `{{.Label "com.mcpproxy.server"}}` into tab/newline-delimited --format text +// and parsing it back is itself exploitable. mcpproxy validates server names +// only for non-empty and no ':' (internal/config/config.go), so a name +// containing an embedded newline+tab forges an extra "record" that a naive +// line parser attributes to whatever text follows the tab — letting a +// same-Docker-daemon server redirect scanning to an arbitrary container ID of +// its choosing. +// +// findServerContainer no longer parses rendered label text at all: ownership +// is established purely through `docker ps --filter label=key=value`, which +// Docker matches against the raw label bytes server-side. This test proves +// that against a REAL daemon (skipped if Docker is unavailable) — the fix +// here is exactly Docker's own filter semantics, which a fake shim cannot +// stand in for. +func TestFindServerContainerRejectsLabelInjection(t *testing.T) { + if !dockerAvailableForTest(t) { + t.Skip("docker daemon not available") + } + ctx := context.Background() + + // A server named "a-b" whose label value embeds a newline + a fabricated + // container ID + a tab + the victim server name "a". If this were ever + // rendered into `{{.ID}}\t{{.Label ...}}` text and parsed line-by-line, it + // would produce a spoofed second line "\ta" — an exact match + // for server "a". + injectedID := "fake" + strings.Repeat("0", 60) // looks like a 64-char container ID + maliciousLabel := "a-b\n" + injectedID + "\ta" + containerName := "mcpproxy-a-b-" + t.Name() + "-poc" + + runCmd := exec.CommandContext(ctx, "docker", "run", "-d", "--rm", + "--label", "com.mcpproxy.managed=true", + "--label", "com.mcpproxy.server="+maliciousLabel, + "--name", containerName, + "alpine", "sleep", "60") + var runOut bytes.Buffer + runCmd.Stdout = &runOut + if err := runCmd.Run(); err != nil { + t.Skipf("could not start docker PoC container (offline image pull?): %v", err) + } + realContainerID := strings.TrimSpace(runOut.String()) + t.Cleanup(func() { + _ = exec.Command("docker", "rm", "-f", containerName).Run() + }) + + r := NewSourceResolver(zap.NewNop()) + + // Scanning victim server "a" must find NOTHING — not the real container + // (whose true label is "a-b\n...", not "a"), and definitely not the + // forged in-band container ID. + id, err := r.findServerContainer(ctx, "a") + if err == nil { + t.Fatalf("findServerContainer(%q) = %q, nil error; want an error (no container owned by %q)", "a", id, "a") + } + if id == injectedID { + t.Fatalf("findServerContainer(%q) returned the INJECTED container id %q — label injection succeeded", "a", injectedID) + } + + // Scanning the genuine owner must find the real container by its exact, + // full (newline-containing) label value. + id, err = r.findServerContainer(ctx, maliciousLabel) + if err != nil { + t.Fatalf("findServerContainer(%q) unexpected error: %v", maliciousLabel, err) + } + if id != realContainerID { + t.Errorf("findServerContainer(%q) = %q, want the real container id %q", maliciousLabel, id, realContainerID) } } -func TestSelectOwnedContainerSkipsMalformedLines(t *testing.T) { - psOutput := "no-tab-here\ncid2\ta\n" - if got := selectOwnedContainer(psOutput, "a"); got != "cid2" { - t.Errorf("selectOwnedContainer(...) = %q, want %q", got, "cid2") +// TestFindServerContainerInstanceScoping proves the second review finding is +// closed: two "instances" (simulated by two different com.mcpproxy.instance +// label values on containers that otherwise share the same managed+server +// labels) must not be able to select each other's container once +// SetInstanceID pins the resolver to one of them. +func TestFindServerContainerInstanceScoping(t *testing.T) { + if !dockerAvailableForTest(t) { + t.Skip("docker daemon not available") + } + ctx := context.Background() + serverName := "shared-name-" + t.Name() + otherContainerName := "mcpproxy-" + serverName + "-other-instance" + + runCmd := exec.CommandContext(ctx, "docker", "run", "-d", "--rm", + "--label", "com.mcpproxy.managed=true", + "--label", "com.mcpproxy.server="+serverName, + "--label", "com.mcpproxy.instance=other-instance-id", + "--name", otherContainerName, + "alpine", "sleep", "60") + if err := runCmd.Run(); err != nil { + t.Skipf("could not start docker PoC container (offline image pull?): %v", err) + } + t.Cleanup(func() { + _ = exec.Command("docker", "rm", "-f", otherContainerName).Run() + }) + + r := NewSourceResolver(zap.NewNop()) + r.SetInstanceID("this-instance-id") + + if id, err := r.findServerContainer(ctx, serverName); err == nil { + t.Fatalf("findServerContainer(%q) = %q, nil error; want an error (container belongs to a different instance)", serverName, id) + } + + // Without an instance pinned, the same server+managed labels are enough + // (back-compat for callers — e.g. existing tests — that never call + // SetInstanceID). + r2 := NewSourceResolver(zap.NewNop()) + id, err := r2.findServerContainer(ctx, serverName) + if err != nil { + t.Fatalf("findServerContainer(%q) with no instance pinned: unexpected error: %v", serverName, err) + } + if id == "" { + t.Errorf("findServerContainer(%q) with no instance pinned returned empty id", serverName) } } diff --git a/internal/security/scanner/service.go b/internal/security/scanner/service.go index 57899c19c..eb916a522 100644 --- a/internal/security/scanner/service.go +++ b/internal/security/scanner/service.go @@ -187,6 +187,13 @@ func (s *Service) SetEmitter(emitter EventEmitter) { s.emitter.Store(&emitter) } +// SetInstanceID scopes this service's Docker container-ownership lookups to +// the given mcpproxy instance ID (internal/upstream/core.GetInstanceID()). +// See SourceResolver.instanceID for why this is injected rather than imported. +func (s *Service) SetInstanceID(instanceID string) { + s.sourceResolver.SetInstanceID(instanceID) +} + // SetScannerDisableNoNewPrivileges controls whether scanner containers are // launched without `--security-opt no-new-privileges`. This is the runtime // knob for SecurityConfig.ScannerDisableNoNewPrivileges. See the config diff --git a/internal/security/scanner/source_resolver.go b/internal/security/scanner/source_resolver.go index 0a8e90f6a..d3640b5d0 100644 --- a/internal/security/scanner/source_resolver.go +++ b/internal/security/scanner/source_resolver.go @@ -8,10 +8,10 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "sync/atomic" - "github.com/smart-mcp-proxy/mcpproxy-go/internal/dockernaming" "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" "go.uber.org/zap" ) @@ -41,6 +41,18 @@ type SourceResolver struct { // Pass-2 goroutine's ResolveFullSource increment is race-free. resolveCalls atomic.Int64 resolveFullSourceCalls atomic.Int64 + + // instanceID, when set, scopes findServerContainer's ownership check to + // containers labeled com.mcpproxy.instance= (the value + // internal/upstream/core.GetInstanceID() assigns at container creation), + // so two mcpproxy processes sharing one Docker daemon and a same-named + // server cannot select each other's container. It is injected via + // SetInstanceID by the wiring layer (internal/server) rather than imported + // directly — internal/upstream/core sits downstream of this package in the + // import graph (core -> storage/oauth -> ... -> security/scanner), so a + // direct import would cycle. Left empty in tests that construct a + // SourceResolver directly, in which case the instance filter is omitted. + instanceID string } // NewSourceResolver creates a new SourceResolver @@ -53,6 +65,12 @@ func (r *SourceResolver) SetFetchPackageSource(enabled bool) { r.fetchPackageSource = enabled } +// SetInstanceID scopes container-ownership lookups to this mcpproxy instance. +// See the instanceID field doc for why this is injected rather than imported. +func (r *SourceResolver) SetInstanceID(instanceID string) { + r.instanceID = instanceID +} + // dockerCmd builds an exec.Cmd that invokes the resolved `docker` binary. // // The binary is looked up via shellwrap.ResolveDockerPath rather than relying @@ -460,60 +478,78 @@ func dirLooksLikeSource(dir string) bool { return found } -// findServerContainer finds the running Docker container for a server. -// MCPProxy names containers as: mcpproxy--. -// The sanitization MUST match the one used to name the container at launch -// (internal/upstream/core), hence the shared dockernaming package — official -// registry names like "com.pulsemcp/google-flights" keep their dots and would -// otherwise never match (MCP-2123). +// findServerContainer finds the running Docker container owned by a server. +// Every container mcpproxy creates is labeled at launch (internal/upstream/core/instance.go) +// with com.mcpproxy.managed=true, com.mcpproxy.instance=, and com.mcpproxy.server=. Ownership here is established ENTIRELY through Docker's own +// `--filter label=key=value`, which the daemon matches against the raw label +// value server-side — never by matching container names or by rendering a +// label into --format text for us to re-parse. +// +// Two earlier approaches were tried and rejected here: // -// The `docker ps --filter name=` clause above is a SUBSTRING match, not an -// anchored one: servers whose sanitized names collide as a prefix (server "a" -// vs "a-b"/"a_b"/"a/b", which all sanitize toward a token "a-b" is a prefix -// of) can make this filter return a container belonging to a different -// server. That container would then be exec'd/copied/diffed and its findings -// reported under the wrong server. To close that gap, every candidate is -// re-verified against the com.mcpproxy.server label — set to the exact, -// unsanitized server name at container creation (internal/upstream/core) — -// via selectOwnedContainer, which rejects any candidate whose label does not -// match serverName exactly. +// 1. `docker ps --filter name=mcpproxy--` is a SUBSTRING match, +// not an anchored one: servers whose sanitized names collide as a prefix +// (server "a" vs "a-b"/"a_b"/"a/b", which all sanitize toward a token "a" +// is a prefix of) could resolve to a different server's container, whose +// filesystem would then be exec'd/copied/diffed and reported under the +// wrong server. +// +// 2. Re-verifying name-filtered candidates by requesting +// `{{.Label "com.mcpproxy.server"}}` in --format text and parsing +// tab/newline-delimited "records" is ITSELF exploitable: mcpproxy server +// names are validated only for non-empty and no ':' (internal/config/config.go), +// so a name containing an embedded newline+tab forges an extra line that a +// naive parser attributes to whatever "owner" follows the tab — redirecting +// extraction to an attacker-chosen container ID. Verified against a real +// Docker daemon: a server named "a-b\n\ta" makes +// `docker ps --filter name=mcpproxy-a- --format '{{.ID}}\t{{.Label ...}}'` +// print a second, fabricated line reading "\ta", which an +// exact-string-equality check on the parsed owner accepts for server "a". +// +// `--filter label=` has neither problem: it compares the argv value against +// the raw label bytes directly with no intermediate text format for a crafted +// label to escape (verified: filtering by the exact embedded-newline value +// matches only the genuine container; filtering by any truncated prefix of it +// matches nothing). func (r *SourceResolver) findServerContainer(ctx context.Context, serverName string) (string, error) { - // Use docker ps with filter to find matching containers - cmd := r.dockerCmd(ctx, "ps", - "--filter", fmt.Sprintf("name=mcpproxy-%s-", dockernaming.SanitizeServerName(serverName)), - "--format", "{{.ID}}\t{{.Label \"com.mcpproxy.server\"}}", - "--no-trunc", - ) + args := []string{"ps", + "--filter", "label=com.mcpproxy.managed=true", + "--filter", fmt.Sprintf("label=com.mcpproxy.server=%s", serverName), + } + if r.instanceID != "" { + args = append(args, "--filter", fmt.Sprintf("label=com.mcpproxy.instance=%s", r.instanceID)) + } + args = append(args, "--format", "{{.ID}}", "--no-trunc") + cmd := r.dockerCmd(ctx, args...) var stdout bytes.Buffer cmd.Stdout = &stdout if err := cmd.Run(); err != nil { return "", fmt.Errorf("docker ps failed: %w", err) } - containerID := selectOwnedContainer(stdout.String(), serverName) + containerID := firstContainerID(stdout.String()) if containerID == "" { return "", fmt.Errorf("no running container found for server %s", serverName) } return containerID, nil } -// selectOwnedContainer parses `docker ps --format {{.ID}}\t{{.Label "com.mcpproxy.server"}}` -// output and returns the ID of the first container whose ownership label -// equals serverName exactly. Candidates arrive from a name-prefix substring -// match (see findServerContainer), so this exact-equality check is what -// actually enforces ownership and rejects a same-prefix collision. -func selectOwnedContainer(psOutput, serverName string) string { +// containerIDPattern matches a Docker container ID as printed by `docker ps +// --no-trunc --format {{.ID}}`: lowercase hex, short (12) or full (64) form. +var containerIDPattern = regexp.MustCompile(`^[0-9a-f]{12,64}$`) + +// firstContainerID returns the first line of `docker ps --format {{.ID}}` +// output that has the shape of a real container ID. The ID is the one field +// in this pipeline Docker guarantees is a safe opaque hex token, but it still +// arrives as free-form command output, so a line that does not match the +// expected shape is discarded rather than trusted blindly. +func firstContainerID(psOutput string) string { for _, line := range strings.Split(strings.TrimSpace(psOutput), "\n") { - if line == "" { - continue - } - parts := strings.SplitN(line, "\t", 2) - if len(parts) != 2 { - continue - } - id, owner := parts[0], parts[1] - if owner == serverName { - return id + line = strings.TrimSpace(line) + if containerIDPattern.MatchString(line) { + return line } } return "" diff --git a/internal/server/server.go b/internal/server/server.go index e82173d4e..c3daf64a8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -2845,6 +2845,7 @@ func (s *Server) startCustomHTTPServer(ctx context.Context, streamableServer *se im := core.NewIsolationManager(liveCfg.DockerIsolation) return string(im.ResolveMode(sc)) }) + secService.SetInstanceID(core.GetInstanceID()) secService.SetEmitter(s.runtime) secService.SetServerInfoProvider(&configServerInfoProvider{ cfg: cfg, From 881b3fc199623d5a80e217466fc6b73db769b323 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:37:40 +0300 Subject: [PATCH 3/3] docs(security): correct overclaimed multi-instance guarantee in container ownership check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/security/scanner/source_resolver.go | 34 ++++++++++++++------ 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/internal/security/scanner/source_resolver.go b/internal/security/scanner/source_resolver.go index d3640b5d0..a5e810f83 100644 --- a/internal/security/scanner/source_resolver.go +++ b/internal/security/scanner/source_resolver.go @@ -44,14 +44,29 @@ type SourceResolver struct { // instanceID, when set, scopes findServerContainer's ownership check to // containers labeled com.mcpproxy.instance= (the value - // internal/upstream/core.GetInstanceID() assigns at container creation), - // so two mcpproxy processes sharing one Docker daemon and a same-named - // server cannot select each other's container. It is injected via - // SetInstanceID by the wiring layer (internal/server) rather than imported - // directly — internal/upstream/core sits downstream of this package in the - // import graph (core -> storage/oauth -> ... -> security/scanner), so a - // direct import would cycle. Left empty in tests that construct a - // SourceResolver directly, in which case the instance filter is omitted. + // internal/upstream/core.GetInstanceID() assigns at container creation). + // It is injected via SetInstanceID by the wiring layer (internal/server) + // rather than imported directly — internal/upstream/core sits downstream + // of this package in the import graph (core -> storage/oauth -> ... -> + // security/scanner), so a direct import would cycle. Left empty in tests + // that construct a SourceResolver directly, in which case the instance + // filter is omitted. + // + // KNOWN LIMITATION: this closes cross-instance ownership hijack only + // between mcpproxy processes that actually receive distinct instance IDs + // — e.g. separate hosts pointed at one shared/remote Docker daemon. + // core.GetInstanceID() persists its ID to a single file under + // os.TempDir(), which is shared by every process on the SAME host/user, + // so two mcpproxy processes running side by side on one machine (e.g. a + // scratch dev instance next to the main app — a workflow this repo's own + // tooling supports) currently receive the SAME instance ID and are not + // distinguished by this filter. That is a pre-existing property of + // GetInstanceID() (already relied on, with the same gap, by + // internal/upstream/manager.go's container cleanup) — fixing it means + // changing what gets written onto the label at container creation and + // every reader of that label, which is out of scope for this + // scanner-focused fix. This filter still fully closes the label/name + // injection this package's ownership check was vulnerable to. instanceID string } @@ -537,7 +552,8 @@ func (r *SourceResolver) findServerContainer(ctx context.Context, serverName str } // containerIDPattern matches a Docker container ID as printed by `docker ps -// --no-trunc --format {{.ID}}`: lowercase hex, short (12) or full (64) form. +// --format {{.ID}}`: lowercase hex, anywhere from a short prefix (12) up to +// the full (64-char) form `--no-trunc` normally emits. var containerIDPattern = regexp.MustCompile(`^[0-9a-f]{12,64}$`) // firstContainerID returns the first line of `docker ps --format {{.ID}}`