allowing wildcards in exec args#805
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTrust kernel-reported ChangesExec-path spoofing defense
Overlay label late-propagation handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@coderabbitai review Collective re-review request after applying your atomic-split advisory + rabbit-feedback fixes across the full SBOB upstream PR set. Each PR is now scoped to a single tier per your dependency map: Tier 0 (opens):
Tier 2 (execs):
Tier 3 (network):
Tier 4 (tamper/sign):
Umbrellas:
Please confirm atomicity per your earlier advisory and flag any remaining cross-PR coupling we missed. |
🧩 Analysis chain
🏁 Script executed: Length of output: 3046 🏁 Script executed: Length of output: 12669
✅ Atomicity: Confirmed CleanPR
No file overlap with any sibling PR in the tier map:
🔗 Cross-PR Coupling FlagsExpected (clean tier dependency):
Rebase risk (not
Cross-repo Tier 2 sibling:
🐛 Code-Level Issues Found1. Stale docstring in
|
| // 3. argv[0] when bare and exepath empty (fexecve / AT_EMPTY_PATH). | ||
| // 4. comm as last resort. | ||
| func resolveExecPath(exepath, comm string, args []string) string { | ||
| if len(args) > 0 && len(args[0]) > 0 && args[0][0] == '/' { |
There was a problem hiding this comment.
argv[0] is still fully user-controlled here, even when it is absolute. The Linux execve(2) contract only says argv[0] should contain the executed filename by convention; it is not kernel-verified. A quick repro is exec -a /bin/sh sleep 2, which yields cmdline=/bin/sh 2 while /proc/<pid>/exe still points to /usr/bin/sleep.
So this new tier-1 rule lets a process masquerade as /bin/sh (or any other approved absolute path) while the real executable is something else, which reopens the spoofing hole the old exepath-first behavior was protecting against.
There was a problem hiding this comment.
ok, now addressed also covered in tests. It changes the behavior now in that it resolves across a symlink, this may break peoples profiles if they use linked executables. but it makes it tamper proof (at least thats my intention) .
there is also a fallback for people that still use the cel rules with 2 args . so the new behavior is for using 3 args
| if len(values) != 3 { | ||
| return types.NewErr("expected 3 arguments, got %d", len(values)) | ||
| } | ||
| return l.getExecPathWithExePath(values[0], values[1], values[2]) |
There was a problem hiding this comment.
Adding the 3-arg overload alone doesn't actually fix the fork-shell mismatch in this repo: the bundled rule expressions still call parse.get_exec_path(event.args, event.comm) (see tests/chart/templates/node-agent/default-rules.yaml at lines 21, 201, 336, 361, and 434), so they never hit this new event.exepath fallback.
That means cases like sh -c ... still evaluate to bare sh on the rule side while the recording side now stores /bin/sh, so the regression remains until the rule definitions are migrated too.
There was a problem hiding this comment.
should be addressed by my comment above.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/rulemanager/cel/expression_rewrite.go`:
- Around line 39-58: The current rewriteDeprecatedHelpers function uses raw
regexes (twoArgEventGetExecPath / anyTwoArgGetExecPath) to replace text which
can mutate string literals; change this to parse the CEL expression into an AST,
walk the AST to find CallExpr nodes that match the 2-arg parse.get_exec_path
pattern (with event.args/event.comm) and only rewrite those call nodes to the
3-arg form, leaving string literals and other nodes untouched, then serialize
the modified AST back to source and preserve the existing notices logic (append
the same messages when you perform an AST rewrite or when non-upgradable 2-arg
forms are found) so callers of rewriteDeprecatedHelpers get identical notice
semantics while avoiding rewriting inside literals.
In `@pkg/rulemanager/cel/libraries/parse/parsing_test.go`:
- Around line 139-146: Update the contract comment in
TestGetExecPath_SymmetryWithRecordingSide to match the actual resolver
precedence used by resolveExecPath: the kernel-provided exepath is
authoritative; argv[0] is used only when exepath is empty; and comm is used
last. Replace the current wording that implies unconditional argv[0] precedence
with a precise sequence: exepath, then argv[0] if exepath is empty, then comm,
and mention that this is the value stored in the ApplicationProfile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0edff09b-d5e8-4cee-9b43-0a704c31c875
📒 Files selected for processing (10)
pkg/containerprofilemanager/v1/event_reporting.gopkg/containerprofilemanager/v1/event_reporting_test.gopkg/rulemanager/cel/cel.gopkg/rulemanager/cel/expression_rewrite.gopkg/rulemanager/cel/expression_rewrite_test.gopkg/rulemanager/cel/libraries/parse/default_rules_yaml_lint_test.gopkg/rulemanager/cel/libraries/parse/parse.gopkg/rulemanager/cel/libraries/parse/parselib.gopkg/rulemanager/cel/libraries/parse/parsing_test.gotests/chart/templates/node-agent/default-rules.yaml
| func rewriteDeprecatedHelpers(expression string) (string, []string) { | ||
| var notices []string | ||
|
|
||
| if twoArgEventGetExecPath.MatchString(expression) { | ||
| original := expression | ||
| expression = twoArgEventGetExecPath.ReplaceAllString(expression, | ||
| "parse.get_exec_path(event.args, event.comm, event.exepath)") | ||
| notices = append(notices, fmt.Sprintf( | ||
| "auto-rewrote 2-arg parse.get_exec_path → 3-arg with event.exepath fallback; the 2-arg form trusts argv[0] which is user-controllable and bypassable via `exec -a <allowed-path> <real-binary>`. Update RuleBindings to call the 3-arg form explicitly. Original expression: %q", | ||
| original)) | ||
| } | ||
|
|
||
| // After auto-rewrite the canonical pattern is gone. Anything still | ||
| // matching the 2-arg shape is non-canonical — flag it for operator | ||
| // attention but do not modify. | ||
| for _, m := range anyTwoArgGetExecPath.FindAllString(expression, -1) { | ||
| notices = append(notices, fmt.Sprintf( | ||
| "2-arg parse.get_exec_path with non-event arguments cannot be auto-upgraded and remains argv[0]-spoofable: %s", | ||
| m)) | ||
| } |
There was a problem hiding this comment.
Avoid raw-regex rewrites that can modify CEL string literals.
Line 44 rewrites source text globally, so a literal like "parse.get_exec_path(event.args, event.comm)" is also rewritten. That changes expression semantics unexpectedly. Please rewrite only parsed call-expression nodes (AST-based transform) and leave literals untouched.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/rulemanager/cel/expression_rewrite.go` around lines 39 - 58, The current
rewriteDeprecatedHelpers function uses raw regexes (twoArgEventGetExecPath /
anyTwoArgGetExecPath) to replace text which can mutate string literals; change
this to parse the CEL expression into an AST, walk the AST to find CallExpr
nodes that match the 2-arg parse.get_exec_path pattern (with
event.args/event.comm) and only rewrite those call nodes to the 3-arg form,
leaving string literals and other nodes untouched, then serialize the modified
AST back to source and preserve the existing notices logic (append the same
messages when you perform an AST rewrite or when non-upgradable 2-arg forms are
found) so callers of rewriteDeprecatedHelpers get identical notice semantics
while avoiding rewriting inside literals.
There was a problem hiding this comment.
hi rabbit, thats supposed to be a shim, so that users that dont upgrade to the new default rules, dont have a breaking expierence. So should I put a "this will be deprecated in next major version comment"?
Ideally users will use the new tamper proof rules
There was a problem hiding this comment.
@entlein — yes, a comment (both in code and in the operator-visible notice) is the right move here. The shim design is fine; my concern about string literals is theoretically valid but practically very low-risk since nobody writes CEL security rules with string literals that happen to contain the exact call pattern.
Here's what I'd suggest adding to make the intent and lifecycle explicit:
1. A prominent code comment on rewriteDeprecatedHelpers:
// rewriteDeprecatedHelpers is a compatibility shim that auto-upgrades
// deprecated 2-arg parse.get_exec_path calls to the safer 3-arg form.
// It is intentionally implemented as a text-level rewrite rather than an
// AST transform. This trades theoretical risk of rewriting inside a string
// literal (considered negligible for CEL security-rule expressions) for
// zero dependency on the CEL parser at this layer.
//
// DEPRECATION NOTICE: The 2-arg parse.get_exec_path overload is deprecated
// as of <version>. This shim will be removed in the next major version.
// Operators should migrate RuleBindings to the 3-arg form explicitly.2. Update the operator-visible notice string to include a "deprecated as of X, will be removed in next major" line, e.g.:
"[DEPRECATED as of vX.Y, to be removed in next major] auto-rewrote 2-arg parse.get_exec_path → ..."This way the shim intent is self-documenting, operators see the signal at log time, and reviewers understand the conscious tradeoff. No AST machinery needed.
Would you like me to open a tracking issue for the removal so it doesn't get forgotten?
🐇 thoughtful nod
🧠 Learnings used
Learnt from: slashben
Repo: kubescape/node-agent PR: 753
File: pkg/sbomscanner/v1/server.go:62-64
Timestamp: 2026-03-23T09:40:51.594Z
Learning: When reviewing Go code that uses Syft’s `CreateSBOMConfig` builder methods (e.g., `WithCatalogers`, `WithCatalogerSelection`, `WithFilesConfig`, etc.), note they use pointer receivers and mutate the `CreateSBOMConfig` in place, then return the same `*CreateSBOMConfig` for chaining. Therefore, expressions like `cfg.WithCatalogers(...)` are functionally correct even if the return value is not captured, as long as `cfg` is a non-nil `*CreateSBOMConfig` (e.g., from `DefaultCreateSBOMConfig()`). Don’t require assignment purely to “preserve” builder return values for this specific Syft type.
|
claude, please create a new e2e benchmark between NA805 and ST322 and link the results here |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Rechecked the previous blockers. The exec-path resolver is back to kernel-authoritative exepath-first behavior on both the recording and CEL sides, and the bundled default rules now use the 3-arg parse.get_exec_path(..., event.exepath) form. I don't see any remaining blockers in the current diff.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Prevents Go MVS from picking up a higher storage version pulled in transitively by other deps. PR kubescape#805 (parse.get_exec_path 3-arg overload, event_reporting spoof revert, CEL auto-rewrite shim) does not reference any post-v0.0.258 storage symbol, so v0.0.258 is the floor — and now also the ceiling.
Prevents Go MVS from picking up a higher storage version pulled in transitively by other deps. PR kubescape#805 (parse.get_exec_path 3-arg overload, event_reporting spoof revert, CEL auto-rewrite shim) does not reference any post-v0.0.258 storage symbol, so v0.0.258 is the floor — and now also the ceiling. Signed-off-by: entlein <einentlein@gmail.com>
5403e02 to
d9a6041
Compare
|
Picking up your earlier ask about an e2e between this PR and Starting with the build leg: image build kicked off via the fork's regression harness.
|
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
…just maybe Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
…rage , lets do a benchmark and get this green before pivoting abck to storage Signed-off-by: entlein <einentlein@gmail.com>
…x, good thing this ll be squashed Signed-off-by: entlein <einentlein@gmail.com>
…se the cel rule will be slow Signed-off-by: entlein <einentlein@gmail.com>
…ow various wildcards Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
Signed-off-by: entlein <einentlein@gmail.com>
7c4f269 to
40647b0
Compare
regress-upstream-pr-mirror: upstream-pr=kubescape#805 source=upstream-pr/sbob-execs
| // this entry matches ONLY `echo star *` and must NOT broaden | ||
| // to `echo star <other>`. CT-level mirror of storage's | ||
| // TestAP_LiteralStarVsDynamic. (busybox + symlink forms.) | ||
| {Path: "/bin/echo", Args: []string{"/bin/echo", "star", "*"}}, |
There was a problem hiding this comment.
@matthyx now testing for the confusion with the former wildcard char (we dont use * anymore)
|
Component tests: 21/21 passing ✅ Validated this PR rebased on top of #807, with the storage side from kubescape/storage#328 (exec-arg matching moved into storage), on a coherent node-agent + storage image pair built from these branches. Full matrix green, including the exec-args case: Benchmark for this exact stack is tracked separately (like-for-like against the current merge-base). |
|
Benchmark — the exec-arg move pays off: node-agent memory down ~20% ✅ Like-for-like isolation: storage held constant at kubescape/storage#328 in both arms, so the only variable is this PR's exec-arg commits.
Quality gate passed (no metric degraded beyond 10%). Moving exec-arg matching out of the node-agent CEL path into storage cuts node-agent memory by ~20% at flat CPU — pairs with the green component-test matrix above. |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Rechecked the current head. The earlier exec-arg matching blocker in exec.go is fixed now, and the targeted applicationprofile tests are green.
Signed-off-by: entlein <einentlein@gmail.com>
|
Superseded and split: the engine (3-arg |
…n Test_27 #3 (the original #805 intent): bump kubescape/storage v0.0.278 -> v0.0.287, which introduces ExecArgsWildcard ("⋯⋯") as the zero-or-more exec-args wildcard and makes "*" a literal in exec args. Migrate exec_test.go's "*"-as-wildcard profile entries to dynamicpathdetector.ExecArgsWildcard, revert Test_32 from the v0.0.278 WildcardIdentifier stopgap back to ExecArgsWildcard, and un-skip echo_literal_star_does_not_broaden_R0040 (it now passes — "*" is literal). Engine code is unchanged: it routes through dynamicpathdetector.MatchExecArgs (stable signature); only test fixtures supply the sentinel. #2: Test_27_ApplicationProfileOpens enables R0002 ("Files Access Anomalies"), which ships disabled in the synced bundle, for its own cluster only — by applying tests/resources/r0002-files-access-enabled.yaml (an override Rules CRD with R0002 enabled). The rules-watcher filters disabled rules before its per-ID merge, so it uses the enabled override; default-rules.yaml is untouched. Component-test matrix jobs run on isolated clusters. (Test_33 also exercises R0002 but is not in the matrix.) Unit tests (applicationprofile, parse, objectcache) pass under v0.0.287; the component tests compile. Docs-exempt: dependency bump + test updates; the exec-args wildcard sentinel is documented in storage and the rulelibrary R0040 rule, not in node-agent docs. Signed-off-by: matthyx <matthias.bertschy@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…st_32 + lint guard (#844) * test(rules): sync default-rules.yaml from rulelibrary; add R0040, Test_32, lint guard Syncs the bundled tests/chart/templates/node-agent/default-rules.yaml from kubescape/rulelibrary's rules-crd.yaml (the source of truth after rulelibrary#39): - adds R0040 "Unexpected process arguments" (3-arg get_exec_path overload + ap.was_executed_with_args, both in node-agent v0.3.147) - the migrated exec rules (R0001/R0007/R1001/R1004) use the backward-compatible exepath-first ternary rather than the 3-arg overload - adds R2000/R2001 and picks up the rule renames already in rulelibrary - reconciles default-rule-binding.yaml: enables R0040 and updates the renamed rule names (R1000/R1003/R1015/R2001) so every binding still resolves Adds Test_32_UnexpectedProcessArguments (+ fixtures) and registers it in the component-tests matrix. Test_32 uses dynamicpathdetector.WildcardIdentifier (the zero-or-more exec-args sentinel in the pinned storage v0.0.278), matching the existing exec_test.go convention. The ExecArgsWildcard ("⋯⋯") form and the "*"->literal flip arrived in storage v0.0.287 and are a separate coordinated storage bump. Adds pkg/rulemanager/cel/libraries/parse/default_rules_yaml_lint_test.go to guard the bundled rules: no UNGUARDED 2-arg get_exec_path (the exepath-first ternary or the 3-arg form is required), no string(event.args), and R1000 detects /dev/shm via argv[0]. All three pass against the synced bundle. Replaces node-agent#805 (its engine landed separately via #843). Signed-off-by: matthyx <matthias.bertschy@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Docs-exempt: test-only + rule-bundle sync; node-agent engine/behavior unchanged (R0040 engine shipped in #843, rule bodies are sourced from kubescape/rulelibrary). * test: fix Test_02 rule rename; skip storage-version-dependent Test_32 case - Test_02_AllAlertsFromMaliciousApp: update the R1000 expectation key from the old "Process executed from malicious source" to the synced name "Process Executed from /dev/shm" (rule renamed in rulelibrary). - Test_32/echo_literal_star_does_not_broaden_R0040: t.Skip — it asserts the "*"-is-literal semantics introduced in storage v0.0.287, but main pins v0.0.278 where "*" (WildcardIdentifier) is a zero-or-more wildcard, so a literal "*" does broaden. Re-enable with the coordinated storage bump. The other 9 Test_32 subtests pass. Test_27 (R0002 was disabled by the wholesale bundle sync) is still pending a decision on whether to keep R0002 enabled in node-agent's bundle. Docs-exempt: test-only fix, no node-agent behavioral/API change. Signed-off-by: matthyx <matthias.bertschy@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(rules): bump storage v0.0.287 (ExecArgsWildcard); enable R0002 in Test_27 #3 (the original #805 intent): bump kubescape/storage v0.0.278 -> v0.0.287, which introduces ExecArgsWildcard ("⋯⋯") as the zero-or-more exec-args wildcard and makes "*" a literal in exec args. Migrate exec_test.go's "*"-as-wildcard profile entries to dynamicpathdetector.ExecArgsWildcard, revert Test_32 from the v0.0.278 WildcardIdentifier stopgap back to ExecArgsWildcard, and un-skip echo_literal_star_does_not_broaden_R0040 (it now passes — "*" is literal). Engine code is unchanged: it routes through dynamicpathdetector.MatchExecArgs (stable signature); only test fixtures supply the sentinel. #2: Test_27_ApplicationProfileOpens enables R0002 ("Files Access Anomalies"), which ships disabled in the synced bundle, for its own cluster only — by applying tests/resources/r0002-files-access-enabled.yaml (an override Rules CRD with R0002 enabled). The rules-watcher filters disabled rules before its per-ID merge, so it uses the enabled override; default-rules.yaml is untouched. Component-test matrix jobs run on isolated clusters. (Test_33 also exercises R0002 but is not in the matrix.) Unit tests (applicationprofile, parse, objectcache) pass under v0.0.287; the component tests compile. Docs-exempt: dependency bump + test updates; the exec-args wildcard sentinel is documented in storage and the rulelibrary R0040 rule, not in node-agent docs. Signed-off-by: matthyx <matthias.bertschy@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test: also enable R0002 in Test_02; share the enable helper Test_02_AllAlertsFromMaliciousApp expects "Files Access Anomalies in container" (R0002) to fire, but R0002 ships disabled in the synced bundle — so last commit's rename fix alone left Test_02 red on R0002. Extract enableR0002ForTest (apply the override Rules CRD, return a cleanup func) and use it from both Test_02 and Test_27, replacing Test_27's inline copy. Docs-exempt: test-only change. Signed-off-by: matthyx <matthias.bertschy@gmail.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Signed-off-by: matthyx <matthias.bertschy@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Overview
Adds a 3-arg
parse.get_exec_path(args, comm, exepath)CEL overloadand recording-side
resolveExecPathso rules that need to distinguisha symlinked exec from its underlying binary (busybox utilities,
buildroot-style image where every /bin/* is a symlink to /bin/busybox)
get the absolute argv[0] path when available, falling back to exepath
then the bare comm.
Additional Information
has a sister PR in storage
Checklist before requesting a review
put an [x] in the box to get it checked
Benchmark: all incoming PRs have been benchmarked
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests