Skip to content

fix(security): close shell denylist bypass vectors#820

Closed
GoCeylan wants to merge 4 commits intosipeed:mainfrom
GoCeylan:fix/shell-denylist-bypasses
Closed

fix(security): close shell denylist bypass vectors#820
GoCeylan wants to merge 4 commits intosipeed:mainfrom
GoCeylan:fix/shell-denylist-bypasses

Conversation

@GoCeylan
Copy link
Copy Markdown
Contributor

@GoCeylan GoCeylan commented Feb 26, 2026

📝 Description

Fixes 5 bypass patterns in the shell command deny list that could allow prompt injection attacks to execute arbitrary commands on the device.

Bypasses closed:

  • Dot-sourcing: . evil.sh was not caught (only source keyword was blocked)
  • Shell by path: | /bin/bash bypassed | bash check
  • Source without .sh: source /etc/profile bypassed the .sh extension
    requirement
  • Here-string: bash <<< "cmd" was not caught
  • su -c: sudo alternative was not blocked

Adds 13 test cases covering each bypass vector.

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)

🤖 AI Code Generation

  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

Security audit finding, no existing issue.

📚 Technical Context (Skip for Docs)

  • Reference URL: N/A
  • Reasoning: The deny list is the primary guard against arbitrary shell execution triggered by prompt injection from external chat channels. Since the codebase is public, attackers can read the exact regex patterns and craft inputs that evade them.

🧪 Test Environment

  • Hardware: MacBook (Apple Silicon)
  • OS: macOS (Darwin 23.3.0)
  • Model/Provider: N/A
  • Channels: N/A

📸 Evidence (Optional)

13 new subtests in TestShellTool_DenylistBypasses covering each vector.
Run with: go test ./pkg/tools/ -run TestShellTool_DenylistBypasses -v

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these regular expression should be compiled in a list of text instead of changing the code all the time

@sipeed-bot sipeed-bot bot added type: bug Something isn't working domain: tool labels Mar 3, 2026
@blib
Copy link
Copy Markdown
Contributor

blib commented Mar 4, 2026

Regex guards are trivially bypassable. Variable indirection (x=rm; $x -rf /), command substitution (`echo rm` -rf /), quoting tricks, IFS manipulation, hex/octal encoding, and Unicode escapes all evade string-level pattern matching. The guard operates on surface syntax, not semantics.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@GoCeylan
Copy link
Copy Markdown
Contributor Author

GoCeylan commented Mar 6, 2026

Regex guards are trivially bypassable. Variable indirection (x=rm; $x -rf /), command substitution (`echo rm` -rf /), quoting tricks, IFS manipulation, hex/octal encoding, and Unicode escapes all evade string-level pattern matching. The guard operates on surface syntax, not semantics.

Regex guards are trivially bypassable. Variable indirection (x=rm; $x -rf /), command substitution (`echo rm` -rf /), quoting tricks, IFS manipulation, hex/octal encoding, and Unicode escapes all evade string-level pattern matching. The guard operates on surface syntax, not semantics.

Hey @blib you are technically correct, this solution is good as a stop gap. Your PR is the long-term fix, the tradeoff is complexity; it is 2500 lines and a new dependancy.

For now let's implement @sky5454 's suggestion of gathering to a txt file, then proceed with refining your PR.

@GoCeylan GoCeylan requested a review from sky5454 March 6, 2026 14:55
@GoCeylan
Copy link
Copy Markdown
Contributor Author

GoCeylan commented Mar 6, 2026

@sky5454 regex denylist now lives in a standalone text file.
All merge conflicts are resolved.

Copy link
Copy Markdown
Contributor

@sky5454 sky5454 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended that you include the parsing of TXT, which is helpful for CI

return fmt.Errorf("invalid allow pattern %q: %w", p, err)
}
t.allowPatterns = append(t.allowPatterns, re)
compiled, err := compileRegexPatterns(patterns)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This encapsulates the regular expression, and the error printing is moved to the compiler egexpatterns function. Good

@GoCeylan
Copy link
Copy Markdown
Contributor Author

GoCeylan commented Mar 8, 2026

@sky5454 Added TestDefaultDenyPatternsTxt in e98a491. It reads defaultDenyPatternsText (the embedded file), parses it via parsePatternLines, and compiles every pattern via compileRegexPatterns, failing fast if any regex is invalid. CI will now catch syntax errors introduced directly in default_deny_patterns.txt.

@GoCeylan GoCeylan requested a review from sky5454 March 8, 2026 11:14
@GoCeylan GoCeylan force-pushed the fix/shell-denylist-bypasses branch from 1767e30 to e98a491 Compare March 8, 2026 11:19
Copy link
Copy Markdown
Contributor

@sky5454 sky5454 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVIEWED OK

@sipeed-bot
Copy link
Copy Markdown

sipeed-bot bot commented Mar 25, 2026

@GoCeylan Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue.

@sipeed-bot sipeed-bot bot closed this Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: tool type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants