fix: security hardening (bash redirection, unicode sanitization, edit tool resolver)
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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{
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user