Merge pull request #11837 from yahya-nusseibeh/fix/control-socket-path

🤖 Agent: queued. When `/tmp` is shared the socket goes into a private `horizon-<uid>` folder, so another user on the machine can't connect to or squat on the control socket.

crates/horizon-client/src/control.rs

@@ -95,6 +95,7 @@ fn read_endpoint(path: &Path) -> io::Result<(u16, String)> {
 
 /// Connect to a running game's control endpoint (and authenticate over TCP).
 pub fn connect(path: &Path) -> io::Result<Stream> {
+    private_dir(path, false)?;
     #[cfg(unix)]
     if !uses_tcp(path) {
         return UnixStream::connect(path).map(Stream::Unix);
@@ -158,10 +159,82 @@ pub fn default_path() -> PathBuf {
     if !cfg!(unix) {
         return std::env::temp_dir().join("horizon-control.json");
     }
-    let dir = std::env::var_os("XDG_RUNTIME_DIR").map_or_else(std::env::temp_dir, PathBuf::from);
+    let dir = std::env::var_os("XDG_RUNTIME_DIR")
+        .map(PathBuf::from)
+        .or_else(fallback_dir)
+        .unwrap_or_else(std::env::temp_dir);
     dir.join("horizon-control.sock")
 }
 
+/// Without `$XDG_RUNTIME_DIR` (containers, some WSL and minimal setups), when the temporary
+/// directory is shared by every user (`/tmp`), a folder of this user's own in it,
+/// `horizon-<uid>`: a fixed name there let another user bind the socket first, so that
+/// `--control` failed and `horizon-ctl` talked to theirs. [`private_dir`] checks it before the
+/// socket is made or reached. A temporary directory of the user's own (macOS's `$TMPDIR`) is
+/// used as it is. The launcher finds the endpoint the same way (`game_tools.rs`).
+fn fallback_dir() -> Option<PathBuf> {
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::PermissionsExt;
+        let temp = std::env::temp_dir();
+        let shared = std::fs::metadata(&temp).is_ok_and(|m| m.permissions().mode() & 0o002 != 0);
+        let uid = own_uid().filter(|_| shared)?;
+        Some(temp.join(format!("horizon-{uid}")))
+    }
+    #[cfg(not(unix))]
+    None
+}
+
+/// This process's user: the owner of `/proc/self` where there is one, else of the home folder.
+#[cfg(unix)]
+fn own_uid() -> Option<u32> {
+    use std::os::unix::fs::MetadataExt;
+    std::fs::metadata("/proc/self")
+        .ok()
+        .or_else(|| std::fs::metadata(std::env::var_os("HOME")?).ok())
+        .map(|m| m.uid())
+}
+
+/// For an endpoint in [`fallback_dir`]: [`check_private`] on that folder. Elsewhere (the user's
+/// `$HORIZON_CONTROL`, the runtime directory) nothing to check.
+fn private_dir(path: &Path, make: bool) -> io::Result<()> {
+    match fallback_dir() {
+        Some(dir) if path.parent() == Some(dir.as_path()) => check_private(&dir, make),
+        _ => Ok(()),
+    }
+}
+
+/// `dir` made (mode 0700) when `make`, and refused unless it is a real folder, this user's, that
+/// no one else may enter.
+#[cfg(unix)]
+fn check_private(dir: &Path, make: bool) -> io::Result<()> {
+    use std::os::unix::fs::{DirBuilderExt, MetadataExt, PermissionsExt};
+    if make {
+        match std::fs::DirBuilder::new().mode(0o700).create(dir) {
+            Err(e) if e.kind() != io::ErrorKind::AlreadyExists => return Err(e),
+            _ => {}
+        }
+    }
+    let meta = std::fs::symlink_metadata(dir)?;
+    if !meta.is_dir()
+        || meta.permissions().mode() & 0o077 != 0
+        || own_uid().is_some_and(|uid| uid != meta.uid())
+    {
+        return Err(io::Error::new(
+            io::ErrorKind::PermissionDenied,
+            format!(
+                "{} isn't a folder only this user can open; set $HORIZON_CONTROL or $XDG_RUNTIME_DIR",
+                dir.display()
+            ),
+        ));
+    }
+    Ok(())
+}
+#[cfg(not(unix))]
+fn check_private(_: &Path, _: bool) -> io::Result<()> {
+    Ok(())
+}
+
 /// Whether `path` can be an endpoint at all, without touching it: a Unix socket's path has a
 /// length limit (`sockaddr_un`); a TCP endpoint is an ordinary file and has none.
 pub fn check_path(path: &Path) -> io::Result<()> {
@@ -184,6 +257,7 @@ pub fn check_path(path: &Path) -> io::Result<()> {
 impl Server {
     pub fn start(path: &Path) -> io::Result<Self> {
         check_path(path)?;
+        private_dir(path, true)?;
         // An endpoint left by a crashed game refuses connections; replace it.
         if path.exists() && connect(path).is_err() {
             std::fs::remove_file(path)?;
@@ -392,6 +466,36 @@ fn serve(stream: Stream, sender: &mpsc::Sender<Request>) {
 
 #[cfg(test)]
 mod tests {
+    #[cfg(unix)]
+    #[test]
+    fn the_fallback_socket_folder_must_be_private() {
+        use std::os::unix::fs::PermissionsExt;
+        let base = std::env::temp_dir().join(format!("horizon-control-dir-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&base);
+        std::fs::create_dir_all(&base).unwrap();
+        // Made 0700 when missing, and then fine to reach.
+        let made = base.join("made");
+        super::check_private(&made, true).unwrap();
+        let mode = std::fs::metadata(&made).unwrap().permissions().mode();
+        assert_eq!(mode & 0o777, 0o700);
+        super::check_private(&made, false).unwrap();
+        // One another user could enter or write: refused, made or not.
+        let open = base.join("open");
+        std::fs::create_dir(&open).unwrap();
+        std::fs::set_permissions(&open, std::fs::Permissions::from_mode(0o777)).unwrap();
+        assert!(super::check_private(&open, true).is_err());
+        assert!(super::check_private(&open, false).is_err());
+        // A link to a private folder, or a file: refused.
+        std::os::unix::fs::symlink(&made, base.join("link")).unwrap();
+        assert!(super::check_private(&base.join("link"), false).is_err());
+        std::fs::write(base.join("file"), b"").unwrap();
+        assert!(super::check_private(&base.join("file"), true).is_err());
+        // Missing and not to be made: an error, not a folder made by whoever connects.
+        assert!(super::check_private(&base.join("missing"), false).is_err());
+        assert!(!base.join("missing").exists());
+        let _ = std::fs::remove_dir_all(&base);
+    }
+
     use super::*;
 
     /// The check agrees with the system at every length around its limit.

crates/horizon-client/src/game/control.rs

@@ -1127,6 +1127,10 @@ impl Game<'_> {
             }
             "bench" => {
                 let seconds: f64 = arg(1)?.parse()?;
+                // `Duration::from_secs_f64` panics on a negative, NaN or huge number.
+                if !(seconds.is_finite() && (0.0..=86_400.0).contains(&seconds)) {
+                    return Err("bench takes 0 to 86400 seconds".into());
+                }
                 if self.pending_bench.is_some() {
                     return Err("a benchmark is already running".into());
                 }

docs/native-client.md

@@ -366,7 +366,9 @@ in an MCP client as a stdio server with the command `horizon-ctl` and the
 argument `mcp` (`--socket PATH` before `mcp` to pick a socket).
 
 **Transport.** On Linux and macOS the endpoint is a Unix socket:
-`$HORIZON_CONTROL`, else `$XDG_RUNTIME_DIR/horizon-control.sock`, or
+`$HORIZON_CONTROL`, else `$XDG_RUNTIME_DIR/horizon-control.sock` (without a runtime
+directory and with a temporary directory every user shares, as `/tmp`,
+`horizon-<uid>/horizon-control.sock` in it, a folder only that user may open), or
 `--control-socket PATH` (`horizon-ctl --socket PATH`). Windows has no Unix
 sockets: there the game listens on a random `127.0.0.1` port and writes
 `{"transport":"tcp","port":N,"token":"..."}` to the endpoint path

launcher/src/game_tools.rs

@@ -44,10 +44,13 @@ pub fn control_args() -> Vec<String> {
 /// The endpoint a game listens on when nothing names one, which is where Agent Control opens it.
 /// The launcher runs whichever version is installed, so this is a contract with every game
 /// rather than shared code: `control::default_path` in horizon-client, and `horizon-ctl help`.
+/// `shared_uid` is the user's id when `temp` is shared by every user (`/tmp`): the game then
+/// keeps its socket in a folder of that user's own there, `horizon-<uid>`.
 fn default_endpoint(
     control: Option<OsString>,
     runtime: Option<OsString>,
     temp: PathBuf,
+    shared_uid: Option<u32>,
 ) -> PathBuf {
     if let Some(path) = control {
         return path.into();
@@ -55,9 +58,34 @@ fn default_endpoint(
     if !cfg!(unix) {
         return temp.join("horizon-control.json");
     }
-    runtime
-        .map_or(temp, PathBuf::from)
-        .join("horizon-control.sock")
+    match (runtime, shared_uid) {
+        (Some(runtime), _) => PathBuf::from(runtime),
+        (None, Some(uid)) => temp.join(format!("horizon-{uid}")),
+        (None, None) => temp,
+    }
+    .join("horizon-control.sock")
+}
+
+/// The user's id when the temporary directory is shared by every user (`control::fallback_dir`
+/// in horizon-client): the owner of `/proc/self`, else of the home folder.
+fn shared_temp_uid(temp: &Path) -> Option<u32> {
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt};
+        let shared = std::fs::metadata(temp).is_ok_and(|m| m.permissions().mode() & 0o002 != 0);
+        if !shared {
+            return None;
+        }
+        std::fs::metadata("/proc/self")
+            .ok()
+            .or_else(|| std::fs::metadata(std::env::var_os("HOME")?).ok())
+            .map(|m| m.uid())
+    }
+    #[cfg(not(unix))]
+    {
+        let _ = temp;
+        None
+    }
 }
 
 /// The endpoint to talk to: the game `play` started, else the game answering on the default
@@ -77,6 +105,7 @@ fn current_endpoint() -> PathBuf {
             std::env::var_os("HORIZON_CONTROL"),
             std::env::var_os("XDG_RUNTIME_DIR"),
             std::env::temp_dir(),
+            shared_temp_uid(&std::env::temp_dir()),
         ),
     )
 }
@@ -369,21 +398,32 @@ mod tests {
         let temp = PathBuf::from("temp");
         let runtime = || Some(OsString::from("run"));
         assert_eq!(
-            default_endpoint(Some("mine.sock".into()), runtime(), temp.clone()),
+            default_endpoint(
+                Some("mine.sock".into()),
+                runtime(),
+                temp.clone(),
+                Some(1000)
+            ),
             PathBuf::from("mine.sock")
         );
         if cfg!(unix) {
             assert_eq!(
-                default_endpoint(None, runtime(), temp.clone()),
+                default_endpoint(None, runtime(), temp.clone(), Some(1000)),
                 Path::new("run").join("horizon-control.sock")
             );
+            // A temporary directory of the user's own, as macOS's.
             assert_eq!(
-                default_endpoint(None, None, temp.clone()),
+                default_endpoint(None, None, temp.clone(), None),
                 temp.join("horizon-control.sock")
             );
+            // A shared one: the user's own folder in it.
+            assert_eq!(
+                default_endpoint(None, None, temp.clone(), Some(1000)),
+                temp.join("horizon-1000/horizon-control.sock")
+            );
         } else {
             assert_eq!(
-                default_endpoint(None, runtime(), temp.clone()),
+                default_endpoint(None, runtime(), temp.clone(), None),
                 temp.join("horizon-control.json")
             );
         }