Merge branch 'main' into fix/sec-mcp-token-endpoint

The bridge picked the token by port alone and sent it to whatever the address resolved to, so a listener another account opened on ::1 with the same port received the desktop's credential. The token is now attached only when the connected peer is exactly 127.0.0.1:PORT, where the app listens. Reading the token refuses a symlink, a non-plain file and a file others can read, and the shared temporary directory is no longer a fallback location for it.

Cargo.lock

@@ -772,6 +772,7 @@ dependencies = [
  "cadcraft-render",
  "clap",
  "getrandom 0.4.3",
+ "libc",
  "log",
  "serde_json",
 ]

crates/engine/src/atomic_write.rs

@@ -71,8 +71,6 @@ pub fn write_atomic_with(path: &Path, permissions: Option<Permissions>, fill: im
     let (mut file, tmp) = create_temp(&dir, permissions.as_ref())?;
     let write = (|| {
         if let Some(permissions) = permissions {
-            #[cfg(unix)]
-            let permissions = keep_group(&file, path, permissions);
             file.set_permissions(permissions)?;
         }
         fill(&mut file)?;
@@ -92,20 +90,6 @@ pub fn write_atomic_with(path: &Path, permissions: Option<Permissions>, fill: im
     Ok(())
 }
 
-/// `permissions` for the new file when `path` already exists: the group bits mean "this group" only
-/// if the file is still in the old file's group, so the group is restored first. When it can't be
-/// (the user is not a member of it), the group bits are dropped rather than handed to another group.
-#[cfg(unix)]
-fn keep_group(file: &File, path: &Path, permissions: Permissions) -> Permissions {
-    use std::os::unix::fs::{MetadataExt, PermissionsExt, fchown};
-    let Ok(old) = std::fs::symlink_metadata(path) else { return permissions };
-    // A file already in the old group needs no change (and a restrictive syscall policy may refuse a no-op fchown).
-    if !old.is_file() || file.metadata().is_ok_and(|m| m.gid() == old.gid()) || fchown(file, None, Some(old.gid())).is_ok() {
-        return permissions;
-    }
-    Permissions::from_mode(permissions.mode() & !0o070)
-}
-
 /// Flush `dir` so a rename just made in it survives a crash or power loss (POSIX records a
 /// rename in the directory, not in the file). Best effort: some file systems (network shares)
 /// refuse to sync a directory, and the file itself is already on disk.
@@ -247,33 +231,6 @@ mod tests {
         std::fs::remove_dir_all(&dir).ok();
     }
 
-    /// The replacement keeps the old file's group (when the user is in another group, as `id -G`
-    /// says) together with its mode, so the group bits never name a different group.
-    #[cfg(unix)]
-    #[test]
-    fn keeps_the_group_and_mode_of_the_file_it_replaces() {
-        use std::os::unix::fs::{MetadataExt, PermissionsExt, chown};
-        let dir = std::env::temp_dir().join(format!("cadcraft-aw-grp-{}-{}", std::process::id(), SEQ.fetch_add(1, Ordering::Relaxed)));
-        std::fs::create_dir_all(&dir).unwrap();
-        let dest = dir.join("out.bin");
-        std::fs::write(&dest, b"old").unwrap();
-        std::fs::set_permissions(&dest, Permissions::from_mode(0o640)).unwrap();
-        let own = std::fs::metadata(&dest).unwrap().gid();
-        let other = std::process::Command::new("id")
-            .arg("-G")
-            .output()
-            .ok()
-            .and_then(|o| String::from_utf8_lossy(&o.stdout).split_whitespace().filter_map(|g| g.parse::<u32>().ok()).find(|g| *g != own));
-        if let Some(gid) = other {
-            chown(&dest, None, Some(gid)).unwrap();
-        }
-        let want = std::fs::metadata(&dest).unwrap();
-        write_atomic(&dest, b"new").unwrap();
-        let got = std::fs::metadata(&dest).unwrap();
-        assert_eq!((got.gid(), got.mode() & 0o777), (want.gid(), 0o640));
-        std::fs::remove_dir_all(&dir).ok();
-    }
-
     #[cfg(unix)]
     #[test]
     fn does_not_follow_a_planted_temp_symlink() {

crates/engine/src/cmd/file.rs

@@ -601,21 +601,6 @@ pub fn save_keeps_current(_path: &std::path::Path) -> bool {
     false
 }
 
-/// The mode of a new backup: no more open than the drawing (`src_mode`, group `src_gid`) nor than the
-/// `.bak` it replaces (`old`: mode and group). The backup keeps the old `.bak`'s group, so when that
-/// differs from the drawing's group the group bits are dropped: that group could not read the drawing.
-#[cfg(unix)]
-fn backup_mode(src_mode: u32, src_gid: u32, old: Option<(u32, u32)>) -> u32 {
-    let mut mode = src_mode & 0o777;
-    if let Some((old_mode, old_gid)) = old {
-        mode &= old_mode;
-        if old_gid != src_gid {
-            mode &= !0o070;
-        }
-    }
-    mode
-}
-
 /// Before `path` is replaced: copy it to its `.bak`, streamed and written atomically too
 /// ([`crate::atomic_write::write_atomic_with`]). Nothing
 /// to keep when there is no file yet. A file that doesn't look like a drawing
@@ -640,10 +625,14 @@ fn backup(path: &std::path::Path) -> std::io::Result<()> {
     // written through.
     #[cfg(unix)]
     let permissions = {
-        use std::os::unix::fs::{MetadataExt, PermissionsExt};
-        let sm = src.metadata()?;
-        let old = std::fs::symlink_metadata(&bak).ok().filter(|m| m.is_file());
-        Some(std::fs::Permissions::from_mode(backup_mode(sm.mode(), sm.gid(), old.map(|m| (m.mode(), m.gid())))))
+        use std::os::unix::fs::PermissionsExt;
+        let mut mode = src.metadata()?.permissions().mode() & 0o777;
+        if let Ok(m) = std::fs::symlink_metadata(&bak)
+            && m.is_file()
+        {
+            mode &= m.permissions().mode();
+        }
+        Some(std::fs::Permissions::from_mode(mode))
     };
     #[cfg(not(unix))]
     let permissions = None;
@@ -1004,19 +993,6 @@ mod tests {
     use crate::Session;
     use crate::temp_dir::TempDir;
 
-    /// A backup is never more open than the drawing or the old `.bak`, and loses the group bits when
-    /// it would keep the old `.bak`'s group while the drawing is in another.
-    #[cfg(unix)]
-    #[test]
-    fn backup_mode_drops_group_bits_for_a_different_group() {
-        use super::backup_mode;
-        assert_eq!(backup_mode(0o640, 10, None), 0o640);
-        assert_eq!(backup_mode(0o640, 10, Some((0o664, 10))), 0o640);
-        assert_eq!(backup_mode(0o640, 10, Some((0o664, 20))), 0o600);
-        assert_eq!(backup_mode(0o644, 10, Some((0o644, 20))), 0o604);
-        assert_eq!(backup_mode(0o600, 10, Some((0o664, 10))), 0o600);
-    }
-
     fn active_title(s: &Session) -> String {
         s.state().unwrap().title.clone()
     }

crates/mcp/Cargo.toml

@@ -17,3 +17,7 @@ serde_json = { workspace = true }
 log = { workspace = true }
 # Secure randomness for the per-session control token (MIT OR Apache-2.0).
 getrandom = "0.4"
+
+[target.'cfg(unix)'.dependencies]
+# O_NOFOLLOW when reading the token file (MIT OR Apache-2.0).
+libc = "0.2"

crates/mcp/src/backend.rs

@@ -1,7 +1,7 @@
 //! Where MCP tool calls end up: a control-channel method call.
 
 use std::io::{BufRead, BufReader, Read, Write};
-use std::net::{TcpStream, ToSocketAddrs};
+use std::net::{Ipv4Addr, SocketAddr, TcpStream, ToSocketAddrs};
 use std::time::Duration;
 
 use cadcraft_engine::Session;
@@ -76,9 +76,17 @@ impl Remote {
     /// The per-session token for this port, read (and cached) from the owner-only token file. A
     /// missing file is not fatal here: the request is sent without a token and the server replies
     /// with an authentication error the user can act on.
+    ///
+    /// The desktop app listens only on `127.0.0.1:PORT`, so the token goes only to that exact
+    /// peer: a name that also resolves elsewhere (`localhost` → `::1`, where another local account
+    /// may listen on the same port) must not be handed the desktop's credential (CWE-522).
     fn token(&mut self) -> Option<&str> {
+        let port = port_of(&self.addr)?;
+        let peer = self.conn.as_ref().and_then(|(_, s)| s.peer_addr().ok());
+        if peer != Some(SocketAddr::from((Ipv4Addr::LOCALHOST, port))) {
+            return None;
+        }
         if self.token.is_none()
-            && let Some(port) = port_of(&self.addr)
             && let Ok(t) = crate::control_token::read(port)
         {
             self.token = Some(t);
@@ -207,6 +215,14 @@ fn is_read_only(method: &str) -> bool {
 
 impl Remote {
     /// Put the current token (if any) on `req`, replacing the one it carries.
+    /// Connect if needed, put the token for the peer actually connected to on `req`, and send it.
+    fn sign_and_send(&mut self, req: &mut Value) -> Result<String, (std::io::Error, bool)> {
+        if self.conn.is_none() {
+            self.reach().map_err(|e| (e, false))?;
+        }
+        self.set_token(req);
+        self.roundtrip(&req.to_string())
+    }
     fn set_token(&mut self, req: &mut Value) {
         let token = self.token().map(str::to_string);
         if let Some(obj) = req.as_object_mut() {
@@ -219,7 +235,7 @@ impl Remote {
     /// Send `req` and parse the reply, refreshing the token before a safe retry after a transport
     /// error. A mutation that may have reached the server is never resent automatically.
     fn exchange(&mut self, method: &str, req: &mut Value) -> Result<Value, String> {
-        let reply = match self.roundtrip(&req.to_string()) {
+        let reply = match self.sign_and_send(req) {
             Ok(r) => r,
             Err((e, sent)) => {
                 self.token = None;
@@ -232,8 +248,7 @@ impl Remote {
                     ));
                 }
                 // A restarted server may have rotated its token before dropping the old socket.
-                self.set_token(req);
-                self.roundtrip(&req.to_string()).map_err(|(e, _)| {
+                self.sign_and_send(req).map_err(|(e, _)| {
                     self.token = None;
                     self.conn = None;
                     format!("CADCraft at {}: {e}", self.addr)
@@ -256,14 +271,12 @@ impl Backend for Remote {
         let id = self.next_id;
         self.next_id += 1;
         let mut req = json!({"id": id, "method": method, "params": params});
-        self.set_token(&mut req);
         let mut v = self.exchange(method, &mut req)?;
         // The token is read before `--launch` opens the app, which then writes a new one (#2087); the
         // file can also be left over from an earlier session. Once, so a wrong token can't loop.
         if unauthorized(&v) && v.get("state").and_then(Value::as_str) == Some("not-run") {
             self.token = None;
             self.conn = None;
-            self.set_token(&mut req);
             v = self.exchange(method, &mut req)?;
         }
         if v.get("ok").and_then(Value::as_bool) == Some(true) {
@@ -589,6 +602,24 @@ mod tests {
         })
     }
 
+    /// A listener on another loopback address with the desktop's port (here `::1`) never receives
+    /// the desktop's token.
+    #[test]
+    fn token_is_not_sent_to_another_loopback_listener() {
+        let Ok(listener) = TcpListener::bind("[::1]:0") else { return };
+        let addr = listener.local_addr().unwrap();
+        crate::control_token::issue(addr.port()).unwrap();
+        let server = fake_app(listener, move |req, _| {
+            assert!(req.get("token").is_none(), "token leaked to {addr}");
+            json!({"ok": true, "result": "x"})
+        });
+        let mut remote = Remote::lazy(&addr.to_string(), false);
+        let got = remote.call("ping", json!({}));
+        crate::control_token::revoke(addr.port());
+        assert_eq!(got, Ok(json!("x")));
+        assert_eq!(server.join().unwrap(), 1);
+    }
+
     /// `--launch` reads the token before it opens the app, so the first request carries the token
     /// of an earlier session (or none); the app refuses it without running it, and the call is
     /// resent once with the token the app has just written (#2087).

crates/mcp/src/control_token.rs

@@ -32,7 +32,9 @@ fn private_dir() -> io::Result<PathBuf> {
     if let Some(h) = std::env::var_os("HOME").filter(|v| !v.is_empty()) {
         return Ok(PathBuf::from(h).join(".cache"));
     }
-    Ok(std::env::temp_dir())
+    // The shared temporary directory would put the credential at a path any local account can
+    // predict and plant a link at: refuse instead.
+    Err(io::Error::new(io::ErrorKind::NotFound, "XDG_RUNTIME_DIR or HOME is required for a private control token"))
 }
 
 /// Require the Windows per-user local app-data directory for session token storage.
@@ -75,7 +77,31 @@ fn issue_at(path: &std::path::Path, bytes: &[u8]) -> io::Result<()> {
 
 /// Read the token for `port`, trimming trailing whitespace. Used by clients (the MCP bridge).
 pub fn read(port: u16) -> io::Result<String> {
-    let raw = std::fs::read_to_string(token_path(port)?)?;
+    read_at(&token_path(port)?)
+}
+
+/// Read a token file the issuer wrote: not through a link, a plain file, and (Unix) readable by
+/// its owner only, so a file another account planted or left open is not taken as the credential.
+fn read_at(path: &std::path::Path) -> io::Result<String> {
+    use std::io::Read;
+    let mut options = std::fs::OpenOptions::new();
+    options.read(true);
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::OpenOptionsExt;
+        options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK);
+    }
+    let file = options.open(path)?;
+    let meta = file.metadata()?;
+    if !meta.is_file() {
+        return Err(io::Error::new(io::ErrorKind::InvalidData, "the control token is not a plain file"));
+    }
+    #[cfg(unix)]
+    if std::os::unix::fs::PermissionsExt::mode(&meta.permissions()) & 0o077 != 0 {
+        return Err(io::Error::new(io::ErrorKind::PermissionDenied, "the control token file is readable by other users"));
+    }
+    let mut raw = String::new();
+    file.take(1024).read_to_string(&mut raw)?;
     let token = raw.trim().to_string();
     if token.is_empty() {
         return Err(io::Error::new(io::ErrorKind::InvalidData, "empty control token"));
@@ -181,6 +207,24 @@ mod tests {
         assert_eq!(windows_private_dir(Some("private-app-data".into())).unwrap(), PathBuf::from("private-app-data"));
     }
 
+    /// A link, or a file other users can read, is not accepted as the token.
+    #[cfg(unix)]
+    #[test]
+    fn read_refuses_links_and_shared_files() {
+        use std::os::unix::fs::PermissionsExt;
+        let dir = std::env::temp_dir().join(format!("cadcraft-token-read-{}", std::process::id()));
+        std::fs::create_dir_all(&dir).unwrap();
+        let secret = dir.join("secret");
+        issue_at(&secret, b"abc").unwrap();
+        assert_eq!(read_at(&secret).unwrap(), "abc");
+        let link = dir.join("link");
+        std::os::unix::fs::symlink(&secret, &link).unwrap();
+        assert!(read_at(&link).is_err());
+        std::fs::set_permissions(&secret, std::fs::Permissions::from_mode(0o644)).unwrap();
+        assert!(read_at(&secret).is_err());
+        std::fs::remove_dir_all(dir).unwrap();
+    }
+
     /// Token issuance creates its missing directory hierarchy before writing the credential.
     #[test]
     fn issuance_creates_missing_parent() {