Address feedback

CHANGELOG.md

@@ -42,7 +42,7 @@ Making a new release? Simply add the new header with the version and date undern
 * Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265])
 * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266])
 * Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267])
-* `rojo serve` now validates the `Host`/`Origin` headers on loopback binds to protect the local server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a non-loopback (network-reachable) address. ([#1270])
+* `rojo serve` now validates the `Host`/`Origin` headers to protect the local/private server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a network-reachable address. ([#1270])
 
 [#1176]: https://github.com/rojo-rbx/rojo/pull/1176
 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179

src/web/api.rs

@@ -359,16 +359,16 @@ impl ApiService {
         // Opening a file launches a local program, so it must never be reachable
         // by a remote client even when the server is bound to an exposed address.
         //
-        // `remote_addr` is the immediate peer, which is the strongest locality
+        // `remote_addr` is the immediate peer, which is the best locality signal
-        // signal available: the legitimate caller is a sandboxed Roblox plugin
+        // we have: the legitimate caller is a sandboxed Roblox plugin whose only
-        // whose only credential is being able to reach the port, so there is no
+        // credential is being able to reach the port, so there is no secret to
-        // secret to authenticate it with. A connection forwarded over loopback by
+        // authenticate it with. A connection forwarded over loopback by an
-        // an SSH/Tailscale tunnel or a local reverse proxy therefore appears local
+        // SSH/Tailscale tunnel or a local reverse proxy therefore appears local
-        // and is allowed. We treat that as delegated trust rather than a bypass:
+        // and is allowed. That is delegated trust rather than a bypass: by
-        // by deliberately standing up that tunnel/proxy the user has decided the
+        // standing up that tunnel or proxy the user has decided the remote end is
-        // remote end is trusted, and reachability is bounded by *that* hop's own
+        // trusted, and reachability is bounded by that hop's own authentication
-        // authentication (SSH keys, Tailscale ACLs, ...). This gate's job is to
+        // (e.g. SSH keys or Tailscale ACLs). This gate only stops direct,
-        // stop direct, unauthenticated peers, which it does.
+        // unauthenticated peers.
         if !self.remote_addr.ip().is_loopback() {
             return msgpack(
                 ErrorResponse::forbidden("/api/open is only available to local clients"),

src/web/origin.rs

@@ -6,17 +6,29 @@
 //! browser Same-Origin Policy. However, a DNS rebinding attack can defeat that:
 //! the page points its own hostname at `127.0.0.1` after loading, so the browser
 //! treats requests to the Rojo server as same-origin. Validating that the `Host`
-//! (and, if present, `Origin`) header refers to a local address blocks this,
+//! (and, if present, `Origin`) header refers to an address we recognize blocks
-//! because the rebound request still carries the attacker's hostname.
+//! this, because the rebound request still carries the attacker's hostname, which
+//! is a domain name rather than one of the IP literals we accept.
 //!
-//! Validation is only enforced for loopback binds. If the user has explicitly
+//! Enforcement covers two kinds of bind:
-//! exposed the server on a non-loopback address we can't know which hostnames
+//!
-//! legitimately resolve to it, so enforcement is disabled (they are warned at
+//! * Loopback (the default): only `localhost` and loopback literals are
-//! startup instead). One consequence worth being explicit about: an exposed bind
+//!   accepted.
-//! therefore has *no* rebinding protection, even for a browser on the same
+//! * A specific private/LAN address: `localhost`, loopback literals, and that exact bind
-//! machine, because the `Host` check that would catch it is turned off. That is
+//!   IP are accepted. Because the defense works by rejecting any `Host` that
-//! an accepted trade-off: exposing the unauthenticated API to the network is
+//!   isn't a recognized IP literal, clients must connect to a private bind by
-//! already the dominant risk, which the startup warning addresses.
+//!   IP. A hostname (e.g. `mypc.local`) is indistinguishable from an attacker
+//!   domain and is rejected.
+//!
+//! Enforcement is disabled for unspecified (`0.0.0.0`/`::`) and public binds: the
+//! user has asked for broad, possibly-public exposure where arbitrary hostnames
+//! may legitimately resolve to the server, so we can't build a meaningful
+//! allowlist. Those binds get a startup warning instead. Two consequences worth
+//! being explicit about: such a bind has no rebinding protection even for a
+//! browser on the same machine; and even on a protected private bind this check
+//! does nothing against a hostile peer already on the LAN, who can reach the
+//! unauthenticated API directly. Both are the network-exposure risk the startup
+//! warning addresses, not something the `Host` check is meant to cover.
 
 use std::net::IpAddr;
 
@@ -26,46 +38,84 @@ use hyper::{
     Body, Request, Response, StatusCode,
 };
 
-use crate::web::{interface::ErrorResponse, util::msgpack};
+use crate::web::util::response;
 
-/// The set of `Host`/`Origin` values accepted while the server is bound to a
+/// The set of `Host`/`Origin` values accepted while enforcement is active.
-/// loopback address.
 #[derive(Debug, Clone)]
 pub struct AllowedHosts {
     port: u16,
+    /// The bind IP accepted in addition to `localhost`/loopback, set only for a
+    /// private (LAN) bind. `None` for a loopback bind, which accepts localhost
+    /// and loopback literals only.
+    bind_ip: Option<IpAddr>,
 }
 
 impl AllowedHosts {
-    /// Hostnames considered local, compared case-insensitively against the host
+    /// Returns whether the given host and optional port are allowed. The host is
-    /// portion of the `Host`/`Origin` header (IPv6 brackets stripped first).
+    /// accepted if it is `localhost`, a loopback IP literal, or (on a private
-    const LOCAL_HOSTS: [&'static str; 3] = ["localhost", "127.0.0.1", "::1"];
+    /// bind) the exact bind IP. A request with no explicit port is accepted (the
-
+    /// host already has to be one we recognize).
-    /// Returns whether the given host and optional port are allowed. A request
-    /// with no explicit port is accepted (the host already has to be local).
     fn allows(&self, host: &str, port: Option<u16>) -> bool {
         let host = normalize_host(host);
-        let host_is_local = Self::LOCAL_HOSTS
+        let host_ok = host.eq_ignore_ascii_case("localhost")
-            .iter()
+            || host
-            .any(|local| host.eq_ignore_ascii_case(local));
+                .parse::<IpAddr>()
+                .ok()
+                .map(canonical)
+                .is_some_and(|ip| ip.is_loopback() || self.bind_ip == Some(ip));
 
-        host_is_local && port.is_none_or(|port| port == self.port)
+        host_ok && port.is_none_or(|port| port == self.port)
     }
 }
 
 /// Builds the allowlist for a given bind address. Returns `None` (validation
-/// disabled) when the server is bound to a non-loopback address.
+/// disabled) when bound to an unspecified (`0.0.0.0`/`::`) or public address,
+/// where arbitrary hostnames may legitimately resolve to the server.
 pub fn allowed_hosts(bind: IpAddr, port: u16) -> Option<AllowedHosts> {
     if bind.is_loopback() {
-        Some(AllowedHosts { port })
+        Some(AllowedHosts {
+            port,
+            bind_ip: None,
+        })
+    } else if is_private_bind(bind) {
+        Some(AllowedHosts {
+            port,
+            bind_ip: Some(canonical(bind)),
+        })
     } else {
         None
     }
 }
 
+/// Collapses an IPv4-mapped IPv6 address (`::ffff:192.168.0.1`) to its IPv4 form
+/// so it classifies and compares consistently with the bare IPv4 address.
+fn canonical(ip: IpAddr) -> IpAddr {
+    match ip {
+        IpAddr::V6(v6) => v6.to_ipv4_mapped().map(IpAddr::V4).unwrap_or(ip),
+        v4 => v4,
+    }
+}
+
+/// Returns whether a bind address is a specific private/link-local address, the
+/// case where enforcement stays on with the bind IP added to the allowlist.
+/// Unspecified, loopback, and public addresses are excluded (loopback is handled
+/// separately by [`allowed_hosts`]).
+fn is_private_bind(ip: IpAddr) -> bool {
+    match canonical(ip) {
+        IpAddr::V4(v4) => v4.is_private() || v4.is_link_local(),
+        IpAddr::V6(v6) => {
+            let first = v6.segments()[0];
+            // Unique local (fc00::/7) or link-local (fe80::/10). The std methods
+            // for these are still nightly-only, so test the prefix directly.
+            (first & 0xfe00) == 0xfc00 || (first & 0xffc0) == 0xfe80
+        }
+    }
+}
+
 /// Validates the `Host` and `Origin` headers of an incoming request against the
-/// allowlist. Returns `Some` with a ready-to-send `403` response when the
+/// allowlist. Returns `Some` with a ready-to-send `404` response when the
 /// request should be rejected, or `None` when it is allowed to proceed. When
-/// `allowed` is `None` (non-loopback bind) every request is accepted.
+/// `allowed` is `None` (unspecified or public bind) every request is accepted.
 pub fn check_request_origin(
     request: &Request<Body>,
     allowed: Option<&AllowedHosts>,
@@ -103,12 +153,16 @@ pub fn check_request_origin(
     None
 }
 
-/// Strips the surrounding brackets from an IPv6 host literal (e.g. `[::1]`), so
+/// Normalizes a host literal for parsing/comparison: strips the surrounding
-/// it can be compared against the unbracketed names in `LOCAL_HOSTS`.
+/// brackets from an IPv6 literal (e.g. `[::1]`) and drops any IPv6 zone id (e.g.
+/// `fe80::1%eth0`), which `Ipv6Addr::from_str` would otherwise reject.
 fn normalize_host(host: &str) -> &str {
-    host.strip_prefix('[')
+    let host = host
+        .strip_prefix('[')
         .and_then(|host| host.strip_suffix(']'))
-        .unwrap_or(host)
+        .unwrap_or(host);
+
+    host.split('%').next().unwrap_or(host)
 }
 
 /// Parses a `Host` header value (an authority such as `localhost:34872`) into
@@ -142,14 +196,12 @@ fn reject_userinfo(authority: &Authority) -> Option<()> {
     }
 }
 
+/// Builds the response sent when a request fails Host/Origin validation. It is a
+/// generic `404` with no Rojo-identifying body: a rejected request may be a
+/// prober (or a DNS-rebound page's same-origin script, which could read the
+/// body), so we reveal nothing rather than confirming a Rojo server is here.
 fn reject() -> Response<Body> {
-    msgpack(
+    response(StatusCode::NOT_FOUND, "text/plain", "Not Found")
-        ErrorResponse::forbidden(
-            "Request rejected because its Host or Origin header is not an allowed local \
-             address. This protects the Rojo server from DNS rebinding attacks.",
-        ),
-        StatusCode::FORBIDDEN,
-    )
 }
 
 #[cfg(test)]
@@ -164,6 +216,11 @@ mod test {
         allowed_hosts(IpAddr::V4(Ipv4Addr::LOCALHOST), PORT)
     }
 
+    /// An allowlist for a server bound to a specific private (LAN) address.
+    fn private_allowlist() -> Option<AllowedHosts> {
+        allowed_hosts(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), PORT)
+    }
+
     fn request_with(headers: &[(&'static str, &str)]) -> Request<Body> {
         let mut builder = Request::builder().uri("/api/rojo");
         for (name, value) in headers {
@@ -179,9 +236,35 @@ mod test {
     }
 
     #[test]
-    fn non_loopback_bind_disables_enforcement() {
+    fn private_bind_enables_enforcement() {
-        assert!(allowed_hosts(IpAddr::V4(Ipv4Addr::UNSPECIFIED), PORT).is_none());
+        for bind in [
-        assert!(allowed_hosts(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), PORT).is_none());
+            IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), // 192.168.0.0/16
+            IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1)),    // 10.0.0.0/8
+            IpAddr::V4(Ipv4Addr::new(172, 16, 0, 1)),  // 172.16.0.0/12
+            IpAddr::V4(Ipv4Addr::new(169, 254, 0, 1)), // link-local
+            IpAddr::V6(Ipv6Addr::new(0xfc00, 0, 0, 0, 0, 0, 0, 1)), // unique local
+            IpAddr::V6(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 1)), // link-local
+        ] {
+            assert!(
+                allowed_hosts(bind, PORT).is_some(),
+                "private bind {bind} should enable enforcement"
+            );
+        }
+    }
+
+    #[test]
+    fn unspecified_or_public_bind_disables_enforcement() {
+        for bind in [
+            IpAddr::V4(Ipv4Addr::UNSPECIFIED),     // 0.0.0.0
+            IpAddr::V6(Ipv6Addr::UNSPECIFIED),     // ::
+            IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)), // public
+            IpAddr::V6(Ipv6Addr::new(0x2001, 0x4860, 0, 0, 0, 0, 0, 0x8888)), // public
+        ] {
+            assert!(
+                allowed_hosts(bind, PORT).is_none(),
+                "bind {bind} should disable enforcement"
+            );
+        }
     }
 
     #[test]
@@ -269,9 +352,93 @@ mod test {
     }
 
     #[test]
-    fn disabled_allowlist_accepts_foreign_host() {
+    fn private_bind_accepts_local_and_bind_ip_hosts() {
-        let allowed = allowed_hosts(IpAddr::V4(Ipv4Addr::UNSPECIFIED), PORT);
+        let allowed = private_allowlist();
-        let request = request_with(&[("host", "evil.com")]);
+        for host in [
+            format!("192.168.1.5:{PORT}"),
+            format!("localhost:{PORT}"),
+            format!("127.0.0.1:{PORT}"),
+            format!("[::1]:{PORT}"),
+            "192.168.1.5".to_owned(),
+        ] {
+            let request = request_with(&[("host", &host)]);
+            assert!(
+                check_request_origin(&request, allowed.as_ref()).is_none(),
+                "host {host} should be allowed on a private bind"
+            );
+        }
+    }
+
+    #[test]
+    fn private_bind_rejects_other_hosts() {
+        let allowed = private_allowlist();
+        for host in [
+            "evil.com",    // a rebound attacker domain
+            "192.168.1.6", // a different private IP
+            "8.8.8.8",     // a public IP
+        ] {
+            let request = request_with(&[("host", host)]);
+            assert!(
+                check_request_origin(&request, allowed.as_ref()).is_some(),
+                "host {host} should be rejected on a private bind"
+            );
+        }
+    }
+
+    #[test]
+    fn private_bind_keeps_origin_strict() {
+        // A different private IP as Origin must be rejected even though the Host
+        // is the valid bind IP: the Origin check is not widened to arbitrary
+        // private addresses.
+        let allowed = private_allowlist();
+        let request = request_with(&[
+            ("host", &format!("192.168.1.5:{PORT}")),
+            ("origin", &format!("http://192.168.1.6:{PORT}")),
+        ]);
+        assert!(check_request_origin(&request, allowed.as_ref()).is_some());
+    }
+
+    #[test]
+    fn private_bind_accepts_bind_ip_origin() {
+        let allowed = private_allowlist();
+        let request = request_with(&[
+            ("host", &format!("192.168.1.5:{PORT}")),
+            ("origin", &format!("http://192.168.1.5:{PORT}")),
+        ]);
         assert!(check_request_origin(&request, allowed.as_ref()).is_none());
     }
+
+    #[test]
+    fn accepts_ipv4_mapped_bind_ip() {
+        // `::ffff:192.168.1.5` is the IPv4-mapped form of the bind IP and must
+        // be treated as equal to it.
+        let allowed = private_allowlist();
+        let request = request_with(&[("host", &format!("[::ffff:192.168.1.5]:{PORT}"))]);
+        assert!(check_request_origin(&request, allowed.as_ref()).is_none());
+    }
+
+    #[test]
+    fn normalize_host_strips_brackets_and_zone_id() {
+        // Brackets and an IPv6 zone id must be removed so the result parses as an
+        // `IpAddr` (`Ipv6Addr::from_str` rejects zone ids).
+        assert_eq!(normalize_host("[::1]"), "::1");
+        assert_eq!(normalize_host("[fe80::1%eth0]"), "fe80::1");
+        assert_eq!(normalize_host("localhost"), "localhost");
+        assert!(normalize_host("[fe80::1%eth0]").parse::<IpAddr>().is_ok());
+    }
+
+    #[test]
+    fn disabled_allowlist_accepts_foreign_host() {
+        for bind in [
+            IpAddr::V4(Ipv4Addr::UNSPECIFIED),
+            IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)),
+        ] {
+            let allowed = allowed_hosts(bind, PORT);
+            let request = request_with(&[("host", "evil.com")]);
+            assert!(
+                check_request_origin(&request, allowed.as_ref()).is_none(),
+                "disabled allowlist for bind {bind} should accept any host"
+            );
+        }
+    }
 }

tests/rojo_test/serve_util.rs

@@ -247,9 +247,13 @@ impl TestServeSession {
     }
 
     /// Sends a GET to `/api/rojo` with the given extra request headers and
-    /// returns the response status code. Used to exercise the Host/Origin
+    /// returns the full response. Used to exercise the Host/Origin allowlist that
-    /// allowlist that guards against DNS rebinding.
+    /// guards against DNS rebinding, including asserting that a rejection reveals
-    pub fn api_rojo_status_with_headers(&self, headers: &[(&str, &str)]) -> reqwest::StatusCode {
+    /// nothing about the server.
+    pub fn api_rojo_response_with_headers(
+        &self,
+        headers: &[(&str, &str)],
+    ) -> reqwest::blocking::Response {
         let client = reqwest::blocking::Client::new();
         let url = format!("http://localhost:{}/api/rojo", self.port);
 
@@ -258,7 +262,7 @@ impl TestServeSession {
             request = request.header(*name, *value);
         }
 
-        request.send().expect("Failed to send request").status()
+        request.send().expect("Failed to send request")
     }
 
     /// Sends a POST to `/api/open/<id>` and returns the response status code.

tests/tests/serve.rs

@@ -18,29 +18,52 @@ fn rejects_dns_rebinding_requests() {
 
         // A request carrying a local Host header is served normally.
         assert_eq!(
-            session.api_rojo_status_with_headers(&[("host", &local_host)]),
+            session
+                .api_rojo_response_with_headers(&[("host", &local_host)])
+                .status(),
             reqwest::StatusCode::OK,
         );
 
         // A request whose Host is a foreign hostname, as a DNS-rebound page
-        // would send, is rejected.
+        // would send, is rejected with a generic 404 that reveals nothing about
-        assert_eq!(
+        // the server.
-            session.api_rojo_status_with_headers(&[("host", "evil.com")]),
+        assert_rejected(session.api_rojo_response_with_headers(&[("host", "evil.com")]));
-            reqwest::StatusCode::FORBIDDEN,
-        );
 
         // Even with a local Host, a present-but-foreign Origin is rejected.
         let foreign_origin = format!("http://evil.com:{port}");
-        assert_eq!(
+        assert_rejected(
-            session.api_rojo_status_with_headers(&[
+            session.api_rojo_response_with_headers(&[
                 ("host", &local_host),
                 ("origin", &foreign_origin),
             ]),
-            reqwest::StatusCode::FORBIDDEN,
         );
     });
 }
 
+/// Asserts that a Host/Origin rejection is a generic 404 whose body and
+/// content-type do not identify the server as Rojo.
+fn assert_rejected(response: reqwest::blocking::Response) {
+    assert_eq!(response.status(), reqwest::StatusCode::NOT_FOUND);
+
+    let content_type = response
+        .headers()
+        .get(reqwest::header::CONTENT_TYPE)
+        .and_then(|value| value.to_str().ok())
+        .unwrap_or_default()
+        .to_owned();
+    assert!(
+        !content_type.contains("msgpack"),
+        "rejection should not use the msgpack API content-type, got {content_type:?}",
+    );
+
+    let body = response.text().expect("Failed to read response body");
+    let body_lower = body.to_lowercase();
+    assert!(
+        !body_lower.contains("rojo") && !body_lower.contains("rebinding"),
+        "rejection body should not identify the server, got {body:?}",
+    );
+}
+
 #[test]
 fn allows_api_open_from_loopback_peer() {
     run_serve_test("empty", |session, _redactions| {