fix(sandbox): match the read deny-list against a rule's resolved path, greening shared macOS/Windows CI #5724
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,7 +49,19 @@ | |
| //! exempted back open. | ||
| //! - **Symlinks.** Both the literal path and its `canonicalize()`d target are | ||
| //! tested, so a symlink pointing into `~/.ssh` is denied by its target even | ||
| //! though its own name is innocuous. The *rules* are resolved the same way | ||
| //! when they are built: a rule spelled `/etc/ssh` also denies | ||
| //! `/private/etc/ssh` on macOS, where `/etc` is itself a symlink, and a rule | ||
| //! written against a `/var/...` directory fires for the `/private/var/...` | ||
| //! spelling that `canonicalize` and `current_dir` hand back. Without that, | ||
| //! the resolved candidate never matched a literal rule, which is exactly the | ||
| //! shape hosted macOS CI runs in (`$TMPDIR` under `/var/folders`). | ||
| //! Exemptions are matched the same way, so exempting `/private/etc/sudoers` | ||
| //! — the spelling a denial message may name — reopens the `/etc/sudoers` | ||
| //! rule it resolves from. Rules are resolved when the list is built | ||
| //! (startup and config reload); a symlink retargeted afterwards is seen | ||
| //! through the literal candidate only until the list is rebuilt, which is | ||
| //! within the defense-in-depth posture above. | ||
| //! - **`..` and relative paths.** Two candidates are tested. The literal one is | ||
| //! lexically normalized (`.`/`..` folded without touching the disk), which | ||
| //! catches traversal into a denied tree even when nothing on the path exists | ||
|
|
@@ -114,8 +126,13 @@ impl ReadDenial { | |
| pub enum DenyRule { | ||
| /// Everything at or below a directory (or a single file at that path). | ||
| Subtree { | ||
| /// Normalized absolute path, as configured. | ||
| path: PathBuf, | ||
| /// `path` with symlinks resolved, kept only when it differs. A read is | ||
| /// matched against both spellings: the literal one catches | ||
| /// `/etc/sudoers` as written, this one catches `/private/etc/sudoers` | ||
| /// — the same file, reached by the name the OS actually uses. | ||
| resolved: Option<PathBuf>, | ||
| /// Human label, e.g. "SSH keys (~/.ssh)". | ||
| label: &'static str, | ||
| }, | ||
|
|
@@ -128,6 +145,30 @@ pub enum DenyRule { | |
| } | ||
|
|
||
| impl DenyRule { | ||
| /// A subtree rule that also remembers where its path really leads. | ||
| fn subtree(path: PathBuf, label: &'static str) -> Self { | ||
| let resolved = canonicalize_best_effort(&path); | ||
| DenyRule::Subtree { | ||
| resolved: (resolved != path).then_some(resolved), | ||
| path, | ||
| label, | ||
| } | ||
| } | ||
|
|
||
| /// True when this rule, under either spelling, lies at or below one of | ||
| /// `roots`. Exemptions subtract whole rules, so both spellings count. | ||
| fn is_within_any(&self, roots: &[PathBuf]) -> bool { | ||
| let DenyRule::Subtree { path, resolved, .. } = self else { | ||
| return false; | ||
| }; | ||
| roots.iter().any(|root| { | ||
| path_is_within(path, root) | ||
| || resolved | ||
| .as_deref() | ||
| .is_some_and(|real| path_is_within(real, root)) | ||
| }) | ||
| } | ||
|
|
||
| #[must_use] | ||
| fn describe(&self) -> String { | ||
| match self { | ||
|
|
@@ -161,11 +202,19 @@ impl ReadDenylist { | |
| /// from the built-in defaults only. | ||
| #[must_use] | ||
| pub fn build(include_defaults: bool, extra: &[PathBuf], exempt: &[PathBuf]) -> Self { | ||
| // Each exemption is kept in both spellings too, so exempting the path a | ||
| // denial named (`/private/etc/sudoers` on macOS) reopens the rule it | ||
| // resolved from (`/etc/sudoers`), and vice versa. | ||
| let exempt_normalized: Vec<PathBuf> = exempt | ||
| .iter() | ||
| .cloned() | ||
| .map(expand_home_prefix) | ||
| .map(|p| normalize_lexically(&p)) | ||
| .flat_map(|p| { | ||
| let resolved = canonicalize_best_effort(&p); | ||
| let real = (resolved != p).then_some(resolved); | ||
| std::iter::once(p).chain(real) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let mut subtrees = Vec::new(); | ||
|
|
@@ -182,11 +231,11 @@ impl ReadDenylist { | |
| .is_some_and(|name| name == std::ffi::OsStr::new(".env")) | ||
| }); | ||
| for (raw, label) in default_denied_subtrees() { | ||
| let rule = DenyRule::subtree(normalize_lexically(&raw), label); | ||
| if rule.is_within_any(&exempt_normalized) { | ||
| continue; | ||
| } | ||
| subtrees.push(rule); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -197,10 +246,10 @@ impl ReadDenylist { | |
| if path.as_os_str().is_empty() { | ||
| continue; | ||
| } | ||
| subtrees.push(DenyRule::subtree( | ||
| path, | ||
| "a path in `sandbox_denied_read_paths`", | ||
| )); | ||
| } | ||
|
|
||
| Self { | ||
|
|
@@ -257,10 +306,19 @@ impl ReadDenylist { | |
| }); | ||
| } | ||
| for rule in &self.subtrees { | ||
| let DenyRule::Subtree { | ||
| path, | ||
| resolved: rule_resolved, | ||
| .. | ||
| } = rule | ||
| else { | ||
| continue; | ||
| }; | ||
| let hit = path_is_within(candidate, path) | ||
| || rule_resolved | ||
| .as_deref() | ||
| .is_some_and(|real| path_is_within(candidate, real)); | ||
| if hit { | ||
| return Err(ReadDenial { | ||
| requested: requested.to_path_buf(), | ||
| rule: rule.clone(), | ||
|
|
@@ -664,6 +722,17 @@ mod tests { | |
| ReadDenylist::build(false, paths, &[]) | ||
| } | ||
|
|
||
| // Two tests below move the process-wide cwd. libtest runs tests as | ||
| // parallel threads of one process, so they take this lock; nextest runs | ||
| // each test in its own process and never contends for it. | ||
| static CWD_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | ||
|
|
||
| fn lock_cwd() -> std::sync::MutexGuard<'static, ()> { | ||
| CWD_LOCK | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn empty_denylist_denies_nothing_and_reports_full_disk_read() { | ||
| let list = ReadDenylist::empty(); | ||
|
|
@@ -804,9 +873,10 @@ mod tests { | |
|
|
||
| /// A relative read issued with the process cwd INSIDE a denied subtree | ||
| /// must be refused: the absolutization against cwd lands under the rule. | ||
| /// (Serialized with the other cwd-moving test; restores cwd regardless.) | ||
| #[test] | ||
| fn relative_read_from_inside_a_denied_tree_is_refused() { | ||
| let _cwd = lock_cwd(); | ||
| let tmp = tempfile::tempdir().expect("tempdir"); | ||
| let denied = tmp.path().join("denied"); | ||
| std::fs::create_dir_all(&denied).expect("mkdir"); | ||
|
|
@@ -1055,9 +1125,106 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn root_parent_traversal_does_not_escape_above_root() { | ||
| // Spell the traversal from the current drive/root so the assertion | ||
| // holds on Windows too: there `/../../etc` resolves against the cwd's | ||
| // drive and normalizes to `D:\etc`, not a bare `/etc`. | ||
| let cwd = std::env::current_dir().expect("cwd"); | ||
| let root: PathBuf = cwd | ||
| .components() | ||
| .take_while(|c| matches!(c, Component::Prefix(_) | Component::RootDir)) | ||
| .collect(); | ||
| let traversal = root.join("..").join("..").join("etc"); | ||
| assert_eq!(normalize_lexically(&traversal), root.join("etc")); | ||
| if cfg!(unix) { | ||
| assert_eq!( | ||
| normalize_lexically(Path::new("/../../etc")), | ||
| PathBuf::from("/etc") | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// Hosted macOS CI hands `tempfile` a `/var/folders/...` directory that is | ||
| /// really `/private/var/folders/...`; `canonicalize` and `current_dir` | ||
| /// return the resolved spelling while the rule was written against the | ||
| /// literal one, and no symlink test above fired. Build that shape | ||
| /// explicitly so the regression is caught on every host, not only where | ||
| /// `$TMPDIR` happens to be a symlink. | ||
| #[test] | ||
| #[cfg(unix)] | ||
| fn rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling() { | ||
| let _cwd = lock_cwd(); | ||
| let tmp = tempfile::tempdir().expect("tempdir"); | ||
| let real_root = tmp.path().join("real"); | ||
| let denied = real_root.join("secrets"); | ||
| std::fs::create_dir_all(&denied).expect("mkdir"); | ||
| std::fs::write(denied.join("id_rsa"), "KEY").expect("write"); | ||
| let alias_root = tmp.path().join("alias"); | ||
| std::os::unix::fs::symlink(&real_root, &alias_root).expect("symlink"); | ||
|
|
||
| // The rule names the alias, the way a `/var/...` tempdir rule does. | ||
| let list = denylist_for(&[alias_root.join("secrets")]); | ||
|
|
||
| // A read spelled through the real directory is the same file. | ||
| list.check(&denied.join("id_rsa")) | ||
| .expect_err("the resolved spelling of a denied tree must be refused"); | ||
| // An innocuous symlink resolves to the real spelling, never the alias. | ||
| let link = tmp.path().join("notes.txt"); | ||
| std::os::unix::fs::symlink(denied.join("id_rsa"), &link).expect("symlink"); | ||
| list.check(&link) | ||
| .expect_err("a symlink into the denied tree must be refused by its target"); | ||
| // A relative read from inside it absolutizes against the real cwd. | ||
| let prior = std::env::current_dir().expect("cwd"); | ||
| struct Restore(std::path::PathBuf); | ||
| impl Drop for Restore { | ||
| fn drop(&mut self) { | ||
| let _ = std::env::set_current_dir(&self.0); | ||
| } | ||
| } | ||
| let _restore = Restore(prior); | ||
| std::env::set_current_dir(&denied).expect("chdir into the denied tree"); | ||
|
There was a problem hiding this comment.
Under the documented AGENTS.md reference: crates/tui/AGENTS.md:L34-L40 Useful? React with 👍 / 👎. |
||
| list.check(Path::new("id_rsa")) | ||
| .expect_err("a relative read from inside the denied tree must be refused"); | ||
| // And the innocent sibling of the real directory stays readable. | ||
| let sibling = real_root.join("notes.md"); | ||
| std::fs::write(&sibling, "notes").expect("write"); | ||
| assert!(list.check(&sibling).is_ok(), "sibling must stay readable"); | ||
| } | ||
|
|
||
| /// A denial names the path as requested, so on macOS it may say | ||
| /// `/private/etc/sudoers`; exempting that spelling must reopen the | ||
| /// `/etc/sudoers` rule it resolves from, and the literal spelling must | ||
| /// keep working too. Unrelated defaults stay armed either way. | ||
| #[cfg(target_os = "macos")] | ||
| #[test] | ||
| fn exempting_either_spelling_of_a_symlinked_default_rule_reopens_it() { | ||
| for exempt in ["/private/etc/sudoers", "/etc/sudoers"] { | ||
| let list = ReadDenylist::build(true, &[], &[PathBuf::from(exempt)]); | ||
| assert!( | ||
| list.check(Path::new("/etc/sudoers")).is_ok(), | ||
| "exempting {exempt} must reopen the literal spelling" | ||
| ); | ||
| assert!( | ||
| list.check(Path::new("/private/etc/sudoers")).is_ok(), | ||
| "exempting {exempt} must reopen the resolved spelling" | ||
| ); | ||
| list.check(Path::new("/private/etc/ssh/ssh_host_ed25519_key")) | ||
| .unwrap_err_or_panic("an unrelated default rule stays armed"); | ||
| } | ||
| } | ||
|
|
||
| /// On macOS `/etc` is a symlink to `/private/etc`, so the machine-wide | ||
| /// default rules were readable under their real names. | ||
| #[cfg(target_os = "macos")] | ||
| #[test] | ||
| fn macos_private_spelling_of_a_machine_wide_rule_is_refused() { | ||
| let list = ReadDenylist::build(true, &[], &[]); | ||
| list.check(Path::new("/private/etc/sudoers")) | ||
| .unwrap_err_or_panic("/private/etc/sudoers is /etc/sudoers"); | ||
| list.check(Path::new("/private/etc/ssh/ssh_host_ed25519_key")) | ||
| .unwrap_err_or_panic("/private/etc/ssh is /etc/ssh"); | ||
| assert!( | ||
| list.check(Path::new("/private/etc/hosts")).is_ok(), | ||
| "/etc/hosts is not a credential store" | ||
| ); | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a configured deny path is a symlink that is replaced or retargeted during a long-running session,
resolvedremains a startup-time snapshot. A direct read beneath the new target is then allowed becausecheckcompares it only with the literal alias and the old target, while the old target remains denied even after the alias stops pointing there. This can occur when another process atomically rotates a symlinked credential directory, so the rule should be re-resolved during checks or refreshed when its filesystem topology changes.Useful? React with 👍 / 👎.