fix(amwscan): the discriminator is in the parentheses, and the => line is source

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.

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
 		}
 	}

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
 		}
 	}

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")
+	}
+}

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")
 	}
 }