fix(collab-net): never send the server key over plain ws:// to another machine (#1923)

* fix(collab-net): never send the server key over plain ws:// to another machine

Publishing a plugin, uploading or sharing with the server's key accepted a
ws:// address for a remote host, so the reusable administrative key went
out in clear text. The key now goes only over wss://, or over ws:// to
this machine (loopback or localhost); browsing without a key is unchanged.

* address review: connect only to loopback addresses for a plain ws://localhost endpoint

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<Published, String> {
     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<Admin>, limits: &N
 fn share(server: &str, file: &str, owner: &str, revoke: &[String], create: bool, limits: &NetLimits) -> Result<Option<Shared>, 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<PluginReply, String> {
     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)
 }
 

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<TcpStream, String> {
+/// 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<TcpStream, String> {
     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::<SocketAddr>().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");
+        }
+    }
+}

crates/collab-net/src/requests.rs

@@ -32,6 +32,15 @@ pub(crate) fn server_key(given: Option<String>, 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<AdminReply>) -> Option<Adm
         other => 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());
+    }
+}