diff --git a/crates/rustmotion/src/include.rs b/crates/rustmotion/src/include.rs index 32507cf8..dacb953d 100644 --- a/crates/rustmotion/src/include.rs +++ b/crates/rustmotion/src/include.rs @@ -1,3 +1,4 @@ +use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; use std::path::{Path, PathBuf}; use crate::error::Result; @@ -11,6 +12,30 @@ use crate::schema::{ const MAX_INCLUDE_DEPTH: u8 = 8; +/// Response-size cap for a remote `include` fetch. Scenario JSON is +/// not expected to be large; this is deliberately far below ureq's own 10 MB +/// default for `read_to_vec`/`read_to_string`. +const MAX_REMOTE_INCLUDE_BYTES: u64 = 4 * 1024 * 1024; + +/// The CLI flag whose semantics this module implements the mechanism for: +/// remote `include` is denied unless a caller explicitly opts in by passing +/// `RemoteIncludePolicy::Allow` to [`resolve_includes_with_policy`]. Naming +/// it here keeps the error message and the flag rustmotion's CLI is expected +/// to expose in sync. +const ALLOW_REMOTE_INCLUDE_FLAG: &str = "--allow-remote-include"; + +/// Whether a [`resolve_includes_with_policy`] pass may perform outbound +/// network requests for `include: "https://..."` directives ( remote +/// fetching is deliberate design, but it previously had no allowlist and no +/// opt-out — any scenario, including one merely being `validate`d, could +/// make this process issue arbitrary GETs). Defaults to [`Self::Deny`]. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum RemoteIncludePolicy { + #[default] + Deny, + Allow, +} + /// Where the parent scenario was loaded from — determines how relative paths are resolved. pub enum IncludeSource { /// Loaded from a file; relative paths resolve against this file's directory. @@ -20,7 +45,23 @@ pub enum IncludeSource { } /// Expand all include directives in a scenario, producing resolved views. +/// +/// Remote (`http(s)://`) includes are denied by default — see +/// [`resolve_includes_with_policy`] to opt in. This is exactly +/// `resolve_includes_with_policy(scenario, source, RemoteIncludePolicy::Deny)`, +/// kept as its own entry point so every existing caller stays secure by +/// default without having to be rewritten to pass a policy. pub fn resolve_includes(scenario: Scenario, source: &IncludeSource) -> Result { + resolve_includes_with_policy(scenario, source, RemoteIncludePolicy::Deny) +} + +/// Same as [`resolve_includes`], with explicit control over whether remote +/// `include` directives may reach the network. +pub fn resolve_includes_with_policy( + scenario: Scenario, + source: &IncludeSource, + remote_policy: RemoteIncludePolicy, +) -> Result { let mut audio = scenario.audio; let mut included_paths = Vec::new(); let has_scenes = !scenario.scenes.is_empty(); @@ -36,8 +77,14 @@ pub fn resolve_includes(scenario: Scenario, source: &IncludeSource) -> Result Result, included_paths: &mut Vec, + remote_policy: RemoteIncludePolicy, ) -> Result> { let mut result = Vec::new(); @@ -103,8 +157,14 @@ fn resolve_entries( path: directive.include.clone(), }); } - let scenes = - fetch_and_resolve(&directive, source, depth + 1, audio, included_paths)?; + let scenes = fetch_and_resolve( + &directive, + source, + depth + 1, + audio, + included_paths, + remote_policy, + )?; result.extend(scenes); } } @@ -119,12 +179,22 @@ fn fetch_and_resolve( depth: u8, audio: &mut Vec, included_paths: &mut Vec, + remote_policy: RemoteIncludePolicy, ) -> Result> { let is_remote = directive.include.starts_with("http://") || directive.include.starts_with("https://"); let (json_str, child_source) = if is_remote { - let body = fetch_remote(&directive.include)?; + if remote_policy != RemoteIncludePolicy::Allow { + return Err(RustmotionError::Generic(format!( + "include '{}' is a remote URL, but remote includes are disabled by default \ + — pass {ALLOW_REMOTE_INCLUDE_FLAG} to opt in", + directive.include + ))); + } + let verified = verify_remote_url(&directive.include)?; + eprintln!("include: fetching remote scenario {}", verified.as_str()); + let body = fetch_remote(&verified)?; let child_source = IncludeSource::File(PathBuf::from(&directive.include)); (body, child_source) } else { @@ -178,6 +248,7 @@ fn fetch_and_resolve( depth, audio, included_paths, + remote_policy, )?; // Apply scene index filter if specified @@ -217,22 +288,130 @@ fn resolve_local_path(relative: &str, source: &IncludeSource) -> Result } } -fn fetch_remote(url: &str) -> Result { - let response = ureq::get(url) +/// A remote `include` URL that has already passed the SSRF policy check in +/// [`verify_remote_url`] — every address its host resolves to was confirmed +/// public. This is the only way to reach [`fetch_remote`]: an unchecked +/// `&str` cannot be passed to it, by construction. +struct VerifiedRemoteUrl(String); + +impl VerifiedRemoteUrl { + fn as_str(&self) -> &str { + &self.0 + } +} + +/// Resolves `url`'s host and rejects it if any resolved address is not +/// publicly routable: loopback, link-local (169.254.169.254, the cloud +/// metadata endpoint, included), or an RFC1918/ULA private range. +/// +/// This check runs once, ahead of the request `fetch_remote` makes moments +/// later; a DNS answer that changes between this resolution and that +/// connection ("DNS rebinding") is not defended against — the audit's +/// remediation asks for a resolve-time check, and closing the rebinding gap +/// fully would mean replacing `ureq`'s connector rather than configuring it. +fn verify_remote_url(url: &str) -> Result { + let (host, port) = split_host_port(url)?; + let addrs: Vec = (host.as_str(), port) + .to_socket_addrs() + .map_err(|e| { + RustmotionError::Generic(format!("include '{url}' could not be resolved: {e}")) + })? + .collect(); + if addrs.is_empty() { + return Err(RustmotionError::Generic(format!( + "include '{url}' resolved to no addresses" + ))); + } + for addr in &addrs { + let ip = addr.ip().to_canonical(); + if is_non_public(ip) { + return Err(RustmotionError::Generic(format!( + "include '{url}' resolves to {ip}, which is not a public address \ + (loopback/link-local/private range) — refusing to prevent SSRF" + ))); + } + } + Ok(VerifiedRemoteUrl(url.to_string())) +} + +fn is_non_public(ip: IpAddr) -> bool { + match ip { + IpAddr::V4(v4) => { + v4.is_loopback() + || v4.is_private() + || v4.is_link_local() + || v4.is_unspecified() + || v4.is_broadcast() + || v4.is_documentation() + || v4.is_multicast() + } + IpAddr::V6(v6) => { + v6.is_loopback() + || v6.is_unspecified() + || v6.is_unique_local() + || v6.is_unicast_link_local() + || v6.is_multicast() + } + } +} + +/// Splits `scheme://[user:pass@]host[:port][/...]` into `(host, port)`, +/// defaulting the port from the scheme. Deliberately minimal — this crate +/// takes no `url` dependency for this, and only needs to know where to point +/// the resolver; `ureq` itself is still the one that rejects a malformed URL +/// when the actual request is made. +fn split_host_port(url: &str) -> Result<(String, u16)> { + let (rest, default_port) = if let Some(rest) = url.strip_prefix("https://") { + (rest, 443) + } else if let Some(rest) = url.strip_prefix("http://") { + (rest, 80) + } else { + return Err(RustmotionError::Generic(format!( + "include '{url}' is not an http(s) URL" + ))); + }; + let authority = rest.split(['/', '?', '#']).next().unwrap_or(rest); + let authority = authority.rsplit('@').next().unwrap_or(authority); + + if let Some(rest) = authority.strip_prefix('[') { + let end = rest.find(']').ok_or_else(|| { + RustmotionError::Generic(format!("include '{url}' has an unterminated IPv6 host")) + })?; + let host = rest[..end].to_string(); + let port = rest[end + 1..] + .strip_prefix(':') + .and_then(|p| p.parse().ok()) + .unwrap_or(default_port); + return Ok((host, port)); + } + + match authority.rsplit_once(':') { + Some((host, port_str)) => match port_str.parse() { + Ok(port) => Ok((host.to_string(), port)), + Err(_) => Ok((authority.to_string(), default_port)), + }, + None => Ok((authority.to_string(), default_port)), + } +} + +fn fetch_remote(url: &VerifiedRemoteUrl) -> Result { + let response = rustmotion_core::engine::renderer::http_agent() + .get(url.as_str()) .call() .map_err(|e| RustmotionError::IncludeRemoteFetch { - url: url.to_string(), + url: url.as_str().to_string(), reason: e.to_string(), })?; - let body = - response - .into_body() - .read_to_string() - .map_err(|e| RustmotionError::IncludeRemoteFetch { - url: url.to_string(), - reason: e.to_string(), - })?; - Ok(body) + response + .into_body() + .with_config() + .limit(MAX_REMOTE_INCLUDE_BYTES) + .lossy_utf8(true) + .read_to_string() + .map_err(|e| RustmotionError::IncludeRemoteFetch { + url: url.as_str().to_string(), + reason: e.to_string(), + }) } // --- Background template resolution --- diff --git a/crates/rustmotion/tests/audit_ws_h.rs b/crates/rustmotion/tests/audit_ws_h.rs new file mode 100644 index 00000000..1a83b147 --- /dev/null +++ b/crates/rustmotion/tests/audit_ws_h.rs @@ -0,0 +1,90 @@ +//! Regression tests for the workstream H (untrusted scenario ingestion) +//! audit finding that lives in the `rustmotion` crate:. +//! +//! Remote fetching for `include: "https://..."` is deliberate design; the +//! finding is the absence of any control around it. These tests exercise +//! `include::resolve_includes`/`resolve_includes_with_policy` directly +//! rather than through `rustmotion::loader`, so they do not depend on the +//! CLI wiring a sibling workstream owns. + +use rustmotion::include::{ + resolve_includes, resolve_includes_with_policy, IncludeSource, RemoteIncludePolicy, +}; +use rustmotion::schema::Scenario; + +fn scenario_with_remote_include(url: &str) -> Scenario { + serde_json::from_value(serde_json::json!({ + "video": { "width": 100, "height": 100 }, + "scenes": [{ "include": url }] + })) + .expect("scenario with a remote include parses") +} + +// ----, part 1: remote includes are denied by default ---- + +#[test] +fn remote_include_is_denied_by_default() { + let scenario = scenario_with_remote_include("https://example.invalid/scenario.json"); + let err = resolve_includes(scenario, &IncludeSource::Inline) + .expect_err("a remote include must be refused unless explicitly opted into"); + let msg = err.to_string(); + assert!( + msg.contains("disabled by default"), + "the denial must explain that remote includes are opt-in: {msg}" + ); +} + +#[test] +fn remote_include_denial_names_the_opt_in() { + let scenario = scenario_with_remote_include("https://example.invalid/scenario.json"); + let err = + resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Deny) + .expect_err("Deny must refuse the same way the default does"); + assert!( + err.to_string().contains("--allow-remote-include"), + "the error should name the flag that opts in, for a CLI to surface: {err}" + ); +} + +// ----, part 2: once opted in, resolved addresses are still checked +// against loopback/link-local/private ranges before any request is made ---- + +#[test] +fn opted_in_remote_include_still_refuses_the_cloud_metadata_endpoint() { + // A literal IP in the URL resolves without any DNS/network I/O (`(&str, + // u16): ToSocketAddrs` parses a numeric host directly), so this is a + // deterministic, offline test of the SSRF check itself. + let scenario = scenario_with_remote_include( + "http://169.254.169.254/latest/meta-data/iam/security-credentials/", + ); + let err = + resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Allow) + .expect_err( + "the cloud metadata address must be refused even when remote includes are allowed", + ); + let msg = err.to_string(); + assert!( + msg.contains("169.254.169.254"), + "the error should name the disallowed address: {msg}" + ); +} + +#[test] +fn opted_in_remote_include_still_refuses_loopback() { + let scenario = scenario_with_remote_include("http://127.0.0.1:8500/scenario.json"); + let err = + resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Allow) + .expect_err("loopback must be refused even when remote includes are allowed"); + assert!(err.to_string().contains("127.0.0.1"), "{err}"); +} + +#[test] +fn opted_in_remote_include_still_refuses_rfc1918_private_ranges() { + let scenario = scenario_with_remote_include("http://10.0.0.5/scenario.json"); + let err = + resolve_includes_with_policy(scenario, &IncludeSource::Inline, RemoteIncludePolicy::Allow) + .expect_err( + "an RFC1918 private address must be refused even when remote includes are allowed", + ); + assert!(err.to_string().contains("10.0.0.5"), "{err}"); +}