diff --git a/internal/security/scanner/container_ownership_test.go b/internal/security/scanner/container_ownership_test.go new file mode 100644 index 000000000..2c2f81ff5 --- /dev/null +++ b/internal/security/scanner/container_ownership_test.go @@ -0,0 +1,167 @@ +package scanner + +import ( + "bytes" + "context" + "os/exec" + "strings" + "testing" + + "go.uber.org/zap" +) + +// 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 + }{ + {"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) { + if got := firstContainerID(tc.in); got != tc.want { + t.Errorf("firstContainerID(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// 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) + } +} + +// 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 e1e5be7d8..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 @@ -1013,6 +1020,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 +1229,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..a5e810f83 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,33 @@ 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). + // 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 } // NewSourceResolver creates a new SourceResolver @@ -53,6 +80,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 @@ -98,6 +131,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 +192,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", @@ -458,32 +493,82 @@ 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: +// +// 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}}", - "--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) } - lines := strings.Split(strings.TrimSpace(stdout.String()), "\n") - if len(lines) == 0 || lines[0] == "" { + containerID := firstContainerID(stdout.String()) + if containerID == "" { return "", fmt.Errorf("no running container found for server %s", serverName) } + return containerID, nil +} - // Return first match - return lines[0], nil +// containerIDPattern matches a Docker container ID as printed by `docker ps +// --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}}` +// 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") { + line = strings.TrimSpace(line) + if containerIDPattern.MatchString(line) { + return line + } + } + return "" } // extractFromContainer extracts changed files from a running container. @@ -953,10 +1038,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 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,