Conversation
$python_socket and $perl_socket in the built-in reverse_shell rule
(src/skillspector/yara_rules/malware.yar.b64) use `.*` between the
socket-family constant and the connect call with no dotall modifier.
YARA's `.` does not match `\n` by default, so the pattern only matches
when the whole snippet is on one physical line. A reverse shell
bundled as a script file (rather than passed to `python3 -c '...'` /
`perl -e '...'`) writes socket.socket(...) and .connect(...) as
separate statements and the rule never fires — reverse_shell falls
back to any of the other, unrelated strings in the same rule, so a
plain multi-line Python or Perl socket reverse shell with no other
telltale string is scored SAFE.
Add `s` (dotall) with a bounded {0,200} gap on both strings so the
match still requires the socket call and the connect call to be
close together, rather than opening an unbounded any-file-wide match
that would trade the false negative for a false positive.
Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Reviewed exact head 6a3cd51f18b09ac0b351c26135ac419ff869efeb. The encoded Python fixture is a real reverse shell and the bounded dot-all spans fix the multiline miss, but the actual YARA match stops at socket creation and connect; none of the fixture's later descriptor redirection or shell execution is required. Consequently an ordinary multiline Python TCP client is now classified as CRITICAL reverse_shell/YR1. The widened Perl expression is even broader—use Socket plus socket creation—and its changed behavior has no Perl regression in this PR.
Require bounded reverse-shell-specific evidence, add ordinary-client negative regressions and real-shell positives for both Python and Perl, and preserve the current encoded-fixture safety convention. PRs #593, #594, and #599 alter the same packaged signatures, so only one corrected implementation should land.
All six hosted checks pass and GitHub reports mergeable/clean, but the detector false positives and missing Perl coverage require changes before approval.
| is unambiguously a working reverse shell. | ||
| """ | ||
| findings = _run_builtin( | ||
| _multiline_python_socket_reverse_shell_fixture(), |
There was a problem hiding this comment.
[P0] This positive fixture contains shell behavior, but the revised regex succeeds before reaching any of it—an ordinary client ending after connect() would also pass. Add that benign client as a negative regression and make the packaged rule require bounded descriptor-redirection/shell evidence. Add equivalent positive and negative Perl coverage for the simultaneously changed Perl signature.
$python_socket and $perl_socket matched any multiline TCP client that reached connect() (or, for Perl, just imported Socket and called socket()), since neither required any evidence the code goes on to spawn a shell. Bound both strings to require descriptor-redirection or shell-exec evidence (dup2/subprocess/os.system/pty.spawn/execve for Python; open(STDIN,...)/exec( for Perl) within 200 chars after connect(), so an ordinary client no longer classifies as CRITICAL reverse_shell/YR1. Adds a benign-client negative test for each language plus a real multiline Perl reverse-shell positive, since the prior Perl string had no test coverage at all. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Re-reviewed exact head 095f3e51c6b1b2ec949ffda8c58495a16af3798e. The update resolves part of the prior request: plain send/receive clients are now negative controls, and Perl has both a real reverse-shell positive and an ordinary-client negative.
The required false-positive issue is not fully resolved. $python_socket accepts the bare token subprocess (or any os.system / execve) within 200 characters after any .connect(; it does not require descriptor redirection to that socket or a shell payload. An ordinary TCP client that invokes a benign helper subprocess immediately after connecting is therefore still classified as CRITICAL reverse_shell/YR1. $perl_socket has the same structural issue: either any open(STDIN or any exec( after connect is sufficient, rather than requiring redirection to the socket and/or an explicit shell command.
Please bind the corroborating evidence to reverse-shell behavior—for example, socket-backed standard-stream redirection plus a shell execution marker—and add negative regressions for clients that run an unrelated helper or reopen standard input after connecting. Keep the real Python and Perl positives and the encoded-fixture convention. PRs #593, #594, and #599 overlap, so only one corrected implementation should land.
Five hosted checks pass; test-unit is currently pending. The pending check is a merge gate, not the reason for this decision. Static review only; contributor code/tests were not executed locally.
… a bare token $python_socket accepted any of dup2/subprocess/os.system/pty.spawn/execve as alternatives, so a client that ran an unrelated helper subprocess right after connect() (no descriptor redirection to the socket, no shell) still matched. $perl_socket had the same shape: open(STDIN,...) or exec( alone was enough, so reopening stdin without ever exec'ing, or exec'ing a non-shell helper, also matched. Require both pieces of evidence together instead of either alone: dup2 redirecting a descriptor onto the socket followed by a shell-exec marker (subprocess.call/Popen/run, os.system, pty.spawn, or execve invoking something ending in sh) for Python; open(STDIN|STDOUT|STDERR,...) followed by exec(...) of a shell for Perl. Adds four negative regressions matching the false positives named in review: a Python client running an unrelated helper subprocess, a Python client that reopens stdin without a shell, and the same two shapes in Perl. The existing positive/negative fixtures for both languages still pass unchanged. Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
2 participants
Fixes #592
What was wrong
The built-in
reverse_shellrule's$python_socketand$perl_socketstrings (src/skillspector/yara_rules/malware.yar.b64, base64-packaged) use.*between the socket-family constant and the connect call with no dotall modifier:YARA's
.does not match\nby default, so both only match when the whole snippet is on one physical line — thepython3 -c '...'/perl -e '...'one-liner form. A reverse shell bundled as an actual script file writessocket.socket(...)and.connect(...)as separate statements, and neither string — nor any other string in the rule — matches it.static_yarareportscompleted, the scan stays SAFE,--fail-on-incompleteexits 0.Who reaches this / entry point: every
skillspector scaninvocation goes through the built-inreverse_shellYARA rule on every scanned artifact — this is the default, always-on detection path, not an opt-in flag. Triggered by: a skill bundle containing a Python (or Perl) source file with a raw socket-based reverse shell written as normal multi-line code, which is how this payload looks when it ships as a file rather than a one-liner.Fix
Add
s(dotall) to both strings, bounded with.{0,200}instead of unbounded.*, so the match still requires the socket call and the connect call to sit within roughly a couple of screens of each other rather than opening an any-file-wide match. Verified this bound does not introduce a false positive: an unrelatedsocket.socket(...)/.connect(...)pair 200 lines apart in the same file does not match.Confirmed the regex content is unchanged since the initial release (
7ced4fb) and untouched by the base64-repackaging commit (90a9181, #236) — that commit changed only the on-disk encoding, not any rule text.Testing
New test
test_reverse_shell_rule_matches_multiline_python_socketbuilds a realistic multi-line Python reverse shell (base64-encoded fixture, matching this test file's existing convention of not embedding raw malware-signature strings in the source, per the module docstring) and assertsreverse_shell/YR1fires.Negative control, reverting only
malware.yar.b64and keeping the new test:restoring the fix:
ruff check src/ tests/nodes/analyzers/test_static_yara.pyandruff format --check tests/nodes/analyzers/test_static_yara.py: all clean. (malware.yar.b64is not Python;ruff check src/ tests/in directory mode correctly skips it, matching how the repo's ownmake lintruns.)Note on the diff: because the rule file is base64-packaged, this two-line source change renders as a full-file diff in the
.b64blob. The decoded before/after above is the actual change; nothing else in the rule file moved.Impact: silent-wrong-result
Signed-off-by: Udaya Tejas udayatejas2004@gmail.com