From ba369f82ba3e7bbed5f96f0722743436ae2d28f6 Mon Sep 17 00:00:00 2001
From: "K. Isom" <kyle@imap.cc>
Date: Tue, 22 Sep 2026 21:44:29 -0700
Subject: [PATCH] brokerd: refuse / and a symbolic link as a socket's directory

brokerd makes a socket's directory 0700. With a socket directly in / it would
chmod /, and through a symbolic link it would change the link's target. Both
are now refused at start with #brokerd-start-failed. Without the fix the link
case started and served, with the shared directory made private.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---
 crates/brokerd/src/serve.rs            | 26 +++++++++++++--
 crates/brokerd/tests/serve_pointers.rs | 45 ++++++++++++++++++++++++++
 docs/runbook.md                        |  4 +++
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/crates/brokerd/src/serve.rs b/crates/brokerd/src/serve.rs
index cab7fac..09379d3 100644
--- a/crates/brokerd/src/serve.rs
+++ b/crates/brokerd/src/serve.rs
@@ -99,9 +99,18 @@ pub fn start(
 /// Bind one socket: prepare its directory at 0700, make it private, remove any stale
 /// socket, bind, then make the socket itself private at 0600.
 fn listen(socket: &Path) -> Result<UnixListener, ServeError> {
+    // A socket needs a directory of its own: step 2 makes that directory 0700, which must never
+    // be `/` or a shared directory reached through a symbolic link.
     let dir = match socket.parent() {
-        Some(parent) => parent.to_path_buf(),
-        None => PathBuf::from("/"),
+        Some(parent) if parent.parent().is_some() && !parent.as_os_str().is_empty() => {
+            parent.to_path_buf()
+        }
+        _ => {
+            return Err(ServeError::Dir(
+                PathBuf::from(socket),
+                io::Error::other("a socket needs a directory of its own, not / or none"),
+            ));
+        }
     };
 
     // 1. The directory, at 0700.
@@ -110,6 +119,19 @@ fn listen(socket: &Path) -> Result<UnixListener, ServeError> {
         .mode(0o700)
         .create(&dir)
         .map_err(|e| ServeError::Dir(dir.clone(), e))?;
+    // `set_permissions` follows a symbolic link, so a link would make the directory it points to
+    // private instead: refuse it.
+    let kind = std::fs::symlink_metadata(&dir)
+        .map_err(|e| ServeError::Dir(dir.clone(), e))?
+        .file_type();
+    if !kind.is_dir() {
+        return Err(ServeError::Dir(
+            dir,
+            io::Error::other(
+                "is a symbolic link or not a directory; brokerd will not change its mode",
+            ),
+        ));
+    }
     // 2. Make it private, always, even when it was already there at 0755.
     std::fs::set_permissions(&dir, Permissions::from_mode(0o700))
         .map_err(|e| ServeError::Dir(dir.clone(), e))?;
diff --git a/crates/brokerd/tests/serve_pointers.rs b/crates/brokerd/tests/serve_pointers.rs
index d9860bd..1fe5ff3 100644
--- a/crates/brokerd/tests/serve_pointers.rs
+++ b/crates/brokerd/tests/serve_pointers.rs
@@ -99,6 +99,51 @@ fn a_socket_directory_that_cannot_be_made_names_the_entry() {
     ]));
 }
 
+/// The socket's directory is made private (0700). Through a symbolic link that would change the
+/// directory it points to, so a link is refused, and the target keeps its mode.
+#[test]
+fn a_socket_directory_that_is_a_symbolic_link_is_refused() {
+    use std::os::unix::fs::PermissionsExt;
+    let dir = TempDir::new("ptr-link");
+    let shared = dir.path().join("shared");
+    std::fs::create_dir(&shared).unwrap();
+    std::fs::set_permissions(&shared, std::fs::Permissions::from_mode(0o755)).unwrap();
+    let link = dir.path().join("link");
+    std::os::unix::fs::symlink(&shared, &link).unwrap();
+    let path = config(
+        &dir,
+        &format!("broker = \"{}\"", link.join("broker.sock").display()),
+    );
+
+    let out = brokerd([
+        OsStr::new("serve"),
+        OsStr::new("--config"),
+        path.as_os_str(),
+    ]);
+    fails_with_pointer(&out);
+    assert!(stderr(&out).contains("symbolic link"), "{}", stderr(&out));
+    let mode = std::fs::metadata(&shared).unwrap().permissions().mode() & 0o777;
+    assert_eq!(mode, 0o755, "the link's target keeps its mode");
+}
+
+/// A socket directly in `/` would make `/` private: refused before anything is changed.
+#[test]
+fn a_socket_in_the_root_directory_is_refused() {
+    let dir = TempDir::new("ptr-root");
+    let path = config(&dir, "broker = \"/broker.sock\"");
+    let out = brokerd([
+        OsStr::new("serve"),
+        OsStr::new("--config"),
+        path.as_os_str(),
+    ]);
+    fails_with_pointer(&out);
+    assert!(
+        stderr(&out).contains("directory of its own"),
+        "{}",
+        stderr(&out)
+    );
+}
+
 /// A config path that is not UTF-8 is still a path: it is read (here: not found), not a panic.
 #[test]
 fn a_config_path_that_is_not_utf8_is_read_as_a_path() {
diff --git a/docs/runbook.md b/docs/runbook.md
index 9fa6e15..a81a45e 100644
--- a/docs/runbook.md
+++ b/docs/runbook.md
@@ -182,6 +182,10 @@ programs off it.
 - `cannot prepare <dir>: …` — the directory cannot be made or `chmod`ed:
   `ls -ld <dir> "$(dirname <dir>)"`. A path that runs through a file, or a directory owned by
   another user, gives this.
+- `cannot prepare <socket or dir>: a socket needs a directory of its own` or `…: is a symbolic
+  link or not a directory` — `brokerd` makes a socket's directory 0700, so it refuses `/` and a
+  directory reached through a link (the link's target would be changed instead). Give each socket
+  its own real directory, as the defaults under `$BOXMAKER_HOME/run/` are.
 - `cannot listen on <socket>: …` — `path must be shorter than SUN_LEN` means the socket path is
   longer than 107 bytes; `Address already in use` means something still listens there
   (`ss -xlp | grep <socket>`).