From f5b136e419d3f1c5deadb8049f170224bebdb94b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 15:17:27 +0000 Subject: [PATCH] fix(mcp): send the control token only to 127.0.0.1:PORT and read it only from a private file 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 | 1 + crates/mcp/Cargo.toml | 3 +++ crates/mcp/src/backend.rs | 45 ++++++++++++++++++++++++++----- crates/mcp/src/control_token.rs | 48 +++++++++++++++++++++++++++++++-- 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9b3467cfb..f36c9607c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -776,6 +776,7 @@ dependencies = [ "cadcraft-render", "clap", "getrandom 0.4.3", + "libc", "log", "serde_json", "tiny-skia", diff --git a/crates/mcp/Cargo.toml b/crates/mcp/Cargo.toml index dc5d8ca2a..cb597504f 100644 --- a/crates/mcp/Cargo.toml +++ b/crates/mcp/Cargo.toml @@ -21,3 +21,6 @@ getrandom = "0.4" [dev-dependencies] # Reads the pixels of rendered PNGs in the tests. tiny-skia = { workspace = true } +[target.'cfg(unix)'.dependencies] +# O_NOFOLLOW when reading the token file (MIT OR Apache-2.0). +libc = "0.2" diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index d1ac6727d..efdb9c9ea 100644 --- a/crates/mcp/src/backend.rs +++ b/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 { + 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 { - 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) { @@ -595,6 +608,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). diff --git a/crates/mcp/src/control_token.rs b/crates/mcp/src/control_token.rs index 084f8b455..97052be60 100644 --- a/crates/mcp/src/control_token.rs +++ b/crates/mcp/src/control_token.rs @@ -32,7 +32,9 @@ fn private_dir() -> io::Result { 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 { - 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 { + 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() {