Skip to content

fix(tool-sandbox): attribute daemonized callers to their command (cgroup on linux, verified daemon-pid on macos) - #1417

Merged
SequeI merged 4 commits into
nolabs-ai:mainfrom
kipz:kipz/tool-sandbox-session-membership
Jul 16, 2026
Merged

fix(tool-sandbox): attribute daemonized callers to their command (cgroup on linux, verified daemon-pid on macos)#1417
SequeI merged 4 commits into
nolabs-ai:mainfrom
kipz:kipz/tool-sandbox-session-membership

Conversation

@kipz

@kipz kipz commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Closes #1412

Summary

  • Tool Sandbox authorizes a command by walking the caller's process ancestry back to the session. A daemonized ancestor (setsid + double-fork → reparented to pid 1) severs that walk, so a caller that legitimately descends from a session command is blocked (caller ancestry did not reach session root).
  • This PR attributes such a severed caller to the command that spawned the daemon (gated by that command's from-edges), or leaves it blocked if it can't be attributed — never the session. An in-sandbox process cannot forge the attribution.
  • Linux: an unforgeable per-command cgroup marker, self-attached pre-exec, survives reparenting; Landlock denies in-sandbox cgroupfs writes. Needs cgroup delegation; where absent, severed callers stay blocked (unchanged).
  • macOS (no unprivileged kernel marker): a command-declared daemon_pid_source helper reports the daemon's pid, accepted only if it matches the walked-to daemon, pinned by kernel identity. The helper receives the daemon's own argv/env/cwd (read from the kernel) as a JSON object on stdin so it can locate or verify its server.
  • Security caveat: the macOS helper runs unsandboxed, so its launch is hardened (absolute-path program, cleared env, neutral cwd, hard timeout) and a declared helper must only read and emit a pid — never run workspace-controlled code.

Agent Disclosure (if applicable)

Authored by an AI agent, driven and reviewed by me. Area: tool-sandbox/platform/{macos,linux}.rs, lineage_cgroup.rs, command_policy.rs, profile/mod.rs, the profile schema, and the Tool Sandbox docs. Complies with CLAUDE.md for the area: no unwrap/expect, NonoError, bounds-checked FFI.

Test Plan

  • make ci clean (check, test, audit, lint-aliases, lint-docs).
  • Unit, both platforms: severed caller → spawning command; deny on non-match / dead / reused pid; platform_overrides + extends merge; schema shape; daemon_pid_source serde/merge.
  • macOS: KERN_PROCARGS2 parse; helper stdin-context round-trip (verify mode); env allowlist honoured + credential-name denylist enforced.
  • Live (#[ignore], run with --ignored): real setsid+double-fork daemon attributes to its command; Linux child cgroup.procs write → EACCES.

Checklist

  • An issue exists and is linked above
  • All commits are signed-off, using DCO
  • All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
  • Public-facing changes are paired with documentation updates
  • Release note has been added to CHANGELOG.md if needed

Agent Compliance Check (Required for AI/Automated PRs)

  • I am not prohibited from contributing under this policy
  • An issue already exists
  • I disclosed that I am an agent in the issue discussion
  • I described my intent and approach in the issue discussion
  • I reviewed repository coding and security rules for the affected area
  • I provided required attribution for reused or adapted code
  • I did not use forbidden patterns such as unwrap/expect
  • I used NonoError where required
  • I validated and canonicalized all relevant paths
  • This PR matches the approved or disclosed issue scope

@github-actions github-actions Bot added bug Something isn't working nono-cli size/large labels Jul 14, 2026
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +2305
Lines removed -8
Total changed 2313
Classification Large (> 300 lines)

Affected crates

  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Moderate

This PR touches: source code,configuration / policy files


Updated automatically on each push to this PR.

@kipz
kipz force-pushed the kipz/tool-sandbox-session-membership branch from b6c3d15 to 80f01cb Compare July 14, 2026 09:15
@kipz
kipz force-pushed the kipz/tool-sandbox-session-membership branch from 80f01cb to 446435a Compare July 14, 2026 10:19
@kipz
kipz force-pushed the kipz/tool-sandbox-session-membership branch from 446435a to e63c788 Compare July 14, 2026 10:29
kipz added a commit to kipz/nono that referenced this pull request Jul 14, 2026
…oup on linux, verified daemon-pid on macos)

A caller whose parent walk is severed by a daemonized ancestor (reparented to pid 1) previously fell back to over-broad, replayable session attribution; attribute it to the spawning command instead, or fail closed.

Signed-off-by: James Carnegie <me@kipz.org>
@kipz
kipz force-pushed the kipz/tool-sandbox-session-membership branch from e63c788 to f58325c Compare July 14, 2026 12:46
kipz added a commit to kipz/nono that referenced this pull request Jul 14, 2026
@kipz
kipz marked this pull request as ready for review July 14, 2026 14:43

@nogent-nolabs-ai nogent-nolabs-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nogent code review

1 potential security issue (OOM panic vector in supervisor ancestry resolution) identified.

Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.

Comment thread crates/nono-cli/src/tool-sandbox/platform/macos.rs Outdated
…args2

An untrusted process can report an argc far larger than its KERN_PROCARGS2
buffer actually holds; Vec::with_capacity(argc) then drives an OOM panic
on the supervisor. Cap the allocation by the parsed buffer length instead.

Signed-off-by: James Carnegie <me@kipz.org>
@oliviagallucci

oliviagallucci commented Jul 14, 2026

Copy link
Copy Markdown

I have not been following every aspect of Nono, so I might be out of the loop. Thus, I've written a lot to explain my thoughts, and the logic behind them. Here is my read from the limited info I've gathered.

I think this is a solid approach to a gnarly macOS problem. While the helper direction is a bit heavy compared to Linux cgroups, it has the right shape: fail closed, never fall back to the session, and only attribute a severed daemon to a specific command when a command-specific verifier can name the same kernel-pinned pid. Thus, the abstraction boundary here makes sense.

For a future, truly generic "macOS cgroup-ish" backend, I think we should look into graphing p_puniqueid from PROC_PIDUNIQIDENTIFIERINFO (since it survives reparenting) combined with a kqueue(NOTE_TRACK) (correction: does not look like this exists anymore tho, so this might not be good suggestion. maybe there is newer alternative) listener to catch the intermediate forks before they exit. I also agree with leaving launchd coalitions (PROC_PIDCOALITIONINFO) as a separate spike, given how heavily they'd disrupt PTYs and standard process launch semantics.

I found some general information about this here (https://www.newosxbook.com/ChangeLog.html), but we should ideally look at newer info. I saw Apple has some general docs here, so we should be OK.

For the goals of this PR, the current approach is good as a per-daemon attribution mechanism. However, there are a few security and ops issues we should to fix before merging.

Potential issues IMO

1. Stdin timeout deadlock

run_daemon_pid_source serializes the daemon context, then does a blocking stdin.write_all(&payload) before the timeout loop.

I'm pretty sure macOS pipe buffers are typically 16KB. Because KERN_PROCARGS2 can carry large argv/env data, a heavy environment can easily exceed the pipe buffer. If the helper script doesn't actively read stdin (e.g., it just runs cat /tmp/daemon.pid), the write_all will block forever, the timeout loop will never tick, and the supervisor thread will deadlock. We need to either cap/truncate the payload or use a non-blocking/asynchronous write under the same deadline.

2. Helper executable hardening

The helper runs unsandboxed in the supervisor. Right now, it only has absolute-path and env-clearing hardening, which is a trust boundary. We potentially need to reuse the pinned executable/writable-path checks we use elsewhere: path, O_NOFOLLOW, inode/digest binding, and explicit rejecting workspace-writable helper paths unless an unsafe flag is set.

3. Negative caching (DoS prevention)

Positive attributions are cached, but repeated unmatched severed callers will repeatedly run every declared helper, each with up to a 5-second timeout (as you described above).

A malicious same-user daemon could spin up a loop that rapidly forks, reparents, and sends socket requests to instantly exhaust the supervisor's thread pool. Maybe we add a short negative cache keyed by (pid, p_uniqueid) to preserve the fail-closed semantics without allowing thread starvation?

Potential fixes IMO

1. Drop candidate_pid from the JSON payload

Giving the helper the candidate_pid makes it easy for a to write a helper that just echos it back out to satisfy the verifier, granting any reparented process the command's authority, right?

Maybe make the helper compute "my daemon pid" independently (via pidfile/socket). If you must keep it, the docs prolly need warnings that this is untrusted input.

2. Expose more kernel identity

Since you are already reading PROC_PIDUNIQIDENTIFIERINFO, maybe include candidate_uniqueid, candidate_parent_uniqueid, and start time in the JSON payload. This gives daemon-specific helpers a way to verify much more than just a PID match.

NOTE: Although this API (proc_pidinfo(), specifically when used with the PROC_PIDUNIQIDENTIFIERINFO flavor) is technically private and missing from the public SDK, folks widely rely on its 64-bit value as a permanent anchor to definitively solve PID-reuse races. This might be helpful, and it is recent: https://developer.apple.com/forums/thread/824760

3. Tighten schema validation

daemon_pid_source.argv should have minItems: 1; this ensures that at least the executable path is provided before the profile is even accepted. Also, helper paths should ideally be validated during profile finalization rather than failing at the first severed call.

TLDR: my overall thoughts on direction is thumbs up, but I think you need to patch up the helper execution boundaries and the stdin deadlock before merging.

Addresses review feedback on the severed-daemon attribution helper:

- Write the JSON payload to the helper's stdin on a background thread
  instead of blocking before the timeout-poll loop starts. A severed
  daemon's argv/env is unbounded (read straight from the kernel), so
  the payload can exceed the pipe buffer; a helper that doesn't drain
  stdin before doing other work could wedge the write and skip the
  deadline entirely.
- Pre-resolve daemon_pid_source helper binaries at plan-build time
  (dev/ino/size/mtime/sha256), gate them behind the same
  non-writable-executable trust check already applied to `exec`
  intercept helpers, and re-verify identity right before spawn to
  close the TOCTOU window.
- Add a short-TTL negative cache to DaemonPidLineage, pinned by kernel
  identity, so a fork/reparent loop can't force a full helper sweep on
  every unmatched severed-daemon check.
- Reject an empty daemon_pid_source.argv at profile-load time instead
  of silently disabling attribution for that command at runtime.

Signed-off-by: James Carnegie <me@kipz.org>
@kipz

kipz commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review — pushed a fix for all three blocking issues in 4ab94e4:

  1. Stdin timeout deadlock — confirmed real: daemon_argv/daemon_env in the JSON payload are unbounded (read straight from the kernel via KERN_PROCARGS2), so they can exceed the pipe buffer. The write now happens on its own thread instead of blocking before the timeout-poll loop starts, so a non-draining helper can no longer wedge the write and skip the deadline.
  2. Helper executable hardeningdaemon_pid_source helpers are now pre-resolved at plan-build time (dev/ino/size/mtime/sha256, same as command binaries) and gated behind the same non-writable-executable trust check already applied to exec intercept helpers, with a TOCTOU re-verify right before spawn.
  3. Negative caching — added a short-TTL (2s) negative cache to DaemonPidLineage, pinned by kernel identity, so a fork/reparent loop can't force a full helper sweep on every unmatched check.

Also tightened schema validation to reject an empty daemon_pid_source.argv at profile-load time rather than silently disabling attribution at runtime.

On candidate_pid: I don't think this needs to change. It's derived from nono's own kernel-level ancestry walk (parent_pid/daemon_identity), not attacker-supplied — the attacker can't spoof what pid nono believes the severed daemon is. A helper that chooses to just echo it back without real verification is a misconfiguration by the (trusted) profile author, not something nono needs to defend against. Happy to revisit if you see an attacker-controlled path into that value I'm missing.

daemon_pid_source is only consumed by the macOS tool-sandbox module
(mod macos is cfg(target_os = "macos")-gated). Mirroring exec_helpers'
cfg(any(test, target_os = "linux", target_os = "macos")) left these two
functions uncalled on Linux, tripping -D warnings dead-code errors in CI.

Signed-off-by: James Carnegie <me@kipz.org>
kipz added a commit to kipz/nono that referenced this pull request Jul 16, 2026

@SequeI SequeI left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SequeI
SequeI merged commit a519ee6 into nolabs-ai:main Jul 16, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working nono-cli size/large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tool-sandbox blocks a command called by a descendant of a daemonized process

3 participants