diff --git a/CHANGELOG.md b/CHANGELOG.md index b28c14a8..db1e274e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ Pre-1.0 note: while `pg_durable` is in major version `0`, minor releases may inc - **Restricted HTTP transport (#342):** restricted allow-list builds now require HTTPS so credentials and request bodies cannot be sent over plaintext HTTP; development-only `http-allow-all` builds continue to permit HTTP. +### Security + +- **HTTP allow-list URL parsing:** request URLs are now parsed once and the same canonical URL is used for validation and transport, preventing parser differences from approving a different host than the request targets. + ## [0.2.6] - 2026-08-23 ### Added diff --git a/Cargo.lock b/Cargo.lock index 90d65583..4d65b0a7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1926,6 +1926,7 @@ dependencies = [ "sqlx", "tokio", "tracing-subscriber", + "url", "uuid", ] diff --git a/Cargo.toml b/Cargo.toml index afbb5048..25b48861 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,6 +60,11 @@ chrono = "0.4" # stays on native-tls with no rustls/ring/aws-lc in the tree. reqwest = { version = "0.13", default-features = false, features = ["json", "native-tls", "multipart"] } +# Canonical URL parsing for SSRF validation. Semver-compatible with reqwest's own +# url dependency, so Cargo unifies them and `url::Url` is `reqwest::Url` — the +# validated URL and the sent URL are the same value. +url = "2.5" + # For df.http_multipart() — base64-decodes part payloads carried in the config JSON. base64 = "0.22" diff --git a/docs/http-security.md b/docs/http-security.md index d57c7252..88512ceb 100644 --- a/docs/http-security.md +++ b/docs/http-security.md @@ -49,7 +49,7 @@ df.http() is disabled. Rebuild with the 'http-allow-azure-domains' Cargo feature Because `df.nodes` rows can be inserted by hand (bypassing the DSL), the same block is enforced again at execution time inside `execute_http.rs` via -`validate_url_allowlist`. +`validate_allowlist`. --- @@ -225,6 +225,11 @@ The SSRF-safe DNS resolver (`SsrfSafeResolver`) wraps the system resolver and filters blocked IPs **inline** — the same IP that passes the check is the one used for the TCP connection. There is no window for a rebinding attack. +Restricted builds disable reqwest's system and environment proxy discovery. +An HTTP proxy resolves the destination itself, outside `SsrfSafeResolver`, so +inheriting `HTTP_PROXY`, `HTTPS_PROXY`, or platform proxy settings would bypass +the destination-IP check. + Only the single IP address that `reqwest` actually connects to is checked. If DNS returns multiple A/AAAA records, the others are not checked because they are never used. This is intentional, not a gap — checking unused addresses @@ -234,7 +239,7 @@ would create false positives without any security benefit. Bare IP literals in URLs (e.g. `http://169.254.169.254/...`) bypass DNS entirely — `reqwest` connects directly without calling the resolver. -`validate_url_allowlist` blocks all bare IPs unconditionally, so these never +`validate_allowlist` blocks all bare IPs unconditionally, so these never reach the resolver. --- @@ -285,11 +290,28 @@ Matched exactly — subdomains and lookalikes are rejected. ### 5.4 Bare IP rejection -All bare IPv4 and IPv6 addresses are rejected by `validate_url_allowlist` +All bare IPv4 and IPv6 addresses are rejected by `validate_allowlist` regardless of feature flag — even under `http-allow-azure-domains`. Because the allowlist blocks all bare IPs, there is no separate IP-literal check; the allowlist is the definitive gate for IP-literal URLs. +### 5.4 One parse, one URL + +The host judged by the allow-list is read from the `url::Url` that is then +handed to `reqwest`, never from the caller's string. Comparing a separately +parsed host against the allow-list would let the two parsers disagree: WHATWG +ends the authority of an `http(s)` URL at `\`, so in +`https://evil.example\@acct.blob.core.windows.net/` the host is +`evil.example` and the allow-listed name is merely path. A scan that read the +name after the last `@` would approve a request aimed elsewhere — and with an +IP literal in that position it would also clear the bare-IP rule, the only +gate for targets that skip DNS. + +Judging the parsed host means the allow-list follows canonicalisation as +`reqwest` performs it: percent-encoded host characters are decoded, +non-dotted IPv4 notation (`http://2130706433/`) becomes an IP literal, and +internationalised names are compared in their Punycode form. + --- ## 6. Additional Hardening diff --git a/scripts/test-e2e-docker.sh b/scripts/test-e2e-docker.sh index ad011120..6bffdbf8 100755 --- a/scripts/test-e2e-docker.sh +++ b/scripts/test-e2e-docker.sh @@ -34,6 +34,7 @@ SKIP_TESTS=( "45_connection_limit_timeout" "46_connection_limit_startup_validation" "66_new_transaction_launch_limit" + "67_host_guc" "47_http_dsl_disabled" "48_http_allow_all" # Needs the "reconcile" phase GUCs (reconcile_interval=2, retention_days=0) diff --git a/src/activities/execute_http.rs b/src/activities/execute_http.rs index a98feb8c..e7441310 100644 --- a/src/activities/execute_http.rs +++ b/src/activities/execute_http.rs @@ -58,6 +58,10 @@ async fn check_http_privilege(pool: &PgPool, submitted_by: &str) -> Result<(), S /// could host a 302 redirecting to `http://169.254.169.254/...`, and reqwest /// would follow it without calling our DNS resolver (since the target is an IP /// literal). +/// +/// Restricted builds also disable environment/system proxies. A proxy resolves +/// the destination itself, which would bypass `SsrfSafeResolver`'s check of the +/// address reqwest ultimately reaches. pub(crate) fn build_client(timeout: Duration) -> Result { let builder = reqwest::Client::builder() .timeout(timeout) @@ -70,7 +74,7 @@ pub(crate) fn build_client(timeout: Duration) -> Result use crate::ssrf::{SsrfSafeResolver, SystemResolver}; use std::sync::Arc; let resolver = SsrfSafeResolver::wrapping(Arc::new(SystemResolver)); - builder.dns_resolver(Arc::new(resolver)) + builder.no_proxy().dns_resolver(Arc::new(resolver)) }; builder @@ -108,6 +112,9 @@ pub async fn execute( // 3. DNS resolver (SsrfSafeResolver): catches DNS rebinding — a hostname // that passes the allowlist but resolves to a private IP at // connect time. + // + // Steps 1 and 2 inspect the parsed URL that step 4 sends, so no parser + // differential can separate what we approve from what we request. // --- Privilege check (Layer 0): submitted_by must have EXECUTE on df.http() --- check_http_privilege(&pool, audit_user) @@ -119,8 +126,15 @@ pub async fn execute( )); })?; + let request_url = crate::ssrf::parse_request_url(&config.url).inspect_err(|_| { + ctx.trace_info(format!( + "HTTP BLOCKED (malformed) url={} submitted_by={audit_user}", + config.url + )); + })?; + // --- Scheme validation (always enforced, regardless of feature flag) --- - crate::ssrf::validate_url_scheme(&config.url).inspect_err(|_| { + crate::ssrf::validate_scheme(&request_url).inspect_err(|_| { ctx.trace_info(format!( "HTTP BLOCKED (scheme) url={} submitted_by={audit_user}", config.url @@ -128,7 +142,7 @@ pub async fn execute( })?; // --- Azure endpoint allow-list (blocks all bare IPs + non-Azure domains) --- - crate::ssrf::validate_url_allowlist(&config.url).inspect_err(|_| { + crate::ssrf::validate_allowlist(&request_url).inspect_err(|_| { ctx.trace_info(format!( "HTTP BLOCKED (allowlist) url={} submitted_by={audit_user}", config.url @@ -146,11 +160,11 @@ pub async fn execute( // Build request based on method let mut request = match config.method.as_str() { - "GET" => client.get(&config.url), - "POST" => client.post(&config.url), - "PUT" => client.put(&config.url), - "DELETE" => client.delete(&config.url), - "PATCH" => client.patch(&config.url), + "GET" => client.get(request_url), + "POST" => client.post(request_url), + "PUT" => client.put(request_url), + "DELETE" => client.delete(request_url), + "PATCH" => client.patch(request_url), _ => return Err(format!("Unsupported HTTP method: {}", config.method)), }; @@ -249,3 +263,89 @@ pub async fn execute( // 4xx are client errors - user should handle in workflow logic Ok(result.to_string()) } + +#[cfg(all(test, not(feature = "http-allow-all")))] +mod tests { + use super::*; + use std::ffi::OsString; + use std::io::{Read, Write}; + use std::net::TcpListener; + use std::sync::mpsc; + + struct EnvGuard { + name: &'static str, + original: Option, + } + + impl EnvGuard { + fn set(name: &'static str, value: &str) -> Self { + let original = std::env::var_os(name); + std::env::set_var(name, value); + Self { name, original } + } + + fn remove(name: &'static str) -> Self { + let original = std::env::var_os(name); + std::env::remove_var(name); + Self { name, original } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.original { + Some(value) => std::env::set_var(self.name, value), + None => std::env::remove_var(self.name), + } + } + } + + #[tokio::test] + async fn restricted_builds_ignore_environment_proxy() { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let proxy_url = format!("http://{}", listener.local_addr().unwrap()); + + let _http_proxy_upper = EnvGuard::set("HTTP_PROXY", &proxy_url); + let _http_proxy_lower = EnvGuard::set("http_proxy", &proxy_url); + let _no_proxy_upper = EnvGuard::remove("NO_PROXY"); + let _no_proxy_lower = EnvGuard::remove("no_proxy"); + + let (stop_tx, stop_rx) = mpsc::channel(); + let proxy_thread = std::thread::spawn(move || loop { + match listener.accept() { + Ok((mut stream, _)) => { + stream + .set_read_timeout(Some(Duration::from_secs(1))) + .unwrap(); + let mut request = [0; 1024]; + let _ = stream.read(&mut request); + stream + .write_all(b"HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n") + .unwrap(); + return true; + } + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { + if stop_rx.try_recv().is_ok() { + return false; + } + std::thread::yield_now(); + } + Err(error) => panic!("proxy listener failed: {error}"), + } + }); + + let client = build_client(Duration::from_secs(1)).unwrap(); + let _ = client + .get("http://pg-durable-proxy-test.invalid/") + .send() + .await; + stop_tx.send(()).unwrap(); + let proxy_was_used = proxy_thread.join().unwrap(); + + assert!( + !proxy_was_used, + "restricted HTTP modes must bypass system proxies" + ); + } +} diff --git a/src/activities/execute_multipart.rs b/src/activities/execute_multipart.rs index 422b3c9a..016db5ec 100644 --- a/src/activities/execute_multipart.rs +++ b/src/activities/execute_multipart.rs @@ -108,8 +108,15 @@ pub async fn execute( )); })?; + let request_url = crate::ssrf::parse_request_url(&config.url).inspect_err(|_| { + ctx.trace_info(format!( + "HTTP_MULTIPART BLOCKED (malformed) url={} submitted_by={audit_user}", + config.url + )); + })?; + // --- Scheme validation (always enforced) --- - crate::ssrf::validate_url_scheme(&config.url).inspect_err(|_| { + crate::ssrf::validate_scheme(&request_url).inspect_err(|_| { ctx.trace_info(format!( "HTTP_MULTIPART BLOCKED (scheme) url={} submitted_by={audit_user}", config.url @@ -117,7 +124,7 @@ pub async fn execute( })?; // --- Azure endpoint allow-list --- - crate::ssrf::validate_url_allowlist(&config.url).inspect_err(|_| { + crate::ssrf::validate_allowlist(&request_url).inspect_err(|_| { ctx.trace_info(format!( "HTTP_MULTIPART BLOCKED (allowlist) url={} submitted_by={audit_user}", config.url @@ -139,9 +146,9 @@ pub async fn execute( // body-carrying methods; the DSL guard restricts to POST/PUT/PATCH and we // defend in depth here. let mut request = match config.method.as_str() { - "POST" => client.post(&config.url), - "PUT" => client.put(&config.url), - "PATCH" => client.patch(&config.url), + "POST" => client.post(request_url), + "PUT" => client.put(request_url), + "PATCH" => client.patch(request_url), _ => { return Err(format!( "Unsupported HTTP method for multipart: {}", diff --git a/src/dsl.rs b/src/dsl.rs index 7070e52b..a8926544 100644 --- a/src/dsl.rs +++ b/src/dsl.rs @@ -497,7 +497,7 @@ pub fn http( // Skip the check when the URL contains variable placeholders ({...}) — // substitution happens at execution time so the scheme is not yet known. if !url.contains('{') { - if let Err(e) = crate::ssrf::validate_url_scheme(url) { + if let Err(e) = crate::ssrf::precheck_url_scheme(url) { pgrx::error!("{}", e); } } @@ -578,7 +578,7 @@ pub fn http_multipart( // Validate URL scheme at DSL time (skip when URL contains variable // placeholders — substitution happens at execution time). Mirrors df.http. if !url.contains('{') { - if let Err(e) = crate::ssrf::validate_url_scheme(url) { + if let Err(e) = crate::ssrf::precheck_url_scheme(url) { pgrx::error!("{}", e); } } diff --git a/src/ssrf.rs b/src/ssrf.rs index 904d393b..2e2a69b8 100644 --- a/src/ssrf.rs +++ b/src/ssrf.rs @@ -15,7 +15,12 @@ //! //! The blocklist and allow-list are hardcoded and cannot be bypassed by any //! database user, including superusers. See docs/http-security.md for details. +//! +//! Every check that inspects a URL runs on the [`Url`] produced by +//! [`parse_request_url`], and that same value is handed to reqwest. A second, +//! independent parser would reintroduce the differential described there. +use reqwest::Url; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; // --------------------------------------------------------------------------- @@ -152,19 +157,24 @@ fn check_blocked_ipv6(ip: Ipv6Addr) -> Option<&'static str> { } } -/// Validate a URL scheme against the build's outbound HTTP policy. -/// Restricted allow-list builds require HTTPS; development-only `http-allow-all` -/// builds retain plaintext HTTP support. Builds without HTTP support accept both -/// schemes here so the feature-specific disabled error remains authoritative. -pub fn validate_url_scheme(url: &str) -> Result<(), String> { +/// DSL-time scheme pre-check, so `df.http('file:///etc/passwd')` fails at +/// definition time instead of at execution time. +/// +/// This is advisory only — it runs on a raw string that may still contain +/// unsubstituted variables. [`validate_scheme`] is the enforcing check. +pub fn precheck_url_scheme(url: &str) -> Result<(), String> { let scheme = url.split("://").next().unwrap_or("").to_ascii_lowercase(); + validate_scheme_value(&scheme) +} + +fn validate_scheme_value(scheme: &str) -> Result<(), String> { let allows_plaintext = cfg!(feature = "http-allow-all") || !cfg!(any( feature = "http-allow-azure-domains", feature = "http-allow-test-domains" )); - match scheme.as_str() { + match scheme { "https" => Ok(()), "http" if allows_plaintext => Ok(()), "http" => Err( @@ -184,11 +194,36 @@ pub fn validate_url_scheme(url: &str) -> Result<(), String> { } } +// --------------------------------------------------------------------------- +// Canonical URL parsing +// --------------------------------------------------------------------------- + +/// Parse `url` into the exact value that will be handed to reqwest. +/// +/// Callers must validate *this* value and then send *this* value. Validating +/// the raw string with a second parser opens a parser differential: WHATWG +/// treats `\` as a path separator for http(s), so in +/// `https://evil.example\@acct.blob.core.windows.net/` the authority is +/// `evil.example` and the rest is path — while a hand-rolled authority scan +/// reads the trailing allow-listed name and approves the request. +pub fn parse_request_url(url: &str) -> Result { + Url::parse(url).map_err(|e| format!("Blocked: malformed URL ({e}).")) +} + +/// Validate the scheme of a canonically parsed URL against the build's +/// outbound HTTP policy. +pub fn validate_scheme(url: &Url) -> Result<(), String> { + validate_scheme_value(url.scheme()) +} + // --------------------------------------------------------------------------- // Endpoint allow-list validation // --------------------------------------------------------------------------- -/// Validate a URL against the endpoint allow-list. +/// Validate a canonically parsed URL against the endpoint allow-list. +/// +/// Takes the parsed [`Url`] rather than a string so the host checked here is +/// the host reqwest will connect to. /// /// Behaviour depends on Cargo features (most to least restrictive): /// @@ -198,7 +233,7 @@ pub fn validate_url_scheme(url: &str) -> Result<(), String> { /// * `http-allow-test-domains` — same as above **plus** `httpbingo.org` /// (for E2E tests). /// * `http-allow-all` — allow-list check is skipped entirely; all domains pass. -pub fn validate_url_allowlist(url: &str) -> Result<(), String> { +pub fn validate_allowlist(url: &Url) -> Result<(), String> { // http-allow-all: skip all domain checks. #[cfg(feature = "http-allow-all")] { @@ -226,18 +261,21 @@ pub fn validate_url_allowlist(url: &str) -> Result<(), String> { feature = "http-allow-test-domains", ))] { - let host = extract_host(url) - .ok_or_else(|| "Blocked: unable to extract hostname from URL.".to_string())?; + // Matching on Host (rather than inspecting the host string) keeps + // the IP-literal case total: WHATWG canonicalises decimal, octal and + // IPv4-mapped forms into these variants before we ever see them. + let host = match url.host() { + Some(url::Host::Domain(domain)) => domain, + Some(url::Host::Ipv4(_)) | Some(url::Host::Ipv6(_)) => { + return Err("Blocked: requests to bare IP addresses are not permitted. \ + Use an approved service hostname instead." + .to_string()); + } + None => return Err("Blocked: unable to extract hostname from URL.".to_string()), + }; let host_lower = host.to_ascii_lowercase(); - // Block ALL bare IP addresses (IPv4 and IPv6). - if host_lower.parse::().is_ok() { - return Err("Blocked: requests to bare IP addresses are not permitted. \ - Use an approved Azure service hostname instead." - .to_string()); - } - // Check Azure suffixes (always present when either azure or test feature is on). for suffix in AZURE_DOMAIN_SUFFIXES { if host_lower.ends_with(suffix) { @@ -269,53 +307,6 @@ pub fn validate_url_allowlist(url: &str) -> Result<(), String> { } } -// --------------------------------------------------------------------------- -// Host extraction helper -// --------------------------------------------------------------------------- - -/// Extract the hostname (without port or brackets) from a URL. -/// -/// Returns `None` for malformed URLs or URLs without a `://` scheme separator. -#[cfg(any( - feature = "http-allow-azure-domains", - feature = "http-allow-test-domains" -))] -fn extract_host(url: &str) -> Option { - // Strip scheme - let after_scheme = url.find("://").map(|i| &url[i + 3..])?; - // Strip path, query, and fragment — isolate authority (host + optional port). - // Per RFC 3986 / WHATWG URL, the authority is terminated by '/', '?', or '#'. - // Splitting only on '/' would let an attacker embed '?' or '#' to smuggle a - // fake suffix past our allowlist while reqwest connects to the real host. - let authority = after_scheme - .split(['/', '?', '#']) - .next() - .unwrap_or(after_scheme); - // Strip userinfo (user:pass@) - let host_port = match authority.rfind('@') { - Some(i) => &authority[i + 1..], - None => authority, - }; - // Extract host, handling bracketed IPv6 like [::1]:8080 - let host = if host_port.starts_with('[') { - // IPv6 literal in brackets - let end = host_port.find(']')?; - &host_port[1..end] - } else { - // IPv4 or hostname — strip port - match host_port.rfind(':') { - Some(i) => &host_port[..i], - None => host_port, - } - }; - - if host.is_empty() { - return None; - } - - Some(host.to_string()) -} - // Keep this marker in sync with the error message in SsrfSafeResolver::resolve(). const SSRF_BLOCK_MARKER: &str = "Blocked:"; const SSRF_RESTRICTED_MARKER: &str = "restricted"; @@ -573,23 +564,31 @@ mod tests { #[cfg(not(feature = "http-allow-all"))] #[test] fn restricted_builds_require_https() { - assert!(validate_url_scheme("https://example.com").is_ok()); - assert!(validate_url_scheme("HTTPS://example.com").is_ok()); - assert!(validate_url_scheme("http://example.com") + assert!(precheck_url_scheme("https://example.com").is_ok()); + assert!(precheck_url_scheme("HTTPS://example.com").is_ok()); + assert!(precheck_url_scheme("http://example.com") .unwrap_err() .contains("HTTPS is required")); - assert!(validate_url_scheme("HTTP://EXAMPLE.COM") + assert!(precheck_url_scheme("HTTP://EXAMPLE.COM") .unwrap_err() .contains("HTTPS is required")); + assert!(validate_scheme(&parse_request_url("https://example.com").unwrap()).is_ok()); + assert!( + validate_scheme(&parse_request_url("http://example.com").unwrap()) + .unwrap_err() + .contains("HTTPS is required") + ); } #[cfg(feature = "http-allow-all")] #[test] fn allow_all_builds_accept_http_and_https() { - assert!(validate_url_scheme("http://example.com").is_ok()); - assert!(validate_url_scheme("https://example.com").is_ok()); - assert!(validate_url_scheme("HTTP://EXAMPLE.COM").is_ok()); - assert!(validate_url_scheme("HTTPS://example.com").is_ok()); + assert!(precheck_url_scheme("http://example.com").is_ok()); + assert!(precheck_url_scheme("https://example.com").is_ok()); + assert!(precheck_url_scheme("HTTP://EXAMPLE.COM").is_ok()); + assert!(precheck_url_scheme("HTTPS://example.com").is_ok()); + assert!(validate_scheme(&parse_request_url("http://example.com").unwrap()).is_ok()); + assert!(validate_scheme(&parse_request_url("HTTPS://example.com").unwrap()).is_ok()); } #[cfg(not(any( @@ -599,93 +598,69 @@ mod tests { )))] #[test] fn disabled_builds_defer_http_rejection_to_feature_policy() { - assert!(validate_url_scheme("http://example.com").is_ok()); - assert!(validate_url_scheme("https://example.com").is_ok()); + assert!(precheck_url_scheme("http://example.com").is_ok()); + assert!(precheck_url_scheme("https://example.com").is_ok()); } #[test] fn blocks_file_scheme() { - assert!(validate_url_scheme("file:///etc/passwd").is_err()); + assert!(precheck_url_scheme("file:///etc/passwd").is_err()); + assert!(validate_scheme(&parse_request_url("file:///etc/passwd").unwrap()).is_err()); } #[test] fn blocks_ftp_scheme() { - assert!(validate_url_scheme("ftp://ftp.example.com").is_err()); + assert!(precheck_url_scheme("ftp://ftp.example.com").is_err()); + assert!(validate_scheme(&parse_request_url("ftp://ftp.example.com").unwrap()).is_err()); } #[test] fn blocks_gopher_scheme() { - assert!(validate_url_scheme("gopher://evil.com").is_err()); + assert!(precheck_url_scheme("gopher://evil.com").is_err()); + assert!(validate_scheme(&parse_request_url("gopher://evil.com").unwrap()).is_err()); } #[test] fn blocks_empty_and_malformed() { - assert!(validate_url_scheme("").is_err()); - assert!(validate_url_scheme("no-scheme").is_err()); + assert!(precheck_url_scheme("").is_err()); + assert!(precheck_url_scheme("no-scheme").is_err()); } - // --- extract_host helper --- + // --- Canonical URL parsing --- + + // Tests drive the allow-list through the same parse-then-validate path the + // activities use, so an unparseable URL is a rejection like any other. + #[cfg(not(feature = "http-allow-all"))] + fn validate_url_allowlist(url: &str) -> Result<(), String> { + validate_allowlist(&parse_request_url(url)?) + } - #[cfg(any( - feature = "http-allow-azure-domains", - feature = "http-allow-test-domains" - ))] #[test] - fn extract_host_basic() { + fn parse_request_url_canonicalises_authority() { + let u = parse_request_url("http://user:pass@Host:8080/p").unwrap(); + assert_eq!(u.host_str(), Some("host")); + assert_eq!(u.port(), Some(8080)); assert_eq!( - extract_host("http://example.com/path"), - Some("example.com".into()) + parse_request_url("https://myaccount.blob.core.windows.net?comp=list") + .unwrap() + .host_str(), + Some("myaccount.blob.core.windows.net") ); - assert_eq!( - extract_host("https://foo.blob.core.windows.net/c"), - Some("foo.blob.core.windows.net".into()) - ); - assert_eq!(extract_host("http://host:8080/p"), Some("host".into())); - assert_eq!(extract_host("http://[::1]:80/p"), Some("::1".into())); - assert_eq!(extract_host("http://user:pass@host/p"), Some("host".into())); } - #[cfg(any( - feature = "http-allow-azure-domains", - feature = "http-allow-test-domains" - ))] #[test] - fn extract_host_query_and_fragment() { - // Query-only URL (no path slash after authority) - assert_eq!( - extract_host("https://myaccount.blob.core.windows.net?comp=list"), - Some("myaccount.blob.core.windows.net".into()) - ); - // Fragment-only URL - assert_eq!( - extract_host("https://example.com#section"), - Some("example.com".into()) - ); - // Query before path — authority must stop at '?' - assert_eq!( - extract_host("https://evil.com?.blob.core.windows.net/exfil"), - Some("evil.com".into()) - ); - // Fragment before path - assert_eq!( - extract_host("https://evil.com#.blob.core.windows.net"), - Some("evil.com".into()) - ); - // Userinfo confusion via query — '@' is in the query, not the authority - assert_eq!( - extract_host("https://evil.com?@myaccount.blob.core.windows.net"), - Some("evil.com".into()) - ); + fn parse_request_url_treats_backslash_as_path_separator() { + // The authority ends at the backslash, so the '@' and everything after + // it are path — this is the differential the allow-list must not see. + let u = parse_request_url(r"https://evil.example\@api.github.com/repos").unwrap(); + assert_eq!(u.host_str(), Some("evil.example")); + assert_eq!(u.path(), "/@api.github.com/repos"); } - #[cfg(any( - feature = "http-allow-azure-domains", - feature = "http-allow-test-domains" - ))] #[test] - fn extract_host_none_cases() { - assert_eq!(extract_host("no-scheme"), None); - assert_eq!(extract_host(""), None); + fn parse_request_url_rejects_malformed() { + assert!(parse_request_url("no-scheme").is_err()); + assert!(parse_request_url("").is_err()); } // --- Endpoint allow-list validation --- @@ -855,11 +830,12 @@ mod tests { feature = "http-allow-test-domains" ))] #[test] - fn allowlist_blocks_percent_encoded_host() { - // %2E is a percent-encoded '.'; our parser does no decoding so the - // encoded form never matches the suffix — the request is blocked. - // This locks the safe current behavior against accidental URL decoding. - assert!(validate_url_allowlist("https://foo%2Eblob%2Ecore%2Ewindows%2Enet/c").is_err()); + fn allowlist_follows_percent_decoded_host() { + // %2E is a percent-encoded '.'. WHATWG decodes it during host parsing, + // so the request really does go to the allow-listed host and the verdict must + // match that — the allow-list judges the host reqwest will connect to, + // never the spelling the caller used. + assert!(validate_url_allowlist("https://foo%2Eblob%2Ecore%2Ewindows%2Enet/c").is_ok()); assert!(validate_url_allowlist("https://evil%2Ecom/steal").is_err()); } @@ -935,6 +911,55 @@ mod tests { ); } + // --- Backslash authority-termination vectors --- + // + // WHATWG ends the authority of an http(s) URL at '\', so the allow-listed + // name after the backslash is path, not host. Any parser that misses this + // approves a request aimed somewhere else entirely. + + #[cfg(not(feature = "http-allow-all"))] + #[test] + fn allowlist_blocks_backslash_userinfo_bypass() { + assert!( + validate_url_allowlist(r"https://evil.example\@acct.blob.core.windows.net/c").is_err() + ); + assert!( + validate_url_allowlist(r"https://evil.example\@acct.queue.core.windows.net/s").is_err() + ); + assert!( + validate_url_allowlist(r"https://evil.example:443\@acct.file.core.windows.net/") + .is_err() + ); + // No userinfo marker at all — the whole tail is path. + assert!( + validate_url_allowlist(r"https://evil.example\acct.blob.core.windows.net/").is_err() + ); + } + + // The bare-IP rule is the only gate for IP-literal targets: reqwest skips + // DNS for them, so SsrfSafeResolver never runs. These must never pass. + #[cfg(not(feature = "http-allow-all"))] + #[test] + fn allowlist_blocks_backslash_ip_literal_bypass() { + assert!(validate_url_allowlist( + r"http://169.254.169.254\@acct.blob.core.windows.net/../metadata/instance" + ) + .is_err()); + assert!( + validate_url_allowlist(r"http://127.0.0.1:5432\@acct.queue.core.windows.net/").is_err() + ); + assert!(validate_url_allowlist(r"http://[::1]\@acct.blob.core.windows.net/").is_err()); + } + + // Bare IPs written in non-dotted notation are canonicalised by WHATWG, so + // they reach the connector as IP literals and must be caught as such. + #[cfg(not(feature = "http-allow-all"))] + #[test] + fn allowlist_blocks_non_dotted_ip_notation() { + assert!(validate_url_allowlist("http://2130706433/").is_err()); + assert!(validate_url_allowlist("http://0x7f.1/").is_err()); + } + // --- Test domains (only with http-allow-test-domains) --- #[cfg(feature = "http-allow-test-domains")] @@ -950,6 +975,19 @@ mod tests { assert!(validate_url_allowlist("https://evil.com/steal").is_err()); } + // The exact-match branch is no safer than the suffix branch when the host + // itself is taken from the wrong parse, so it gets the same coverage. + #[cfg(feature = "http-allow-test-domains")] + #[test] + fn allowlist_blocks_backslash_bypass_of_exact_domains() { + assert!(validate_url_allowlist(r"https://evil.example\@api.github.com/repos").is_err()); + assert!(validate_url_allowlist(r"https://user\name@api.github.com/repos").is_err()); + assert!(validate_url_allowlist( + r"http://169.254.169.254\@api.github.com/../metadata/instance" + ) + .is_err()); + } + // --- SsrfSafeResolver behavioral tests --- // // These tests drive the resolver directly with a mock inner resolver so we diff --git a/tests/e2e/sql/06_http_and_ssrf.sql b/tests/e2e/sql/06_http_and_ssrf.sql index 561ad6e1..26580ca7 100644 --- a/tests/e2e/sql/06_http_and_ssrf.sql +++ b/tests/e2e/sql/06_http_and_ssrf.sql @@ -1218,6 +1218,55 @@ END $$; DROP TABLE _test_ssrf11c; +-- Test 12: A backslash cannot smuggle an allow-listed name past the allow-list. +-- WHATWG ends the authority of an http(s) URL at '\', so each URL below reaches +-- evil.example / 169.254.169.254 while the allow-listed name is only path. The +-- IP-literal vector is the sharpest: such targets skip DNS, so the allow-list is +-- the only gate they ever meet. +CREATE TEMP TABLE _test_ssrf12 (vector TEXT, expected TEXT, instance_id TEXT); + +INSERT INTO _test_ssrf12 VALUES + ('exact test domain', 'not in the allowed', + df.start(df.http('https://evil.example\@api.github.com/repos', 'GET'), + 'test-ssrf-backslash-exact')), + ('allowed suffix', 'not in the allowed', + df.start(df.http('https://evil.example\@testaccount.blob.core.windows.net/c', 'GET'), + 'test-ssrf-backslash-suffix')), + ('ip literal', 'bare IP', + df.start(df.http('https://169.254.169.254\@api.github.com/../metadata/instance', 'GET'), + 'test-ssrf-backslash-ip')); + +DO $$ +DECLARE + r RECORD; + status TEXT; + node_result TEXT; +BEGIN + FOR r IN SELECT * FROM _test_ssrf12 LOOP + RAISE NOTICE 'Testing backslash bypass (%): %', r.vector, r.instance_id; + + SELECT df.await_instance(r.instance_id) INTO status; + + IF status != 'failed' THEN + RAISE EXCEPTION 'TEST FAILED: backslash bypass (%) should be blocked, got status = %', + r.vector, status; + END IF; + + SELECT result::text INTO node_result + FROM df.nodes + WHERE instance_id = r.instance_id AND node_type = 'HTTP'; + + IF node_result IS NULL OR node_result NOT ILIKE '%' || r.expected || '%' THEN + RAISE EXCEPTION 'TEST FAILED: expected "%" for backslash bypass (%), got: %', + r.expected, r.vector, node_result; + END IF; + END LOOP; + + RAISE NOTICE 'TEST PASSED: ssrf_backslash_authority_bypass_blocked'; +END $$; + +DROP TABLE _test_ssrf12; + RESET SESSION AUTHORIZATION; -- ============================================================================