fix(security): harden exec path exemption matching (#721)

- Add absolute path exemption for dataDir/skills-store/ (fixes skill
  scripts using absolute paths like /app/data/skills-store/ being denied)
- Strip surrounding quotes before prefix matching (LLMs often quote paths)
- Reject path traversal ("..") in exempt fields to prevent escape
- Switch from "any field exempt → skip" to per-field matching: only exempt
  if ALL fields that match the deny pattern are individually exempt
- Closes pipe/comment bypass vectors where an exempt path in one argument
  would exempt the entire command including non-exempt paths

Includes 27 test cases covering: legitimate access, quoted paths,
path traversal, unicode bypass, pipe/comment bypass, mixed args.
This commit is contained in:
Viet Tran authored and GitHub committed 2026-04-06 13:18:02 +07:00
1 parent e88686b13a
commit 76385f2fc3
3 files changed
+278 -12

No files matched your search

+25 -11
View File
@@ -159,22 +159,36 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *Result {
// Check for dangerous commands (applies to both host and sandbox).
for _, pattern := range allPatterns {
if pattern.MatchString(normalizedCommand) {
// Check if any exemption applies (e.g. skills-store within .goclaw).
// Uses argument-level prefix matching to prevent bypass via comments
// (e.g. "echo pwned # .goclaw/skills-store/") while still allowing
// commands like "cat .goclaw/skills-store/tool.py".
// Check if exemption applies. Only exempt if EVERY field that
// individually matches the deny pattern is covered by an exemption.
// This prevents pipe/comment bypass: "cat /app/data/skills-store/x | cat /app/data/secret"
// — the second field matches deny but has no exemption → denied.
// Strips surrounding quotes (LLMs often quote paths) and rejects
// path traversal ("..") to prevent exemption escape.
exempt := false
trimmed := strings.TrimSpace(normalizedCommand)
for _, ex := range t.denyExemptions {
for _, field := range strings.Fields(trimmed) {
if strings.HasPrefix(field, ex) {
exempt = true
fields := strings.Fields(trimmed)
matchingFields := 0
exemptFields := 0
for _, field := range fields {
clean := strings.Trim(field, `"'`)
if !pattern.MatchString(clean) {
continue // field doesn't trigger this deny pattern
}
matchingFields++
if strings.Contains(clean, "..") {
continue // path traversal — never exempt
}
for _, ex := range t.denyExemptions {
if strings.HasPrefix(clean, ex) {
exemptFields++
break
}
}
if exempt {
break
}
}
// Exempt only if at least one field matched AND all matched fields are exempt.
if matchingFields > 0 && exemptFields == matchingFields {
exempt = true
}
if exempt {
continue