diff --git a/CHANGELOG.md b/CHANGELOG.md index 83c8d85a..f99bc015 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/web/api.rs b/src/web/api.rs index 532be770..c8eea7ea 100644 --- a/src/web/api.rs +++ b/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 - // signal available: the legitimate caller is a sandboxed Roblox plugin - // whose only credential is being able to reach the port, so there is no - // secret to authenticate it with. A connection forwarded over loopback by - // an SSH/Tailscale tunnel or a local reverse proxy therefore appears local - // and is allowed. We treat that as delegated trust rather than a bypass: - // by deliberately standing up that tunnel/proxy the user has decided the - // remote end is trusted, and reachability is bounded by *that* hop's own - // authentication (SSH keys, Tailscale ACLs, ...). This gate's job is to - // stop direct, unauthenticated peers, which it does. + // `remote_addr` is the immediate peer, which is the best locality signal + // we have: the legitimate caller is a sandboxed Roblox plugin whose only + // credential is being able to reach the port, so there is no secret to + // authenticate it with. A connection forwarded over loopback by an + // SSH/Tailscale tunnel or a local reverse proxy therefore appears local + // and is allowed. That is delegated trust rather than a bypass: by + // standing up that tunnel or proxy the user has decided the remote end is + // trusted, and reachability is bounded by that hop's own authentication + // (e.g. SSH keys or Tailscale ACLs). This gate only stops direct, + // unauthenticated peers. if !self.remote_addr.ip().is_loopback() { return msgpack( ErrorResponse::forbidden("/api/open is only available to local clients"), diff --git a/src/web/origin.rs b/src/web/origin.rs index 1c6ffad5..d224b134 100644 --- a/src/web/origin.rs +++ b/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, -//! because the rebound request still carries the attacker's hostname. +//! (and, if present, `Origin`) header refers to an address we recognize blocks +//! 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 -//! 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 -//! startup instead). One consequence worth being explicit about: an exposed bind -//! therefore has *no* rebinding protection, even for a browser on the same -//! machine, because the `Host` check that would catch it is turned off. That is -//! an accepted trade-off: exposing the unauthenticated API to the network is -//! already the dominant risk, which the startup warning addresses. +//! Enforcement covers two kinds of bind: +//! +//! * Loopback (the default): only `localhost` and loopback literals are +//! accepted. +//! * A specific private/LAN address: `localhost`, loopback literals, and that exact bind +//! IP are accepted. Because the defense works by rejecting any `Host` that +//! isn't a recognized IP literal, clients must connect to a private bind by +//! 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 -/// loopback address. +/// The set of `Host`/`Origin` values accepted while enforcement is active. #[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, } impl AllowedHosts { - /// Hostnames considered local, compared case-insensitively against the host - /// portion of the `Host`/`Origin` header (IPv6 brackets stripped first). - const LOCAL_HOSTS: [&'static str; 3] = ["localhost", "127.0.0.1", "::1"]; - - /// 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). + /// Returns whether the given host and optional port are allowed. The host is + /// accepted if it is `localhost`, a loopback IP literal, or (on a private + /// bind) the exact bind IP. A request with no explicit port is accepted (the + /// host already has to be one we recognize). fn allows(&self, host: &str, port: Option) -> bool { let host = normalize_host(host); - let host_is_local = Self::LOCAL_HOSTS - .iter() - .any(|local| host.eq_ignore_ascii_case(local)); + let host_ok = host.eq_ignore_ascii_case("localhost") + || host + .parse::() + .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 { 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, 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 -/// it can be compared against the unbracketed names in `LOCAL_HOSTS`. +/// Normalizes a host literal for parsing/comparison: strips the surrounding +/// 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 { - msgpack( - 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, - ) + response(StatusCode::NOT_FOUND, "text/plain", "Not Found") } #[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 { + allowed_hosts(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), PORT) + } + fn request_with(headers: &[(&'static str, &str)]) -> Request { 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() { - assert!(allowed_hosts(IpAddr::V4(Ipv4Addr::UNSPECIFIED), PORT).is_none()); - assert!(allowed_hosts(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 5)), PORT).is_none()); + fn private_bind_enables_enforcement() { + for bind in [ + 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() { - let allowed = allowed_hosts(IpAddr::V4(Ipv4Addr::UNSPECIFIED), PORT); - let request = request_with(&[("host", "evil.com")]); + fn private_bind_accepts_local_and_bind_ip_hosts() { + let allowed = private_allowlist(); + 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::().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" + ); + } + } } diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index b7853abe..4a817a8a 100644 --- a/tests/rojo_test/serve_util.rs +++ b/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 - /// allowlist that guards against DNS rebinding. - pub fn api_rojo_status_with_headers(&self, headers: &[(&str, &str)]) -> reqwest::StatusCode { + /// returns the full response. Used to exercise the Host/Origin allowlist that + /// guards against DNS rebinding, including asserting that a rejection reveals + /// 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/` and returns the response status code. diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index ef9ec464..490c13db 100644 --- a/tests/tests/serve.rs +++ b/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. - assert_eq!( - session.api_rojo_status_with_headers(&[("host", "evil.com")]), - reqwest::StatusCode::FORBIDDEN, - ); + // would send, is rejected with a generic 404 that reveals nothing about + // the server. + assert_rejected(session.api_rojo_response_with_headers(&[("host", "evil.com")])); // Even with a local Host, a present-but-foreign Origin is rejected. let foreign_origin = format!("http://evil.com:{port}"); - assert_eq!( - session.api_rojo_status_with_headers(&[ + assert_rejected( + 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| {