diff --git a/DECISIONS.md b/DECISIONS.md index ae3dbff..6d1ab7d 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -2181,3 +2181,72 @@ not be asked about". The fix that made coverage honest made this dishonesty loud number visible does not make it true, and the visibility is what exposed that this one was not. The lesson is not "check the JSON" — it is that surfacing a count obliges you to check what the count actually means. + +## D-057 — the discriminator is in the parentheses, and a real capture is not enough + +**Context**: found by reading the raw AMWScan report from a live account, which only became +possible with #94 — until then `raw_ref` pointed at the process's stdout, which `--silent` +keeps empty of findings. + +The real engine names the rule generically and puts the specific thing in parentheses: + +```text + => [!] Function (exec) [line 147] + - Potentially dangerous function `exec` + => exec('kill -' . (int) $signal . ' ' . (int) $pid . ' 2>/dev/null', $out, $code) +``` + +`Function` is not in the rule table and never will be. `exec` is, and always was. The +adapter captured it into a field used only for display and classified on the rule name, so +on that account 288 of 333 `Function` findings — 199 of them `eval` — came out as +`other`/`medium`/`heuristic`. + +And the indented `=> ...` line was read as the category the engine assigned, **with +priority over everything else**. It is the source that matched. On that account the +resulting "categories" read `lave`, `tressa`, `ipconfig`, `suhosin`: fragments of somebody +else's code. `lave` is `eval` backwards, because AMWScan detects `strrev`-obfuscated calls +and prints the reversed string — so an obfuscated backdoor classified as unknown while the +plain word sat one pair of brackets away. + +**Decision**: classify on the parenthesised token, fall back to the rule name, never on the +matched source. + +- **The token first, because it is strictly more specific.** A hash from + `Signature (11413268)` matches nothing in the table and falls through to `signature`, + which is the intended answer for it. +- **The matched source is kept as evidence and never classified on.** It comes out of the + scanned file, which means the file's author chooses it. Nothing exploits that today — + the snippet is usually far too long to collide with a table key — but what a finding IS + must not be selectable by the thing being examined. + +**This changes scores, and that is stated rather than buried.** The `Function` cases do +not: `eval` and the unknown fallback are both `heuristic`, so the multiplier is identical +and only the category and severity a human reads to triage change. `Signature` does. Those +were classified by the snippet — the word `backdoor` in the container's corpus, a heuristic +entry — and now reach `signature`: + +```text +before: vote: amwscan weight 0.80 x heuristic = 0.64 (rule Signature) +after: vote: amwscan weight 0.80 x signature = 0.80 (rule Signature) +``` + +That is the weight the project always intended for a signature hit. It also means a site +with a signature plus one weak vote can now cross from `likely` to `confirmed`, and +`confirmed` is what authorises an automatic quarantine. In the validation corpus nothing +crossed: `confirmed=1 likely=3 suspicious=3` both with and without the change, same image, +only the classification line reverted. + +**The lesson, which is not the obvious one**: the fixture this was all built on is +**genuine**. `=> backdoor` really was printed, because that hit's matched content happened +to be the word "backdoor". A real capture was taken, and a wrong model was still built on +it, because one sample cannot tell you which parts of a line are structure and which are +content. D-022 says verify against reality rather than assumption; this adds that **a real +capture is necessary and not sufficient** — where a format matters, capture more than one +file, from more than one site. The second capture disagreed with the first about the only +thing that mattered. + +**What was deliberately not done**: the `Exploit` vocabulary — `execution` (732 on that +account), `base64_long`, `hex_char`, `nano`, `etc_passwd`, `php_uname`, `clever_include` — +is still unmapped. Each needs the engine's own description read before being mapped, and +those descriptions are in the report on the `- ` line under each finding. Mapping them from +the names alone is the guess this entry is about not making. diff --git a/internal/adapter/amwscan/amwscan.go b/internal/adapter/amwscan/amwscan.go index aeff4de..f53379e 100644 --- a/internal/adapter/amwscan/amwscan.go +++ b/internal/adapter/amwscan/amwscan.go @@ -487,10 +487,14 @@ func (a *Adapter) Parse(raw adapter.RawOutput) (schema.ScanReport, error) { allowed := allowedTargets(raw) type pending struct { - rule string + rule string + // extra is the parenthesised token: the function name for Function (eval), the + // pattern name for Exploit (base64_long), the hash for Signature (11413268). It + // is what classify keys on. extra string line int64 - tag string + // snippet is the matched source line, shown as evidence and never classified on. + snippet string } var ( @@ -516,7 +520,7 @@ func (a *Adapter) Parse(raw adapter.RawOutput) (schema.ScanReport, error) { return } - m, known := classify(current.rule, current.tag) + m, known := classify(current.rule, current.extra) if !known { unknown++ } @@ -536,8 +540,8 @@ func (a *Adapter) Parse(raw adapter.RawOutput) (schema.ScanReport, error) { if current.extra != "" { snippet += " (" + current.extra + ")" } - if current.tag != "" { - snippet += " => " + current.tag + if current.snippet != "" { + snippet += " => " + current.snippet } rep.Findings = append(rep.Findings, schema.Finding{ @@ -621,9 +625,24 @@ func (a *Adapter) Parse(raw adapter.RawOutput) (schema.ScanReport, error) { if sectionRe.MatchString(line) { continue } - // The line " => backdoor" is the category the engine assigned. - if m := tagRe.FindStringSubmatch(line); m != nil && current != nil && current.tag == "" { - current.tag = strings.TrimSpace(m[1]) + // The indented " => ..." line is the MATCHED SOURCE, not a category. + // + // It was read as the category, and given priority over everything else, because the + // stored fixture happened to show short values there — "=> backdoor", "=> eval" — + // that look exactly like category names. Against the real engine the same position + // holds the code that matched: + // + // => exec('kill -' . (int) $signal . ' ' . (int) $pid . ' 2>/dev/null', $out, $code) + // + // which is content of the scanned file. Classifying on it means the category and + // severity of a finding could be steered by whatever the file's author put there, + // and on a real account it produced "categories" like `lave`, `ipconfig` and + // `suhosin` — fragments of somebody's source code. + // + // It is kept, because it is the evidence a user needs to see why a file was + // flagged. It is no longer allowed to decide what the finding IS. + if m := tagRe.FindStringSubmatch(line); m != nil && current != nil && current.snippet == "" { + current.snippet = strings.TrimSpace(m[1]) continue } } diff --git a/internal/adapter/amwscan/rules.go b/internal/adapter/amwscan/rules.go index 140fb45..401fe59 100644 --- a/internal/adapter/amwscan/rules.go +++ b/internal/adapter/amwscan/rules.go @@ -67,12 +67,27 @@ var ruleTable = map[string]mapping{ // // The second return value says whether the rule was known — the adapter uses it // to count how many new rules showed up. -func classify(rule, tag string) (mapping, bool) { - // The tag (the report's "=> backdoor" line) is more specific than the rule - // name, so it wins: "Signature" with tag "backdoor" says more than - // "Signature" alone. - if tag != "" { - if m, ok := ruleTable[strings.ToLower(strings.TrimSpace(tag))]; ok { +func classify(rule, detail string) (mapping, bool) { + // The parenthesised token is the discriminator, and it used to be thrown away. + // + // Real AMWScan output names the rule generically and puts the specific thing in + // parentheses: + // + // => [!] Function (eval) + // => [!] Function (exec) [line 147] + // => [!] Signature (11413268) [line 66] + // + // "Function" is not in this table and never will be, so every one of those landed on + // other/medium/heuristic while the word that decides the answer sat one pair of + // brackets away. On one real account that was 199 `eval` and 29 `exec` findings shown + // as generic medium heuristics — eval being the single strongest backdoor indicator + // PHP has. + // + // Tried before the rule name because it is strictly more specific. A hash from + // Signature (11413268) matches nothing here and falls through to "signature", which is + // the intended answer for it. + if detail != "" { + if m, ok := ruleTable[strings.ToLower(strings.TrimSpace(detail))]; ok { return m, true } } diff --git a/tests/contract/amwscan_real_format_test.go b/tests/contract/amwscan_real_format_test.go new file mode 100644 index 0000000..66423fa --- /dev/null +++ b/tests/contract/amwscan_real_format_test.go @@ -0,0 +1,134 @@ +package contract_test + +import ( + "strings" + "testing" + + "github.com/thiagoluga/SentinelHost/internal/adapter/amwscan" + "github.com/thiagoluga/SentinelHost/internal/schema" +) + +// The real engine names the rule generically and puts the discriminator in parentheses. +// +// This fixture is a sanitised capture from a live hosting account, not a hand-written +// sample. It matters that it is captured: the fixture the adapter was built against shows +// +// => [!] Signature (d30fc49e) [line 4] +// - Malware Signature (hash: d30fc49e) +// => backdoor +// +// where that last line looks like a category — and the parser was written to read it as +// one, with priority over everything else. Against the real engine the same position holds +// the source that matched: +// +// => [!] Function (exec) [line 147] +// - Potentially dangerous function `exec` +// => exec('kill -' . (int) $signal . ' ' . (int) $pid . ' 2>/dev/null', $out, $code) +// +// So "Function" — which is not in the rule table and never will be — decided the answer, +// and `exec`, which IS in the table, was captured into a field used only for display. On +// the account this was taken from, that is 199 eval and 29 exec findings reported as +// generic medium heuristics. +func TestTheParenthesisedTokenClassifiesTheFinding(t *testing.T) { + a := amwscan.New().WithStat(fakeStat) + + rep, err := a.Parse(raw(amwscan.Slug, + fixture(t, "amwscan", "real-0.15.1-function-and-exploit.txt"), schema.StatusCompleted)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if err := rep.Validate(); err != nil { + t.Fatalf("the report does not validate: %v", err) + } + + want := map[string]struct { + category schema.Category + severity schema.Severity + }{ + "exec": {schema.CategoryWebshell, schema.SeverityCritical}, + "eval": {schema.CategoryBackdoor, schema.SeverityCritical}, + "assert": {schema.CategoryBackdoor, schema.SeverityHigh}, + "shell_exec": {schema.CategoryWebshell, schema.SeverityCritical}, + } + + seen := map[string]bool{} + for _, f := range rep.Findings { + for token, exp := range want { + // The rule stays as the engine wrote it; the snippet carries the token. + if !strings.Contains(strings.ToLower(f.MatchedContent), "("+token+")") { + continue + } + seen[token] = true + if f.Category != exp.category { + t.Errorf("%s: category %q, wanted %q — the token in the parentheses is the "+ + "only thing that says what this finding is", token, f.Category, exp.category) + } + if f.Severity != exp.severity { + t.Errorf("%s: severity %q, wanted %q", token, f.Severity, exp.severity) + } + } + } + for token := range want { + if !seen[token] { + t.Errorf("no finding carried the token %q; the fixture or the parser changed", token) + } + } +} + +// A hash in the parentheses falls through to the rule name, which is the right answer. +// +// Signature (11413268) has no entry — hashes never will — so classification has to reach +// "signature" and report known malware. Trying the token first must not cost this. +func TestASignatureHashFallsThroughToTheRuleName(t *testing.T) { + a := amwscan.New().WithStat(fakeStat) + + rep, _ := a.Parse(raw(amwscan.Slug, + fixture(t, "amwscan", "real-0.15.1-function-and-exploit.txt"), schema.StatusCompleted)) + + found := false + for _, f := range rep.Findings { + if f.Rule != "Signature" { + continue + } + found = true + if f.Category != schema.CategoryKnownMalware { + t.Errorf("a Signature hit came out as %q, wanted known_malware: %s", f.Category, f.MatchedContent) + } + if f.Confidence != schema.ConfidenceSignature { + t.Errorf("confidence %q, wanted signature — this is the one case where the "+ + "engine claims to recognise the threat, not a pattern", f.Confidence) + } + } + if !found { + t.Fatal("the fixture carries no Signature finding") + } +} + +// The matched source must never decide what a finding is. +// +// The engine detects strrev-obfuscated calls, and then the matched source reads `Lave` or +// `tressa` — eval and assert spelled backwards. Under the old parser those strings WERE +// the category, so an obfuscated backdoor classified as unknown while the plain word +// `eval` sat in the parentheses. Worse in principle than in practice: the string comes out +// of the scanned file, so its author chooses it. +func TestTheMatchedSourceIsEvidenceNotACategory(t *testing.T) { + a := amwscan.New().WithStat(fakeStat) + + rep, _ := a.Parse(raw(amwscan.Slug, + fixture(t, "amwscan", "real-0.15.1-function-and-exploit.txt"), schema.StatusCompleted)) + + checked := 0 + for _, f := range rep.Findings { + low := strings.ToLower(f.MatchedContent) + if !strings.Contains(low, "lave") && !strings.Contains(low, "tressa") { + continue + } + checked++ + if f.Category == schema.CategoryOther { + t.Errorf("an obfuscated call classified as other: %s", f.MatchedContent) + } + } + if checked == 0 { + t.Fatal("the fixture lost its strrev-obfuscated findings, which are the point here") + } +} diff --git a/tests/contract/contract_test.go b/tests/contract/contract_test.go index ba6e51e..8fb19f6 100644 --- a/tests/contract/contract_test.go +++ b/tests/contract/contract_test.go @@ -100,18 +100,37 @@ func TestAMWScanParsesFindings(t *testing.T) { } } -func TestAMWScanTheTagBeatsTheRuleName(t *testing.T) { - // The line " => backdoor" is more specific than the rule name: - // "Signature" alone does not say which family the finding belongs to. +// This test used to assert the opposite, and it was wrong in the way D-022 describes: +// an assumption about an external format, plus a test written to confirm the assumption. +// +// It read the indented " => backdoor" line as the category the engine assigned, and +// gave it priority over everything else. That line is the SOURCE THAT MATCHED. It looked +// like a category only because this fixture was shaped by hand; the real engine puts code +// there, and on a live account it produced "categories" reading `lave`, `ipconfig` and +// `suhosin` — one of which is `eval` spelled backwards, from AMWScan's own detection of +// strrev-obfuscated calls. +// +// A Signature hit is known malware. That is what the engine means by it, and no line of +// the scanned file's own source gets to say otherwise — least of all because its author +// chose what is written there. +func TestAMWScanASignatureIsKnownMalware(t *testing.T) { a := amwscan.New().WithStat(fakeStat) rep, err := a.Parse(raw(amwscan.Slug, fixture(t, "amwscan", "success-with-findings.txt"), schema.StatusCompleted)) if err != nil { t.Fatalf("Parse: %v", err) } + seen := 0 for _, f := range rep.Findings { - if f.Rule == "Signature" && f.Category != schema.CategoryBackdoor { - t.Errorf("the `backdoor` tag should have set the category, got %q", f.Category) + if f.Rule != "Signature" { + continue } + seen++ + if f.Category != schema.CategoryKnownMalware { + t.Errorf("a Signature hit came out as %q, wanted known_malware", f.Category) + } + } + if seen == 0 { + t.Fatal("the fixture carries no Signature finding") } } diff --git a/tests/testdata/raw/amwscan/PROVENANCE.md b/tests/testdata/raw/amwscan/PROVENANCE.md index 4b210d6..6862bdc 100644 --- a/tests/testdata/raw/amwscan/PROVENANCE.md +++ b/tests/testdata/raw/amwscan/PROVENANCE.md @@ -28,6 +28,12 @@ hold for any new fixture: A new fixture only enters here after coming out of a real run. Run `make validate-engines` and copy it from the container. +3. **A real capture is necessary and not sufficient.** The `=> backdoor` line below came + out of a real run and was still read wrongly, because one sample cannot tell you which + parts of a line are structure and which are content. Where a format matters, capture + more than one file — from more than one site if you can. The second capture here came + from a live account and disagreed with the first about the thing that mattered most. + ## Format ```text @@ -40,10 +46,31 @@ Exploits: ``` Hierarchy: `File:` opens a block and everything below belongs to it until the next -`File:`. `Exploits:` and `Functions:` only separate sections. The line -` => ` is the category the engine assigned, and the adapter gives it -priority over the rule name — `Signature` with tag `backdoor` says more than -`Signature` alone. +`File:`. `Exploits:` and `Functions:` only separate sections. + +**The indented ` => ...` line is the SOURCE THAT MATCHED, not a category.** This +document used to say the opposite, and the adapter was built on it — reading that line as +the engine's category and giving it priority over everything else. + +The fixture above is genuine. That is the part worth sitting with. `=> backdoor` really +was what the engine printed, because the matched content of that particular hit happened +to be the word "backdoor". One sample's coincidence was read as structure, and a parser +was written to it. Against a live account the same position holds things like: + +```text + => [!] Function (exec) [line 147] + - Potentially dangerous function `exec` + => exec('kill -' . (int) $signal . ' ' . (int) $pid . ' 2>/dev/null', $out, $code) +``` + +and the resulting "categories" read `lave`, `tressa`, `ipconfig`, `suhosin` — fragments of +somebody's source. (`lave` is `eval` backwards: AMWScan detects strrev-obfuscated calls and +prints the reversed string it found.) + +**The discriminator is the parenthesised token**, and it is what the adapter classifies on: +`Function (eval)` is a backdoor indicator, `Function (exec)` a webshell one, +`Signature (11413268)` a hash that falls through to the rule name and reports known +malware. See `real-0.15.1-function-and-exploit.txt`, captured from a live hosting account. ## Peculiarities the adapter has to handle @@ -69,6 +96,35 @@ priority over the rule name — `Signature` with tag `backdoor` says more than ideal, but AMWScan simply does not know how to scan a file list. - The engine does not report a hash. The orchestrator computes the sha256, because it is the deduplication key across engines. -- The txt report **does not include the code snippet**, only the rule and the line. - Better that way: malicious content does not need to travel through the system for - the user to understand a finding. +- The txt report **does include the matched source**, on the indented `=>` line, and it + can be long — hundreds of characters of the scanned file, truncated by the engine with + an ellipsis. This document previously claimed the opposite. The adapter keeps it as + evidence, through `SanitizeSnippet`, and never classifies on it. + +## `real-0.15.1-function-and-exploit.txt` + +Captured from a live cPanel hosting account on 2026-08-18, from the report the orchestrator +archives at `raw_ref` (which only holds the report at all since #94 — before that it +pointed at the process's stdout, which `--silent` keeps empty). + +Sanitised: real paths rewritten to `/home/user/...`, and snippets longer than 200 +characters truncated the way the engine itself truncates them. Nothing else is altered — +the spacing, the `[line N]` that is present on some findings and absent on others, and the +blank line between blocks are all as the engine wrote them. + +It covers what the container's corpus never produced: `Function (eval)`, `Function (exec)`, +`Function (assert)`, `Function (shell_exec)`, `Function (proc_open)`, `Exploit (execution)`, +`Exploit (hex_char)`, and `Signature ()` — including two strrev-obfuscated hits whose +matched source reads `Lave` and `tressa`. + +The distribution on that account, for anyone deciding what to add to the rule table next: + +| rule | count | notes | +|---|---|---| +| `Exploit` | 779 | `execution` 732, then `base64_long`, `nano`, `hex_char`, … | +| `Function` | 333 | `eval` 199, `assert` 42, `exec` 29, `shell_exec` 12, … | +| `Signature` | 38 | hashes | + +`Function` tokens are largely covered by the table already. The `Exploit` vocabulary is +not, and each entry needs the engine's own description read before it is mapped — they are +in the report, on the `- ` line under each finding. diff --git a/tests/testdata/raw/amwscan/real-0.15.1-function-and-exploit.txt b/tests/testdata/raw/amwscan/real-0.15.1-function-and-exploit.txt new file mode 100644 index 0000000..14536fe --- /dev/null +++ b/tests/testdata/raw/amwscan/real-0.15.1-function-and-exploit.txt @@ -0,0 +1,72 @@ +Scan date: 2026-08-18 00:18:00 +File: /home/user/public_html/sentinel/index.php +Exploits: + => [!] Function (exec) + - Potentially dangerous function `exec` + => exec($cmd); } flock($lock, LOCK_UN); } fclose($lock); while (microtime(true) < $deadline) { if (panelIsUp($upstream)) { return true; } usleep(150000); } return panelIsUp($upstream); } func... + +File: /home/user/public_html/sentinel/lib/path.php +Exploits: + => [!] Signature (11413268) [line 66] + - Malware Signature (hash: 11413268) + => exploit + +File: /home/user/.trash/997154.2/423510/index.php +Exploits: + => [!] Exploit (hex_char) [line 8] + - Hex char is usually used for the obfuscation of malicious code + => \x5f + => [!] Function (eval) + - Potentially dangerous function `eval` + => eval($P71oL($l9sQU("\x41\x58\101\121\163\x46\x47\x74\x68\127\x6c\53\x38\x6a\x4b\x77\x47\171\x6b\x73\157\123\117\166\147\104\157\111\x39\x4a\102\141\124\131\x55\166\131\172\122\x75\165\x34\... + +File: /home/user/.trash/job/bootstrap/cache/665726/index.php +Exploits: + => [!] Exploit (execution) [line 1030] + - RCE (Remote Code Execution) allow remote attackers to execute PHP code on the target machine via HTTP + => base64_decode($_GET['img']) + => [!] Exploit (execution) [line 1054] + - RCE (Remote Code Execution) allow remote attackers to execute PHP code on the target machine via HTTP + => base64_decode($_GET['download']) + => [!] Exploit (execution) [line 1657] + - RCE (Remote Code Execution) allow remote attackers to execute PHP code on the target machine via HTTP + => base64_decode($_GET['zip']) + => [!] Exploit (execution) [line 1677] + - RCE (Remote Code Execution) allow remote attackers to execute PHP code on the target machine via HTTP + => base64_decode($_GET['gz']) + => [!] Exploit (execution) [line 1713] + - RCE (Remote Code Execution) allow remote attackers to execute PHP code on the target machine via HTTP + => base64_decode($_GET['decompress']) + => [!] Exploit (execution) [line 1738] + - RCE (Remote Code Execution) allow remote attackers to execute PHP code on the target machine via HTTP + => base64_decode($_GET['gzfile']) + => [!] Function (eval) + - Potentially dangerous function `eval` + => eval(trim($string)); $text = ob_get_contents(); ob_end_clean(); ini_set('display_errors', $display_errors); return $text; } function fm_sql_connect(){ global $fm_auth_config; return new my... + +File: /home/user/.trash/job/vendor/fakerphp/faker/src/Faker/Provider/fr_FR/Text.php +Exploits: + => [!] Signature (11413268) [line 7994] + - Malware Signature (hash: 11413268) + => exploit + => [!] Function (eval) [line 1852] + - Encoded Function `eval` + => Lave + => [!] Function (eval) [line 14296] + - Encoded Function `eval` + => lave + => [!] Function (assert) [line 2932] + - Encoded Function `assert` + => tressa + +File: /home/user/.trash/job/vendor/sebastian/environment/src/Console.php +Exploits: + => [!] Function (shell_exec) + - Potentially dangerous function `shell_exec` + => shell_exec('stty size') ?: '', $match) === 1) { if ((int) $match[1] > 0) { return (int) $match[1]; } } if (function_exists('shell_exec') && preg_match('#columns = (\d+);#', shell_exec('stt... + => [!] Function (proc_close) + - Potentially dangerous function `proc_close` + => proc_close($process); if (preg_match('/--------+\r?\n.+?(\d+)\r?\n.+?(\d+)\r?\n/', $info, $matches)) { $columns = (int) + => [!] Function (proc_open) + - Potentially dangerous function `proc_open` + => proc_open( 'mode CON', [ 1 => ['pipe', 'w'], 2 => ['pipe', 'w'], ], $pipes, null, null, ['suppress_errors' => true] ); if (is_resource($process)) { $info = stream_get_contents($pipes[1]); ...