From 244ecd97e50cc90544aa5cce01f3e87b81d168cc Mon Sep 17 00:00:00 2001 From: vikingowl Date: Thu, 21 May 2026 23:29:48 +0200 Subject: [PATCH] fix: security hardening (bash redirection, unicode sanitization, edit tool resolver) --- internal/config/config.go | 4 +- internal/security/sanitize.go | 4 ++ internal/security/security_test.go | 9 +++ internal/tool/bash/security.go | 101 +++++++++++++++++++++++----- internal/tool/bash/security_test.go | 17 +++++ internal/tool/fs/edit.go | 2 +- 6 files changed, 119 insertions(+), 18 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 9ef3e2b..a8cb2c2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -201,14 +201,14 @@ type ProviderSection struct { Default string `toml:"default"` Model string `toml:"model"` MaxTokens int64 `toml:"max_tokens"` - Temperature *float64 `toml:"temperature"` // TODO(M8): wire to provider.Request.Temperature + Temperature *float64 `toml:"temperature"` APIKeys map[string]string `toml:"api_keys"` Endpoints map[string]string `toml:"endpoints"` } type ToolsSection struct { BashTimeout Duration `toml:"bash_timeout"` - MaxFileSize int64 `toml:"max_file_size"` // TODO(M8): wire to fs tool WithMaxFileSize option + MaxFileSize int64 `toml:"max_file_size"` } // RateLimitSection allows overriding default rate limits per provider. diff --git a/internal/security/sanitize.go b/internal/security/sanitize.go index cecfca2..21eda54 100644 --- a/internal/security/sanitize.go +++ b/internal/security/sanitize.go @@ -44,6 +44,10 @@ func shouldStrip(r rune) bool { if unicode.Is(unicode.Co, r) { return true } + // Strip unassigned characters (Cn) — unregistered characters + if unicode.Is(unicode.Cn, r) { + return true + } // Strip specific dangerous ranges switch { diff --git a/internal/security/security_test.go b/internal/security/security_test.go index 79a5552..653780b 100644 --- a/internal/security/security_test.go +++ b/internal/security/security_test.go @@ -360,6 +360,15 @@ func TestSanitizeUnicode_PreservesEmoji(t *testing.T) { } } +func TestSanitizeUnicode_StripsUnassigned(t *testing.T) { + // Unassigned character (Cn) e.g., U+0378 + unassigned := "Hello\u0378world" + result := SanitizeUnicode(unassigned) + if result != "Helloworld" { + t.Errorf("should strip unassigned characters, got %q", result) + } +} + // --- Incognito --- func TestIncognito_DefaultOff(t *testing.T) { diff --git a/internal/tool/bash/security.go b/internal/tool/bash/security.go index 91fd53c..1229e66 100644 --- a/internal/tool/bash/security.go +++ b/internal/tool/bash/security.go @@ -2,8 +2,11 @@ package bash import ( "fmt" + "path/filepath" "strings" "unicode" + + "mvdan.cc/sh/v3/syntax" ) // SecurityCheck identifies a specific validation check. @@ -251,7 +254,7 @@ func checkStandaloneSemicolon(cmd string) *SecurityViolation { } // checkSensitiveRedirection blocks output redirection to sensitive paths. -// Detects: >, >>, fd redirects (2>), and no-space variants (>/etc/passwd). +// Uses a POSIX shell parser to reliably identify all output redirections. func checkSensitiveRedirection(cmd string) *SecurityViolation { sensitiveTargets := []string{ "/etc/passwd", "/etc/shadow", "/etc/sudoers", @@ -260,22 +263,90 @@ func checkSensitiveRedirection(cmd string) *SecurityViolation { ".env", } - for _, target := range sensitiveTargets { - // Match any form: >, >>, 2>, 2>>, &> followed by optional whitespace then target - idx := strings.Index(cmd, target) - if idx <= 0 { - continue - } - // Check what precedes the target (skip whitespace backwards) - pre := strings.TrimRight(cmd[:idx], " \t") - if len(pre) > 0 && (pre[len(pre)-1] == '>' || strings.HasSuffix(pre, ">>")) { - return &SecurityViolation{ - Check: CheckRedirection, - Message: fmt.Sprintf("redirection to sensitive path: %s", target), - } + reader := strings.NewReader(cmd) + parser := syntax.NewParser() + file, err := parser.Parse(reader, "") + if err != nil { + return &SecurityViolation{ + Check: CheckIncomplete, + Message: fmt.Sprintf("invalid command syntax: %v", err), } } - return nil + + var violation *SecurityViolation + printer := syntax.NewPrinter() + + syntax.Walk(file, func(node syntax.Node) bool { + if violation != nil { + return false + } + + if stmt, ok := node.(*syntax.Stmt); ok { + for _, redir := range stmt.Redirs { + op := redir.Op + // Check all redirection operators that write or modify files: + // Skip read-only/heredoc operators: RdrIn (<), DplIn (<&), Hdoc (<<), DashHdoc (<<-), WordHdoc (<<<) + if op == syntax.RdrIn || op == syntax.DplIn || op == syntax.Hdoc || op == syntax.DashHdoc || op == syntax.WordHdoc { + continue + } + + if redir.Word == nil { + continue + } + + var b strings.Builder + _ = printer.Print(&b, redir.Word) + targetPath := b.String() + + // Strip single/double quotes around the target word if present + targetPath = strings.TrimSpace(targetPath) + if (strings.HasPrefix(targetPath, "\"") && strings.HasSuffix(targetPath, "\"")) || + (strings.HasPrefix(targetPath, "'") && strings.HasSuffix(targetPath, "'")) { + if len(targetPath) >= 2 { + targetPath = targetPath[1 : len(targetPath)-1] + } + } + + cleaned := filepath.Clean(targetPath) + + for _, target := range sensitiveTargets { + if strings.HasPrefix(target, "/") { + // Absolute targets: exact match + if cleaned == target { + violation = &SecurityViolation{ + Check: CheckRedirection, + Message: fmt.Sprintf("redirection to sensitive path: %s", target), + } + return false + } + } else { + // Relative targets: suffix/base match + if target == ".env" || target == ".bashrc" || target == ".zshrc" || target == ".profile" || target == ".bash_profile" { + if filepath.Base(cleaned) == target { + violation = &SecurityViolation{ + Check: CheckRedirection, + Message: fmt.Sprintf("redirection to sensitive path: %s", target), + } + return false + } + } else { + // Relative paths with directory components (e.g. .ssh/config) + if strings.HasSuffix(cleaned, "/"+target) || cleaned == target { + violation = &SecurityViolation{ + Check: CheckRedirection, + Message: fmt.Sprintf("redirection to sensitive path: %s", target), + } + return false + } + } + } + } + } + } + return true + }) + + return violation } // checkJQInjection detects jq commands with embedded shell metacharacters in the filter. diff --git a/internal/tool/bash/security_test.go b/internal/tool/bash/security_test.go index a880da3..609224d 100644 --- a/internal/tool/bash/security_test.go +++ b/internal/tool/bash/security_test.go @@ -229,6 +229,12 @@ func TestCheckSensitiveRedirection_Blocked(t *testing.T) { "echo evil > /etc/passwd", "echo evil>>/etc/shadow", "echo evil >> /etc/shadow", + "echo evil >\\\n.env", + "echo evil > \".env\"", + "echo evil > '.env'", + "echo evil > ./.env", + "echo evil > sub/.env", + "echo evil > /home/user/workspace/.env", } for _, cmd := range blocked { t.Run(cmd, func(t *testing.T) { @@ -240,6 +246,17 @@ func TestCheckSensitiveRedirection_Blocked(t *testing.T) { } } +func TestCheckSensitiveRedirection_SyntaxError(t *testing.T) { + v := ValidateCommand("echo hello > \"unclosed quote") + if v == nil { + t.Error("expected violation for invalid syntax") + return + } + if v.Check != CheckIncomplete { + t.Errorf("expected CheckIncomplete, got %d", v.Check) + } +} + func TestCheckProcessSubstitution_Allowed(t *testing.T) { // Process substitution <() and >() should NOT be blocked allowed := []string{ diff --git a/internal/tool/fs/edit.go b/internal/tool/fs/edit.go index bf74078..f31e002 100644 --- a/internal/tool/fs/edit.go +++ b/internal/tool/fs/edit.go @@ -79,7 +79,7 @@ func (t *EditTool) Execute(_ context.Context, args json.RawMessage) (tool.Result path := a.Path if t.guard != nil { - resolved, err := t.guard.ResolveRead(path) + resolved, err := t.guard.ResolveWrite(path) if err != nil { return tool.Result{Output: fmt.Sprintf("Error: %v", err)}, nil }