Open
Conversation
Detect processes launched via sandbox-exec and expose the sandbox profile path to CEL rules through `process.sandbox_policy`. - Changes the authorizer to be a TreeAwareClient.
github-actions
bot
added
comp/santad
Contributor
📝 WalkthroughWalkthroughAdds sandbox-exec detection and sandbox_policy annotations to the ProcessTree, wires ProcessTree into EndpointSecurity authorizer, passes optional process annotations into CEL Activation, exposes a new "process" CEL variable, and updates build/test targets and annotator registration. Changes
Sequence Diagram(s)sequenceDiagram
participant ES as EndpointSecurityEvent
participant Auth as SNTEndpointSecurityAuthorizer
participant PT as ProcessTree
participant ACT as Activation (CEL)
participant CEL as CEL Engine
ES->>Auth: deliver exec/fork event (audit token)
Auth->>PT: ExportAnnotations(targetPid)
PT-->>Auth: optional Annotations (sandbox_policy)
Auth->>ACT: construct Activation(..., process_annotations)
ACT->>CEL: expose variables (including process proto)
CEL->>ACT: request "process" value
ACT-->>CEL: return process proto (sets process_accessed_)
CEL->>CEL: evaluate policy (cacheability affected by access)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Source/santad/ProcessTree/annotations/sandbox_exec_test.mm (1)
183-225: Add a regression test for nestedsandbox-execre-sandboxing.Please add a case where a process with confirmed sandbox annotation execs another
/usr/bin/sandbox-exec -f <new_profile> ...and verify the new profile takes effect after the next exec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santad/ProcessTree/annotations/sandbox_exec_test.mm` around lines 183 - 225, Add a new regression test that simulates a process already annotated as confirmed-sandbox that execs another sandbox-exec with a different profile and then execs a real target, verifying the new profile is applied: reuse the existing pattern in testAnnotationPropagatesExecToChildren by creating a process, mark it (or simulate) with a confirmed SandboxExecAnnotator, call HandleExec with a Program whose .executable is "/usr/bin/sandbox-exec" and arguments including "-f" and a new profile, then call HandleExec again to a target binary and assert via tree->GetAnnotation<SandboxExecAnnotator>(...) that the annotation exists and that (*annotation)->info()->status (and any profile identifier field you have) reflects the new profile (use HandleFork/HandleExec, Get, and GetAnnotation<SandboxExecAnnotator> helpers and assert SandboxPolicyStatus::kConfirmed where appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/santad/ProcessTree/annotations/sandbox_exec.cc`:
- Around line 71-95: AnnotateExec currently checks existing annotations on
orig_process before seeing if new_process is a sandbox-exec, so nested execs
keep the old policy path; change the order so you first check
IsSandboxExec(new_process) and if true call
ParseSandboxExecArgv(new_process.program_->arguments) and, when it returns info,
call tree.AnnotateProcess(new_process,
std::make_shared<SandboxExecAnnotator>(std::move(*info))) and return; only if
new_process is not sandbox-exec continue with the existing logic that calls
tree.GetAnnotation<SandboxExecAnnotator>(orig_process), promotes pending info to
confirmed, or propagates the annotation with tree.AnnotateProcess(new_process,
...) as before.
---
Nitpick comments:
In `@Source/santad/ProcessTree/annotations/sandbox_exec_test.mm`:
- Around line 183-225: Add a new regression test that simulates a process
already annotated as confirmed-sandbox that execs another sandbox-exec with a
different profile and then execs a real target, verifying the new profile is
applied: reuse the existing pattern in testAnnotationPropagatesExecToChildren by
creating a process, mark it (or simulate) with a confirmed SandboxExecAnnotator,
call HandleExec with a Program whose .executable is "/usr/bin/sandbox-exec" and
arguments including "-f" and a new profile, then call HandleExec again to a
target binary and assert via tree->GetAnnotation<SandboxExecAnnotator>(...) that
the annotation exists and that (*annotation)->info()->status (and any profile
identifier field you have) reflects the new profile (use HandleFork/HandleExec,
Get, and GetAnnotation<SandboxExecAnnotator> helpers and assert
SandboxPolicyStatus::kConfirmed where appropriate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a8df256-2af7-4816-8e23-ee6eb00e0728
📒 Files selected for processing (17)
Source/common/cel/Activation.hSource/common/cel/Activation.mmSource/common/cel/BUILDSource/santad/BUILDSource/santad/CELActivation.mmSource/santad/EventProviders/SNTEndpointSecurityAuthorizer.hSource/santad/EventProviders/SNTEndpointSecurityAuthorizer.mmSource/santad/EventProviders/SNTEndpointSecurityAuthorizerTest.mmSource/santad/ProcessTree/annotations/BUILDSource/santad/ProcessTree/annotations/sandbox_exec.ccSource/santad/ProcessTree/annotations/sandbox_exec.hSource/santad/ProcessTree/annotations/sandbox_exec_test.mmSource/santad/ProcessTree/process_tree.ccSource/santad/ProcessTree/process_tree.protoSource/santad/Santad.mmSource/santad/SantadDeps.mmSource/santad/SantadTest.mm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Source/santad/ProcessTree/annotations/sandbox_exec_test.mm (1)
227-253: Add a regression test for malformedsandbox-execexec-path handling.Current tests cover pending/confirmed propagation well, but they do not assert behavior when
sandbox-execis exec’d and argv parsing fails. A focused test here would lock in non-propagation of stale policy state from prior annotations (relevant to Line 227 semantics).🧪 Suggested test shape
+// Test: malformed sandbox-exec argv should not inherit stale annotation on exec. +- (void)testMalformedSandboxExecExecDoesNotInheritStalePolicy { + uint64_t event_id = 1; + + // Build confirmed annotation first: sandbox-exec -f /old.sb -> target + const struct Pid fork_pid = {.pid = 2, .pidversion = 2}; + self.tree->HandleFork(event_id++, *self.initProc, fork_pid); + const struct Pid sandbox_pid = {.pid = 2, .pidversion = 3}; + const struct Program sandbox_prog = { + .executable = "/usr/bin/sandbox-exec", + .arguments = {"sandbox-exec", "-f", "/old.sb", "/usr/bin/ls"}, + .code_signing = kSandboxExecCS, + }; + auto forked = *self.tree->Get(fork_pid); + self.tree->HandleExec(event_id++, *forked, sandbox_pid, sandbox_prog, cred); + + const struct Pid confirmed_pid = {.pid = 2, .pidversion = 4}; + const struct Program confirmed_prog = {.executable = "/usr/bin/ls", .arguments = {"/usr/bin/ls"}}; + auto sandbox_proc = *self.tree->Get(sandbox_pid); + self.tree->HandleExec(event_id++, *sandbox_proc, confirmed_pid, confirmed_prog, cred); + + // Exec into sandbox-exec with malformed argv (no -f path). + const struct Pid malformed_pid = {.pid = 2, .pidversion = 5}; + const struct Program malformed_prog = { + .executable = "/usr/bin/sandbox-exec", + .arguments = {"sandbox-exec", "/usr/bin/id"}, + .code_signing = kSandboxExecCS, + }; + auto confirmed_proc = *self.tree->Get(confirmed_pid); + self.tree->HandleExec(event_id++, *confirmed_proc, malformed_pid, malformed_prog, cred); + + auto malformed_proc = *self.tree->Get(malformed_pid); + auto ann = self.tree->GetAnnotation<SandboxExecAnnotator>(*malformed_proc); + XCTAssertFalse(ann.has_value()); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santad/ProcessTree/annotations/sandbox_exec_test.mm` around lines 227 - 253, Add a regression test that simulates a malformed sandbox-exec exec (argv parsing failure) and asserts that pending sandbox-exec annotations do not propagate to forked children: reuse the existing test structure in testPendingDoesNotPropagateToFork by creating a fork via HandleFork, then call HandleExec on the forked process with a Program whose arguments/executable are intentionally malformed to force argv parsing to fail, then perform another HandleFork from the sandbox-exec process and assert via tree->Get and tree->GetAnnotation<SandboxExecAnnotator> that the child process has no annotation; ensure you exercise the same symbols (HandleExec, HandleFork, Get, GetAnnotation<SandboxExecAnnotator>) and check absence with XCTAssertFalse(child_opt.has_value()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/santad/ProcessTree/annotations/sandbox_exec.cc`:
- Around line 73-96: When a new_process is a sandbox-exec (IsSandboxExec) but
ParseSandboxExecArgv(new_process.program_->arguments) fails, the current code
falls through and can re-attach the previous SandboxExecAnnotator from
orig_process; change the logic so that if IsSandboxExec(new_process) is true and
ParseSandboxExecArgv returns no value, you do NOT propagate the prior
annotation—either return early (skip propagation) or explicitly annotate
new_process with a cleared/neutral SandboxExecAnnotator; update the block around
IsSandboxExec, ParseSandboxExecArgv, SandboxExecAnnotator, and the subsequent
tree.AnnotateProcess / tree.GetAnnotation calls to prevent carrying stale
annotations to new sandbox-exec processes.
---
Nitpick comments:
In `@Source/santad/ProcessTree/annotations/sandbox_exec_test.mm`:
- Around line 227-253: Add a regression test that simulates a malformed
sandbox-exec exec (argv parsing failure) and asserts that pending sandbox-exec
annotations do not propagate to forked children: reuse the existing test
structure in testPendingDoesNotPropagateToFork by creating a fork via
HandleFork, then call HandleExec on the forked process with a Program whose
arguments/executable are intentionally malformed to force argv parsing to fail,
then perform another HandleFork from the sandbox-exec process and assert via
tree->Get and tree->GetAnnotation<SandboxExecAnnotator> that the child process
has no annotation; ensure you exercise the same symbols (HandleExec, HandleFork,
Get, GetAnnotation<SandboxExecAnnotator>) and check absence with
XCTAssertFalse(child_opt.has_value()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6945bbd-bd84-48c1-9810-3204883ec50d
📒 Files selected for processing (2)
Source/santad/ProcessTree/annotations/sandbox_exec.ccSource/santad/ProcessTree/annotations/sandbox_exec_test.mm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Source/santad/ProcessTree/annotations/sandbox_exec_test.mm (1)
55-74: Add a regression case for-fin target command args.Please add a parser test where sandbox-exec options do not include
-f, but the target command does (e.g.,... /usr/bin/grep -f pattern). This guards against false policy-path extraction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/santad/ProcessTree/annotations/sandbox_exec_test.mm` around lines 55 - 74, Add a regression test to ensure ParseSandboxExecArgv does not misinterpret a target command’s "-f" as the sandbox-exec profile flag: in Source/santad/ProcessTree/annotations/sandbox_exec_test.mm add a test (e.g., testParseDoesNotPickFlagFromTargetCmd) that calls ParseSandboxExecArgv with args like {"sandbox-exec", "/usr/bin/grep", "-f", "pattern"} (no "-f" option for sandbox-exec itself) and assert result.has_value() is false or that profile_path is not set/incorrectly extracted (match the existing tests’ assertion style), using the existing ParseSandboxExecArgv function and test class patterns to guard against false policy-path extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Source/santad/ProcessTree/annotations/sandbox_exec.cc`:
- Around line 43-50: ParseSandboxExecArgv currently scans all argv entries and
can pick up a target-command '-f'. Limit the scan to the sandbox-exec option
scope by stopping when the option/args separator is reached (e.g., "--") or when
the sandbox-exec options end; only examine entries before that separator for
"-f". Update ParseSandboxExecArgv to return SandboxPolicyInfo with profile_path
and SandboxPolicyStatus::kPending only when "-f" is found before the separator,
referencing ParseSandboxExecArgv, SandboxPolicyInfo, profile_path, and
SandboxPolicyStatus::kPending to locate the logic to change.
---
Nitpick comments:
In `@Source/santad/ProcessTree/annotations/sandbox_exec_test.mm`:
- Around line 55-74: Add a regression test to ensure ParseSandboxExecArgv does
not misinterpret a target command’s "-f" as the sandbox-exec profile flag: in
Source/santad/ProcessTree/annotations/sandbox_exec_test.mm add a test (e.g.,
testParseDoesNotPickFlagFromTargetCmd) that calls ParseSandboxExecArgv with args
like {"sandbox-exec", "/usr/bin/grep", "-f", "pattern"} (no "-f" option for
sandbox-exec itself) and assert result.has_value() is false or that profile_path
is not set/incorrectly extracted (match the existing tests’ assertion style),
using the existing ParseSandboxExecArgv function and test class patterns to
guard against false policy-path extraction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61ffc26e-3a1a-46dc-a7a6-990d55db3aa8
📒 Files selected for processing (2)
Source/santad/ProcessTree/annotations/sandbox_exec.ccSource/santad/ProcessTree/annotations/sandbox_exec_test.mm
Comment on lines
+43
to
+50
| for (size_t i = 0; i < argv.size(); i++) { | ||
| if (argv[i] == "-f" && i + 1 < argv.size()) { | ||
| return SandboxPolicyInfo{ | ||
| .profile_path = argv[i + 1], | ||
| .status = SandboxPolicyStatus::kPending, | ||
| }; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Restrict -f parsing to sandbox-exec option scope.
ParseSandboxExecArgv scans the entire argv, so it can incorrectly capture -f from the target command args (e.g., ... /usr/bin/grep -f pattern). That can misattribute sandbox_policy.profile_path.
🔧 Proposed fix
std::optional<SandboxPolicyInfo> ParseSandboxExecArgv(
const std::vector<std::string> &argv) {
- // argv: ["sandbox-exec", "-f", "<path>", ...]
- // Scan for the -f flag.
- for (size_t i = 0; i < argv.size(); i++) {
- if (argv[i] == "-f" && i + 1 < argv.size()) {
+ // argv: ["sandbox-exec", [options...], "<command>", ...]
+ // Only scan sandbox-exec option tokens (before the command).
+ for (size_t i = 1; i < argv.size(); ++i) {
+ const auto &arg = argv[i];
+ if (arg == "--") break;
+ if (!arg.empty() && arg[0] != '-') break; // command starts
+ if (arg == "-f" && i + 1 < argv.size()) {
return SandboxPolicyInfo{
.profile_path = argv[i + 1],
.status = SandboxPolicyStatus::kPending,
};
}
}
return std::nullopt;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Source/santad/ProcessTree/annotations/sandbox_exec.cc` around lines 43 - 50,
ParseSandboxExecArgv currently scans all argv entries and can pick up a
target-command '-f'. Limit the scan to the sandbox-exec option scope by stopping
when the option/args separator is reached (e.g., "--") or when the sandbox-exec
options end; only examine entries before that separator for "-f". Update
ParseSandboxExecArgv to return SandboxPolicyInfo with profile_path and
SandboxPolicyStatus::kPending only when "-f" is found before the separator,
referencing ParseSandboxExecArgv, SandboxPolicyInfo, profile_path, and
SandboxPolicyStatus::kPending to locate the logic to change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds an annotator to the process tree for sandbox-exec events
Demo video here
https://www.youtube.com/watch?v=HVTLD6qY7AU
Work for SNT-350
Other changes: