Conversation
parse.get_exec_path(event.args, event.comm) returns argv[0] verbatim, which disagrees with the kernel-authoritative event.exepath that the recorder stores in the AP for two real-world patterns: - Relative argv[0] (e.g. "./python") — executes from cwd, AP records the resolved absolute path. - Empty argv[0] from fexecve / AT_EMPTY_PATH (e.g. modern libpam -> unix_chkpwd) — AP records the resolved exepath. In both cases the rule's lookup key (argv[0]) does not match the AP's storage key (exepath), so the rule fires on a legitimate, profiled exec. This patch adds a second AP lookup using event.exepath, with an empty- string guard so legacy AP entries with Path: "" can't false-suppress. The rule fires only when both lookups miss. Mirrors the recorder-side fix in node-agent PR #800. Signed-off-by: Ben H <ben@armosec.io>
📝 WalkthroughWalkthroughThis PR extends three detection rules (R0001, R1001, R1004) to support ChangesRule Expression Updates & Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/rules/r0001-unexpected-process-launched/rule_test.go (1)
182-198: ⚡ Quick winAdd a direct regression case for legacy AP
Path: ""with empty exepath.Current “empty exepath guard” coverage does not directly assert the exact legacy-empty-path scenario called out in the PR rationale. Adding one table row would lock this down and prevent regressions.
Proposed test-case addition
@@ { name: "empty exepath fallback guard — argv[0] match suppresses", @@ description: "exepath='' must not poll the AP; argv[0] '/usr/bin/foo' alone suffices to suppress", }, + { + name: "empty exepath does not suppress via legacy empty-path AP entry", + event: &utils.StructEvent{ + Args: []string{"./foo"}, + Comm: "foo", + Container: "test", + ContainerID: "test", + EventType: utils.ExecveEventType, + ExePath: "", + Pcomm: "bash", + Pid: 1234, + }, + profileExecs: []v1beta1.ExecCalls{ + {Path: "", Args: []string{"./foo"}}, + }, + expectTrigger: true, + description: "guard should skip AP lookup on empty exepath; legacy empty-path AP entry must not suppress", + }, { name: "both miss — rule still fires",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rules/r0001-unexpected-process-launched/rule_test.go` around lines 182 - 198, Add a regression table test row that covers the legacy policy-agent empty-path case: create a case where the event has ExePath: "" and Args: []string{"/usr/bin/foo"} (Comm "foo", Pcomm "bash", EventType utils.ExecveEventType, etc.) and profileExecs contains a legacy ExecCalls entry with Path: "" and Args: []string{"/usr/bin/foo"}; set expectTrigger to false and give it a descriptive name like "legacy empty path guard — argv[0] match suppresses" so the test ensures empty exepath + legacy Path:"" is handled as a suppressing match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/rules/r0001-unexpected-process-launched/rule_test.go`:
- Around line 182-198: Add a regression table test row that covers the legacy
policy-agent empty-path case: create a case where the event has ExePath: "" and
Args: []string{"/usr/bin/foo"} (Comm "foo", Pcomm "bash", EventType
utils.ExecveEventType, etc.) and profileExecs contains a legacy ExecCalls entry
with Path: "" and Args: []string{"/usr/bin/foo"}; set expectTrigger to false and
give it a descriptive name like "legacy empty path guard — argv[0] match
suppresses" so the test ensures empty exepath + legacy Path:"" is handled as a
suppressing match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fc2f298-67a2-4523-8f96-33cef865be09
📒 Files selected for processing (6)
pkg/rules/r0001-unexpected-process-launched/rule_test.gopkg/rules/r0001-unexpected-process-launched/unexpected-process-launched.yamlpkg/rules/r1001-exec-binary-not-in-base-image/exec-binary-not-in-base-image.yamlpkg/rules/r1001-exec-binary-not-in-base-image/rule_test.gopkg/rules/r1004-exec-from-mount/exec-from-mount.yamlpkg/rules/r1004-exec-from-mount/rule_test.go
|
as I said, just check R0007 for safety |
Mirrors the same fix applied to R0001/R1001/R1004 in this branch: parse.get_exec_path(event.args, event.comm) returns argv[0] verbatim, which disagrees with the kernel-authoritative event.exepath that the recorder stores in the AP. The exec branch of R0007 has the same shape, so apply the same per-rule patch with empty-exepath guard. Suggested by Matthias on PR review. Signed-off-by: Ben H <ben@armosec.io>
|
Good call — same pattern in R0007's exec branch ( Pushed |
Summary
Patches R0001, R0007, R1001, R1004 to also check
event.exepathagainst the AP when the originalparse.get_exec_path(event.args, event.comm)lookup misses. Closes the symmetric-with-recorder gap left by node-agent PR #800: the recorder prefers kernel-authoritativeevent.exepath, so the rule's lookup key should match.Why
parse.get_exec_path(event.args, event.comm)returnsargv[0]verbatim. For two real-world patterns, that disagrees with the path the AP recorder stores:./pythonfrom a_work/_tool/Python/<ver>/x64/bincwd). AP records the resolved absolute path (post-PR #800, kernel-authoritative). Rule looks up./python→ no match → fires.AT_EMPTY_PATH): modern libpam invokesunix_chkpwdviafexecve, which surfaces asargv[0] == "". PR #800 fixes the recorder to fall back to exepath; this PR mirrors the fix on the rule side.Plus argv[0] spoofing as a quiet detection gap covered for free by the same change.
Approach
Per-rule expression patch — no helper changes. Each rule keeps the existing
parse.get_exec_path(...)lookup and adds a secondap.was_executed(..., event.exepath)lookup with an empty-string guard. Suppresses if either lookup matches; fires only when both miss.The empty-string guard avoids false suppression for legacy AP entries with
Path: ""(pre-#800 unix_chkpwd recordings — separate I013 issue).Why not change
parse.get_exec_path?Considered, but signature changes propagate across every rule call site and test. Operational burden too high for the value. The per-rule patch is three lines of CEL across three YAMLs.
Test plan
go test ./pkg/rules/r0001-unexpected-process-launched/...go test ./pkg/rules/r1001-exec-binary-not-in-base-image/...go test ./pkg/rules/r1004-exec-from-mount/..../python), empty argv[0] (fexecve), empty exepath fallback, both-miss-still-fires.References