diff --git a/internal/application/service/mcp_service.go b/internal/application/service/mcp_service.go index 79899d00c..ed6c5b00e 100644 --- a/internal/application/service/mcp_service.go +++ b/internal/application/service/mcp_service.go @@ -31,6 +31,22 @@ func NewMCPServiceService( // CreateMCPService creates a new MCP service func (s *mcpServiceService) CreateMCPService(ctx context.Context, service *types.MCPService) error { + // Security validation for stdio transport type + if service.TransportType == types.MCPTransportStdio { + if service.StdioConfig == nil { + return fmt.Errorf("stdio_config is required for stdio transport") + } + // Validate stdio configuration to prevent command injection (CWE-78) + if err := secutils.ValidateStdioConfig( + service.StdioConfig.Command, + service.StdioConfig.Args, + service.EnvVars, + ); err != nil { + logger.GetLogger(ctx).Warnf("MCP service creation blocked due to security validation: %v", err) + return fmt.Errorf("security validation failed: %w", err) + } + } + // Set default advanced config if not provided if service.AdvancedConfig == nil { service.AdvancedConfig = types.GetDefaultAdvancedConfig() @@ -113,6 +129,33 @@ func (s *mcpServiceService) UpdateMCPService(ctx context.Context, service *types return fmt.Errorf("MCP service not found") } + // Security validation for stdio transport type when updating stdio config + // Determine the final transport type and stdio config after merge + finalTransportType := existing.TransportType + if service.TransportType != "" { + finalTransportType = service.TransportType + } + finalStdioConfig := existing.StdioConfig + if service.StdioConfig != nil { + finalStdioConfig = service.StdioConfig + } + finalEnvVars := existing.EnvVars + if service.EnvVars != nil { + finalEnvVars = service.EnvVars + } + + // Validate if the final configuration uses stdio transport + if finalTransportType == types.MCPTransportStdio && finalStdioConfig != nil { + if err := secutils.ValidateStdioConfig( + finalStdioConfig.Command, + finalStdioConfig.Args, + finalEnvVars, + ); err != nil { + logger.GetLogger(ctx).Warnf("MCP service update blocked due to security validation: %v", err) + return fmt.Errorf("security validation failed: %w", err) + } + } + // Store old enabled state BEFORE any updates oldEnabled := existing.Enabled diff --git a/internal/mcp/client.go b/internal/mcp/client.go index 98cc05a9b..a9d4ec4dd 100644 --- a/internal/mcp/client.go +++ b/internal/mcp/client.go @@ -9,6 +9,7 @@ import ( "github.com/Tencent/WeKnora/internal/logger" "github.com/Tencent/WeKnora/internal/types" + secutils "github.com/Tencent/WeKnora/internal/utils" "github.com/mark3labs/mcp-go/client" "github.com/mark3labs/mcp-go/client/transport" "github.com/mark3labs/mcp-go/mcp" @@ -122,6 +123,16 @@ func NewMCPClient(config *ClientConfig) (MCPClient, error) { return nil, fmt.Errorf("stdio_config is required for stdio transport") } + // Security validation: validate command, args, and env vars before execution + // This prevents command injection attacks (CWE-78) + if err := secutils.ValidateStdioConfig( + config.Service.StdioConfig.Command, + config.Service.StdioConfig.Args, + config.Service.EnvVars, + ); err != nil { + return nil, fmt.Errorf("stdio configuration validation failed: %w", err) + } + // Convert env vars map to []string format (KEY=value) envVars := make([]string, 0, len(config.Service.EnvVars)) for key, value := range config.Service.EnvVars { diff --git a/internal/utils/security.go b/internal/utils/security.go index 63b348acf..ccaeb1752 100644 --- a/internal/utils/security.go +++ b/internal/utils/security.go @@ -1,6 +1,7 @@ package utils import ( + "fmt" "html" "regexp" "strings" @@ -217,3 +218,163 @@ func SanitizeForLogArray(input []string) []string { return sanitized } + +// AllowedStdioCommands defines the whitelist of allowed commands for MCP stdio transport +// These are the standard MCP server launchers that are considered safe +var AllowedStdioCommands = map[string]bool{ + "uvx": true, // Python package runner (uv) + "npx": true, // Node.js package runner +} + +// DangerousArgPatterns contains patterns that indicate potentially dangerous arguments +var DangerousArgPatterns = []*regexp.Regexp{ + regexp.MustCompile(`(?i)^-c$`), // Shell command execution flag + regexp.MustCompile(`(?i)^--command$`), // Shell command execution flag + regexp.MustCompile(`(?i)^-e$`), // Eval flag + regexp.MustCompile(`(?i)^--eval$`), // Eval flag + regexp.MustCompile(`(?i)[;&|]`), // Shell command chaining + regexp.MustCompile(`(?i)\$\(`), // Command substitution + regexp.MustCompile("(?i)`"), // Backtick command substitution + regexp.MustCompile(`(?i)>\s*[/~]`), // Output redirection to absolute/home path + regexp.MustCompile(`(?i)<\s*[/~]`), // Input redirection from absolute/home path + regexp.MustCompile(`(?i)^/bin/`), // Direct binary path + regexp.MustCompile(`(?i)^/usr/bin/`), // Direct binary path + regexp.MustCompile(`(?i)^/sbin/`), // Direct binary path + regexp.MustCompile(`(?i)^/usr/sbin/`), // Direct binary path + regexp.MustCompile(`(?i)^\.\./`), // Path traversal + regexp.MustCompile(`(?i)/\.\./`), // Path traversal in middle + regexp.MustCompile(`(?i)^(bash|sh|zsh|ksh|csh|tcsh|fish|dash)$`), // Shell interpreters as args + regexp.MustCompile(`(?i)^(curl|wget|nc|netcat|ncat)$`), // Network tools as args + regexp.MustCompile(`(?i)^(rm|dd|mkfs|fdisk)$`), // Destructive commands as args +} + +// DangerousEnvVarPatterns contains patterns for dangerous environment variable names or values +var DangerousEnvVarPatterns = []*regexp.Regexp{ + regexp.MustCompile(`(?i)^LD_PRELOAD$`), // Library injection + regexp.MustCompile(`(?i)^LD_LIBRARY_PATH$`), // Library path manipulation + regexp.MustCompile(`(?i)^DYLD_`), // macOS dynamic linker + regexp.MustCompile(`(?i)^PATH$`), // PATH manipulation + regexp.MustCompile(`(?i)^PYTHONPATH$`), // Python path manipulation + regexp.MustCompile(`(?i)^NODE_OPTIONS$`), // Node.js options injection + regexp.MustCompile(`(?i)^BASH_ENV$`), // Bash environment file + regexp.MustCompile(`(?i)^ENV$`), // Shell environment file + regexp.MustCompile(`(?i)^SHELL$`), // Shell override +} + +// ValidateStdioCommand validates the command for MCP stdio transport +// Returns an error if the command is not in the whitelist or contains dangerous patterns +func ValidateStdioCommand(command string) error { + if command == "" { + return fmt.Errorf("command cannot be empty") + } + + // Normalize command (extract base name if it's a path) + baseCommand := command + if strings.Contains(command, "/") { + parts := strings.Split(command, "/") + baseCommand = parts[len(parts)-1] + } + + // Check against whitelist + if !AllowedStdioCommands[baseCommand] { + return fmt.Errorf("command '%s' is not in the allowed list. Allowed commands: uvx, npx, node, python, python3, deno, bun", baseCommand) + } + + // Additional check: command should not contain path traversal + if strings.Contains(command, "..") { + return fmt.Errorf("command path contains invalid characters") + } + + return nil +} + +// ValidateStdioArgs validates the arguments for MCP stdio transport +// Returns an error if any argument contains dangerous patterns +func ValidateStdioArgs(args []string) error { + if len(args) == 0 { + return nil + } + + for i, arg := range args { + // Check length + if len(arg) > 1024 { + return fmt.Errorf("argument %d exceeds maximum length (1024 characters)", i) + } + + // Check against dangerous patterns + for _, pattern := range DangerousArgPatterns { + if pattern.MatchString(arg) { + return fmt.Errorf("argument %d contains potentially dangerous pattern: %s", i, SanitizeForLog(arg)) + } + } + + // Check for null bytes + if strings.Contains(arg, "\x00") { + return fmt.Errorf("argument %d contains null bytes", i) + } + } + + return nil +} + +// ValidateStdioEnvVars validates environment variables for MCP stdio transport +// Returns an error if any env var name or value is dangerous +func ValidateStdioEnvVars(envVars map[string]string) error { + if len(envVars) == 0 { + return nil + } + + for key, value := range envVars { + // Check key against dangerous patterns + for _, pattern := range DangerousEnvVarPatterns { + if pattern.MatchString(key) { + return fmt.Errorf("environment variable '%s' is not allowed for security reasons", key) + } + } + + // Check key length + if len(key) > 256 { + return fmt.Errorf("environment variable name '%s' exceeds maximum length", SanitizeForLog(key[:50])) + } + + // Check value length + if len(value) > 4096 { + return fmt.Errorf("environment variable '%s' value exceeds maximum length", key) + } + + // Check for null bytes in value + if strings.Contains(value, "\x00") { + return fmt.Errorf("environment variable '%s' value contains null bytes", key) + } + + // Check value for shell injection patterns + for _, pattern := range DangerousArgPatterns { + if pattern.MatchString(value) { + return fmt.Errorf("environment variable '%s' value contains potentially dangerous pattern", key) + } + } + } + + return nil +} + +// ValidateStdioConfig performs comprehensive validation of stdio configuration +// This should be called before creating or executing any stdio-based MCP client +func ValidateStdioConfig(command string, args []string, envVars map[string]string) error { + // Validate command + if err := ValidateStdioCommand(command); err != nil { + return fmt.Errorf("invalid command: %w", err) + } + + // Validate arguments + if err := ValidateStdioArgs(args); err != nil { + return fmt.Errorf("invalid arguments: %w", err) + } + + // Validate environment variables + if err := ValidateStdioEnvVars(envVars); err != nil { + return fmt.Errorf("invalid environment variables: %w", err) + } + + return nil +}