Better handling of overrides after all fr.Behaviors are added #487

egibs · 2024-10-04T03:56:20Z

Closes #486

We hit the edge case where an override rule was being evaluated before the original rule, so we never found it in the slice of behaviors.

This PR handles overrides as a post-processing step, rather than during the mrs loop.

This PR also introduces our first Critical -> High override for a third-party (!) rule:

HIGH  override/plugx                  detect PlugX in memory, [malware_PlugX_config], by   $v2b                                                                                                                     
                                      JPCERT/CC Incident Response Group                    $v2f                                                                                                                     
                                                                                           $v2g                                                                                                                     
                                                                                           $v2h                                                                                                                     
                                                                                           �DDDD                                                                                                                    
                                                                                           �DDDD                                                                                                                    
                                                                                           �3333                                                                                                                    
                                                                                           �DDDD

Because of this rule, I was able to see that the original override logic was insufficient. The new approach is more resilient and we should successfully apply all overrides now.

Signed-off-by: egibs <[email protected]>

pkg/report/report.go

egibs · 2024-10-04T03:57:32Z

pkg/report/report.go

+		if originalExists && overrideExists {
+			for _, b := range fr.Behaviors {
+				if b.RuleName == override.RuleName {
+					b.RuleAuthor = original.RuleAuthor


I was getting incorrect results if I did b = original, so I was more prescriptive with the fields here. I think it adds helpful context anyway.

I think this should be reversed: preserve the original rule, but apply the necessary override (priority) to it: otherwise we're going to just keep on adding fields to this code as new ones come up.

One thing I just realized is that we probably shouldn't modify the rules description directly: otherwise it gets messy if multiple overrides apply to a rule. Instead we can add the rule name to a list of overrides that apply to the behavior (for example: Behavior.Overrides), and modify the description at the presentation layer.

Flipped the logic in a4c70f4 (#487).

rules/override/plugx.yara

Signed-off-by: egibs <[email protected]>

egibs · 2024-10-04T13:15:28Z

Currently working through a discrepancy between analyze and scan. The former works, but the latter seems to ignore the override completely (i.e., validOverrides isn't even populated).

tstromberg

Could you make it so that overridden rules use the original name rather than the override name?

Signed-off-by: egibs <[email protected]>

egibs · 2024-10-04T13:31:32Z

rules/override/shellcode.yara

+    meta:
+        malware_shellcode_hash = "high"
+
+    strings:


@tstromberg -- can you think of a less redundant way to handle this?

This is a new override rule for another third-party false positive we saw with Pulumi, but it feels gross to duplicate the strings/conditions. Using something less restrictive will allow other override rules to match, though (e.g., condition: true).

We should invert how this override functions: it should simply detect the appropriate Pulumi binary rather than repeat the logic of the offending rule.

Here's what I would do in your shoes:

Rename this file to "pulumi.yara"

Rename the rule to the binary you are overriding, like pulumictl (if it's not obviously pulumi, maybe "pulumi_".

Add 1-2 long strings you expect to show up in the binary, such as "PULUMI_HOME" or "How did Pulumi come up with such a weird name?"

Ditch the rest of the rules: we're only trying to detect that this binary is probably pulumi, not the rules it is overriding.

Makes sense. Your Bash example comes to mind and that's a good way to go about it.

Signed-off-by: egibs <[email protected]>

egibs · 2024-10-04T13:44:27Z

Could you make it so that overridden rules use the original name rather than the override name?

Updated in 01ddf7d (#487).

tstromberg · 2024-10-04T13:50:41Z

rules/override/shellcode.yara

+    meta:
+        malware_shellcode_hash = "high"
+
+    strings:


We should invert how this override functions: it should simply detect the appropriate Pulumi binary rather than repeat the logic of the offending rule.

Here's what I would do in your shoes:

Rename this file to "pulumi.yara"

Rename the rule to the binary you are overriding, like pulumictl (if it's not obviously pulumi, maybe "pulumi_".

Add 1-2 long strings you expect to show up in the binary, such as "PULUMI_HOME" or "How did Pulumi come up with such a weird name?"

Ditch the rest of the rules: we're only trying to detect that this binary is probably pulumi, not the rules it is overriding.

rules/override/shellcode.yara

tstromberg · 2024-10-04T13:55:36Z

Ideal: instead of a generic overrides/ folder within rules, maybe a known_good/ folder specifically geared toward addressing false positives?

I figure that overrides that add severity should probably live elsewhere?

Signed-off-by: egibs <[email protected]>

tstromberg · 2024-10-04T16:30:22Z

pkg/report/report.go

@@ -521,6 +517,72 @@ func Generate(ctx context.Context, path string, mrs yara.MatchRules, c malconten
 		// TODO: If we match multiple rules within a single namespace, merge matchstrings
 	}



Can some of the override handling code move to a new function? This one is getting overly large and complicated.

Moved to a new function in 660024a (#487).

tstromberg · 2024-10-04T16:32:32Z

pkg/report/report.go

+		// If this is the override rule, delete it from the behavior slice
+		// Otherwise, treat the rule as a possible original (vo.Original makes this an O(1) lookup)
+		if isOverride {
+			fr.Behaviors = slices.Delete(fr.Behaviors, i, i+1)


Instead of deleting overrides from Behaviors, would the logic be simpler if overrides were stored elsewhere instead? For example, fr.Overrides or a different variable altogether.

By the time fr.Behaviors is populated, we already know if the rule has an override tag.

Added two new fields for overrides in 660024a (#487).

tstromberg · 2024-10-04T16:36:35Z

pkg/report/report.go

+	switch c.Scan {
+	case true:
+		switch {
+		case newRisk != overallRiskScore && newRisk >= 3:


Can this use a const of some sort? I don't remember if "3" is "HIGH" or "CRITICAL". I'm not sure why either needs to be hardcoded here to be honest, but I think it'd improve readability.

Updated in 660024a (#487).

tstromberg · 2024-10-04T16:41:49Z

pkg/report/report.go

+	// Regardless of running an `analyze` or `scan`, adjust the overall risk if we deviated from overallRiskScore
+	// Scans will still need to drop medium or lower results
+	newRisk := highestBehaviorRisk(fr)
+	switch c.Scan {


I don't think the nested switches are helping readability. Maybe something like:

if newRisk != overallRiskScore { overallRiskScore = newRisk } if c.Scan && newRisk < HIGH: return malcontent.FileReport{}, nil }

Is this just checking if the command is "scan" vs "analyze"? If so, I don't think this function should need to know: all scan should need to do in main is set a default minRisk of "HIGH" and everything else should just work out.

Cleaned this up in 660024a (#487).

Signed-off-by: egibs <[email protected]>

egibs · 2024-10-04T18:16:51Z

pkg/report/report.go

+	}
+
+	for _, o := range fr.Overrides {
+		if b, exists := behaviorMap[o.Override]; exists {


I tested this out with a critical false positive and it works.

Test override rule:

rule pulumi_shellcode : override { meta: malware_shellcode_hash = "low" description = "Ignore shellcode findings in the pulumi binary" strings: $pulumi = { 50 75 6C 75 6D 69 } $pulumi2 = { 70 75 6C 75 6D 69 } condition: any of ($pulumi*) }

Result:

Signed-off-by: egibs <[email protected]>

egibs · 2024-10-04T18:37:45Z

rules/net/dns-servers.yara

@@ -1,4 +1,4 @@
-rule go_dns_refs {
+rule go_dns_refs_local {


There were two go_dns_refs rules and there was a chance one would clobber the other and result in different report results:

malcontent/rules/net/dns.yara

Lines 1 to 11 in 0acb2e0

rule go_dns_refs {

meta:

description = "Uses DNS (Domain Name Service)"

strings:

$dnsmessage = "dnsmessage"

$edns = "SetEDNS0"

$cname = "CNAMEResource"

$nodejs = /require$['"]dns['"]$/

condition:

any of them

}

malcontent/rules/net/dns-servers.yara

Lines 1 to 10 in 0acb2e0

rule go_dns_refs {

meta:

description = "Examines local DNS servers"

strings:

$resolv = "resolv.conf" fullword

$dns_getservers = "dns.getServers"

$cname = "CNAMEResource"

condition:

any of them

}

Better handling of overrides after all fr.Behaviors are added

2341730

Signed-off-by: egibs <[email protected]>

egibs requested a review from tstromberg October 4, 2024 03:56

egibs commented Oct 4, 2024

View reviewed changes

pkg/report/report.go Show resolved Hide resolved

egibs commented Oct 4, 2024

View reviewed changes

rules/override/plugx.yara Outdated Show resolved Hide resolved

Appease the linter

d73ccad

Signed-off-by: egibs <[email protected]>

tstromberg requested changes Oct 4, 2024

View reviewed changes

Fix scan override behavior, use original rule's name + ID

01ddf7d

Signed-off-by: egibs <[email protected]>

egibs commented Oct 4, 2024

View reviewed changes

Fix overall risk score after downgrading

43083b9

Signed-off-by: egibs <[email protected]>

tstromberg requested changes Oct 4, 2024

View reviewed changes

egibs added 2 commits October 4, 2024 09:07

Retain original rule, drop override, remove test rules

a4c70f4

Signed-off-by: egibs <[email protected]>

Allow for severity upgrades as well

0a2d3a2

Signed-off-by: egibs <[email protected]>

egibs requested a review from tstromberg October 4, 2024 14:13

Comments

929a432

Signed-off-by: egibs <[email protected]>

egibs force-pushed the fix-overrides branch from a133b19 to 929a432 Compare October 4, 2024 14:19

tstromberg requested changes Oct 4, 2024

View reviewed changes

Address PR comments

660024a

Signed-off-by: egibs <[email protected]>

egibs commented Oct 4, 2024

View reviewed changes

egibs added 4 commits October 4, 2024 13:18

Appease the linter

5730fca

Signed-off-by: egibs <[email protected]>

Remove unused struct

82cee51

Signed-off-by: egibs <[email protected]>

Only pass in the behavior slices

bcc993f

Signed-off-by: egibs <[email protected]>

Fix rule that was causing the archive scan test to break

c017781

Signed-off-by: egibs <[email protected]>

egibs commented Oct 4, 2024

View reviewed changes

tstromberg approved these changes Oct 4, 2024

View reviewed changes

egibs merged commit ab8a15b into chainguard-dev:main Oct 4, 2024
6 checks passed

egibs deleted the fix-overrides branch October 6, 2024 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handling of overrides after all fr.Behaviors are added #487

Better handling of overrides after all fr.Behaviors are added #487

egibs commented Oct 4, 2024

egibs Oct 4, 2024

tstromberg Oct 4, 2024 •

edited

Loading

egibs Oct 4, 2024

egibs commented Oct 4, 2024

tstromberg left a comment

egibs Oct 4, 2024

tstromberg Oct 4, 2024 •

edited

Loading

egibs Oct 4, 2024

egibs commented Oct 4, 2024

tstromberg Oct 4, 2024 •

edited

Loading

tstromberg commented Oct 4, 2024

tstromberg Oct 4, 2024

egibs Oct 4, 2024

tstromberg Oct 4, 2024

egibs Oct 4, 2024

tstromberg Oct 4, 2024

egibs Oct 4, 2024

tstromberg Oct 4, 2024 •

edited

Loading

egibs Oct 4, 2024

egibs Oct 4, 2024 •

edited

Loading

egibs Oct 4, 2024 •

edited

Loading

		@@ -521,6 +517,72 @@ func Generate(ctx context.Context, path string, mrs yara.MatchRules, c malconten
		// TODO: If we match multiple rules within a single namespace, merge matchstrings
		}

	rule go_dns_refs {
	meta:
	description = "Uses DNS (Domain Name Service)"
	strings:
	$dnsmessage = "dnsmessage"
	$edns = "SetEDNS0"
	$cname = "CNAMEResource"
	$nodejs = /require\(['"]dns['"]\)/
	condition:
	any of them
	}

	rule go_dns_refs {
	meta:
	description = "Examines local DNS servers"
	strings:
	$resolv = "resolv.conf" fullword
	$dns_getservers = "dns.getServers"
	$cname = "CNAMEResource"
	condition:
	any of them
	}

Better handling of overrides after all fr.Behaviors are added #487

Better handling of overrides after all fr.Behaviors are added #487

Conversation

egibs commented Oct 4, 2024

Choose a reason for hiding this comment

tstromberg Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egibs commented Oct 4, 2024

tstromberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egibs commented Oct 4, 2024

tstromberg Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

tstromberg commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstromberg Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egibs Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

egibs Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

tstromberg Oct 4, 2024 •

edited

Loading

tstromberg Oct 4, 2024 •

edited

Loading

tstromberg Oct 4, 2024 •

edited

Loading

tstromberg Oct 4, 2024 •

edited

Loading

egibs Oct 4, 2024 •

edited

Loading

egibs Oct 4, 2024 •

edited

Loading