Bugfix/rules - Githubissues

amitschendel commented 8 months ago

User description

Overview

Type

bug_fix, tests, enhancement

Description

Introduced new rules for detecting fileless execution, XMR crypto mining, crypto mining domain communication, and crypto mining related port usage.
Fixed relative path handling in malicious source execution rule.
Added tests for new and modified rules.

Changes walkthrough

Relevant files

Enhancement

5 files

factory.go `Update Rule Factory to Include New and Modified Rules` pkg/ruleengine/v1/factory.go Enabled `R0003UnexpectedSystemCallRuleDescriptor`. Added `R1005FilelessExecutionRuleDescriptor`, `R1007XMRCryptoMiningRuleDescriptor`, `R1008CryptoMiningDomainCommunicationRuleDescriptor`, and `R1009CryptoMiningRelatedPortRuleDescriptor` to the rule factory.	+7/-2
r1005_fileless_execution.go `Introduce Fileless Execution Detection Rule` pkg/ruleengine/v1/r1005_fileless_execution.go - Introduced a new rule for detecting fileless execution.	+151/-0
r1007_xmr_crypto_mining.go `Introduce XMR Crypto Mining Detection Rule` pkg/ruleengine/v1/r1007_xmr_crypto_mining.go Introduced a new rule for detecting XMR crypto mining through randomx algorithm usage.	+96/-0
r1008_crypto_mining_domain.go `Introduce Crypto Mining Domain Communication Detection Rule` pkg/ruleengine/v1/r1008_crypto_mining_domain.go Introduced a new rule for detecting crypto mining domain communication.	+23/-86
r1009_crypto_mining_port.go `Introduce Crypto Mining Related Port Detection Rule` pkg/ruleengine/v1/r1009_crypto_mining_port.go Introduced a new rule for detecting crypto mining related port usage.	+101/-0

Bug_fix

2 files

r1000_exec_from_malicious_source.go `Fix Relative Path Handling in Malicious Source Execution Rule` pkg/ruleengine/v1/r1000_exec_from_malicious_source.go Fixed the handling of relative paths in `execPath` to ensure they are absolute.	+7/-1
rule_manager.go `Comment Out OCI Config Code with TODO for Syscall Details` pkg/rulemanager/v1/rule_manager.go Commented out OCI config related code with a TODO for figuring out UID, GID, and comm from the syscall.	+4/-3

Tests

5 files

r1000_exec_from_malicious_source_test.go `Add Tests for Malicious Source Execution Rule Path Handling` pkg/ruleengine/v1/r1000_exec_from_malicious_source_test.go Added tests for handling of different `execPath` scenarios in malicious source execution rule.	+38/-0
r1005_fileless_execution_test.go `Add Tests for Fileless Execution Detection Rule` pkg/ruleengine/v1/r1005_fileless_execution_test.go - Added tests for the fileless execution detection rule.	+76/-0
r1007_xmr_crypto_mining_test.go `Add Tests for XMR Crypto Mining Detection Rule` pkg/ruleengine/v1/r1007_xmr_crypto_mining_test.go - Added tests for the XMR crypto mining detection rule.	+31/-0
r1008_crypto_mining_domain_test.go `Add Tests for Crypto Mining Domain Communication Detection Rule` pkg/ruleengine/v1/r1008_crypto_mining_domain_test.go Added tests for the crypto mining domain communication detection rule.	+39/-0
r1009_crypto_mining_port_test.go `Add Tests for Crypto Mining Related Port Detection Rule` pkg/ruleengine/v1/r1009_crypto_mining_port_test.go - Added tests for the crypto mining related port detection rule.	+45/-0

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-free[bot] commented 8 months ago

PR Description updated to latest commit (https://github.com/kubescape/node-agent/commit/5b74fa069fb0ae6a45d5ebc2c47888b517cc3fa4)

codiumai-pr-agent-free[bot] commented 8 months ago

PR Review

⏱️ Estimated effort to review [1-5]	2, because the PR introduces several new rules and modifications to existing ones, focusing on crypto mining detection and handling of fileless execution. The changes are well-structured and include tests, which should make the review process straightforward. However, understanding the context and implications of these rules requires domain-specific knowledge.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: In `pkg/ruleengine/v1/r1000_exec_from_malicious_source.go`, the handling of relative paths (`"./"` and `"../"`) assumes that the current working directory (`execEvent.Cwd`) is always correctly set and that the path construction is secure. Consider edge cases where `execEvent.Cwd` could be manipulated or not safely handled.
🔍 Possible issues	Performance Concern: The addition of multiple crypto mining detection rules (`R1005FilelessExecution`, `R1007XMRCryptoMining`, `R1008CryptoMiningDomainCommunication`, `R1009CryptoMiningRelatedPort`) might impact the system's performance, especially in environments with high network traffic or system call rates. It's recommended to evaluate the performance impact of these rules in a real-world scenario.
🔒 Security concerns	No

✨ Review tool usage guide:

**Overview:** The `review` tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be [added](https://pr-agent-docs.codium.ai/tools/review/#general-configurations) by configuring the tool. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on any PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L23) related to the review tool (`pr_reviewer` section), use the following template: ``` /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_reviewer] some_config1=... some_config2=... ``` See the review [usage page](https://pr-agent-docs.codium.ai/tools/review/) for a comprehensive guide on using this tool.

codiumai-pr-agent-free[bot] commented 8 months ago

PR Code Suggestions

Category	Suggestions
Enhancement	Add error handling for type assertion failure in `ProcessEvent`. ___ Consider adding error handling or logging for the `ProcessEvent` method to handle cases where the type assertion fails. This will improve the robustness of the code and ensure that unexpected types do not cause silent failures. [pkg/ruleengine/v1/r1008_crypto_mining_domain.go [168]](https://github.com/kubescape/node-agent/pull/239/files#diff-6789c9f27a5f55cdaf52ed9dbf4b560714092348f28a38ccdfe86cc64e356259R168-R168) ```diff if dnsEvent, ok := event.(*tracerdnstype.Event); ok { +} else { + // Log error or handle the unexpected type +} ```
	Implement a reset mechanism for `alreadyNotified` to allow re-alerting. ___ Consider implementing a mechanism to reset `alreadyNotified` back to `false` under certain conditions. This would allow the rule to trigger alerts again if the same malicious activity reoccurs after being resolved. [pkg/ruleengine/v1/r1005_fileless_execution.go [76]](https://github.com/kubescape/node-agent/pull/239/files#diff-ef0155bc510f47092b9799316b90b72f6eda3347ee96da897e27fe9961f06792R76-R76) ```diff rule.alreadyNotified = true +// Implement mechanism to reset alreadyNotified under certain conditions ```
	Convert to table-driven tests for better maintainability. ___ To improve test readability and maintainability, consider using table-driven tests for `TestHandleExecveEvent`. This approach allows you to define test cases in a structured format and reduces code duplication by iterating over them. [pkg/ruleengine/v1/r1005_fileless_execution_test.go [13-75]](https://github.com/kubescape/node-agent/pull/239/files#diff-f71fc9abe6b0df34fe5a98c83e804e93d802710192a482a9643f9d776de3f9c5R13-R75) ```diff -t.Run("Test with /proc/self/fd prefix", func(t testing.T) { -... -t.Run("Test without /proc/self/fd prefix", func(t testing.T) { +tests := []struct { + name string + event tracerexectype.Event + result bool // true if result is expected to be not nil, false otherwise +}{ + {"Test with /proc/self/fd prefix", &tracerexectype.Event{Cwd: "/proc/self/fd", ...}, true}, + {"Test without /proc/self/fd prefix", &tracerexectype.Event{Cwd: "/not/proc/self/fd", ...}, false}, + ... +} +for _, tc := range tests { + t.Run(tc.name, func(t testing.T) { + result := rule.handleExecveEvent(tc.event) + if tc.result { + assert.NotNil(t, result) + } else { + assert.Nil(t, result) + } + }) +} ```
	Refactor tests into table-driven format for maintainability. ___ To avoid code duplication and improve test maintainability, consider refactoring the test cases into table-driven tests. This approach allows defining test cases in a structured format and iterating over them, which simplifies adding or modifying tests in the future. [pkg/ruleengine/v1/r1008_crypto_mining_domain_test.go [31-37]](https://github.com/kubescape/node-agent/pull/239/files#diff-310d2c2e8b82368f61711e9df5623a398fa2af7cb7a40f87a7eea8a2ce56c054R31-R37) ```diff -e2.DNSName = "xmr.gntl.uk" -... -e2.DNSName = "amit.com" +tests := []struct { + name string + DNSName string + expected bool // true if ruleResult is expected, false otherwise +}{ + {"Domain in crypto miners list", "xmr.gntl.uk", true}, + {"Domain not in crypto miners list", "amit.com", false}, +} +for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + e2.DNSName = tc.DNSName + ruleResult := r.ProcessEvent(utils.DnsEventType, e2, &RuleObjectCacheMock{}) + if tc.expected { + assert.NotNil(t, ruleResult, "Expected ruleResult to be not nil") + } else { + assert.Nil(t, ruleResult, "Expected ruleResult to be nil") + } + }) +} ```
Security	Validate or sanitize `execPath` to prevent path traversal. ___ To avoid potential path traversal vulnerabilities, validate or sanitize the `execPath` before using it in `handleExecveEvent`. Ensure that the path does not lead to unintended directories. [pkg/ruleengine/v1/r1005_fileless_execution.go [106]](https://github.com/kubescape/node-agent/pull/239/files#diff-ef0155bc510f47092b9799316b90b72f6eda3347ee96da897e27fe9961f06792R106-R106) ```diff execPath := getExecPathFromEvent(execEvent) +// Validate or sanitize execPath here ```
Best practice	Use constants for protocol and packet type checks. ___ Instead of checking for "TCP" and "OUTGOING" as strings, consider defining these as constants to avoid potential typos and facilitate changes in the future. [pkg/ruleengine/v1/r1009_crypto_mining_port.go [67]](https://github.com/kubescape/node-agent/pull/239/files#diff-947f67400948fbd3b67f9d5322bc87e5b74955b4d4b87ebd290a75dc6c189914R67-R67) ```diff -if networkEvent.Proto == "TCP" && networkEvent.PktType == "OUTGOING" && slices.Contains(CommonlyUsedCryptoMinersPorts, networkEvent.Port) { +if networkEvent.Proto == ProtoTCP && networkEvent.PktType == PktTypeOutgoing && slices.Contains(CommonlyUsedCryptoMinersPorts, networkEvent.Port) { ```
	Use `t.Fatalf` for immediate test termination upon error. ___ Consider using `t.Fatalf` instead of `t.Errorf` followed by a return statement in your test cases. This approach immediately terminates the current test upon encountering an error, which is more efficient and makes the test cases cleaner. [pkg/ruleengine/v1/r1000_exec_from_malicious_source_test.go [27-28]](https://github.com/kubescape/node-agent/pull/239/files#diff-0335c2abb82667ee7c0f955f8a4f1ab795958d30e89dc3965de2d67c6b610b00R27-R28) ```diff -t.Errorf("Expected ruleResult to be Failure because of dns name is in the commonly used crypto miners domains") -return +t.Fatalf("Expected ruleResult to be Failure because of dns name is in the commonly used crypto miners domains") ```
	Normalize paths using `filepath.Clean`. ___ Ensure that `filepath.Clean` is used on `execPath` after constructing it to normalize the path. This will remove any redundant `../` or `./` segments and ensure the path is in its simplest form, which is crucial for subsequent operations that might rely on path comparison or manipulation. [pkg/ruleengine/v1/r1000_exec_from_malicious_source.go [80]](https://github.com/kubescape/node-agent/pull/239/files#diff-00351ce4b75a85e267a68663f357265bbcc7721c2e461fa6112363fdfb2150c5R80-R80) ```diff -execPath = filepath.Dir(execPath) +execPath = filepath.Clean(filepath.Dir(execPath)) ```
	Use `assert` functions for concise test checks. ___ Instead of manually checking `result != nil` and printing errors, use `assert` functions for more concise and readable tests. For example, use `assert.NotNil(t, result)` when expecting a non-nil result, and `assert.Nil(t, result)` when expecting a nil result. This change enhances test readability and maintainability. [pkg/ruleengine/v1/r1009_crypto_mining_port_test.go [18-19]](https://github.com/kubescape/node-agent/pull/239/files#diff-65379ebc569ab9a4d998bb668e936706b55c60b1206fd8f1ee78daf672e154eaR18-R19) ```diff -if result != nil { - t.Errorf("Expected nil, got %v", result) -} +assert.Nil(t, result) ```
Maintainability	Refactor `ProcessEvent` logic into a separate method for clarity. ___ To improve code readability and maintainability, consider moving the logic within the `if` statement of `ProcessEvent` into a separate method. This will make the `ProcessEvent` method cleaner and easier to understand. [pkg/ruleengine/v1/r1007_xmr_crypto_mining.go [60]](https://github.com/kubescape/node-agent/pull/239/files#diff-97cc4a4c5d887aedfe4d9384769e20249ab46018c83988d9ddedf57a984016c0R60-R60) ```diff if randomXEvent, ok := event.(*tracerrandomxtype.Event); ok { + return rule.handleRandomXEvent(randomXEvent) +} ```

✨ Improve tool usage guide:

**Overview:** The `improve` tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered [automatically](https://pr-agent-docs.codium.ai/usage-guide/automations_and_usage/#github-app-automatic-tools-when-a-new-pr-is-opened) every time a new PR is opened, or can be invoked manually by commenting on a PR. - When commenting, to edit [configurations](https://github.com/Codium-ai/pr-agent/blob/main/pr_agent/settings/configuration.toml#L78) related to the improve tool (`pr_code_suggestions` section), use the following template: ``` /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=... ``` - With a [configuration file](https://pr-agent-docs.codium.ai/usage-guide/configuration_options/), use the following template: ``` [pr_code_suggestions] some_config1=... some_config2=... ``` See the improve [usage page](https://pr-agent-docs.codium.ai/tools/improve/) for a comprehensive guide on using this tool.

github-actions[bot] commented 8 months ago

Summary:

License scan: failure
Credentials scan: failure
Vulnerabilities scan: success
Unit test: success
Go linting: success

Category	Suggestions
Enhancement	Add error handling for type assertion failure in `ProcessEvent`. ___ Consider adding error handling or logging for the `ProcessEvent` method to handle cases where the type assertion fails. This will improve the robustness of the code and ensure that unexpected types do not cause silent failures. [pkg/ruleengine/v1/r1008_crypto_mining_domain.go [168]](https://github.com/kubescape/node-agent/pull/239/files#diff-6789c9f27a5f55cdaf52ed9dbf4b560714092348f28a38ccdfe86cc64e356259R168-R168) ```diff if dnsEvent, ok := event.(*tracerdnstype.Event); ok { +} else { + // Log error or handle the unexpected type +} ```
	Implement a reset mechanism for `alreadyNotified` to allow re-alerting. ___ Consider implementing a mechanism to reset `alreadyNotified` back to `false` under certain conditions. This would allow the rule to trigger alerts again if the same malicious activity reoccurs after being resolved. [pkg/ruleengine/v1/r1005_fileless_execution.go [76]](https://github.com/kubescape/node-agent/pull/239/files#diff-ef0155bc510f47092b9799316b90b72f6eda3347ee96da897e27fe9961f06792R76-R76) ```diff rule.alreadyNotified = true +// Implement mechanism to reset alreadyNotified under certain conditions ```
	Convert to table-driven tests for better maintainability. ___ To improve test readability and maintainability, consider using table-driven tests for `TestHandleExecveEvent`. This approach allows you to define test cases in a structured format and reduces code duplication by iterating over them. [pkg/ruleengine/v1/r1005_fileless_execution_test.go [13-75]](https://github.com/kubescape/node-agent/pull/239/files#diff-f71fc9abe6b0df34fe5a98c83e804e93d802710192a482a9643f9d776de3f9c5R13-R75) ```diff -t.Run("Test with /proc/self/fd prefix", func(t testing.T) { -... -t.Run("Test without /proc/self/fd prefix", func(t testing.T) { +tests := []struct { + name string + event tracerexectype.Event + result bool // true if result is expected to be not nil, false otherwise +}{ + {"Test with /proc/self/fd prefix", &tracerexectype.Event{Cwd: "/proc/self/fd", ...}, true}, + {"Test without /proc/self/fd prefix", &tracerexectype.Event{Cwd: "/not/proc/self/fd", ...}, false}, + ... +} +for _, tc := range tests { + t.Run(tc.name, func(t testing.T) { + result := rule.handleExecveEvent(tc.event) + if tc.result { + assert.NotNil(t, result) + } else { + assert.Nil(t, result) + } + }) +} ```
	Refactor tests into table-driven format for maintainability. ___ To avoid code duplication and improve test maintainability, consider refactoring the test cases into table-driven tests. This approach allows defining test cases in a structured format and iterating over them, which simplifies adding or modifying tests in the future. [pkg/ruleengine/v1/r1008_crypto_mining_domain_test.go [31-37]](https://github.com/kubescape/node-agent/pull/239/files#diff-310d2c2e8b82368f61711e9df5623a398fa2af7cb7a40f87a7eea8a2ce56c054R31-R37) ```diff -e2.DNSName = "xmr.gntl.uk" -... -e2.DNSName = "amit.com" +tests := []struct { + name string + DNSName string + expected bool // true if ruleResult is expected, false otherwise +}{ + {"Domain in crypto miners list", "xmr.gntl.uk", true}, + {"Domain not in crypto miners list", "amit.com", false}, +} +for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + e2.DNSName = tc.DNSName + ruleResult := r.ProcessEvent(utils.DnsEventType, e2, &RuleObjectCacheMock{}) + if tc.expected { + assert.NotNil(t, ruleResult, "Expected ruleResult to be not nil") + } else { + assert.Nil(t, ruleResult, "Expected ruleResult to be nil") + } + }) +} ```
Security	Validate or sanitize `execPath` to prevent path traversal. ___ To avoid potential path traversal vulnerabilities, validate or sanitize the `execPath` before using it in `handleExecveEvent`. Ensure that the path does not lead to unintended directories. [pkg/ruleengine/v1/r1005_fileless_execution.go [106]](https://github.com/kubescape/node-agent/pull/239/files#diff-ef0155bc510f47092b9799316b90b72f6eda3347ee96da897e27fe9961f06792R106-R106) ```diff execPath := getExecPathFromEvent(execEvent) +// Validate or sanitize execPath here ```
Best practice	Use constants for protocol and packet type checks. ___ Instead of checking for "TCP" and "OUTGOING" as strings, consider defining these as constants to avoid potential typos and facilitate changes in the future. [pkg/ruleengine/v1/r1009_crypto_mining_port.go [67]](https://github.com/kubescape/node-agent/pull/239/files#diff-947f67400948fbd3b67f9d5322bc87e5b74955b4d4b87ebd290a75dc6c189914R67-R67) ```diff -if networkEvent.Proto == "TCP" && networkEvent.PktType == "OUTGOING" && slices.Contains(CommonlyUsedCryptoMinersPorts, networkEvent.Port) { +if networkEvent.Proto == ProtoTCP && networkEvent.PktType == PktTypeOutgoing && slices.Contains(CommonlyUsedCryptoMinersPorts, networkEvent.Port) { ```
	Use `t.Fatalf` for immediate test termination upon error. ___ Consider using `t.Fatalf` instead of `t.Errorf` followed by a return statement in your test cases. This approach immediately terminates the current test upon encountering an error, which is more efficient and makes the test cases cleaner. [pkg/ruleengine/v1/r1000_exec_from_malicious_source_test.go [27-28]](https://github.com/kubescape/node-agent/pull/239/files#diff-0335c2abb82667ee7c0f955f8a4f1ab795958d30e89dc3965de2d67c6b610b00R27-R28) ```diff -t.Errorf("Expected ruleResult to be Failure because of dns name is in the commonly used crypto miners domains") -return +t.Fatalf("Expected ruleResult to be Failure because of dns name is in the commonly used crypto miners domains") ```
	Normalize paths using `filepath.Clean`. ___ Ensure that `filepath.Clean` is used on `execPath` after constructing it to normalize the path. This will remove any redundant `../` or `./` segments and ensure the path is in its simplest form, which is crucial for subsequent operations that might rely on path comparison or manipulation. [pkg/ruleengine/v1/r1000_exec_from_malicious_source.go [80]](https://github.com/kubescape/node-agent/pull/239/files#diff-00351ce4b75a85e267a68663f357265bbcc7721c2e461fa6112363fdfb2150c5R80-R80) ```diff -execPath = filepath.Dir(execPath) +execPath = filepath.Clean(filepath.Dir(execPath)) ```
	Use `assert` functions for concise test checks. ___ Instead of manually checking `result != nil` and printing errors, use `assert` functions for more concise and readable tests. For example, use `assert.NotNil(t, result)` when expecting a non-nil result, and `assert.Nil(t, result)` when expecting a nil result. This change enhances test readability and maintainability. [pkg/ruleengine/v1/r1009_crypto_mining_port_test.go [18-19]](https://github.com/kubescape/node-agent/pull/239/files#diff-65379ebc569ab9a4d998bb668e936706b55c60b1206fd8f1ee78daf672e154eaR18-R19) ```diff -if result != nil { - t.Errorf("Expected nil, got %v", result) -} +assert.Nil(t, result) ```
Maintainability	Refactor `ProcessEvent` logic into a separate method for clarity. ___ To improve code readability and maintainability, consider moving the logic within the `if` statement of `ProcessEvent` into a separate method. This will make the `ProcessEvent` method cleaner and easier to understand. [pkg/ruleengine/v1/r1007_xmr_crypto_mining.go [60]](https://github.com/kubescape/node-agent/pull/239/files#diff-97cc4a4c5d887aedfe4d9384769e20249ab46018c83988d9ddedf57a984016c0R60-R60) ```diff if randomXEvent, ok := event.(*tracerrandomxtype.Event); ok { + return rule.handleRandomXEvent(randomXEvent) +} ```

kubescape / node-agent

Bugfix/rules #239

User description

Overview

Type

Description

Changes walkthrough

PR Review

PR Code Suggestions