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>
entlein
force-pushed
the
upstream-pr/sbob-execs
branch
from
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.
|
Mirrors containerprofilecache.Apply's extractExecsByPath shape so exec-args unit tests exercise the real wildcard matcher instead of hitting the absent-key 'no argv constraint' back-compat fallback. Same per-Path append + nil→empty-slice rules. Fixes the four TestExecWithArgsWildcardInProfile cases that were silently returning true (back-compat) and getting marked as failing expected:false (sh -x wrong flag, ls -l no directory, echo goodbye world wrong literal anchor, curl --pass alice wrong literal). Signed-off-by: entlein <einentlein@gmail.com>
…o go mod sync with main 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>
resolveExecPath and getExecPathWithExePath both gained an "if argv[0] is absolute, trust argv[0] over exepath" tier in fd9e262. The justification (busybox-image symlink fidelity — record /bin/sh instead of /bin/busybox) does not survive the argv[0] spoofing case: `exec -a /bin/sh sleep 2` yields cmdline=/bin/sh while /proc/<pid>/exe stays /usr/bin/sleep, so the recorded identity is whatever an attacker chose, not what actually ran. ap.was_executed lookups for allowed paths then pass for arbitrary binaries. Reverts both functions to the v0.3.113 precedence: 1. exepath (kernel-authoritative) 2. argv[0] non-empty when exepath empty (fexecve / AT_EMPTY_PATH) 3. comm Busybox-image profiles record /bin/busybox (kernel-resolved) — the v0.3.113 behaviour. The symlink-faithful tier never shipped in a tagged release; only profiles built against fd9e262 depended on it, all internal. Adds explicit absolute-argv[0]-spoof regression tests on both sides (resolveExecPath and getExecPathWithExePath) pinning that `args=["/bin/sh", …], exepath="/usr/bin/sleep"` resolves to `/usr/bin/sleep`. Updated the busybox tests on both sides to reflect the kernel-authoritative semantics. Addresses matthyx review on event_reporting.go:63 (2026-05-27). 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>
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>
entlein
force-pushed
the
upstream-pr/sbob-execs
branch
from
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.
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