Repository navigation
fix(yara): make reverse-shell socket strings span newlines - #593
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.
$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>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Reviewed head f106e999d29fc52f8a8219dc9fa0c42d472ed26d, including the decoded packaged rules, all added fixtures, and the analyzer integration. The new positives and ordinary-client negatives pass, but additional base-versus-head cases reproduce lost Python/Perl reverse-shell detections and two remaining false-positive mechanisms, detailed in the four inline comments.
Validation: all 94 tests in tests/nodes/analyzers/test_static_yara.py pass with Python 3.12.13 / yara-python 4.5.4; Ruff lint and formatting checks pass; all six hosted checks passed. I also compared the additional cases against both decoded rule versions and the head analyzer with all built-in rules. Fixture source was scanned as text; reverse-shell payloads were not executed. Changes are still needed to preserve supported malicious cases and avoid the demonstrated benign-client classifications.
…-name and descriptor binding The prior fix's alternation let any ONE of dup2/subprocess/os.system/ pty.spawn/execve satisfy the Python shell-exec marker, and matched os.execve but not the equally common os.execl. It also required an unqualified dup2( anywhere nearby rather than binding the descriptor redirection to fd 0 (stdin) or to the socket, so dup2'ing an unrelated local log file onto stdout still counted as corroborating evidence for a shell that was never wired to the connection. The Perl string required parentheses around open/exec, which Perl does not require. Both languages matched any \w*sh token, so a non-shell helper like sha256sum satisfied the shell-execution requirement on its `sh` prefix alone. Rework $python_socket so descriptor redirection must land on fd 0 before a shell-exec marker is accepted, add the equivalent stdin=/stdout=/stderr=.fileno() keyword-redirection form used by subprocess.call/Popen/run, recognize os.execl alongside os.execve, and allow pty.spawn's list argument the same as subprocess's. Rework $perl_socket's open/exec calls to make the parentheses optional. Bind the shell name in both to a closed set (sh, bash, dash, ash, zsh, ksh, csh) with a trailing word boundary so a prefix match no longer qualifies. Adds seven regressions matching the shapes named in review: three previously-undetected Python reverse-shell forms (execl, pty.spawn with a list, and stdin=/stdout=/stderr= keyword redirection with no dup2 at all), a parenthesis-free Perl positive, and three false-positive negatives (a non-shell helper for each language, and dup2 onto an unrelated local log file). Existing fixtures for both languages still pass unchanged. 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 1c85f9189c18a69611a75e24799a891ed26bda48, including the complete two-file diff, the decoded packaged rules, every added fixture/assertion, prior reviews/threads, and YR1 integration. The ordinary-client controls, encoded Perl coverage, Python direct stdin= redirection, pty.spawn/os.execl, and Perl optional-parenthesis findings are addressed for the reported cases. The exact sha256sum and stdout-logging examples are also covered. Two underlying precision requirements remain only partially addressed.
[P1] Require socket-backed redirection, not just descriptor zero or the STDIN name. The decoded $python_socket (decoded line 20) now requires dup2(...fileno(), 0), but still accepts any file object's descriptor; the new direct-stream branch likewise accepts stdin=any_name.fileno(). Consider an ordinary client that connects, closes s, then runs commands = open("local.commands"); os.dup2(commands.fileno(), 0); subprocess.run(["/bin/sh"]). All gaps can be kept below the current bounds, so the signature accepts local command-file execution as a CRITICAL reverse shell even though the closed network socket is not connected to the shell. The Perl expression (decoded line 21) remains broader: open(STDIN, "<", "local.commands"); exec("/bin/sh") after a closed client matches because nothing checks the open operands. Please bind the corroborating input redirection to the connected socket, or otherwise avoid assigning this high-confidence reverse-shell verdict to unproven relationships, and add both local-input and local-output negatives. This continues the existing unrelated-redirection finding; changing fd 1 to fd 0 fixes that particular fixture, not the trust relationship.
[P2] A word boundary is not a complete executable-token boundary. The new shell alternatives end in \b, so subprocess.run(["sh-helper"]) or Perl exec("sh-helper") still satisfies the sh alternative when the preceding socket/redirection shape is present. The hyphen is a non-word character, so this is another non-shell helper false positive. Require the appropriate end of the executable argument/command token, while retaining spaces that introduce legitimate shell arguments. Extend the existing shell-prefix regression with hyphenated helper names in both languages.
All six exact-head hosted checks pass. These remaining cases are source-traced from the decoded expressions and are not covered by the new tests; contributor code/tests were not executed locally. #593, #594, and #599 overlap on the same signatures, so only one corrected implementation should ultimately land.
… local descriptor $python_socket accepted dup2(...) or stdin=.fileno() redirection from ANY file object's descriptor, not specifically the connected socket's. A client that connects, closes the socket, then opens an unrelated local file and dup2's or stdin=-wires *that* descriptor onto a shell still matched as a CRITICAL reverse shell, even though the shell's input channel is never wired to the network. $perl_socket had the analogous gap: it accepted STDIN/STDOUT/STDERR reopened from a literal local file path, not just the socket-duplication idiom (open(STDIN, "<&SOCKET")) real Perl reverse shells use. YARA's regex engine has no backreferences or lookaround, so the redirection cannot be bound to the same variable as the socket by regex alone. Instead: reject $python_socket matches whose span contains a local `= open(` assignment (the shape every local-file redirection needs to acquire a competing descriptor), and require Perl's STDIN/STDOUT/STDERR reopen to use the `&` fd-duplication form that socket redirection actually uses, rather than accepting any open(STD...) call. Also close a second, narrower gap in both: the shell-name alternatives ended in a bare \b word boundary, so a non-shell helper with `sh` as a prefix followed by a non-word character (`sh-helper`, alongside the already-fixed `sha256sum`) still satisfied the shell-execution requirement. Require the shell name be followed by a real token terminator (whitespace, quote, bracket, paren, comma, semicolon, or backslash) instead. Adds six regressions: local-file dup2 and stdin= redirection for Python (mirroring the exact PoCs from review), a non-shell hyphenated helper for each language, and local-file STDIN/STDOUT reopens for Perl. All fail against the prior rule and pass against this one; existing fixtures for both languages are unchanged. 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]
Hi @udsy19, thank you for your contribution to SkillSpector and for the follow-up updates — we really appreciate the time you put into this! A few items need attention before it can be merged; details below.
Re-reviewed exact head 17670f43f357432d7e1d9b834245fe3e18b07bff. Since 1c85f918, the head changes the shell-name terminator, adds a Perl &-dup requirement, and adds a $python_local_open exclusion to the reverse_shell condition. I compared the decoded base, previous and head malware.yar and read all 21 encoded fixtures and the YR1 reporting path in static_yara.py. I checked rule behavior by compiling the decoded rules with yara-python 4.5.4 and scanning my own text samples; contributor code and tests were not run. Every fixture behaves as asserted, and the shell-token boundary is fixed. The new exclusion, however, causes detection misses and misreported locations, and the socket-binding request is still open.
Previous findings
- Ordinary multiline Python/Perl clients reported as CRITICAL, plus missing Perl coverage and the encoded-fixture convention (
6a3cd51review and test-file thread): Resolved. - Bare
subprocess/os.system/execvetoken, and bare Perlopen(STDIN/exec(, accepted afterconnect(095f3e5): Resolved. - [P1] Direct
stdin=s.fileno()redirection,pty.spawn([...]),os.execl: Resolved. - [P1] Perl optional parentheses: Resolved. Single-line and multiline forms match; only the multiline form is tested.
- [P2]
sha256sumprefix match: Resolved. - [P2] Unrelated local-logging
dup2: Resolved (fd 0 is now required). - [P2]
\bis not a complete executable-token boundary (1c85f918): Resolved. The[\s"'\]),;\\]terminator is in place, and thesh-helpernegatives fail on1c85f918and pass here. - [P1] Bind the redirection to the connected socket (
1c85f918): Still open (finding 3). The Python change made for it also causes findings 1 and 2.
Findings
-
[Blocking] Any
= open(in a match span cancels a genuine reverse-shell detection. In the decoded rule (lines 27–38),$python_socketis dropped whenever/=\s*open\s*\(/falls inside its span. Two things make this worse:- The condition uses
for any i, so one tainted span suppresses every clean match in the file. - The spans are greedy, so they extend to later shell-like calls.
Each case below is detected at
1c85f918and missed at head. Removing only theopenline restores detection.- (a) A shell that reads a file between
connectanddup2, e.g.key = open(os.path.expanduser("~/.ssh/id_rsa")).read(). - (b) A probe socket followed by
host = open("/etc/hostname").read(), then a clean secondsocket/dup2/subprocess.call(["/bin/sh", "-i"]). - (c) A clean shell followed by a cleanup
marker = open(...)andsubprocess.call(["sh", "-c", ...]).
A single
_ = open(__file__)afterconnectdefeats this CRITICAL signature. Please remove the negative exclusion; nothing should be able to cancel positive socket-plus-shell evidence. Add positive regressions for (a)–(c). - The condition uses
-
[Blocking] The helper string corrupts YR1 location, snippet and fingerprint.
$python_local_openis an ordinary string of the rule. Wheneverreverse_shellfires, through any string, every= open(in the file becomes a match instance._extract_match_strings(static_yara.py:445-464) reports the minimum offset across all instances, and_match_instances_fingerprinthashes all of them. Two verified cases:- A
bash -i >& /dev/tcp/...payload on line 6, withsettings = open("settings.ini")on line 2, is reported at line 2 on head (line 6 on main), with= open(inmatched_text. - A real multiline Python shell preceded by
cfg = open(...)is reported at theopenline, not the socket line.
This skews JSON/SARIF locations and makes
match_fingerprintchange when unrelated lines are edited. Removing the helper string (finding 1) fixes this too. - A
-
[Blocking] Still open: the redirection is not bound to the connected socket. Idiomatic variants of the previously reported local-command case still produce CRITICAL
reverse_shell/YR1 after the socket is closed:- Python
with open("local.commands") as commands:followed byos.dup2(commands.fileno(), 0)andsubprocess.run(["/bin/sh"]). - Python
subprocess.call(["/bin/sh"], stdin=commands.fileno()), orcommands = io.open(...). - Perl
open(CMDS, "<", "local.commands"); open(STDIN, "<&CMDS"); exec("/bin/sh"). - Perl three-argument
open(STDIN, "<&", $fh).
The
&requirement checks dup syntax, not that the source is the socket. YARA regexes cannot bind identifiers (no backreferences). If positive binding isn't feasible, please take the alternative from the earlier review: report the unbound multiline heuristic from a separate rule with lowerseverity/confidencemeta (_parse_metahonors both), not CRITICAL 0.85. Add these negatives. - Python
-
[Blocking] Single-line reverse shells that main detects now regress. The new required evidence is
dup2(<x>.fileno(), 0)orstdin=<x>.fileno(), plus exact spellings of the shell call. Each one-liner below is detected by main's single-line strings and missed at head (and at1c85f918). The Python ones all havesocket.socket(socket.AF_INET,socket.SOCK_STREAM)and.connect(on the same line.[os.dup2(s.fileno(),fd) for fd in (0,1,2)]os.dup2(s.fileno(),sys.stdin.fileno())subprocess.call(["/bin/sh","-i"],stdin=s,stdout=s,stderr=s)import subprocess as sp…sp.call([...])pty.spawn("/usr/bin/bash")- Perl
socket(SOCK,…)one-liners ending insystem("/bin/sh -i")orexec("/usr/bin/bash -i")
The new multiline support also misses the common
for fd in (0, 1, 2): os.dup2(s.fileno(), fd)loop.One way to rule out any one-liner regression: keep main's single-line
$python_socket/$perl_socketunchanged and add separate bounded multiline strings for the stricter evidence (your multiline negatives stay negative). Otherwise, accept these descriptor and exec forms. Add regressions either way. -
[Non-blocking] Stale PR description. It still describes the original two-line dot-all change ("87 passed"). Please describe the final rule (required redirection and shell evidence, rewritten condition) so the squash commit message is accurate.
-
[Non-blocking, pre-existing]
socket\s*\(\s*SOCKrequires the Perl handle name to start withSOCK. Because of this, the widely published one-linersocket(S,PF_INET,SOCK_STREAM,…)…open(STDIN,">&S")…exec("/bin/sh -i")is missed on both main and head. Since this PR rewrites the expression, consider fixing it here; a follow-up is also fine.
Merge state and overlap: A rebase is needed. The only conflict is in tests/nodes/analyzers/test_static_yara.py: #489 (now on main) inserts _WEBSHELL_FIXTURES/_webshell_fixture at the same spot after _reverse_shell_fixture(), and keeping both blocks resolves it. #489 changed webshells.yar, not malware.yar.b64. The decoded rule merges cleanly and has no interaction with php_webshell_known. #594 and #599 rewrite the same two signatures (both also have changes requested), so only one implementation should land. #608 also re-encodes malware.yar.b64, so whichever of the two lands second must decode, re-apply and re-encode rather than merge base64 text.
Tests/CI: DCO, lint, changes, docker-smoke and OpenCode TypeScript Tests pass. test-unit is still in progress and is not the basis of this decision. All 21 fixtures behave as asserted against the decoded head rule, and the six new negatives fail on 1c85f918, so they are meaningful. Findings 1–4 have no coverage.
Decision: Changes Requested (reviewed head 17670f43f357432d7e1d9b834245fe3e18b07bff)
…ell-multiline Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com> # Conflicts: # tests/nodes/analyzers/test_static_yara.py
…l FPs and restore lost shells Addresses @rng1995's blocking review of the reverse_shell rewrite. The single greedy dot-all rule both over-matched (unbound local redirection scored CRITICAL) and under-matched (the $python_local_open negative exclusion cancelled real shells and skewed finding locations; stricter multiline evidence regressed main's single-line detections). Split detection into two rules: - reverse_shell (CRITICAL, 0.85): restored to main's exact single-line strings and `any of them` condition. Every single-line shell main detects stays CRITICAL (dup2 list-comprehension, sys.stdin.fileno(), stdin=<socket>, subprocess alias, pty.spawn, perl system/exec one- liners), with no new behavior and no negative-exclusion string. - reverse_shell_multiline (HIGH, 0.6): bounded dot-all evidence requiring socket creation + connect, descriptor redirection onto stdin (dup2 to fd 0, the (0,1,2) loop/comprehension forms, or stdin=<x>.fileno()), and a shell-execution marker. Catches real multiline shells (dup2+subprocess /execl/pty.spawn, fileno kwargs, perl open-dup + exec incl. no-paren and three-arg forms). Because YARA cannot bind the redirected descriptor to the connected socket, this is reported at reduced severity/confidence rather than as a certain reverse shell. Point-by-point vs. the review: - [P2] sha256sum / sh-helper prefix: shell token ends in a delimiter class that excludes letters and '-', so prefixes no longer match. - [P1] dup2-less forms (direct stdin= redirection, execl, pty.spawn) and Perl optional-parenthesis / no-paren open+exec: all detected. - [P2] unrelated local logging (dup2 to fd 1): requires fd 0, stays negative. - Finding 1 & 2: removed the $python_local_open exclusion entirely, so a stray `= open(` no longer cancels a real shell or corrupts the finding's reported line/snippet/fingerprint. - Finding 3: unbound local-redirection lookalikes (with open(...), perl <&CMDS and three-arg <&) now report at reduced severity, never CRITICAL. - Finding 4: single-line shells restored at CRITICAL; multiline dup2 loop detected by the heuristic rule. Adds positive regressions (file read before dup2, probe socket + clean shell, trailing cleanup open, dup2 loop), reduced-severity negatives (with open, perl local/three-arg dup), a location-integrity regression, and single-line CRITICAL coverage for both languages. Rebased onto main; kept both the reverse-shell and webshell fixture blocks. 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]
Hi @udsy19, thank you for reworking this again and for splitting it into two rules! That is the cleanest answer to the socket-binding problem so far.
Value and readiness: The head leaves main's single-line $python_socket/$perl_socket strings byte-identical. It adds a separate reverse_shell_multiline rule (HIGH, confidence 0.6) for multiline shells. #592 is now detected, and last round's single-line regressions, = open( cancellation and location skew are gone. Of the three competing PRs (#593, #594, #599), this one is clearly the strongest candidate:
- widest coverage of real variants;
- no regression against main;
- no analyzer code changes;
- smallest worst-case scan-time overhead (comparison below).
Two small test fixes remain, plus one severity decision for the PIC. After that it is ready for final maintainer/PIC review.
Previous findings (review at 17670f43)
- [Blocking] Any
= open(in the match span cancels detection: Resolved.$python_local_openand thenot (...)clause are gone. The read-before-dup2, probe-socket and trailing-cleanup cases are now positive regressions, and all three match at head. - [Blocking] The helper string skews YR1 location, snippet and fingerprint: Resolved. No helper string remains.
test_reverse_shell_location_not_pulled_to_unrelated_open_linepins the payload line. - [Blocking] Redirection is not bound to the socket: Resolved, using the alternative suggested last round. Unbound lookalikes (
with open(...), Perl<&CMDS, three-argument<&) now get only the HIGH/0.6 heuristic, never CRITICAL. - [Blocking] Single-line regressions: Resolved. Main's strings are unchanged. These one-liners all stay CRITICAL: list-comprehension
dup2,sys.stdin.fileno(),stdin=s,sp.call,pty.spawn("/usr/bin/bash")and Perlsystem(...). The multilinefor fd in (0, 1, 2)loop is now detected. - [Non-blocking] Stale PR description: Resolved.
- [Non-blocking, pre-existing] The Perl handle must start with
SOCK: Resolved at the heuristic tier.$pl_sockaccepts any handle, so the classicsocket(S,...)shell is now reported (HIGH). The CRITICAL single-line string still requiresSOCK, as on main.
Material findings
-
[Blocker]
tests/nodes/analyzers/test_static_yara.py:548-597:_single_line_python_socket_shells()and_single_line_perl_socket_shell()are plain text. Several Python entries are complete reverse-shell one-liners: socket, connect, stdio redirection to the socket, and/bin/sh -i. The module docstring and every other signature-triggering fixture in this file use base64 so that Defender and similar scanners do not quarantine the test file. The PR description also says "Fixtures stay base64-encoded". Please encode these the same way as_multiline_python_socket_reverse_shell_fixture. -
[Blocker]
tests/nodes/analyzers/test_static_yara.py:602-611: no test checks the new rule's main promise: HIGH, 0.6, never CRITICAL._has_critical_reverse_shellskips any message that containsreverse_shell_multiline. If that rule's meta were changed to CRITICAL/0.85, all five*_not_criticaltests would still pass.- The multiline positives assert
_has_rule(findings, "reverse_shell").reverse_shell_multilinealso satisfies that substring check, so the tests do not show which rule fired.
Please add assertions for at least one Python and one Perl multiline positive, and for the unbound lookalikes: exactly one
reverse_shell_multilineYR1 finding withseverity == "HIGH"andconfidence == 0.6, and noreverse_shellfinding. -
[Non-blocking] Several test docstrings still describe the abandoned single-rule design:
test_reverse_shell_rule_matches_multiline_python_socket(line 1112) says dot-all on$python_socketmakes it pass.test_reverse_shell_rule_does_not_match_benign_python_socket_client(line 1131) says$python_socketmust requireos.dup2plus a shell.- The Perl benign-client test says
$perl_socketused to match anyuse Socketscript "before this fix". Its single-line behaviour has not changed. - Several fixture docstrings ("the prior fix's alternation", "must NOT match
$python_socket").
Please update them so maintainers are not misled about which rule does what.
-
[Non-blocking] One-liners get a duplicate finding. A single-line shell that main already reports also satisfies the three multiline strings. The report then shows a CRITICAL
reverse_shelland a HIGHreverse_shell_multilineYR1 finding on the same line (reproduced with main's pipeline plus this rule).deduplicatekeeps both because the messages differ. Addingand not reverse_shellto the multiline condition removes the duplicate, and the file is still flagged by the CRITICAL rule. -
[Non-blocking] Multiline coverage gaps. Main misses these too, so none is a regression:
- Perl
system("/bin/sh -i"):$pl_shellonly acceptsexec. - Python
stdin=s, stdout=s, stderr=s, which passes the socket object itself. - Python
os.dup2(s.fileno(), sys.stdin.fileno()).
Consider adding these alternatives, each with a positive regression.
- Perl
-
[Non-blocking] The three strings are ANDed across the whole file with no proximity requirement, so unrelated code can combine into a HIGH finding. Two benign examples:
- A module with a small TCP
ping()helper and, about 4 KB later,subprocess.run(["bash"], stdin=script.fileno())for a local install script. - A Perl client that uses the common
open(STDERR, ">&STDOUT")idiom and ends withexec("/bin/sh", "-c", "make deploy").
Optional tightening: bound the offset between
$py_sockand the redirection/shell matches, and exclude&STD...duplication from$pl_redir. - A module with a small TCP
PIC tradeoffs: Is HIGH/0.6 the right severity for a detected multiline reverse shell? I ran main's static pipeline (--no-llm) on the #592 repro packaged as a skill:
| Rule version | Score | Recommendation |
|---|---|---|
| main | 16 | SAFE |
| main + this PR's rule | 35 | CAUTION |
| CRITICAL/0.85 finding (as in #594/#599) | 71 | DO_NOT_INSTALL |
So a working reverse shell gets CAUTION, not DO_NOT_INSTALL, unless other evidence raises the score. The reduced severity is also what keeps unbound lookalikes (finding 6, the with open(...) cases) from becoming DO_NOT_INSTALL. Please confirm CAUTION is acceptable. If it is not, one middle ground is a CRITICAL rule for a stricter, ordered and bounded pattern, keeping this heuristic as the fallback.
Verification and gaps:
- Rule diff: I decoded
malware.yarat main,17670f43and head. The head only adds the new rule (decoded lines 31-49), leavesreverse_shellbyte-identical to main, and keeps every line CRLF. - Merge commit: the upstream merge (
88cbea1) brought in nothing else. Against current main, the PR still touches only its two files. - Fixtures: I compiled the decoded rules with yara-python 4.5.4 (no new compiler warnings). I extracted all base64 and plain-text fixtures statically from the test file and matched them; each behaves as its test asserts.
- Own samples: I compared 26 real reverse-shell variants and 9 benign shapes across all three competing PRs. Samples were scanned as text and never executed.
- Scores: I ran main's static pipeline with the new rule passed as an extra rule file to get the figures above.
- Not run: contributor tests (policy).
- CI: all 6 checks green.
- Overlap: #608 also re-encodes
malware.yar.b64. Whichever lands second must decode, re-apply and re-encode rather than merge base64 text.
Comparison on my samples (C = CRITICAL reverse_shell, H = HIGH reverse_shell_multiline, - = no finding):
| Sample | main | #593 | #594 | #599 |
|---|---|---|---|---|
#592 repro (multiline, s, dup2, subprocess.call) |
- | H | C | C |
same, socket named conn |
- | H | C | - |
for fd in (0, 1, 2): os.dup2(...) + pty.spawn("/bin/bash") |
- | H | - | - |
dup2 + pty.spawn("sh") |
- | H | - | C |
~200-character comment between connect and dup2 |
- | H | C | - |
Perl multiline, SOCKET handle |
- | H | C | - |
Perl multiline, system("/bin/sh -i") |
- | - | - | C |
one-liner [os.dup2(s.fileno(),fd) for fd in (0,1,2)] |
C | C | - | - |
one-liner, socket named c |
C | C | C | - |
one-liner stdin=s,stdout=s,stderr=s |
C | C | - | - |
one-liner pty.spawn("/usr/bin/bash") |
C | C | - | C |
Perl one-liner, SOCKET handle |
C | C | C | - |
Perl one-liner, system("/bin/sh -i") |
C | C | - | C |
benign: local log dup2 + /bin/sh -c date inside def main() |
- | - | C | - |
benign: local command file on fd 0 + /bin/sh, with a print() |
- | H | C | - |
benign: /bin/sh -c uptime with stdout=sock.fileno() |
- | - | - | C |
| worst crafted 1 MB input (main on the same input) | 4.2 s (0.04 s) | 18.2 s (2.1 s) | 24.8 s (0.6 s) |
Decision: Changes Requested (reviewed head 41f4d5a8379a5a3da83f5739b9347bdf7262e559)
…rity, refresh docstrings Address @rng1995's re-review (test-file-only; packaged rules unchanged): - Base64-encode the single-line reverse-shell fixtures (_single_line_python_socket_shells / _single_line_perl_socket_shell), matching the module's encoded-fixture convention so on-access AV scanners do not quarantine the test file. The single-line main-regression cases (dup2 list-comprehension, sys.stdin.fileno, stdin=s, subprocess alias, pty.spawn, Perl system()) stay CRITICAL reverse_shell. - Tighten the certain-vs-heuristic severity checks to the exact rule name. Add _is_rule / _findings_from_rule (match the quoted rule name in the message, so "reverse_shell" no longer matches "reverse_shell_multiline") and _assert_only_multiline_heuristic: exactly one reverse_shell_multiline YR1 finding at HIGH / confidence 0.6 and no certain reverse_shell. Apply it to the Python and Perl multiline positives and to the unbound local-open lookalikes, so a silent promotion of the heuristic to CRITICAL/0.85 now fails the suite. - Refresh stale docstrings that still described the abandoned single-rule design (dot-all plus dup2/shell evidence bolted onto $python_socket): at this head $python_socket/$perl_socket are main's unchanged single-line strings and the multiline case is reported by reverse_shell_multiline. 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]
Hi @udsy19, thank you for turning the last round's test fixes around so quickly!
Value and readiness: The only new commit, 7eebd72, changes the test file alone. malware.yar.b64 is byte-identical to the head I reviewed last (41f4d5a), so the reverse_shell rule is still byte-identical to main's and the new reverse_shell_multiline rule is unchanged. Both test blockers and the docstring note from last round are fixed. One small item remains: two docstrings still contain the bash reverse-shell signature in plain text, so the test file itself trips the packaged CRITICAL reverse_shell rule. That text was already there at 41f4d5a and I missed it last round; sorry for the extra round. Once those two lines are reworded and the PIC confirms the HIGH/0.6 severity, this is ready for final maintainer/PIC review. It is still the strongest of the competing fixes (#599 was closed unmerged on 2026-10-02; #594 is still open).
Previous findings (review at 41f4d5a)
- [Blocker] Plain-text single-line fixtures: Resolved.
_single_line_python_socket_shells()and_single_line_perl_socket_shell()are base64 now. I decoded all six statically; each is byte-identical to the old plain text, so the CRITICAL regression tests still check the same inputs. - [Blocker] HIGH/0.6 contract not pinned: Resolved.
_is_rulematches the quoted name that_build_messagerenders (YARA rule 'reverse_shell'), soreverse_shellno longer matchesreverse_shell_multiline._assert_only_multiline_heuristicrequires exactly onereverse_shell_multilineYR1 finding at HIGH/0.6 and noreverse_shellfinding. 15 tests use it: the Python and Perl multiline positives, the variant positives and the five unbound lookalikes.static_yaraemits one finding per matched rule and parses"0.6"withfloat(), so the checks are exact. Raising the rule to CRITICAL/0.85 now fails all 15.
- [Non-blocking] Stale docstrings: Resolved for the docstrings I listed. The leftover
$python_local_openmentions are covered in finding 1 below. - [Non-blocking] Duplicate CRITICAL + HIGH finding on one-liners: Unchanged (the rule file did not change). Still optional.
- [Non-blocking] Multiline coverage gaps (Perl
system,stdin=s,sys.stdin.fileno()): Unchanged. Still optional. - [Non-blocking] No proximity bound between the three multiline strings: Unchanged. Still optional. Here is a live example: even after finding 1 is fixed, the raw test file satisfies
reverse_shell_multiline(HIGH) through docstring fragments hundreds of lines apart (lines 209, 475 and 553-554). Prose that discusses these idioms gets the same result. That heuristic is SkillSpector's own, not an antivirus signature, so I am not asking you to change those docstrings.
Material findings
-
[Blocker]
tests/nodes/analyzers/test_static_yara.py:538and:1511: these docstrings spell outbash -i >& /dev/tcp/....- That text is exactly the packaged rule's
$bash_revshellstring. Scanning the raw test file with the decoded head rules gives a CRITICALreverse_shellmatch on those two lines. - Main's copy of this file matches no packaged malware rule, and no file on main contains
/dev/tcp. - The fixture these docstrings describe,
_bash_reverse_shell_after_open_line_fixture, is base64 for the same reason as last round's fixtures: the module docstring says the file avoids real malware signatures so antivirus tools do not flag it.
Please reword both docstrings, e.g. "a bash TCP reverse-shell payload". In the same edit, consider describing the behavior instead of the
$python_local_openhistory (lines 541, 1432, 1511). That string only existed in an earlier revision of this PR. - That text is exactly the packaged rule's
PIC tradeoffs: Unchanged from last round: is HIGH/0.6 the right severity for a detected multiline reverse shell? On main's static pipeline, the #592 repro scores 35 (CAUTION) with this rule, and 71 (DO_NOT_INSTALL) with a CRITICAL/0.85 finding as in #594. The reduced severity is also what keeps unbound lookalikes from becoming DO_NOT_INSTALL. If CAUTION is not acceptable, a stricter, ordered and bounded CRITICAL rule could sit on top of this heuristic.
Verification and gaps:
- Commits: since
41f4d5athere is only7eebd72, by the author, with DCO sign-off.git diff 41f4d5a..7eebd72touches onlytest_static_yara.py(+165/-124). - Main overlap: since the merge-base (
2226747), no commit on main touchesmalware.yar.b64ortest_static_yara.py. The rule file last changed on main in #236 (July).git merge-treeagainst current main (33c9afe) is clean. #608 (open) also re-encodesmalware.yar.b64, so whichever PR lands second must decode, re-apply and re-encode. - Fixtures: I decoded the new base64 statically (AST only, nothing executed) and compared it with the previous plain text. They are identical.
- Rules: I compiled the decoded head rules with yara-python 4.5.4 and re-ran my 40 inert samples. The results match last round's comparison table. I also scanned the raw test file at main,
41f4d5aand head with these rules (results above). - Code paths: I traced
_build_message,_parse_metaand_match_fileinstatic_yara.pyfor the exact-name and HIGH/0.6 assertions. - Not run: contributor tests (policy).
- CI: all 6 checks green on
7eebd72.
Decision: Changes Requested (reviewed head 7eebd72711311158cc734534e3528e9390c589f5)
…ad of embedding its signature The docstrings for _bash_reverse_shell_after_open_line_fixture and test_reverse_shell_location_not_pulled_to_unrelated_open_line spelled out the literal bash reverse-shell command, which is byte-for-byte the packaged rule's $bash_revshell string. That made the raw test file itself match the CRITICAL reverse_shell rule. Reword both to describe the payload instead. Also drop the stale $python_local_open history references; that helper string only existed in an earlier revision of this PR. Test-file docstrings only; no rule or logic change. 143 tests still pass. 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]
Hi @udsy19, thank you for the quick docstring fix, and for clearing out the leftover $python_local_open history in the same commit!
Value and readiness: The only new commit, 8813f41, rewords two docstrings and one comment in the test file. The rule file is unchanged since my last review. reverse_shell is still byte-identical to main, and the reverse_shell_multiline heuristic (HIGH, 0.6) still detects the #592 multiline Python and Perl shells. The raw test file no longer trips the CRITICAL reverse_shell rule, so nothing required remains. This is ready for final maintainer/PIC review; the PIC still needs to confirm the HIGH/0.6 severity (below). It is still the stronger of the two open fixes for #592, and I recommend closing #594 in its favour.
Previous findings (review at 7eebd72)
- [Blocker] Docstrings at lines 538 and 1511 spelled out the bash reverse-shell signature: Resolved. Both now describe "a bash TCP reverse-shell payload", and the file no longer contains
/dev/tcp. I scanned the raw test file with the decoded head rules: at7eebd72,$bash_revshellmatched on both lines; at this head,reverse_shelldoes not match. - Optional: drop the
$python_local_openhistory (lines 541, 1432, 1511): Resolved. No mention is left. - [Non-blocking] Duplicate CRITICAL + HIGH finding on one-liners: Unchanged, still optional. #594's
and not reverse_shellcondition is a ready-made fix. - [Non-blocking] Multiline gaps (Perl
system(...), the socket object passed asstdin=s,sys.stdin.fileno()): Unchanged, still optional. #594 has working alternatives for the first two. - [Non-blocking] No proximity bound between the three multiline strings: Unchanged, still optional.
Material findings
None.
PIC tradeoffs: The open question is unchanged: is HIGH/0.6 the right severity for a detected multiline reverse shell? Two facts are new:
- #599 (CRITICAL/0.85) is closed, and #594 now also reports this evidence at HIGH (0.65). Both remaining candidates use a reduced-severity heuristic.
- Scoring on main has not changed since my measurement. The #592 repro scores 35 (CAUTION) with this rule and 71 (DO_NOT_INSTALL) with a CRITICAL/0.85 finding.
The reduced severity is also what keeps unbound lookalikes (a local command file on stdin plus /bin/sh) out of DO_NOT_INSTALL. If CAUTION is not acceptable, a stricter, ordered and bounded CRITICAL rule can be added on top of this heuristic later.
Verification and gaps:
- Commits: since
7eebd72there is only8813f41, by the author, with DCO sign-off. It changestests/nodes/analyzers/test_static_yara.pyonly (+6/-8): two docstrings and one section comment. No assertion, helper or fixture changed. - Rule file: the
malware.yar.b64blob is identical to7eebd72. I compiled the decoded head rules with yara-python 4.5.4 together with the other packaged rule files; there are no new warnings over main. - Raw test-file scan: results are under the blocker above. The HIGH
reverse_shell_multilinematch on docstring prose that I noted last round is unchanged. That is SkillSpector's own heuristic, not an antivirus signature, and every real-shell fixture is still base64. - Samples: I re-ran my inert samples (last round's 40 plus 8 new), scanned as text and never executed. This PR's results are unchanged.
- Main: main has 31 first-parent merges since the merge-base (
2226747). None touchesmalware.yar.b64,test_static_yara.pyorstatic_yara.py.git merge-tree --write-tree main refs/pr/593againstcdee381is clean, and the merged tree differs from main only in this PR's two files. The analyzer code the new tests rely on (theYARA rule '<name>'text from_build_message, and_parse_meta) is unchanged. - Overlap: #594 adds a rule with the same name in the same place, so only one can land. On my samples, this PR catches five real-shell shapes that #594 misses (zsh/dash shells, Perl
Sandmy $shandles,use Socket qw(...), about 600 characters betweenconnectanddup2). #594 catches two that this PR misses (the socket object asstdin=s, Perlsystem). #608 (open) also re-encodesmalware.yar.b64, so whichever lands second must decode, re-apply and re-encode rather than merge base64 text. - Threads: my docstring thread is addressed by
8813f41. The other 11 threads were already resolved. - Not run: contributor tests (policy). CI: all 6 checks green on
8813f41.
Decision: Approved (reviewed head 8813f41b90c98600ae86222b0410019e21cc6e69)
2 participants
Fixes #592
Problem
The built-in
reverse_shellYARA rule (src/skillspector/yara_rules/malware.yar.b64) only matched single-line reverse shells:$python_socket/$perl_socketuse.*with no dotall, so a shell written as a normal multi-line script file was missed entirely. Earlier iterations of this PR tried to fix that by widening the single rule, but @rng1995's reviews showed the widened rule both over-matched (ordinary clients and unrelated local redirection scored CRITICAL) and under-matched (a stray= open(exclusion cancelled real shells and skewed finding locations; the stricter multiline evidence regressed main's single-line detections).Fix: two rules, one certain and one heuristic
The root limitation is that YARA regexes cannot bind an identifier (no backreferences), so a signature cannot prove the redirected descriptor is the connected socket. Rather than force one rule to carry both roles, detection is split:
reverse_shell(CRITICAL, 0.85) — restored tomain's exact single-line strings andany of themcondition. Every single-line shellmaindetects stays CRITICAL (dup2 list-comprehension,sys.stdin.fileno(),stdin=<socket>,subprocessalias,pty.spawn, Perlsystem/execone-liners). No negative-exclusion string, no behavior change versusmain.reverse_shell_multiline(HIGH, 0.6) — new, bounded dot-all evidence requiring socket creation +connect, descriptor redirection onto stdin (dup2(..fileno(), 0), the(0,1,2)loop/comprehension forms, orstdin=<x>.fileno()), and a shell-execution marker. Catches real multiline shells (dup2 +subprocess/os.execl/pty.spawn,stdin=kwargs, Perl open-dup + exec including no-paren and three-arg forms). Because the descriptor cannot be bound to the socket, it is reported at reduced severity/confidence, never as a certain reverse shell.How this maps to the review
-, so prefixes do not match.stdin=redirection,os.execl,pty.spawn, and Perlopen/execparen variants all detected.$python_local_openexclusion string is removed entirely; a stray= open(no longer cancels a real shell or pulls the finding's line/snippet/fingerprint to the wrong offset.with open(...), Perl<&CMDS, three-arg<&) report at reduced severity, never CRITICAL.for fd in (0,1,2): os.dup2(...)loop is detected by the heuristic rule.Testing
New regressions: positives that must not be cancelled by an unrelated
open(file read before dup2, probe socket + clean shell, trailing cleanup open, dup2 loop); reduced-severity negatives for unbound local redirection (Pythonwith open, Perl local/three-arg dup); a location-integrity regression (bash payload below anopenline is reported at the payload); and single-line CRITICAL coverage for Python and Perl. Each fails against the previous head's rule and passes here. Fixtures stay base64-encoded per this module's convention.Rebased/merged onto
main; the only conflict (test_static_yara.py, #489's webshell fixtures at the same spot) is resolved by keeping both blocks.