fix(security): close rm flag-ordering bypasses#2718
Conversation
|
Hi @maintainers, I’ve addressed the issue related to rm recursive-force flag ordering bypass and added regression tests for the reported cases. Could you please review when you get a chance? Happy to make any changes if needed. |
There was a problem hiding this comment.
Pull request overview
This PR aims to close a security gap in the SDK’s defense-in-depth analyzers where destructive rm invocations could evade detection by reordering -r/-f flags or inserting other options between them.
Changes:
- Updated
rm -rfdetection regexes to allow intervening options and any flag ordering. - Extended policy-rail catastrophic-delete detection to use the updated
rmflag detection. - Added regression tests for intervening/split-flag variants.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
openhands-sdk/openhands/sdk/security/defense_in_depth/pattern.py |
Updates the HIGH-risk rm -rf pattern to detect re-ordered/interleaved flags. |
openhands-sdk/openhands/sdk/security/defense_in_depth/policy_rails.py |
Updates the catastrophic-delete rail’s rm recursive+force detection logic. |
tests/sdk/security/defense_in_depth/test_pattern.py |
Adds new HIGH test cases for intervening/split rm flags. |
tests/sdk/security/defense_in_depth/test_policy_rails.py |
Adds new catastrophic-delete rail tests for intervening/split rm flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| r"\brm\b(?:(?![;&|]).){0,120}" | ||
| r"(?:(?:--recursive\b|-[^\s-]*[rR][^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"|(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--recursive\b|-[^\s-]*[rR][^\s]*))", |
There was a problem hiding this comment.
The new rm -rf detector relies on multiple .{0,120} bounded spans. This still allows bypass by inserting a long positional argument / option value (e.g., a >120-char filename) between rm and -r/-f, or between the two flags, which GNU rm accepts and would evade this regex. Consider switching to a token-based scan (anchor on rm, then scan the rest of the same command segment for recursive+force flags) or otherwise remove the fixed-length dependency so detection can’t be defeated by long-but-valid arguments.
| r"\brm\b(?:(?![;&|]).){0,120}" | |
| r"(?:(?:--recursive\b|-[^\s-]*[rR][^\s]*)" | |
| r"(?:(?![;&|]).){0,120}(?:--force\b|-[^\s-]*f[^\s]*)" | |
| r"|(?:--force\b|-[^\s-]*f[^\s]*)" | |
| r"(?:(?![;&|]).){0,120}(?:--recursive\b|-[^\s-]*[rR][^\s]*))", | |
| r"\brm\b" | |
| r"(?=[^;&|]*(?:--recursive\b|-[^\s-]*[rR][^\s]*))" | |
| r"(?=[^;&|]*(?:--force\b|-[^\s-]*f[^\s]*))" | |
| r"[^;&|]*", |
| r"\brm\b(?:(?![;&|]).){0,120}" | ||
| r"(?:(?:--recursive\b|-[^\s-]*[rR][^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"|(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--recursive\b|-[^\s-]*[rR][^\s]*))", |
There was a problem hiding this comment.
The (?![;&|]) tempered-dot treats any & as a command boundary, but & is also valid inside redirections (e.g., 2>&1, &>/dev/null). An attacker can place such a redirection between -r and -f (e.g., rm -r 2>&1 -f /) and bypass detection while the command remains a single rm invocation. Consider narrowing the boundary logic to actual shell operators (;, |, &&, background &) rather than any raw & character, or use a small shell-operator tokenizer for the segment.
| has_recursive_force = bool( | ||
| re.search( | ||
| r"\brm\s+(?:-[frR]{2,}|-[rR]\s+-f|-f\s+-[rR]" | ||
| r"|--recursive\s+--force|--force\s+--recursive)\b", | ||
| r"\brm\b(?:(?![;&|]).){0,120}" | ||
| r"(?:(?:--recursive\b|-[^\s-]*[rR][^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"|(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--recursive\b|-[^\s-]*[rR][^\s]*))", | ||
| seg, | ||
| ci, | ||
| ) |
There was a problem hiding this comment.
This same complex rm -rf regex is duplicated here and in pattern.py. With the added complexity (tempered dots, ordering alternation), keeping two copies increases the risk of future drift/fixes being applied in only one analyzer. Consider factoring the rm recursive+force detection into a shared helper/constant used by both analyzers.
| re.search( | ||
| r"\brm\s+(?:-[frR]{2,}|-[rR]\s+-f|-f\s+-[rR]" | ||
| r"|--recursive\s+--force|--force\s+--recursive)\b", | ||
| r"\brm\b(?:(?![;&|]).){0,120}" | ||
| r"(?:(?:--recursive\b|-[^\s-]*[rR][^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"|(?:--force\b|-[^\s-]*f[^\s]*)" | ||
| r"(?:(?![;&|]).){0,120}(?:--recursive\b|-[^\s-]*[rR][^\s]*))", | ||
| seg, | ||
| ci, | ||
| ) |
There was a problem hiding this comment.
Like pattern.py, this rm -rf detector depends on fixed .{0,120} windows. A long-but-valid argument between rm and -r/-f (or between the flags) can exceed the bound and prevent the catastrophic-delete rail from firing. Since this rail gates RAIL_CATASTROPHIC_DELETE, consider a token-based scan within the segment (up to the actual command operator boundary) so the rail can’t be bypassed by argument length.
|
|
||
|
|
There was a problem hiding this comment.
The PR description/linked issue call out “post-argument flags” bypasses (e.g., rm / -rf, rm /etc -r -f), but the new regression tests added here don’t cover those cases. Adding explicit cases for flags appearing after positional arguments (and ideally a redirection-interleaving case like rm -r 2>&1 -f /) would better ensure the bypass class is closed.
| def test_catastrophic_delete_root_with_post_argument_flags(self): | |
| decision = _evaluate_rail("rm / -rf") | |
| assert decision.outcome == SecurityRisk.HIGH | |
| assert decision.rule_name == RAIL_CATASTROPHIC_DELETE | |
| def test_catastrophic_delete_non_root_target_with_post_argument_flags(self): | |
| decision = _evaluate_rail("rm /etc -r -f") | |
| assert decision.outcome == SecurityRisk.HIGH | |
| assert decision.rule_name == RAIL_CATASTROPHIC_DELETE | |
| def test_catastrophic_delete_with_redirection_interleaved_between_flags(self): | |
| decision = _evaluate_rail("rm -r 2>&1 -f /") | |
| assert decision.outcome == SecurityRisk.HIGH | |
| assert decision.rule_name == RAIL_CATASTROPHIC_DELETE |
| _HIGH_CASES = [ | ||
| ("rm -rf /", "rm -rf"), | ||
| ("rm -fr /home", "rm -fr"), | ||
| ("rm -r -f /tmp", "rm -r -f"), | ||
| ("rm --no-preserve-root -rf /", "rm with intervening long flag"), | ||
| ("rm --preserve-root=all -r -f /tmp", "rm split flags with long option"), | ||
| ("rm --recursive --force /", "rm --recursive --force"), | ||
| ("sudo rm secret.db", "sudo rm"), |
There was a problem hiding this comment.
New HIGH cases cover intervening long options, but the reported “post-argument flags” bypasses (e.g., rm / -rf, rm /etc -r -f) still aren’t represented in this parametrized list. Adding those commands (and a redirection-interleaving variant like rm -r 2>&1 -f /) would make the regression suite align with the PR description and ensure ordering/position invariance holds.
|
Thanks for the detailed review. I see the concerns around the bounded regex (.{0,120}) allowing bypass via long arguments, as well as edge cases like redirection interleaving and post-argument flags. I’ll rework the detection to remove fixed-length constraints and move toward a more robust approach, along with adding the suggested regression tests and reducing duplication. Pushing an update shortly. |
|
Addressed the review feedback:
Workflows are awaiting approval—once approved, CI should run. Would appreciate a re-review. |
Close flag-ordering bypasses by matching rm recursive and force flags in any order with intervening options, and add regression tests for the reported evasion cases. Made-with: Cursor
Replace bounded rm regex matching with shared token-based shell segment parsing, reuse it across pattern and policy analyzers, and add regression tests for reordered flags, redirections, long arguments, and substring safety. Made-with: Cursor
kushalsai-01
force-pushed
the
fix/security-rm-flag-ordering-bypass
branch
from
bbcbbd9 to
594a039
Compare
|
Hi @maintainers — could you please approve workflows so CI can run? I’ve rebased onto the latest main. Thanks. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @kushalsai-01, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
[Automatic Post]: This PR seems to be currently waiting for review. @copilot-pull-request-reviewer[bot], could you please take a look when you have a chance? |
|
@kushalsai-01, thanks for picking this up. As the issue author, the scope in #2707 is met: your changes catch intervening flags ( A few things I liked specifically:
One note for reviewers: I support the tree-sitter direction in #2721, but that's a cross-package migration on its own timeline. This PR solves a real bypass class now and is merge-shaped and clean. Your CI hasn't been able to run yet because workflows need maintainer approval for first-time contributors. @enyst, could you approve the workflow runs on this so we get a CI signal? |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @kushalsai-01, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
3 similar comments
|
[Automatic Post]: It has been a while since there was any activity on this PR. @kushalsai-01, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @kushalsai-01, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
[Automatic Post]: It has been a while since there was any activity on this PR. @kushalsai-01, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
|
@kushalsai-01, quick context update. The maintainers are moving the broader shell-parsing layer to tree-sitter-bash (issue #2721, phased plan in VascoSch92's comment). Phase 1 (PR #3237) is already open and drops the GPLv3 bashlex dependency. Phase 2 migrates the security analyzer onto the shared parser. That doesn't make this PR obsolete. It makes it the demonstration that motivated the bigger shift. Showing the bypass class was real and that regex couldn't close it is the work that put a structural parser on the table. The shlex direction was the right call for the surface available at the time, and the negative tests ( If you're interested in helping with the migration, Vasco split Phase 2 into focused chunks (2a-2f). The pieces that overlap with what you built here are Phase 2c ( Thanks for picking this up. Your PR is the reason the discussion moved past "another regex tweak" to "structural fix." |
Fixes #2707
Summary
This PR closes a security gap where
rmcommands could bypass safeguards by reordering-rand-fflags or inserting additional options between them.The validation logic now correctly detects recursive and force flags regardless of ordering or intervening arguments, preventing evasion of the safety checks.
Changes
rmflag combinations independent of orderTesting
rm -rf,rm -frrm -r --verbose -f)Notes
This change is backward-compatible and only affects validation logic for potentially destructive
rmcommands.Checklist