diff --git a/crates/collab-net/src/link.rs b/crates/collab-net/src/link.rs index 1cf23c9a2..30bc587c3 100644 --- a/crates/collab-net/src/link.rs +++ b/crates/collab-net/src/link.rs @@ -220,6 +220,7 @@ impl LiveLink for NetLink { let (server, query) = Endpoint::parse(&o.server)?; // Without a key: as anyone may, when the server is open to the public (it says if not). let key = requests::server_key(o.key, query); + requests::key_over_tls(&server, key.as_deref())?; // Until the upload is done the invite names only the server. let cfg = self.guest_config(Invite { server: server.clone(), file: String::new(), token: String::new() }, None, &o.me); let inbox = self.sessions.inbox(); @@ -342,6 +343,7 @@ fn upload(o: PublishOptions, limits: &NetLimits) -> Result { let (server, query) = Endpoint::parse(&o.server)?; let key = requests::server_key(o.key, query) .ok_or("the server address needs the server's key (ws://host:port/?key=…; `figcraft-server key` prints it)")?; + requests::key_over_tls(&server, Some(&key))?; upload_file(&server, Some(&key), &o.name, &o.doc, limits, CONNECT).map(|(_, published, _)| published) } @@ -358,6 +360,7 @@ fn hosting(server: &str, file: &str, owner: &str, req: Option, limits: &N fn share(server: &str, file: &str, owner: &str, revoke: &[String], create: bool, limits: &NetLimits) -> Result, String> { let (endpoint, query) = Endpoint::parse(server)?; let key = requests::server_key(None, query); + requests::key_over_tls(&endpoint, key.as_deref())?; manage::share(&endpoint, key.as_deref(), file, owner, revoke, create, limits, CONNECT) } @@ -365,6 +368,7 @@ fn share(server: &str, file: &str, owner: &str, revoke: &[String], create: bool, fn plugins(server: &str, req: PluginReq, limits: &NetLimits) -> Result { let (server, query) = Endpoint::parse(server)?; let key = query.into_iter().find(|(k, _)| k == "key").map(|(_, v)| v).unwrap_or_default(); + requests::key_over_tls(&server, Some(&key))?; manage::plugins(&server, key.trim(), req, limits, CONNECT) } diff --git a/crates/collab-net/src/net.rs b/crates/collab-net/src/net.rs index 9c35b5d1c..c8c29bbdb 100644 --- a/crates/collab-net/src/net.rs +++ b/crates/collab-net/src/net.rs @@ -242,18 +242,30 @@ pub fn shutdown(s: &TcpStream) { /// Connect to `server` (through TLS for `wss://`): the reader's and the writer's handles, and the /// socket to shut it down with. pub fn open(server: &Endpoint, timeout: Duration, limits: &NetLimits) -> Result<(Duplex, Duplex, TcpStream), String> { - let stream = dial(&server.host, timeout, limits)?; + // A plain `ws://localhost` is trusted as this machine (a key may go over it), so only loopback + // addresses are connected to: a resolver mapping the name elsewhere must not carry it off-machine. + let stream = dial(&server.host, !server.secure && server.loopback(), timeout, limits)?; let tls = if server.secure { Some(tls::handshake(&stream, &server.host)?) } else { None }; Duplex::split(stream, tls).map_err(|e| e.to_string()) } -/// Connect to `host` (`name:port`), trying each of its addresses within `timeout`, with Nagle off -/// and reads and writes bounded by `limits`' hello and write timeouts. -fn dial(host: &str, timeout: Duration, limits: &NetLimits) -> Result { +/// Whether `a` may be connected to: any address, or with `loopback_only` just this machine's. +fn dialable(a: &std::net::SocketAddr, loopback_only: bool) -> bool { + !loopback_only || a.ip().is_loopback() +} + +/// Connect to `host` (`name:port`), trying each of its addresses within `timeout` (only loopback +/// ones with `loopback_only`), with Nagle off and reads and writes bounded by `limits`' hello and +/// write timeouts. +fn dial(host: &str, loopback_only: bool, timeout: Duration, limits: &NetLimits) -> Result { use std::net::ToSocketAddrs; let addrs = host.to_socket_addrs().map_err(|e| format!("can't find {host}: {e}"))?; let mut last = format!("can't find {host}"); for a in addrs { + if !dialable(&a, loopback_only) { + last = format!("{host} resolves to {a}, which is not this machine"); + continue; + } match TcpStream::connect_timeout(&a, timeout) { Ok(s) => { let _ = s.set_nodelay(true); @@ -318,3 +330,21 @@ impl Bucket { self.tokens = (self.tokens + n).min(self.burst); } } + +#[cfg(test)] +mod dial_tests { + use super::dialable; + use std::net::SocketAddr; + + #[test] + fn a_loopback_only_dial_skips_addresses_off_this_machine() { + let at = |s: &str| s.parse::().unwrap(); + for a in ["127.0.0.1:9", "[::1]:9", "127.5.5.5:9"] { + assert!(dialable(&at(a), true), "{a}"); + } + for a in ["10.0.0.5:9", "203.0.113.7:9", "[2001:db8::1]:9"] { + assert!(!dialable(&at(a), true), "{a}"); + assert!(dialable(&at(a), false), "{a} when anywhere is allowed"); + } + } +} diff --git a/crates/collab-net/src/requests.rs b/crates/collab-net/src/requests.rs index e9c63283c..a2c7f9439 100644 --- a/crates/collab-net/src/requests.rs +++ b/crates/collab-net/src/requests.rs @@ -32,6 +32,15 @@ pub(crate) fn server_key(given: Option, query: Vec<(String, String)>) -> given.or_else(|| query.into_iter().find(|(k, _)| k == "key").map(|(_, v)| v)).filter(|k| !k.trim().is_empty()) } +/// Refuse to send the server's key (its whole administrative authority, reusable) in the clear: +/// `ws://` is only for a server on this machine; anywhere else needs `wss://`. +pub(crate) fn key_over_tls(server: &Endpoint, key: Option<&str>) -> Result<(), String> { + if key.is_some_and(|k| !k.trim().is_empty()) && !server.secure && !server.loopback() { + return Err("the server's key is only sent over wss:// (or ws:// to this machine): it would be readable on the network".into()); + } + Ok(()) +} + /// Why asking `server` about its hosting failed with `e`: a server older than open hosting can't /// read the question. pub(crate) fn hosting_error(server: &Endpoint, e: String) -> String { @@ -145,3 +154,21 @@ pub(crate) fn hosted(server: &Endpoint, reply: Option) -> Option other, }) } + +#[cfg(test)] +mod key_tests { + use super::*; + + #[test] + fn the_key_never_goes_over_plain_tcp_to_another_machine() { + let at = |url: &str| Endpoint::parse(url).unwrap().0; + assert!(key_over_tls(&at("ws://share.example:9000/"), Some("k")).is_err()); + assert!(key_over_tls(&at("ws://10.0.0.5:9000/"), Some("k")).is_err()); + assert!(key_over_tls(&at("wss://share.example/"), Some("k")).is_ok()); + assert!(key_over_tls(&at("ws://127.0.0.1:9000/"), Some("k")).is_ok()); + assert!(key_over_tls(&at("ws://localhost:9000/"), Some("k")).is_ok()); + // Browsing or publishing as anyone sends no key, so plain is fine. + assert!(key_over_tls(&at("ws://share.example:9000/"), None).is_ok()); + assert!(key_over_tls(&at("ws://share.example:9000/"), Some(" ")).is_ok()); + } +}