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.
2026-08-19 12:26:32 +00:00 · 2026-08-12 13:00:04 -07:00
parent ed9ce1b561
commit a76a1920dc
4 changed files with 287 additions and 11 deletions
@@ -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)
+}
@@ -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)
+}
@@ -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)
@@ -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))