fix(security): patch shell blocklist bypass and Android rm flag permutation#248
Merged
fix(security): patch shell blocklist bypass and Android rm flag permutation#248
Conversation
…tation Address three GitHub Security Advisories: - GHSA-hhjv-jq77-cmvx (HIGH): Replace naive literal substring blocklist in Android device_shell() with flag-set parsing that catches all rm flag permutations (rm -r -f, rm -fr, rm --recursive --force, etc.) - GHSA-5wp8-q9mx-8jx8 (CRITICAL): Fix four shell security bypasses: 1. Allowlist bypass via command injection — detect metacharacters (;, |, backtick, $()) before allowlist first-token check 2. Regex bypass via argument injection — patterns now match combined flags (python3 -Bc, perl -we, etc.) 3. Literal bypass via glob wildcards — strip bracket contents and convert ?, * to regex wildcards for matching 4. Empty allowlist in Strict mode now blocks all commands instead of silently allowing everything - GHSA-j8q9-r9pq-2hh9 (HIGH): Already fixed — IPv6-to-IPv4 transition address checks were present in web.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughReplaces brittle literal checks with glob-aware regex matching and stricter allowlist/metacharacter checks in Changes
Sequence Diagram(s)(None — changes are internal validation logic and do not introduce a multi-component sequential flow that benefits from a diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/android/actions.rs (1)
477-495:⚠️ Potential issue | 🟡 MinorPotential false positive with "format" substring.
The substring match for
"format"could trigger false positives on commands containing the word "format" in other contexts (e.g.,date +"%Y-%m-%d"with format-related options, or filenames likeformat_output.txt).Consider using word boundaries or a more targeted pattern if this becomes an issue in practice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/android/actions.rs` around lines 477 - 495, The current substring check in blocked_keywords can yield false positives for tokens like "format"; update the check to match whole-word or anchored patterns instead of simple contains: replace the simple array+contains logic (referencing blocked_keywords and the lower variable) with a small list of regex patterns (e.g., use word-boundary patterns like \bformat\b for plain words, and keep exact substring patterns such as "dd if=" or "mkfs" as-is) and test each against lower using the regex crate (or precompile Regexes) so only true dangerous tokens are blocked.
🧹 Nitpick comments (1)
src/tools/android/actions.rs (1)
465-475: Consider extracting recursive flag detection to reduce duplication.The recursive flag detection logic (
--recursiveor short flag containingr) is duplicated betweenis_rm_recursive_forceand this inline check. While acceptable, extracting a helper likehas_recursive_flag(tokens)would reduce redundancy.♻️ Optional refactor to extract helper
+fn has_recursive_flag(tokens: &[&str]) -> bool { + tokens.iter().skip(1).any(|t| { + *t == "--recursive" || (t.starts_with('-') && !t.starts_with("--") && t.contains('r')) + }) +} + fn is_rm_recursive_force(tokens: &[&str]) -> bool { if tokens.is_empty() || tokens[0] != "rm" { return false; } - let mut has_r = false; + let has_r = has_recursive_flag(tokens); let mut has_f = false; - for &tok in &tokens[1..] { - if tok.starts_with("--") { - match tok { - "--recursive" => has_r = true, - "--force" => has_f = true, - _ => {} - } - } else if tok.starts_with('-') && !tok.starts_with("--") { - let flags = &tok[1..]; - if flags.contains('r') { - has_r = true; - } - if flags.contains('f') { - has_f = true; - } + for &tok in &tokens[1..] { + if tok == "--force" || (tok.starts_with('-') && !tok.starts_with("--") && tok.contains('f')) { + has_f = true; } } has_r && has_f }Then use
has_recursive_flag(&lower_tokens)at line 467.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/android/actions.rs` around lines 465 - 475, Extract the duplicated recursive-flag detection into a helper function (e.g., has_recursive_flag) and replace the inline check in the rm block with a call to that helper; specifically, move the logic that checks for "--recursive" or a short flag containing 'r' (currently duplicated in is_rm_recursive_force and the inline rm check using lower_tokens) into has_recursive_flag(tokens: &[&str]) and then call has_recursive_flag(&lower_tokens) inside the if lower_tokens.first() == Some(&"rm") { ... } block and update is_rm_recursive_force to use the new helper as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/security/shell.rs`:
- Around line 375-378: The chaining detection boolean has_chaining_metachar
currently checks command_lower.chars().any(|c| matches!(c, ';' | '|' | '`' |
'\n')) || command_lower.contains("$(") but omits the ampersand, allowing
background/chained commands; update that expression to also treat '&' as a
metacharacter (e.g., include '&' in the matches! pattern or add a check for '&'
on command_lower) so has_chaining_metachar correctly flags commands like "cmd1 &
cmd2" that should be rejected.
---
Outside diff comments:
In `@src/tools/android/actions.rs`:
- Around line 477-495: The current substring check in blocked_keywords can yield
false positives for tokens like "format"; update the check to match whole-word
or anchored patterns instead of simple contains: replace the simple
array+contains logic (referencing blocked_keywords and the lower variable) with
a small list of regex patterns (e.g., use word-boundary patterns like \bformat\b
for plain words, and keep exact substring patterns such as "dd if=" or "mkfs"
as-is) and test each against lower using the regex crate (or precompile Regexes)
so only true dangerous tokens are blocked.
---
Nitpick comments:
In `@src/tools/android/actions.rs`:
- Around line 465-475: Extract the duplicated recursive-flag detection into a
helper function (e.g., has_recursive_flag) and replace the inline check in the
rm block with a call to that helper; specifically, move the logic that checks
for "--recursive" or a short flag containing 'r' (currently duplicated in
is_rm_recursive_force and the inline rm check using lower_tokens) into
has_recursive_flag(tokens: &[&str]) and then call
has_recursive_flag(&lower_tokens) inside the if lower_tokens.first() ==
Some(&"rm") { ... } block and update is_rm_recursive_force to use the new helper
as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d939371a-70ed-4d14-b6af-c69a637e3d13
📒 Files selected for processing (2)
src/security/shell.rssrc/tools/android/actions.rs
Addresses CodeRabbit review: `&` enables background execution and command chaining (e.g. `git status & python -c 'evil'`) which bypasses the first-token allowlist check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/security/shell.rs`:
- Around line 140-143: The inline-code regexes for python/perl/ruby/node
currently end with [\s=] which misses attached quoted forms like -e'...'; update
each pattern (the strings r"python[23]?\s+.*-[A-Za-z]*c[\s=]",
r"perl\s+.*-[A-Za-z]*e[\s=]", r"ruby\s+.*-[A-Za-z]*e[\s=]",
r"node\s+.*-[A-Za-z]*e[\s=]") to accept whitespace, equals, an immediate quote,
or end-of-string — e.g., replace [\s=] with (?:[\s=]|['"]|$) so patterns also
catch -e'code' and -e"code" forms.
- Around line 177-197: build_glob_regex currently strips '[' and ']' which
treats character classes like "[w0]" as literal text; change it to convert any
bracket class in command into a single-character wildcard by pushing '.' into
pat and skipping all characters until the matching ']' (handle an unterminated
'[' by treating it as literal '[' or as '.' plus the rest as appropriate), keep
existing escaping logic for other chars, and then anchor the resulting pattern
with '^' at the start and '$' at the end before calling Regex::new so matches
are for the whole path; update the function build_glob_regex to use pat,
command, and Regex::new accordingly and still return Option<Regex>.
Comment on lines
+140
to
+143
| r"python[23]?\s+.*-[A-Za-z]*c[\s=]", | ||
| r"perl\s+.*-[A-Za-z]*e[\s=]", | ||
| r"ruby\s+.*-[A-Za-z]*e[\s=]", | ||
| r"node\s+.*-[A-Za-z]*e[\s=]", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/security/shell.rs | head -200Repository: qhkm/zeptoclaw
Length of output: 8046
🏁 Script executed:
sed -n '130,160p' src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 1388
🏁 Script executed:
python3 << 'EOF'
import re
# Current patterns from the code
current_patterns = [
r"python[23]?\s+.*-[A-Za-z]*c[\s=]",
r"perl\s+.*-[A-Za-z]*e[\s=]",
r"ruby\s+.*-[A-Za-z]*e[\s=]",
r"node\s+.*-[A-Za-z]*e[\s=]",
]
# Suggested patterns from the review
suggested_patterns = [
r"python[23]?\s+.*-[A-Za-z]*c(?:\s|=|['\"]|$)",
r"perl\s+.*-[A-Za-z]*e(?:\s|=|['\"]|$)",
r"ruby\s+.*-[A-Za-z]*e(?:\s|=|['\"]|$)",
r"node\s+.*-[A-Za-z]*e(?:\s|=|['\"]|$)",
]
# Test cases
test_cases = [
"perl -e 'print 1'", # Space before quote
"perl -e='print 1'", # Equals before quote
"perl -e'print 1'", # Attached quote (NO space/equals)
"python3 -c 'print(1)'", # Space before quote
"python3 -c='print(1)'", # Equals before quote
"python3 -c'print(1)'", # Attached quote
"python3 -P -c'print(1)'", # Extra flags + attached quote
"ruby -e 'puts 1'", # Space before quote
"ruby -e'puts 1'", # Attached quote
"node -e 'console.log(1)'", # Space before quote
"node -e'console.log(1)'", # Attached quote
]
print("=== CURRENT PATTERNS ===")
for i, pattern in enumerate(current_patterns):
regex = re.compile(pattern)
print(f"\nPattern {i}: {pattern}")
for test in test_cases:
match = regex.search(test)
print(f" {'✓' if match else '✗'} {test}")
print("\n\n=== SUGGESTED PATTERNS ===")
for i, pattern in enumerate(suggested_patterns):
regex = re.compile(pattern)
print(f"\nPattern {i}: {pattern}")
for test in test_cases:
match = regex.search(test)
print(f" {'✓' if match else '✗'} {test}")
EOFRepository: qhkm/zeptoclaw
Length of output: 2617
Inline-code regexes miss attached quoted forms (e.g., perl -e'...').
The current patterns require [\s=] after -c/-e, allowing attached quote forms like perl -e'code' to bypass the filter. Update the patterns to include quote boundaries:
🔧 Suggested fix
- r"python[23]?\s+.*-[A-Za-z]*c[\s=]",
- r"perl\s+.*-[A-Za-z]*e[\s=]",
- r"ruby\s+.*-[A-Za-z]*e[\s=]",
- r"node\s+.*-[A-Za-z]*e[\s=]",
+ r"python[23]?\s+.*-[A-Za-z]*c(?:\s|=|['\"]|$)",
+ r"perl\s+.*-[A-Za-z]*e(?:\s|=|['\"]|$)",
+ r"ruby\s+.*-[A-Za-z]*e(?:\s|=|['\"]|$)",
+ r"node\s+.*-[A-Za-z]*e(?:\s|=|['\"]|$)",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/security/shell.rs` around lines 140 - 143, The inline-code regexes for
python/perl/ruby/node currently end with [\s=] which misses attached quoted
forms like -e'...'; update each pattern (the strings
r"python[23]?\s+.*-[A-Za-z]*c[\s=]", r"perl\s+.*-[A-Za-z]*e[\s=]",
r"ruby\s+.*-[A-Za-z]*e[\s=]", r"node\s+.*-[A-Za-z]*e[\s=]") to accept
whitespace, equals, an immediate quote, or end-of-string — e.g., replace [\s=]
with (?:[\s=]|['"]|$) so patterns also catch -e'code' and -e"code" forms.
Comment on lines
+177
to
+197
| /// Convert a command string that may contain shell glob characters into a regex | ||
| /// that can match the *literal* path the glob would expand to. | ||
| /// | ||
| /// `?` → `.` (any single char), `*` → `.*`, `[` / `]` stripped, | ||
| /// all other regex-special characters escaped. | ||
| fn build_glob_regex(command: &str) -> Option<Regex> { | ||
| let mut pat = String::with_capacity(command.len() + 16); | ||
| for ch in command.chars() { | ||
| match ch { | ||
| '?' => pat.push('.'), | ||
| '*' => pat.push_str(".*"), | ||
| '[' | ']' => {} // strip brackets (contents become literal) | ||
| c if ".+^${}()|\\".contains(c) => { | ||
| pat.push('\\'); | ||
| pat.push(c); | ||
| } | ||
| c => pat.push(c), | ||
| } | ||
| } | ||
| Regex::new(&pat).ok() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, get the file size to understand scope
wc -l src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 83
🏁 Script executed:
# Read the build_glob_regex function and surrounding context
sed -n '175,200p' src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 922
🏁 Script executed:
# Read the related code at lines 338-352
sed -n '336,354p' src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 1004
🏁 Script executed:
# Search for usage of build_glob_regex to understand the context
rg -n "build_glob_regex" src/security/shell.rs -A 2 -B 2Repository: qhkm/zeptoclaw
Length of output: 583
🏁 Script executed:
# Check if .zeptoclaw/config.json is in the blocklist
rg -n "zeptoclaw.*config|config.*json" src/security/shell.rsRepository: qhkm/zeptoclaw
Length of output: 450
Fix build_glob_regex to treat bracket classes as single-character wildcards.
The current implementation strips brackets entirely, allowing commands like /etc/pass[w0]d to evade the literal blocklist for /etc/passwd because the pattern w0 is treated as literal text rather than a char-class wildcard. Bracket classes [...] in glob patterns must be converted to . (match any single char) and anchored with ^$ for correct matching.
🔧 Suggested fix
fn build_glob_regex(command: &str) -> Option<Regex> {
let mut pat = String::with_capacity(command.len() + 16);
- for ch in command.chars() {
+ let mut chars = command.chars().peekable();
+ while let Some(ch) = chars.next() {
match ch {
'?' => pat.push('.'),
'*' => pat.push_str(".*"),
- '[' | ']' => {} // strip brackets (contents become literal)
+ '[' => {
+ // Treat any bracket class ([abc], [a-z], [!x]) as one-char wildcard.
+ let mut closed = false;
+ while let Some(next) = chars.next() {
+ if next == ']' {
+ closed = true;
+ break;
+ }
+ }
+ if closed {
+ pat.push('.');
+ } else {
+ pat.push_str(r"\[");
+ }
+ }
c if ".+^${}()|\\".contains(c) => {
pat.push('\\');
pat.push(c);
}
c => pat.push(c),
}
}
- Regex::new(&pat).ok()
+ Regex::new(&format!("^{}$", pat)).ok()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Convert a command string that may contain shell glob characters into a regex | |
| /// that can match the *literal* path the glob would expand to. | |
| /// | |
| /// `?` → `.` (any single char), `*` → `.*`, `[` / `]` stripped, | |
| /// all other regex-special characters escaped. | |
| fn build_glob_regex(command: &str) -> Option<Regex> { | |
| let mut pat = String::with_capacity(command.len() + 16); | |
| for ch in command.chars() { | |
| match ch { | |
| '?' => pat.push('.'), | |
| '*' => pat.push_str(".*"), | |
| '[' | ']' => {} // strip brackets (contents become literal) | |
| c if ".+^${}()|\\".contains(c) => { | |
| pat.push('\\'); | |
| pat.push(c); | |
| } | |
| c => pat.push(c), | |
| } | |
| } | |
| Regex::new(&pat).ok() | |
| } | |
| /// Convert a command string that may contain shell glob characters into a regex | |
| /// that can match the *literal* path the glob would expand to. | |
| /// | |
| /// `?` → `.` (any single char), `*` → `.*`, `[` / `]` stripped, | |
| /// all other regex-special characters escaped. | |
| fn build_glob_regex(command: &str) -> Option<Regex> { | |
| let mut pat = String::with_capacity(command.len() + 16); | |
| let mut chars = command.chars().peekable(); | |
| while let Some(ch) = chars.next() { | |
| match ch { | |
| '?' => pat.push('.'), | |
| '*' => pat.push_str(".*"), | |
| '[' => { | |
| // Treat any bracket class ([abc], [a-z], [!x]) as one-char wildcard. | |
| let mut closed = false; | |
| while let Some(next) = chars.next() { | |
| if next == ']' { | |
| closed = true; | |
| break; | |
| } | |
| } | |
| if closed { | |
| pat.push('.'); | |
| } else { | |
| pat.push_str(r"\["); | |
| } | |
| } | |
| c if ".+^${}()|\\".contains(c) => { | |
| pat.push('\\'); | |
| pat.push(c); | |
| } | |
| c => pat.push(c), | |
| } | |
| } | |
| Regex::new(&format!("^{}$", pat)).ok() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/security/shell.rs` around lines 177 - 197, build_glob_regex currently
strips '[' and ']' which treats character classes like "[w0]" as literal text;
change it to convert any bracket class in command into a single-character
wildcard by pushing '.' into pat and skipping all characters until the matching
']' (handle an unterminated '[' by treating it as literal '[' or as '.' plus the
rest as appropriate), keep existing escaping logic for other chars, and then
anchor the resulting pattern with '^' at the start and '$' at the end before
calling Regex::new so matches are for the whole path; update the function
build_glob_regex to use pat, command, and Regex::new accordingly and still
return Option<Regex>.
Contributor
|
Hello,
For the two adversaries,
https://github.com/qhkm/zeptoclaw/security/advisories/GHSA-5wp8-q9mx-8jx8
and
https://github.com/qhkm/zeptoclaw/security/advisories/GHSA-hhjv-jq77-cmvx,
can you help to request CVEs for them, many thanks!
Peng
qhkm ***@***.***> 于2026年3月5日周四 01:14写道：
… Merged #248 <#248> into main.
—
Reply to this email directly, view it on GitHub
<#248 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGLYUKMVRQZY24GSTQS3BT4PBQAXAVCNFSM6AAAAACWGRIQVCVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMRTGI3DONZVGYYDKNQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This was referenced Mar 5, 2026
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three GitHub Security Advisories reported by @zpbrent:
device_shell()blocklist bypass via argument permutation —rm -r -f,rm -fr,rm --recursive --forceall now caught by flag-set parsing instead of naive literal substring matching;,|,`,$()) now detected before allowlist checkpython/perl/ruby/node -c/-enow match combined flags (python3 -Bc,perl -we)?,*,[x]) in commands no longer bypass literal path blocklistTest plan
-P -c,-Bc,-w -e)/etc/pass[w]d,/etc/shado?,/etc/sh[a]dow)cargo clippy -- -D warningscleancargo fmt -- --checkclean🤖 Generated with Claude Code
Summary by CodeRabbit
Security Improvements
Tests