Skip to content

fix(sandbox): resolve multi-hop symlinks in fs grants - #1776

Open
kipz wants to merge 2 commits into
nolabs-ai:mainfrom
kipz:kipz/multi-hop-symlink-hops
Open

fix(sandbox): resolve multi-hop symlinks in fs grants#1776
kipz wants to merge 2 commits into
nolabs-ai:mainfrom
kipz:kipz/multi-hop-symlink-hops

Conversation

@kipz

@kipz kipz commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Closes #1771

Summary

A capability whose path resolves through more than one symlink hop was denied on macOS, even though the final target was already granted. Seatbelt only had rules for the two endpoints (original and fully-resolved resolved), not the intermediate hops the kernel walks through. This adds a bounded hop-enumeration pass and grants each intermediate hop file-read-metadata, for both FsCapability and UnixSocketCapability. Linux needed no code change, since open_path_rule resolves the whole chain via O_PATH before Landlock sees a literal path, but a new test verifies that rather than assuming it.

Agent Disclosure

This PR was written by an AI coding agent (Claude, via Claude Code) under my direct supervision and review.

Approach: added collect_symlink_hops in crates/nono/src/path.rs, walking a path one component at a time and resolving each symlink hop with a bounded depth to avoid cycles. In crates/nono/src/sandbox/macos.rs, every FsCapability and UnixSocketCapability now also grants file-read-metadata on each intermediate hop and its ancestor directories, alongside the existing endpoint rules. No capability schema change, hops are recomputed on demand.

Files consulted: crates/nono/src/path.rs, crates/nono/src/capability.rs, crates/nono/src/sandbox/macos.rs, crates/nono/src/sandbox/linux.rs.

Intent and approach were disclosed on the linked issue before implementation: #1771 (comment)

Test Plan

  • cargo test (workspace): all pass.
  • crates/nono-cli/tests/symlink_hop_run.rs (macOS): new positive and negative cases.
  • crates/nono-cli/tests/symlink_hop_run_linux.rs: same cases, run under real Landlock enforcement. Both pass.
  • make ci clean.

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

  • I am not prohibited from contributing under this policy
  • An issue already exists
  • 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 nono-cli size/large labels Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +507
Lines removed -14
Total changed 521
Classification Large (> 300 lines)

Affected crates

  • crates/nono (core library) — careful review required. This is the security-critical sandbox primitive. A bug here bypasses OS-level isolation for every downstream user.
  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Contained

This PR touches: source code


Updated automatically on each push to this PR.

@kipz
kipz force-pushed the kipz/multi-hop-symlink-hops branch from 5380b0b to 1514d0b Compare September 3, 2026 13:13
@kipz
kipz force-pushed the kipz/multi-hop-symlink-hops branch from 1514d0b to 3e4a21b Compare September 3, 2026 13:24
@kipz
kipz marked this pull request as ready for review September 3, 2026 14:28

@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

No blocking issues; 1 minor performance suggestion.

Findings (not tied to a changed line):

  • 🐛 [MEDIUM · bug] crates/nono/src/sandbox/macos.rs:785 — In generate_profile, the path metadata directories loop (caps.path_metadata_dirs()) does not resolve or grant file-read-metadata to intermediate symlink hops of $PATH directories. If any $PATH directory (or its parents) uses a multi-hop symlink, command resolution walks inside the sandbox will fail with EPERM (which aborts the $PATH walk entirely) instead of returning ENOENT or succeeding. To resolve this, intermediate symlink hops for path_metadata_dirs must also be collected using collect_symlink_hops and granted file-read-metadata.

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

Comment thread crates/nono/src/path.rs Outdated
Signed-off-by: James Carnegie <me@kipz.org>
@kipz
kipz force-pushed the kipz/multi-hop-symlink-hops branch from 3e4a21b to 86734c2 Compare September 3, 2026 15:05
Signed-off-by: James Carnegie <me@kipz.org>
@kipz

kipz commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Fixed: generate_profile's path_metadata_dirs() loop now also calls collect_symlink_hops on each $PATH dir and grants file-read-metadata on the intermediate hops (and their ancestors), reusing the existing seen_dir/seen_ancestor dedup sets. Added a regression test with a two-hop symlink chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Seatbelt (macOS) denies access through intermediate hops in multi-hop symlink chains

1 participant