From 4f8f74d7706ebd664e91116828a2df6fe8b10c93 Mon Sep 17 00:00:00 2001 From: Vahagn Date: Thu, 10 Sep 2026 00:06:28 +0400 Subject: [PATCH 1/4] [fix] learn: fix write=["/"] from mkdirat on existing target mkdirat fires pre-syscall, so the handler ran even when the target already existed and the syscall would fail with EEXIST. The parent "/" ended up in the write set, which dedup_subsumed then collapsed every other write path under it, producing write = ["/"]. Fix: skip mkdirat/mknodat when the target already exists on the real filesystem. Add a "/" guard to the existing-path fast-path to match the reads side. Add a NOTE when a direct write lands on a Protected or Guarded path. Rewrite path tiers table; add paragraph on direct writes with the "/" exception bolded. Tests: add test_mkdirat_eexist_no_write and test_direct_write_root_skipped. Signed-off-by: Vahagn --- crates/sandlock-cli/src/learn.rs | 30 +++++++++++++---- crates/sandlock-cli/tests/learn_test.rs | 45 +++++++++++++++++++++++-- docs/learn.md | 16 +++++---- 3 files changed, 76 insertions(+), 15 deletions(-) diff --git a/crates/sandlock-cli/src/learn.rs b/crates/sandlock-cli/src/learn.rs index 46256d22..2650a7d4 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", @@ -367,8 +380,12 @@ impl LearnObserver { "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()); + // 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()); + } } } } @@ -484,6 +501,7 @@ 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 { std::fs::canonicalize(&p).unwrap_or(p) } @@ -644,7 +662,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..f1f9bd96 100644 --- a/crates/sandlock-cli/tests/learn_test.rs +++ b/crates/sandlock-cli/tests/learn_test.rs @@ -704,8 +704,8 @@ fn test_write_collapse_skips_root() { let stderr = String::from_utf8_lossy(&output.stderr); 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}"); + assert!(stderr.contains("protected path '/'"), + "expected protected path '/' warning in stderr, got: {stderr}"); } /// Write collapse to a sensitive path emits a warning and observed-vs-granted diff. @@ -747,6 +747,47 @@ 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}"); +} + // ── 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..c99eb067 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 (when a non-existent path's nearest existing ancestor is used as the Landlock grant). Direct writes to an existing path are always recorded; a notice is printed to stderr when the path is Protected or Guarded. **The sole exception is `/`: a direct read or write of the filesystem root is always dropped with a warning, since granting `/` would subsume every other entry in the profile.** 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 ``` From c731be3b400b8c7b5a507b6a26112af8507a48a9 Mon Sep 17 00:00:00 2001 From: Vahagn Date: Sun, 6 Sep 2026 23:28:17 +0400 Subject: [PATCH 2/4] [fix] learn: preserve /proc/self paths, junk only numeric-pid /proc entries --- crates/sandlock-cli/src/learn.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/crates/sandlock-cli/src/learn.rs b/crates/sandlock-cli/src/learn.rs index 2650a7d4..8c0d3134 100644 --- a/crates/sandlock-cli/src/learn.rs +++ b/crates/sandlock-cli/src/learn.rs @@ -203,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 @@ -502,7 +515,13 @@ 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). /// +/// /proc/self is deliberately not resolved: canonicalize would turn +/// /proc/self/maps into /proc//maps, making it look like a volatile +/// pid-specific path and causing is_junk_path to drop it. fn canonicalize_or_keep(p: PathBuf) -> PathBuf { + if p.as_os_str().as_encoded_bytes().starts_with(b"/proc/self") { + return p; + } std::fs::canonicalize(&p).unwrap_or(p) } From e13cd14e8fef87d9a3c9e2add8eaa4211cff24d2 Mon Sep 17 00:00:00 2001 From: Vahagn Date: Thu, 10 Sep 2026 01:56:57 +0400 Subject: [PATCH 3/4] [fix] learn: resolve /proc/self symlinks via event pid, map result back Pass event.pid to canonicalize_or_keep so /proc/self/X is rewritten to /proc//X, canonicalized against the workload (not the supervisor), then mapped back to /proc/self/X. Symlinks like /proc/self/exe correctly resolve to the real binary path instead of staying as a proc alias. Signed-off-by: Vahagn --- crates/sandlock-cli/src/learn.rs | 32 +++++++++++++------- crates/sandlock-cli/tests/learn_test.rs | 40 +++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/crates/sandlock-cli/src/learn.rs b/crates/sandlock-cli/src/learn.rs index 8c0d3134..6bf372ce 100644 --- a/crates/sandlock-cli/src/learn.rs +++ b/crates/sandlock-cli/src/learn.rs @@ -378,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); @@ -392,7 +392,7 @@ 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); + 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() { @@ -426,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); @@ -438,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" => { @@ -449,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()); } @@ -515,12 +515,22 @@ 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). /// -/// /proc/self is deliberately not resolved: canonicalize would turn -/// /proc/self/maps into /proc//maps, making it look like a volatile -/// pid-specific path and causing is_junk_path to drop it. -fn canonicalize_or_keep(p: PathBuf) -> PathBuf { - if p.as_os_str().as_encoded_bytes().starts_with(b"/proc/self") { - return p; +/// /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) } diff --git a/crates/sandlock-cli/tests/learn_test.rs b/crates/sandlock-cli/tests/learn_test.rs index f1f9bd96..b77a8595 100644 --- a/crates/sandlock-cli/tests/learn_test.rs +++ b/crates/sandlock-cli/tests/learn_test.rs @@ -704,8 +704,8 @@ fn test_write_collapse_skips_root() { let stderr = String::from_utf8_lossy(&output.stderr); 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("protected path '/'"), - "expected protected path '/' warning in stderr, got: {stderr}"); + assert!(stderr.contains("filesystem root"), + "expected filesystem root warning in stderr, got: {stderr}"); } /// Write collapse to a sensitive path emits a warning and observed-vs-granted diff. @@ -788,6 +788,42 @@ fn test_direct_write_root_skipped() { "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. From 72e0d9f0ce391769d6c3f8aeabb7eb7477edcb3c Mon Sep 17 00:00:00 2001 From: Vahagn Date: Thu, 10 Sep 2026 02:44:14 +0400 Subject: [PATCH 4/4] [docs] learn: rewrite path tiers paragraph to clarify direct-write behavior Signed-off-by: Vahagn --- docs/learn.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/learn.md b/docs/learn.md index c99eb067..2008af89 100644 --- a/docs/learn.md +++ b/docs/learn.md @@ -59,7 +59,7 @@ optional, so omitting the ancestor would cause `sandlock run` to abort. | **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 (when a non-existent path's nearest existing ancestor is used as the Landlock grant). Direct writes to an existing path are always recorded; a notice is printed to stderr when the path is Protected or Guarded. **The sole exception is `/`: a direct read or write of the filesystem root is always dropped with a warning, since granting `/` would subsume every other entry in the profile.** +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