diff --git a/crates/sandlock-cli/src/learn.rs b/crates/sandlock-cli/src/learn.rs index 46256d22..6bf372ce 100644 --- a/crates/sandlock-cli/src/learn.rs +++ b/crates/sandlock-cli/src/learn.rs @@ -36,7 +36,7 @@ fn classify_path(p: &std::path::Path) -> PathTier { if GUARDED_PATHS.iter().any(|s| b == *s) { return PathTier::Guarded; } - // $HOME itself (non-root) is guarded — apps do legitimately write dotfiles there. + // $HOME itself (non-root) is guarded: apps do legitimately write dotfiles there. if let Ok(home) = std::env::var("HOME") { if b == home.as_bytes() && home != "/root" { return PathTier::Guarded; @@ -76,12 +76,25 @@ fn collapse_write_paths(writes: &BTreeSet) -> Vec { if is_junk_path(p) { continue; } let p = &fold_session_path(p.clone()); if p.exists() { + if p.as_os_str().as_encoded_bytes() == b"/" { + eprintln!( + "sandlock learn: WARNING: observed a direct write of '/', refusing to grant it" + ); + continue; + } + match classify_path(p) { + PathTier::Protected | PathTier::Guarded => { + eprintln!( + "sandlock learn: NOTE: observed a direct write to '{}'", + p.display() + ); + } + PathTier::Normal => {} + } out.insert(p.clone()); continue; } let Some(ancestor) = p.ancestors().skip(1).find(|a| a.exists()) else { continue }; - // "/" is always skipped: granting write access to the filesystem root - // is never useful and would override every other policy entry. if ancestor.as_os_str().as_encoded_bytes() == b"/" { eprintln!( "sandlock learn: WARNING: write collapse for '{}' reaches filesystem root, skipping", @@ -190,8 +203,21 @@ fn collapse_by_threshold( /// Returns true for pid-specific paths that are meaningless across runs. fn is_junk_path(p: &std::path::Path) -> bool { let b = p.as_os_str().as_encoded_bytes(); - b.starts_with(b"/proc/self") - || (b.starts_with(b"/proc/") && b.get(6).map_or(false, u8::is_ascii_digit)) + // /proc//... are pid-specific across runs. + let proc_numeric_pid = b.starts_with(b"/proc/") + && b.get(6).map_or(false, u8::is_ascii_digit); + // Under /proc/self, only sub-trees with volatile numeric components are + // junk. Stable entries like /proc/self/maps are legitimately needed + // across runs (e.g. V8 reads maps on every startup). + let proc_self_volatile = b.starts_with(b"/proc/self/fd/") + || b.starts_with(b"/proc/self/fdinfo/") + || b.starts_with(b"/proc/self/task/") + || b.starts_with(b"/proc/self/map_files/") + || b == b"/proc/self/fd" + || b == b"/proc/self/fdinfo" + || b == b"/proc/self/task" + || b == b"/proc/self/map_files"; + proc_numeric_pid || proc_self_volatile } /// A pty's number changes between sessions; granting the directory is @@ -352,7 +378,7 @@ impl LearnObserver { } "openat" | "open" => { if let Some(path) = event.path { - let path = canonicalize_or_keep(path); + let path = canonicalize_or_keep(path, event.pid); if let Some(fl) = event.flags { if is_write_open(fl) { self.writes.lock().unwrap().insert(path); @@ -366,9 +392,13 @@ impl LearnObserver { // rights, so the parent dir is what sandlock run needs, not the target. "mkdirat" | "mknodat" => { if let Some(p) = event.path { - let p = canonicalize_or_keep(p); - if let Some(parent) = p.parent() { - self.writes.lock().unwrap().insert(parent.to_path_buf()); + let p = canonicalize_or_keep(p, event.pid); + // If the target already exists the syscall will fail with + // EEXIST; no MAKE_DIR right on the parent is needed. + if !p.exists() { + if let Some(parent) = p.parent() { + self.writes.lock().unwrap().insert(parent.to_path_buf()); + } } } } @@ -396,7 +426,7 @@ impl LearnObserver { // dst operates on the link itself, so only the parent is canonicalized. "linkat" => { if let Some(src) = event.path { - self.reads.lock().unwrap().insert(canonicalize_or_keep(src)); + self.reads.lock().unwrap().insert(canonicalize_or_keep(src, event.pid)); } if let Some(dst) = event.path2 { let dst = canonicalize_parent_or_keep(dst); @@ -408,7 +438,7 @@ impl LearnObserver { // truncate: LANDLOCK_ACCESS_FS_TRUNCATE applies to the file itself. "truncate" => { if let Some(p) = event.path { - self.writes.lock().unwrap().insert(canonicalize_or_keep(p)); + self.writes.lock().unwrap().insert(canonicalize_or_keep(p, event.pid)); } } "bind" => { @@ -419,7 +449,7 @@ impl LearnObserver { } else if let Some(p) = event.path { // AF_UNIX named bind: Landlock MAKE_SOCK is a directory right, // so the parent dir is what sandlock run needs. - let p = canonicalize_or_keep(p); + let p = canonicalize_or_keep(p, event.pid); if let Some(parent) = p.parent() { self.writes.lock().unwrap().insert(parent.to_path_buf()); } @@ -484,7 +514,24 @@ impl LearnObserver { /// Resolve symlinks to get the canonical path. Falls back to the original /// if the path doesn't exist yet (e.g. COW-intercepted creates). -fn canonicalize_or_keep(p: PathBuf) -> PathBuf { +/// +/// /proc/self paths are canonicalized via the event pid so symlinks +/// (e.g. /proc/self/exe) resolve against the workload, not the supervisor. +/// Results still under /proc// are mapped back to /proc/self/. +fn canonicalize_or_keep(p: PathBuf, pid: u32) -> PathBuf { + let b = p.as_os_str().as_encoded_bytes(); + if b.starts_with(b"/proc/self") { + let suffix = &b[b"/proc/self".len()..]; + let pid_path = PathBuf::from(format!("/proc/{}{}", pid, String::from_utf8_lossy(suffix))); + let resolved = std::fs::canonicalize(&pid_path).unwrap_or(pid_path); + let resolved_b = resolved.as_os_str().as_encoded_bytes(); + let pid_prefix = format!("/proc/{}/", pid); + if resolved_b.starts_with(pid_prefix.as_bytes()) { + let rest = &resolved_b[pid_prefix.len() - 1..]; // keep leading / + return PathBuf::from(format!("/proc/self{}", String::from_utf8_lossy(rest))); + } + return resolved; + } std::fs::canonicalize(&p).unwrap_or(p) } @@ -644,7 +691,7 @@ pub async fn run(args: LearnArgs) -> Result<()> { // other read in the profile. if p.as_path() == std::path::Path::new("/") { eprintln!( - "sandlock learn: WARNING: observed a read of '/', refusing to grant it" + "sandlock learn: WARNING: observed a direct read of '/', refusing to grant it" ); return false; } diff --git a/crates/sandlock-cli/tests/learn_test.rs b/crates/sandlock-cli/tests/learn_test.rs index f610e078..b77a8595 100644 --- a/crates/sandlock-cli/tests/learn_test.rs +++ b/crates/sandlock-cli/tests/learn_test.rs @@ -705,7 +705,7 @@ fn test_write_collapse_skips_root() { let write_line = stdout.lines().find(|l| l.starts_with("write = [")).unwrap_or(""); assert!(!write_line.contains("\"/\""), "write list must not contain \"/\", got: {write_line}"); assert!(stderr.contains("filesystem root"), - "expected 'filesystem root' warning in stderr, got: {stderr}"); + "expected filesystem root warning in stderr, got: {stderr}"); } /// Write collapse to a sensitive path emits a warning and observed-vs-granted diff. @@ -747,6 +747,83 @@ fn test_write_collapse_skips_protected() { "expected 'protected path' error in stderr, got: {stderr}"); } +/// mkdirat on an existing target (EEXIST) must not add the parent to the write set. +#[test] +fn test_mkdirat_eexist_no_write() { + let base = tempfile::TempDir::new_in("/var/tmp").expect("tempdir in /var/tmp"); + let existing = base.path().join("already_there"); + std::fs::create_dir(&existing).expect("create subdir"); + let existing_str = existing.to_str().unwrap(); + let base_str = base.path().to_str().unwrap(); + // mkdirat fires pre-syscall; without the EEXIST guard the parent + // (base_str) would be inserted into the write set. + let output = sandlock_bin() + .args(["learn", "--", "sh", "-c", &format!("mkdir {existing_str} 2>/dev/null; true")]) + .output() + .expect("failed to run sandlock learn"); + assert!(output.status.success(), + "sandlock learn failed: stderr={}", String::from_utf8_lossy(&output.stderr)); + let stdout = String::from_utf8_lossy(&output.stdout); + let write_line = stdout.lines().find(|l| l.starts_with("write = [")).unwrap_or("write = []"); + assert!(!write_line.contains(base_str), + "mkdirat on existing target must not insert parent {base_str} into writes, got: {write_line}"); +} + +/// Direct write to "/" must be dropped with a warning, not recorded. +#[test] +fn test_direct_write_root_skipped() { + let dir = format!("/sandlock_learn_root_mkdir_{}", std::process::id()); + let output = sandlock_bin() + .args(["learn", "--", "sh", "-c", &format!("mkdir {dir} 2>/dev/null; true")]) + .output() + .expect("failed to run sandlock learn"); + assert!(output.status.success(), + "sandlock learn failed: stderr={}", String::from_utf8_lossy(&output.stderr)); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + let write_line = stdout.lines().find(|l| l.starts_with("write = [")).unwrap_or("write = []"); + assert!(!write_line.contains("\"/\""), + "direct write of \"/\" must be dropped, got: {write_line}"); + assert!(stderr.contains("direct write of '/'"), + "expected direct write warning in stderr, got: {stderr}"); +} + +/// /proc/self/maps must appear in the profile as /proc/self/maps +#[test] +fn test_proc_self_maps_preserved() { + let output = sandlock_bin() + .args(["learn", "--", "python3", "-c", "open('/proc/self/maps').read()"]) + .output() + .expect("failed to run sandlock learn"); + assert!(output.status.success(), + "sandlock learn failed: stderr={}", String::from_utf8_lossy(&output.stderr)); + let stdout = String::from_utf8_lossy(&output.stdout); + let read_line = stdout.lines().find(|l| l.starts_with("read = [")).unwrap_or(""); + assert!(read_line.contains("\"/proc/self/maps\""), + "/proc/self/maps must be recorded as-is, got: {read_line}"); + let has_numeric_pid = read_line.split("\"/proc/").skip(1).any(|s| { + s.chars().next().map_or(false, |c| c.is_ascii_digit()) + }); + assert!(!has_numeric_pid, + "must not contain a numeric-pid /proc//... entry, got: {read_line}"); +} + +/// /proc/self/exe is a symlink to the real binary; it must be resolved to the +/// binary path, not recorded as /proc/self/exe. +#[test] +fn test_proc_self_exe_resolves_to_binary() { + let output = sandlock_bin() + .args(["learn", "--", "python3", "-c", "import os; os.open('/proc/self/exe', os.O_RDONLY)"]) + .output() + .expect("failed to run sandlock learn"); + assert!(output.status.success(), + "sandlock learn failed: stderr={}", String::from_utf8_lossy(&output.stderr)); + let stdout = String::from_utf8_lossy(&output.stdout); + let read_line = stdout.lines().find(|l| l.starts_with("read = [")).unwrap_or(""); + assert!(!read_line.contains("\"/proc/self/exe\""), + "/proc/self/exe must be resolved to the binary path, not recorded as-is: {read_line}"); +} + // ── Merge and canonicalization ──────────────────────────────────────────────── /// --merge unions observations from a new run into an existing profile. diff --git a/docs/learn.md b/docs/learn.md index 6c72d49e..2008af89 100644 --- a/docs/learn.md +++ b/docs/learn.md @@ -53,11 +53,13 @@ optional, so omitting the ancestor would cause `sandlock run` to abort. ### Path tiers -| Tier | Paths | Write (auto) | `--collapse N` | `--collapse-prefix` | -|---|---|---|---|---| -| **Protected** | `/`, `/root`, `~/.ssh`, `~/.aws`, `~/.kube`, `~/.gnupg` | skip + error | never (keep individual file) | refused unless `--force-sensitive-collapse` | -| **Guarded** | `/etc`, `/proc`, `/sys`, `/dev`, `/boot`, `/run/secrets` | emit + warning + diff | never (keep individual file) | refused unless `--force-sensitive-collapse` | -| **Normal** | everything else | collapse freely | collapse freely | collapse freely | +| Tier | Paths | Write collapse | `--collapse` / `--collapse-prefix` | +|---|---|---|---| +| **Protected** | `/`, `/root`, paths ending in `/.ssh` `/.aws` `/.kube` `/.gnupg` | skip + error | never (keep individual files; override with `--force-sensitive-collapse`) | +| **Guarded** | `$HOME`, `/etc`, `/proc`, `/sys`, `/dev`, `/boot`, `/run/secrets` | emit + warning + diff | never (keep individual files; override with `--force-sensitive-collapse`) | +| **Normal** | everything else | collapse freely | collapse freely | + +The tiers apply to write collapse only. **The only path dropped from direct writes/reads is `/`**: granting it would subsume every other entry in the profile. Also, warning is printed to stderr when the direct write path is Protected or Guarded. When a write collapse lands on a guarded path, a warning is printed to stderr along with an **observed-vs-granted diff**, the list of siblings @@ -81,10 +83,10 @@ cargo build -p sandlock-cli Each test spawns a full sandlock process; running too many in parallel exhausts kernel limits and causes hangs, use `--test-threads=4`. ```bash -# learn output tests — verify TOML profile content +# learn output tests - verify TOML profile content cargo test -p sandlock-cli --test learn_test -- --test-threads=4 -# learn round-trip tests — learn → profile → run end-to-end +# learn round-trip tests - learn → profile → run end-to-end cargo test -p sandlock-cli --test learn_integration -- --test-threads=4 ```