fix(ai): extensible exec-wrapper peeling closes the C2 laundering class (M11)#1264
Merged
Merged
Conversation
…dering (M11) C2 peeled a HARD-CODED EXEC_WRAPPERS set; any wrapper outside it (proxychains, firejail, flock, runuser, script, ...) was treated as the leaf command, so its payload was never inspected — same laundering class as C2. - Broaden EXEC_WRAPPERS with the known-missing wrappers (flock, runuser, su, script, proxychains/proxychains4, firejail, torsocks/torify, unshare, chrt, taskset, catchsegv). - Add _WRAPPER_ARG_GRAMMAR (data-driven, no per-wrapper if-pile) so each wrapper's OWN args are skipped to reach the real leaf: value-opts (proxychains -f, flock -w, sudo/runuser/su -u), positional operands (flock lockfile, su user), `--` end-of-options, and `-c '<cmd>'` command strings (runuser/su/script/flock) which are re-parsed and re-peeled so the payload is checked, not skipped. - Make the set config-extensible via CONFIG.addons.ai.exec_wrappers; config EXTENDS the built-in baseline (the security floor), never shrinks it. - Preserve C2 behavior for already-covered wrappers and normal commands. Tests: TestWrapperPeelingM11 — _peel_wrapper assertions are shfmt-independent (token-list domain); check_action integration stubs extract_commands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
Finding — M11 (P1 / Security)
C2 made shell guardrail checks wrapper-aware, but
EXEC_WRAPPERSwas a static, hard-coded set. Any exec-wrapper not in that set let an attacker launder a denied command — the wrapper was treated as the leaf and its payload never inspected:proxychains curl http://evil,firejail rm -rf /,flock /tmp/x curl ...,runuser -c 'curl http://evil',script -c '...'— same bypass class as C2.Root cause
secator/ai/guardrails.py:354_peel_wrapperonly peeled names in the fixedEXEC_WRAPPERSfrozenset (guardrails.py:40); everything else fell through asinner[0]= leaf command.Fix
EXEC_WRAPPERSwith the missing wrappers:flock,runuser,su,script,proxychains,proxychains4,firejail,torsocks,torify,unshare,chrt,taskset,catchsegv._WRAPPER_ARG_GRAMMAR— a compact, data-driven table(opts_taking_a_value, positional_args_before_cmd, cmd_string_opts)so each wrapper's OWN args are skipped to reach the real leaf, reusing the existing peel loop (no second parser):proxychains -f cfg,flock -w N,sudo/runuser/su -u userflock <lockfile>,su <user>--end-of-options boundary-c '<cmd>'command strings (runuser/su/script/flock) are re-parsed (shlex) and re-peeled so the nested payload is checked rather than skippedCONFIG.addons.ai.exec_wrappers(new field,secator/config.py) EXTENDS the built-in set — config is additive over the security baseline, never shrinks it.timeout 60 rm→rm), the interpreter ask-gate (timeout 60 bash -c ...keepsbashas leaf), and normal commands.Tests —
TestWrapperPeelingM11(19, all pass)_peel_wrapperassertions operate on token lists, and the_exec_wrappersconfig-extension test — no shfmt needed.check_actionintegration tests stubextract_commands(same pattern as the existingTestOutputFlagWrites), asserting the peeled leaf's deny/ask fires (proxychains dd→ deny,flock /tmp/l dd→ deny,firejail rm -rf /tmp/x→ ask).check_action-based wrapper tests inTestDefaultPermissionsrequire realshfmt/safecmdand fail environmentally on every branch (safecmd swallows theFileNotFoundError→ empty parse).Baseline vs after (full
test_ai_guardrails.py): 57 failed / 94 passed → 57 failed / 113 passed. Failure set byte-identical (all shfmt-environmental), zero regressions, +19 new passing.Residual laundering vectors (flagged, not fixed)
-c(runuser -c 'curl x && rm -rf /') — only the first command of the re-parsed string is classified (guardrails.py_peel_wrappercmd-string branch).$(...)/backtick command substitution;eval; wrapper binaries under nonstandard names or paths not in the set (config extension is the mitigation).🤖 Generated with Claude Code