From a76a1920dc7587be700a17f1592f58a9512ad214 Mon Sep 17 00:00:00 2001 From: Volodymyr Stoiko Date: Wed, 12 Aug 2026 23:00:04 +0300 Subject: [PATCH] mcp: confine download_file destination and harden start_kubeshark argument handling (#1957) * mcp: confine download_file dest and fix start_kubeshark arg injection download_file wrote fetched bytes to a caller-supplied dest with no validation, and appended the caller-supplied pod_regex to 'kubeshark tap' with no end-of-options separator. Both are reachable via induced-agent (prompt-injection) tool arguments. - download_file: resolve dest through secureDownloadDest, confined to a base directory (CWD by default, relocatable via KUBESHARK_MCP_DOWNLOAD_DIR); reject dest that escapes the base (../ or absolute) and '..' in the Hub path (CWE-22). - start_kubeshark: append pod_regex last, after a '--' separator, so it is always the [POD REGEX] positional and never parsed as a --set flag (CWE-88). - tests: set downloadDir in the download tests; add a traversal-rejection test. Reported by novice-22 via coordinated disclosure. * mcp: reject download destinations that escape the base via symlink Containment in secureDownloadDest was lexical only, so a symlinked subdirectory inside the download dir (or a dest that is itself a symlink) could still redirect the write outside it. Resolve symlinks on the deepest existing ancestor and re-check containment, and open the file with O_NOFOLLOW where available to narrow the TOCTOU window. --- cmd/createNoFollow_unix.go | 15 ++++ cmd/createNoFollow_windows.go | 12 +++ cmd/mcpRunner.go | 156 +++++++++++++++++++++++++++++++--- cmd/mcp_test.go | 115 +++++++++++++++++++++++++ 4 files changed, 287 insertions(+), 11 deletions(-) create mode 100644 cmd/createNoFollow_unix.go create mode 100644 cmd/createNoFollow_windows.go diff --git a/cmd/createNoFollow_unix.go b/cmd/createNoFollow_unix.go new file mode 100644 index 000000000..06743c934 --- /dev/null +++ b/cmd/createNoFollow_unix.go @@ -0,0 +1,15 @@ +//go:build !windows + +package cmd + +import ( + "os" + "syscall" +) + +// createNoFollow creates or truncates name for writing and fails if the final +// path component is a symlink. Used for MCP download destinations so a symlink +// cannot redirect the write outside the confined download directory. +func createNoFollow(name string) (*os.File, error) { + return os.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|syscall.O_NOFOLLOW, 0o644) +} diff --git a/cmd/createNoFollow_windows.go b/cmd/createNoFollow_windows.go new file mode 100644 index 000000000..feca2352b --- /dev/null +++ b/cmd/createNoFollow_windows.go @@ -0,0 +1,12 @@ +package cmd + +import "os" + +// createNoFollow mirrors the Unix helper. Windows has no O_NOFOLLOW; creating a +// symlink there requires either administrator rights or developer mode, so the +// symlink-planting scenario the flag guards against does not apply in the same +// way. secureDownloadDest still rejects destinations that resolve through a +// symlink outside the download directory. +func createNoFollow(name string) (*os.File, error) { + return os.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) +} diff --git a/cmd/mcpRunner.go b/cmd/mcpRunner.go index c3a473ad7..78253fee5 100644 --- a/cmd/mcpRunner.go +++ b/cmd/mcpRunner.go @@ -5,12 +5,14 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" "os" "os/exec" "path" + "path/filepath" "strings" "sync" "time" @@ -161,6 +163,7 @@ type mcpServer struct { cachedAt time.Time // When the cache was populated hubMCPMu sync.Mutex tokenSource func() string // hub SA token source; proxy mode auto-renews, URL mode is the static --token. nil → License-Key + downloadDir string // base dir download_file is confined to (CWE-22); empty → CWD at call time } const hubMCPCacheTTL = 5 * time.Minute @@ -207,6 +210,7 @@ func runMCPWithConfig(setFlags []string, directURL string, allowDestructive bool directURL: directURL, urlMode: urlMode, allowDestructive: allowDestructive, + downloadDir: mcpDownloadDir(), } // If URL mode, validate the URL is accessible on startup @@ -861,17 +865,28 @@ func (s *mcpServer) callDownloadFile(args map[string]any) (string, bool) { return fmt.Sprintf("Error: %v", err), true } - // Ensure path starts with / + // Ensure path starts with / and reject "../" traversal so a caller can't + // climb outside the Hub API namespace (CWE-22). if !strings.HasPrefix(filePath, "/") { filePath = "/" + filePath } + if hasDotDotSegment(filePath) { + return "Error: 'path' must not contain '..' segments", true + } fullURL := strings.TrimSuffix(baseURL, "/") + filePath - // Determine destination file path - dest, _ := args["dest"].(string) - if dest == "" { - dest = path.Base(filePath) + // Resolve the destination, confined to the working directory so a + // caller-supplied 'dest' cannot traverse ("../") or use an absolute path to + // write outside it (CWE-22 — arbitrary file write via an induced agent). + destArg, _ := args["dest"].(string) + baseDir := s.downloadDir + if baseDir == "" { + baseDir = mcpDownloadDir() + } + dest, err := secureDownloadDest(baseDir, destArg, filePath) + if err != nil { + return fmt.Sprintf("Error: %v", err), true } // Use a dedicated HTTP client for file downloads. @@ -900,8 +915,13 @@ func (s *mcpServer) callDownloadFile(args map[string]any) (string, bool) { return fmt.Sprintf("Error downloading file: HTTP %d", resp.StatusCode), true } - // Write to destination - outFile, err := os.Create(dest) + // Write to destination (parent dir is within the confined base). + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return fmt.Sprintf("Error creating directory for %s: %v", dest, err), true + } + // Refuse to follow a symlink at the final component, so a symlink planted + // between the check above and this write cannot redirect the bytes. + outFile, err := createNoFollow(dest) if err != nil { return fmt.Sprintf("Error creating file %s: %v", dest, err), true } @@ -921,14 +941,121 @@ func (s *mcpServer) callDownloadFile(args map[string]any) (string, bool) { return string(resultBytes), false } +// hasDotDotSegment reports whether p contains a ".." path segment. Used to +// reject traversal in the caller-supplied Hub 'path' (CWE-22). +func hasDotDotSegment(p string) bool { + for _, seg := range strings.Split(p, "/") { + if seg == ".." { + return true + } + } + return false +} + +// mcpDownloadDir is the base directory download_file is confined to. It +// defaults to the current working directory (the dir the operator launched +// 'kubeshark mcp' from) and can be relocated via KUBESHARK_MCP_DOWNLOAD_DIR so +// an operator can widen or move the sandbox intentionally. +func mcpDownloadDir() string { + if d := os.Getenv("KUBESHARK_MCP_DOWNLOAD_DIR"); d != "" { + return d + } + if wd, err := os.Getwd(); err == nil { + return wd + } + return "." +} + +// secureDownloadDest resolves the caller-supplied download destination to an +// absolute path confined to baseDir. An empty dest falls back to the base name +// of the Hub file path. A dest (absolute or relative) that resolves outside +// baseDir via "../" traversal is rejected, so download_file cannot be coerced +// (e.g. via an induced agent) into writing to arbitrary locations such as +// ~/.ssh/authorized_keys or ~/.kube/config. See CWE-22. +// +// Containment is checked twice: once lexically, then again after resolving +// symlinks, so a symlinked subdirectory (or a symlinked dest itself) inside the +// base cannot redirect the write outside it. +func secureDownloadDest(baseDir, dest, filePath string) (string, error) { + if dest == "" { + dest = path.Base(filePath) + } + absBase, err := filepath.Abs(baseDir) + if err != nil { + return "", fmt.Errorf("cannot resolve download directory: %w", err) + } + // Resolve symlinks in the base itself, so the comparison below is between + // real paths. The operator may legitimately point the download dir at a + // symlink (e.g. /tmp on macOS, which is a link to /private/tmp). + if resolvedBase, err := filepath.EvalSymlinks(absBase); err == nil { + absBase = resolvedBase + } + full := dest + if filepath.IsAbs(full) { + full = filepath.Clean(full) + } else { + full = filepath.Join(absBase, full) + } + if !pathContained(absBase, full) { + return "", fmt.Errorf("destination %q escapes the download directory %q", dest, absBase) + } + resolvedFull, err := resolveSymlinkedPath(full) + if err != nil { + return "", fmt.Errorf("cannot resolve destination %q: %w", dest, err) + } + if !pathContained(absBase, resolvedFull) { + return "", fmt.Errorf("destination %q resolves through a symlink to %q, outside the download directory %q", dest, resolvedFull, absBase) + } + return full, nil +} + +// pathContained reports whether target is base itself or lies beneath it. Both +// arguments must already be absolute and symlink-resolved. +func pathContained(base, target string) bool { + rel, err := filepath.Rel(base, target) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) +} + +// resolveSymlinkedPath resolves symlinks in the deepest existing ancestor of p +// and re-appends the trailing components that do not exist yet. +// filepath.EvalSymlinks fails outright on a path that does not exist, which is +// the normal case for a download destination, hence the walk upwards. +func resolveSymlinkedPath(p string) (string, error) { + cur := filepath.Clean(p) + rest := "" + for { + resolved, err := filepath.EvalSymlinks(cur) + if err == nil { + if rest == "" { + return resolved, nil + } + return filepath.Join(resolved, rest), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + parent := filepath.Dir(cur) + if parent == cur { + // Walked up to the root without finding an existing ancestor. + return filepath.Clean(p), nil + } + rest = filepath.Join(filepath.Base(cur), rest) + cur = parent + } +} + func (s *mcpServer) callStartKubeshark(args map[string]any) (string, bool) { // Build the kubeshark tap command cmdArgs := []string{"tap"} - // Add pod regex if provided - if v, ok := args["pod_regex"].(string); ok && v != "" { - cmdArgs = append(cmdArgs, v) - } + // Capture the caller-supplied pod regex. It is appended LAST, after a "--" + // end-of-options separator (see below), so a value beginning with "-" + // (e.g. "--set=tap.docker.registry=...") can never be parsed as a flag by + // the 'tap' cobra command. See CWE-88 (argument injection). + podRegex, _ := args["pod_regex"].(string) // Add namespaces if provided if v, ok := args["namespaces"].(string); ok && v != "" { @@ -956,6 +1083,13 @@ func (s *mcpServer) callStartKubeshark(args map[string]any) (string, bool) { // Execute the command in headless mode (no browser popup) cmdArgs = append(cmdArgs, "--set", "headless=true") + // Append the pod regex as a positional argument, guarded by "--" so it is + // always treated as the [POD REGEX] operand and never as a flag (CWE-88). + // This must come after every flag above. + if podRegex != "" { + cmdArgs = append(cmdArgs, "--", podRegex) + } + // Log progress to stderr (MCP clients can see this in their logs) logProgress := func(msg string) { _, _ = fmt.Fprintf(os.Stderr, "[kubeshark-mcp] %s\n", msg) diff --git a/cmd/mcp_test.go b/cmd/mcp_test.go index 4ae45fde3..3c6755c1e 100644 --- a/cmd/mcp_test.go +++ b/cmd/mcp_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -564,6 +565,7 @@ func TestMCP_DownloadFile(t *testing.T) { stdout: &bytes.Buffer{}, hubBaseURL: mockServer.URL + "/api/mcp", backendInitialized: true, + downloadDir: tmpDir, } resp := parseResponse(t, sendRequest(s, "tools/call", 1, mcpCallToolParams{ Name: "download_file", @@ -614,6 +616,7 @@ func TestMCP_DownloadFile_CustomDest(t *testing.T) { stdout: &bytes.Buffer{}, hubBaseURL: mockServer.URL + "/api/mcp", backendInitialized: true, + downloadDir: tmpDir, } resp := parseResponse(t, sendRequest(s, "tools/call", 1, mcpCallToolParams{ Name: "download_file", @@ -638,6 +641,118 @@ func TestMCP_DownloadFile_CustomDest(t *testing.T) { } } +// TestMCP_DownloadFile_RejectsTraversal locks in the CWE-22 fix: a dest that +// escapes the confined download dir (via "../" or an absolute path outside it) +// and a 'path' containing ".." segments must be refused, and nothing written. +func TestMCP_DownloadFile_RejectsTraversal(t *testing.T) { + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("data")) + })) + defer mockServer.Close() + + baseDir := t.TempDir() + outsideDir := t.TempDir() + s := &mcpServer{ + httpClient: &http.Client{}, + stdin: &bytes.Buffer{}, + stdout: &bytes.Buffer{}, + hubBaseURL: mockServer.URL + "/api/mcp", + backendInitialized: true, + downloadDir: baseDir, + } + + cases := []struct { + name string + args map[string]any + }{ + {"relative-traversal", map[string]any{"path": "/snapshots/abc/data.pcap", "dest": "../escaped.pcap"}}, + {"absolute-outside-base", map[string]any{"path": "/snapshots/abc/data.pcap", "dest": filepath.Join(outsideDir, "escaped.pcap")}}, + {"path-dotdot", map[string]any{"path": "/snapshots/../../etc/passwd", "dest": "ok.pcap"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp := parseResponse(t, sendRequest(s, "tools/call", 1, mcpCallToolParams{ + Name: "download_file", + Arguments: tc.args, + })) + result := resp.Result.(map[string]any) + if result["isError"] == nil || !result["isError"].(bool) { + t.Fatalf("Expected an error for %s, got: %v", tc.name, result["content"]) + } + }) + } + + if _, err := os.Stat(filepath.Join(outsideDir, "escaped.pcap")); !os.IsNotExist(err) { + t.Error("File was written outside the confined download directory") + } +} + +// TestMCP_DownloadFile_RejectsSymlinkEscape covers the non-lexical half of the +// CWE-22 fix: a dest that stays inside the download dir lexically but resolves +// outside it through a symlink must be refused. +func TestMCP_DownloadFile_RejectsSymlinkEscape(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation on Windows requires elevated rights") + } + + mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("data")) + })) + defer mockServer.Close() + + baseDir := t.TempDir() + outsideDir := t.TempDir() + + // A symlinked subdirectory inside the base, pointing outside it. + if err := os.Symlink(outsideDir, filepath.Join(baseDir, "linkdir")); err != nil { + t.Fatalf("Failed to create dir symlink: %v", err) + } + // A symlinked file inside the base, pointing at a file outside it. + victim := filepath.Join(outsideDir, "victim.pcap") + if err := os.WriteFile(victim, []byte("original"), 0o600); err != nil { + t.Fatalf("Failed to seed victim file: %v", err) + } + if err := os.Symlink(victim, filepath.Join(baseDir, "linkfile.pcap")); err != nil { + t.Fatalf("Failed to create file symlink: %v", err) + } + + s := &mcpServer{ + httpClient: &http.Client{}, + stdin: &bytes.Buffer{}, + stdout: &bytes.Buffer{}, + hubBaseURL: mockServer.URL + "/api/mcp", + backendInitialized: true, + downloadDir: baseDir, + } + + cases := []struct { + name string + dest string + }{ + {"through-symlinked-dir", "linkdir/escaped.pcap"}, + {"dest-is-symlink", "linkfile.pcap"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp := parseResponse(t, sendRequest(s, "tools/call", 1, mcpCallToolParams{ + Name: "download_file", + Arguments: map[string]any{"path": "/snapshots/abc/data.pcap", "dest": tc.dest}, + })) + result := resp.Result.(map[string]any) + if result["isError"] == nil || !result["isError"].(bool) { + t.Fatalf("Expected an error for %s, got: %v", tc.name, result["content"]) + } + }) + } + + if _, err := os.Stat(filepath.Join(outsideDir, "escaped.pcap")); !os.IsNotExist(err) { + t.Error("File was written outside the base through a symlinked directory") + } + if content, err := os.ReadFile(victim); err != nil || string(content) != "original" { + t.Errorf("Victim file outside the base was modified: %q, err %v", content, err) + } +} + func TestMCP_ToolsList_IncludesFileTools(t *testing.T) { s := newTestMCPServer() resp := parseResponse(t, sendRequest(s, "tools/list", 1, nil))