From 3aaf1a7955814ea39827656caec8fa2efc5f8565 Mon Sep 17 00:00:00 2001 From: forkwright Date: Fri, 3 Jul 2026 09:44:42 -0500 Subject: [PATCH] fix(zetesis): close indexer-fetch SSRF, injection, XML-DoS, and key-leak defects Adversarial audit found a cluster of security defects on the indexer-fetch path. Each is closed at its enforcement point: - SSRF DNS-rebinding TOCTOU: validate_fetch_url checked resolved IPs then discarded them; the shared client re-resolved at connect time. Add an SsrfGuardResolver (reqwest::dns::Resolve) that re-checks every resolved address at connect time and rejects disallowed ones; wire it into the shared client. Pre-check kept as defense-in-depth. - IPv4-compatible IPv6 (::/96) gap: ip_is_disallowed omitted the deprecated ::a.b.c.d embed, so [::127.0.0.1] passed. Port net_validate's exact handling (mapped -> v6 specials -> ::/96 embed). - Cardigann $raw injection: $raw inputs rendered variable expansions unescaped and spliced into the query via set_query, so a term like 'a&b=c' injected extra params. Encode $raw expansions via render_url, matching the search-path encoding; literal $raw structure untouched. - API keys leaked via tracing spans: download()/get_inner() were #[instrument] without skipping the url arg, capturing apikey= as a span field. Skip url on every secret-bearing instrumented fn in the client tree. - cf_bypass skipped the body-size cap: ByparrProxy::get read via response.json() with no cap, making max_response_body_bytes a no-op for cf_bypass indexers. Enforce the cap (Content-Length precheck + streamed counter) on the byparr envelope before buffering. - Untrusted caps XML stack overflow: parse_caps_xml deserialized into the self-referential CapsCategory, overflowing the stack during serde's recursive descent before the iterative converter ran. Bound XML nesting depth on the raw event stream before deserialization. Gate-Passed: kanon 0.1.5 +stages:fmt,check,clippy,nextest,lint sha:29a0bba2e9f4fb3081110d37bc6dc2445c23514d --- crates/archon/src/serve.rs | 1 + crates/zetesis/src/cf_bypass/byparr.rs | 48 +++++++++++- crates/zetesis/src/client/cardigann/mod.rs | 22 +++++- crates/zetesis/src/client/cardigann/tests.rs | 55 ++++++++++++++ crates/zetesis/src/client/mod.rs | 77 +++++++++++++++++++- crates/zetesis/src/client/newznab.rs | 5 +- crates/zetesis/src/client/torznab.rs | 5 +- crates/zetesis/src/client/xml.rs | 65 +++++++++++++++++ crates/zetesis/src/search.rs | 9 ++- 9 files changed, 274 insertions(+), 13 deletions(-) diff --git a/crates/archon/src/serve.rs b/crates/archon/src/serve.rs index d2120efe..280fd510 100644 --- a/crates/archon/src/serve.rs +++ b/crates/archon/src/serve.rs @@ -1087,6 +1087,7 @@ fn build_cf_proxy( Ok(Arc::new(ByparrProxy::new( url.to_string(), std::time::Duration::from_secs(config.cf_proxy_timeout_seconds), + config.max_response_body_bytes, ))) } diff --git a/crates/zetesis/src/cf_bypass/byparr.rs b/crates/zetesis/src/cf_bypass/byparr.rs index 664d46d1..f4e14d57 100644 --- a/crates/zetesis/src/cf_bypass/byparr.rs +++ b/crates/zetesis/src/cf_bypass/byparr.rs @@ -14,6 +14,7 @@ pub struct ByparrProxy { client: reqwest::Client, endpoint: String, timeout: Duration, + max_body_bytes: u64, } #[derive(Debug, Serialize)] @@ -55,7 +56,7 @@ struct ByparrCookie { } impl ByparrProxy { - pub fn new(endpoint: String, timeout: Duration) -> Self { + pub fn new(endpoint: String, timeout: Duration, max_body_bytes: u64) -> Self { let client = reqwest::Client::builder() .timeout(timeout + Duration::from_secs(5)) .build() @@ -65,6 +66,7 @@ impl ByparrProxy { client, endpoint, timeout, + max_body_bytes, } } @@ -95,7 +97,10 @@ impl CloudflareProxy for ByparrProxy { } impl ByparrProxy { - #[instrument(skip(self, ct), fields(endpoint = %self.endpoint))] + // WHY: skip `url` — the indexer URL embeds `apikey=` and #[instrument] + // would capture the raw value as a span field visible to any event + // emitted in the span. + #[instrument(skip(self, url, ct), fields(endpoint = %self.endpoint))] async fn get_inner( &self, url: &str, @@ -124,9 +129,20 @@ impl ByparrProxy { } }; + // WHY: enforce the body-size cap HERE — the cf_bypass fetch paths + // return this ProxyResponse.body directly, skipping read_body_bounded, + // so without a cap on the byparr envelope `max_response_body_bytes` is + // a no-op for cf_bypass indexers. The cap bounds the byparr JSON + // (Content-Length precheck + streamed running counter) before it is + // buffered; the wrapped indexer body is a subset, so the effective + // ceiling is conservative by the envelope overhead. + let raw = crate::client::read_body_bytes_bounded(response, &redacted, self.max_body_bytes) + .await?; let byparr_resp: ByparrResponse = - response.json().await.context(error::HttpRequestSnafu { + serde_json::from_slice(&raw).map_err(|e| SearchIndexerError::ParseResponse { url: redacted.clone(), + error: e.to_string(), + location: snafu::Location::new(file!(), line!(), column!()), })?; if byparr_resp.status != "ok" { @@ -173,6 +189,32 @@ impl ByparrProxy { #[cfg(test)] mod tests { use super::*; + use crate::test_support::spawn_one_shot_http; + + #[tokio::test] + async fn get_enforces_body_size_cap() { + // WHY: the cf_bypass fetch paths return ByparrProxy's body directly, + // so the cap must live here or `max_response_body_bytes` is a no-op + // for cf_bypass indexers. A byparr envelope declaring a size above the + // cap must be rejected before it is buffered. + let big = "x".repeat(10_000); + let json = format!( + r#"{{"status":"ok","message":"","solution":{{"url":"https://e.example","status":200,"response":"{big}","cookies":[],"userAgent":"UA"}}}}"# + ); + let (endpoint, _server) = spawn_one_shot_http(200, "OK", &[], &json).await; + let proxy = ByparrProxy::new(endpoint, Duration::from_secs(5), 64); + let err = proxy + .get( + "https://indexer.example/api?apikey=secret", + CancellationToken::new(), + ) + .await + .unwrap_err(); + assert!( + matches!(err, SearchIndexerError::ResponseTooLarge { limit: 64, .. }), + "expected ResponseTooLarge, got {err:?}" + ); + } #[test] fn byparr_request_serialization() { diff --git a/crates/zetesis/src/client/cardigann/mod.rs b/crates/zetesis/src/client/cardigann/mod.rs index b2ffdad7..8243d6cf 100644 --- a/crates/zetesis/src/client/cardigann/mod.rs +++ b/crates/zetesis/src/client/cardigann/mod.rs @@ -296,12 +296,23 @@ impl CardigannClient { let mut raw_parts: Vec = Vec::new(); let mut pairs: Vec<(&str, String)> = Vec::new(); for (key, value) in inputs { - let rendered = ctx - .render(value) - .map_err(|e| self.invalid(format!("search input {key}: {e}")))?; if key == "$raw" { + // WHY: $raw is spliced verbatim into the query via set_query + // (which does NOT encode), so variable expansions must be + // percent-encoded here exactly as the path template already is + // via render_url — a keyword like "a&b=c" must not inject an + // extra parameter. Only the .Keywords/.Query.* expansions + // encode; the definition-author's literal $raw "&"/"=" survive. + let rendered = ctx + .render_url(value) + .map_err(|e| self.invalid(format!("search input {key}: {e}")))?; raw_parts.push(rendered); } else { + // WHY: non-$raw values pass through append_pair below, which + // percent-encodes the whole value — render unencoded here. + let rendered = ctx + .render(value) + .map_err(|e| self.invalid(format!("search input {key}: {e}")))?; pairs.push((key, rendered)); } } @@ -599,7 +610,10 @@ impl IndexerClient for CardigannClient { } } - #[instrument(skip(self, ct), fields(indexer_id = self.indexer.id))] + // WHY: skip `url` — download URLs carry secrets (apikey/passkey in the + // query) and #[instrument] would capture the raw value as a span field + // visible to any event emitted in the span. + #[instrument(skip(self, url, ct), fields(indexer_id = self.indexer.id))] async fn download( &self, url: &str, diff --git a/crates/zetesis/src/client/cardigann/tests.rs b/crates/zetesis/src/client/cardigann/tests.rs index 351f33a5..bb7d32a1 100644 --- a/crates/zetesis/src/client/cardigann/tests.rs +++ b/crates/zetesis/src/client/cardigann/tests.rs @@ -234,6 +234,61 @@ async fn keywords_inline_in_path_template_are_percent_encoded() { ); } +const RAW_INPUT_DEF: &str = r#"--- +id: raw-input +name: Raw Input +links: + - https://raw-input.example/ +caps: + categorymappings: + - {id: 6, cat: Movies/HD} + modes: + search: [q] +search: + paths: + - path: /browse + inputs: + $raw: "q={{ .Keywords }}&mode=list" + rows: + selector: table#torrents > tbody > tr + fields: + title: + selector: a.title + download: + selector: a.dl + attribute: href +"#; + +#[tokio::test] +async fn raw_input_expansions_are_percent_encoded() { + // WHY: $raw is spliced verbatim into the query via set_query (no encoding), + // so a keyword like "a&b=c" must be encoded to a single value — never split + // into an injected `b=c` parameter. The literal $raw "q="/"&mode=" survive. + let (url, server) = spawn_one_shot_http(200, "OK", &[], "").await; + let query = SearchQuery { + query_text: Some("a&b=c".to_string()), + limit: 100, + ..Default::default() + }; + client(RAW_INPUT_DEF, url, None) + .unwrap() + .search(&query, CancellationToken::new()) + .await + .unwrap(); + + let request_head = server.await.unwrap(); + let request_line = request_head.lines().next().unwrap_or_default().to_string(); + assert_eq!( + request_line, "GET /browse?q=a%26b%3Dc&mode=list HTTP/1.1", + "raw expansion must encode; no injected param" + ); + // WHY: guard against a decoded "&b=c" smuggling a second parameter. + assert!( + !request_line.contains("&b=c"), + "injected param leaked: {request_line}" + ); +} + #[tokio::test] async fn search_without_inputs_appends_no_bare_question_mark() { let yaml = PATH_TEMPLATE_DEF.replace("/browse.php?search={{ .Keywords }}&cat=0", "/browse"); diff --git a/crates/zetesis/src/client/mod.rs b/crates/zetesis/src/client/mod.rs index 38166f87..24ce9cf5 100644 --- a/crates/zetesis/src/client/mod.rs +++ b/crates/zetesis/src/client/mod.rs @@ -3,9 +3,10 @@ pub mod newznab; pub mod torznab; pub mod xml; -use std::net::IpAddr; +use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use futures::StreamExt; +use reqwest::dns::{Addrs, Name, Resolve, Resolving}; use snafu::ResultExt; use tokio_util::sync::CancellationToken; use url::{Host, Url}; @@ -306,14 +307,63 @@ fn ip_is_disallowed(ip: IpAddr) -> bool { if let Some(mapped) = v6.to_ipv4_mapped() { return ip_is_disallowed(IpAddr::V4(mapped)); } - v6.is_loopback() + // WHY: checked before the ::/96 embed below so :: (unspecified) and + // ::1 (loopback) are judged as v6, not as the compatible 0.0.0.0 / + // 0.0.0.1. + if v6.is_loopback() || v6.is_unspecified() || v6.is_unique_local() || v6.is_unicast_link_local() + { + return true; + } + // WHY: deprecated IPv4-compatible IPv6 (::a.b.c.d, ::/96) embeds a + // v4 address in the low 32 bits — ::127.0.0.1 must be judged by + // that v4 or it reaches loopback unchecked. to_ipv4_mapped only + // matches ::ffff:*. + let [s0, s1, s2, s3, s4, s5, s6, s7] = v6.segments(); + if [s0, s1, s2, s3, s4, s5] == [0, 0, 0, 0, 0, 0] { + let embedded = Ipv4Addr::from((u32::from(s6) << 16) | u32::from(s7)); + return ip_is_disallowed(IpAddr::V4(embedded)); + } + false } } } +/// A reqwest DNS resolver that rejects the connection when ANY resolved +/// address is disallowed, then hands reqwest only the vetted addresses. +/// +/// WHY: [`validate_fetch_url`] resolves and checks at validation time but then +/// discards the addresses; the shared client re-resolves at connect time, so a +/// host answering public at validation and private at connect (DNS rebinding) +/// would slip past the pre-check. Re-checking every address here — the moment +/// reqwest is about to connect — closes that TOCTOU window. Fail-closed: +/// resolution failure or any disallowed address rejects the whole connection. +#[derive(Debug, Default, Clone, Copy)] +pub(crate) struct SsrfGuardResolver; + +impl Resolve for SsrfGuardResolver { + fn resolve(&self, name: Name) -> Resolving { + let host = name.as_str().to_string(); + Box::pin(async move { + // WHY: port 0 — the address checks ignore the port and reqwest + // overrides it with the target's real port after resolution. + let resolved: Vec = + tokio::net::lookup_host((host.as_str(), 0)).await?.collect(); + for addr in &resolved { + if ip_is_disallowed(addr.ip()) { + return Err(format!( + "refusing to connect to {host}: resolves to a private or local address" + ) + .into()); + } + } + Ok(Box::new(resolved.into_iter()) as Addrs) + }) + } +} + fn urlencoding(s: &str) -> String { let mut result = String::with_capacity(s.len()); for b in s.bytes() { @@ -518,6 +568,10 @@ mod tests { "http://[fe80::1]/x", "http://[::ffff:127.0.0.1]/x", "http://[::ffff:192.168.1.1]/x", + // WHY: deprecated IPv4-compatible IPv6 (::/96) embed — must be + // judged by the embedded v4 or [::127.0.0.1] reaches loopback. + "http://[::127.0.0.1]/x", + "http://[::169.254.169.254]/x", ] { let err = validate_fetch_url(url).await.expect_err(url); assert!(matches!(err, SearchIndexerError::UnsafeUrl { .. })); @@ -559,9 +613,28 @@ mod tests { assert!(ip_is_disallowed(IpAddr::V4(Ipv4Addr::new(169, 254, 1, 1)))); assert!(ip_is_disallowed(IpAddr::V4(Ipv4Addr::new(100, 127, 0, 1)))); assert!(ip_is_disallowed(IpAddr::V6(Ipv6Addr::LOCALHOST))); + // NOTE: deprecated IPv4-compatible IPv6 (::/96) — judged by embedded v4. + assert!(ip_is_disallowed("::127.0.0.1".parse().unwrap())); + assert!(ip_is_disallowed("::10.0.0.1".parse().unwrap())); + assert!(ip_is_disallowed("::169.254.169.254".parse().unwrap())); assert!(!ip_is_disallowed(IpAddr::V4(Ipv4Addr::new(203, 0, 113, 1)))); + assert!(!ip_is_disallowed("::203.0.113.1".parse().unwrap())); assert!(!ip_is_disallowed(IpAddr::V4(Ipv4Addr::new(8, 8, 8, 8)))); assert!(!ip_is_disallowed(IpAddr::V4(Ipv4Addr::new(100, 63, 0, 1)))); assert!(!ip_is_disallowed(IpAddr::V4(Ipv4Addr::new(172, 32, 0, 1)))); } + + #[tokio::test] + async fn ssrf_resolver_rejects_loopback_resolution() { + // WHY: "localhost" resolves to a loopback address via the system + // resolver — the connect-time guard must reject it, proving the + // rebinding TOCTOU is closed even after validate_fetch_url passed. + let resolver = SsrfGuardResolver; + let name: Name = "localhost".parse().expect("localhost is a valid DNS name"); + let result = resolver.resolve(name).await; + assert!( + result.is_err(), + "resolver must reject a host that resolves to a loopback address" + ); + } } diff --git a/crates/zetesis/src/client/newznab.rs b/crates/zetesis/src/client/newznab.rs index 44558cd9..7a5b6305 100644 --- a/crates/zetesis/src/client/newznab.rs +++ b/crates/zetesis/src/client/newznab.rs @@ -184,7 +184,10 @@ impl IndexerClient for NewznabClient { } } - #[instrument(skip(self, ct), fields(indexer_id = self.config.id))] + // WHY: skip `url` — download URLs carry secrets (apikey in the query) and + // #[instrument] would capture the raw value as a span field visible to any + // event emitted in the span. + #[instrument(skip(self, url, ct), fields(indexer_id = self.config.id))] async fn download( &self, url: &str, diff --git a/crates/zetesis/src/client/torznab.rs b/crates/zetesis/src/client/torznab.rs index 54369d45..689ab070 100644 --- a/crates/zetesis/src/client/torznab.rs +++ b/crates/zetesis/src/client/torznab.rs @@ -193,7 +193,10 @@ impl IndexerClient for TorznabClient { } } - #[instrument(skip(self, ct), fields(indexer_id = self.config.id))] + // WHY: skip `url` — download URLs carry secrets (apikey/passkey in the + // query) and #[instrument] would capture the raw value as a span field + // visible to any event emitted in the span. + #[instrument(skip(self, url, ct), fields(indexer_id = self.config.id))] async fn download( &self, url: &str, diff --git a/crates/zetesis/src/client/xml.rs b/crates/zetesis/src/client/xml.rs index 10747525..13cb7ac1 100644 --- a/crates/zetesis/src/client/xml.rs +++ b/crates/zetesis/src/client/xml.rs @@ -237,7 +237,48 @@ pub fn parse_feed_xml(xml: &str) -> Result { quick_xml::de::from_str(xml) } +/// Maximum XML element nesting depth accepted in a caps document. +/// +/// WHY: caps XML is third-party data whose `...` nesting maps +/// 1:1 onto serde's recursive descent into the self-referential +/// [`CapsCategory`]. Beyond this depth the recursive deserialize overflows the +/// stack DURING parse — before the iterative post-parse [`convert_category`] +/// can run — so the ceiling is enforced on the raw event stream first. Real +/// Torznab caps nest one `` level; 32 leaves generous headroom. +const MAX_CAPS_XML_DEPTH: usize = 32; + +/// Rejects a caps document whose element nesting exceeds [`MAX_CAPS_XML_DEPTH`], +/// scanning the raw event stream so no stack-recursive deserialize is entered. +/// +/// WHY: a malformed-but-shallow document is left for `from_str` to report with +/// its canonical error — this guard only fires on the deep-nesting DoS class. +fn reject_excessive_depth(xml: &str) -> Result<(), quick_xml::DeError> { + use quick_xml::events::Event; + + let mut reader = quick_xml::Reader::from_str(xml); + let mut depth: usize = 0; + loop { + match reader.read_event() { + Ok(Event::Start(_)) => { + depth += 1; + if depth > MAX_CAPS_XML_DEPTH { + return Err(quick_xml::DeError::Custom(format!( + "caps XML nesting exceeds the maximum depth of {MAX_CAPS_XML_DEPTH}" + ))); + } + } + Ok(Event::End(_)) => depth = depth.saturating_sub(1), + Ok(Event::Eof) => return Ok(()), + // WHY: a malformed document is not this guard's concern — stop the + // pre-scan and let from_str surface the authoritative parse error. + Ok(_) => {} + Err(_) => return Ok(()), + } + } +} + pub fn parse_caps_xml(xml: &str) -> Result { + reject_excessive_depth(xml)?; let caps_root: CapsRoot = quick_xml::de::from_str(xml)?; Ok(caps_root.into_indexer_caps()) } @@ -420,6 +461,30 @@ mod tests { assert_eq!(count, DEPTH + 1); } + #[test] + fn parse_caps_xml_rejects_hostile_nesting_depth() { + // WHY: exercises the REAL parse path (from_str) — untrusted caps XML + // with deeply-nested would overflow the stack during serde's + // recursive descent (before the iterative converter runs); the depth + // pre-scan must reject it with a clean error, never crash the process. + const DEPTH: usize = 50_000; + let mut xml = + String::from(r#""#); + for i in 0..DEPTH { + xml.push_str(&format!(r#""#)); + } + for _ in 0..DEPTH { + xml.push_str(""); + } + xml.push_str(""); + + let result = parse_caps_xml(&xml); + assert!( + result.is_err(), + "hostile nesting depth must be rejected, not parsed" + ); + } + #[test] fn convert_category_round_trips_shallow_tree() { let node = CapsCategory { diff --git a/crates/zetesis/src/search.rs b/crates/zetesis/src/search.rs index 1c722393..a02c96d9 100644 --- a/crates/zetesis/src/search.rs +++ b/crates/zetesis/src/search.rs @@ -15,7 +15,7 @@ use crate::cf_bypass::CloudflareProxy; use crate::client::cardigann::CardigannRegistry; use crate::client::newznab::NewznabClient; use crate::client::torznab::TorznabClient; -use crate::client::{DynIndexerClient, IndexerConfig}; +use crate::client::{DynIndexerClient, IndexerConfig, SsrfGuardResolver}; use crate::error::{self, SearchIndexerError}; use crate::rate_limit::RateLimiter; use crate::repo::{self, IndexerRow}; @@ -51,10 +51,15 @@ impl SearchIndexerService { Duration::from_secs(config.per_indexer_rate_limit_window_seconds), ); + // WHY: a custom DNS resolver re-validates every resolved address at + // connect time, closing the DNS-rebinding TOCTOU that validate_fetch_url's + // pre-check alone cannot — the client re-resolves after validation, so a + // host that answers public then private would otherwise bypass the guard. let http = reqwest::Client::builder() .timeout(Duration::from_secs(config.request_timeout_secs)) + .dns_resolver(Arc::new(SsrfGuardResolver)) .build() - .unwrap_or_default(); // WHY: reqwest::Client::default() is a valid fallback; build fails only with invalid TLS config + .unwrap_or_default(); // WHY: reqwest::Client::default() is a valid fallback (build fails only with invalid TLS config); the validate_fetch_url pre-check still guards that path // WHY: definitions are read once at startup — this is the read site // for `cardigann_definitions_dir`; per-search dispatch only resolves