diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 1fae950..12004fa 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -22,7 +22,9 @@ jobs: - name: Install cargo-tarpaulin run: cargo install cargo-tarpaulin - name: Generate coverage - run: cargo tarpaulin --workspace --out xml + # llvm engine: the default ptrace engine segfaults on multi-threaded + # tokio socket tests (tests/url_security.rs) + run: cargo tarpaulin --workspace --engine llvm --out xml - name: Upload coverage uses: codecov/codecov-action@v4 with: diff --git a/AGENTS.md b/AGENTS.md index 97fa39f..9204780 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,7 +29,7 @@ Fix root cause. Unsure: read more code; if stuck, ask w/ short options. Unrecogn | formats/html-extraction | HTML element mapping, stripping, whitespace, depth limits | | formats/format-support | Formats, bytes-in API, detection order, DOCX + spreadsheet mapping | | security/security-model | One-page threat/invariant summary | -| security/threat-model | Threats T1-T6, mitigations, test map, resource limits, invariants S1-S11 | +| security/threat-model | Threats T1-T6, mitigations, test map, resource limits, invariants S1-S12 | | runtimes/python-package | PyO3 `markkit` module, maturin, PyPI | | runtimes/node-package | NAPI-RS `@everruns/markkit`, npm, binding parity table | | operations/testing | Test layout, categories, CI jobs | diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a93e34..6b78190 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Security + +- `extract_url` (CLI, Python and Node `extract_url` too) now refuses non-public destinations by default: + loopback, private, link-local / cloud metadata and other reserved addresses, checked on the URL, + after DNS resolution and on every redirect. Only `http`/`https` are fetched. Redirects are capped + (`max_redirects`, default 5), the body is bounded by `max_input_size` while streaming, and the whole + fetch by `max_processing_secs`. New builder options `allow_private_networks(true)` and `allow_host(..)`; + refusals return the new `Error::UrlBlocked`. + ### Changed - Design docs moved from `specs/` to an OKF v0.2 knowledge bundle in `knowledge/`, validated in CI. - CI workflows declare read-only default permissions and do not persist checkout credentials. - CI runs every example; `just bench` now runs the criterion benchmarks. -- README, SECURITY and binding docs corrected: Python module is `markkit`, URL fetching has no private-network blocking yet. +- README, SECURITY and binding docs corrected: Python module is `markkit`. - Dependencies: drop unused `async-trait`, `futures`, `regex`, `unicode-segmentation`, `encoding_rs`, `base64`, `mime`, `lol_html` (core) and `tracing-subscriber`, plus unused deps of the CLI, bindings and stub crates. Upgrade scraper 0.29, html5ever 0.40, quick-xml 0.42 (now a single version shared diff --git a/README.md b/README.md index 4987e1b..0d9e55a 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,8 @@ markkit = { version = "0.1", default-features = false, features = ["docx", "spre Office-document limits are configurable on the builder: `max_archive_entries`, `max_archive_entry_size`, `max_archive_total_size`, `max_spreadsheet_cells`, -`max_sheet_rows`, `max_sheet_cols`, `include_hidden_sheets`. +`max_sheet_rows`, `max_sheet_cols`, `include_hidden_sheets`. URL fetching: +`allow_private_networks`, `allow_host`, `max_redirects`. ## Security @@ -176,9 +177,11 @@ Markkit treats every input as untrusted. HTML scripts, styles, forms, iframes an SVG are stripped; office containers are decompressed under measured byte limits; spreadsheet grids are pre-scanned so sparse cells cannot force huge allocations; XML entities are never expanded; macros and formulas are never executed; hidden -sheets are skipped by default. URL fetching does not yet block private networks, -so embedders fetching attacker-chosen URLs should fetch themselves and call -`extract_document` (build with `default-features = false`). +sheets are skipped by default. `extract_url` accepts only http/https, refuses +loopback, private, link-local and cloud-metadata addresses (also after DNS +resolution and on every redirect), caps redirects, and bounds the body by +`max_input_size` and the fetch by `max_processing_secs`. Opt in to internal +hosts with `allow_private_networks(true)`; restrict hosts with `allow_host(..)`. See the [threat model](knowledge/security/threat-model.md) and the [security policy](SECURITY.md) for reporting vulnerabilities. diff --git a/SECURITY.md b/SECURITY.md index fb4d01c..93dc1d3 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -32,7 +32,7 @@ Markkit processes untrusted input (HTML, DOCX, spreadsheets) and converts it to - **No code execution**: scripts, styles, forms, iframes and SVG are stripped; macros and formulas are never executed; XML entities are never expanded - **Resource limits**: input size, HTML/XML nesting depth, archive entry count and decompressed size, spreadsheet grid cells, rows and columns - **Explicit failure**: malformed input returns errors, never panics; unsupported formats fail explicitly -- **URL fetching**: enabled by the default `http_client` feature; private-network blocking and allowlists are not implemented yet, so fetch attacker-chosen URLs yourself and build with `default-features = false` +- **URL fetching**: enabled by the default `http_client` feature; http/https only, non-public addresses refused by default (after DNS and on every redirect), optional host allowlist, bounded redirects, body size and fetch time. Behind an environment HTTP proxy, the proxy resolves names, so restrict egress there too Full analysis: [threat model](knowledge/security/threat-model.md). diff --git a/crates/markkit/Cargo.toml b/crates/markkit/Cargo.toml index c5fe350..f6f5d96 100644 --- a/crates/markkit/Cargo.toml +++ b/crates/markkit/Cargo.toml @@ -19,7 +19,7 @@ html = ["dep:scraper", "dep:html5ever"] pdf = [] docx = ["dep:zip", "dep:quick-xml"] spreadsheet = ["dep:calamine", "dep:zip", "dep:quick-xml", "dep:cfb"] -http_client = ["dep:reqwest"] +http_client = ["dep:reqwest", "tokio/net"] logging = ["dep:tracing"] all_formats = ["html", "pdf", "docx", "spreadsheet"] @@ -54,7 +54,7 @@ tracing = { workspace = true, optional = true } [dev-dependencies] anyhow.workspace = true -tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } +tokio = { workspace = true, features = ["macros", "rt-multi-thread", "net", "io-util", "time"] } tempfile.workspace = true proptest.workspace = true criterion.workspace = true diff --git a/crates/markkit/src/config.rs b/crates/markkit/src/config.rs index 14b0f75..9f86747 100644 --- a/crates/markkit/src/config.rs +++ b/crates/markkit/src/config.rs @@ -32,6 +32,14 @@ pub struct MarkkitBuilder { pub max_spreadsheet_cells: u64, /// Render hidden / very hidden sheets. Default: false (skipped with a warning). pub include_hidden_sheets: bool, + /// Let `extract_url` reach loopback, private, link-local and other + /// non-public addresses. Default: false (SSRF protection). + pub allow_private_networks: bool, + /// Hosts `extract_url` may contact, matched exactly and case-insensitively + /// on every hop (empty = any public host). + pub allowed_hosts: Vec, + /// Maximum redirects `extract_url` follows. Default: 5. + pub max_redirects: usize, } impl Default for MarkkitBuilder { @@ -58,6 +66,9 @@ impl Default for MarkkitBuilder { max_sheet_cols: 256, max_spreadsheet_cells: 4_000_000, include_hidden_sheets: false, + allow_private_networks: false, + allowed_hosts: Vec::new(), + max_redirects: 5, } } } @@ -128,6 +139,24 @@ impl MarkkitBuilder { self } + /// Allow `extract_url` to reach non-public addresses (loopback, RFC 1918, + /// link-local, cloud metadata, ...). Only for trusted URLs. + pub fn allow_private_networks(mut self, yes: bool) -> Self { + self.allow_private_networks = yes; + self + } + + /// Restrict `extract_url` to this host (repeatable). Checked on every redirect hop. + pub fn allow_host(mut self, host: impl Into) -> Self { + self.allowed_hosts.push(host.into()); + self + } + + pub fn max_redirects(mut self, n: usize) -> Self { + self.max_redirects = n; + self + } + pub fn build(self) -> Markkit { Markkit::new(self) } diff --git a/crates/markkit/src/error.rs b/crates/markkit/src/error.rs index c291664..0474b18 100644 --- a/crates/markkit/src/error.rs +++ b/crates/markkit/src/error.rs @@ -16,6 +16,11 @@ pub enum Error { #[error("HTTP error: {0}")] Http(String), + /// `extract_url` refused a URL: disallowed scheme, host outside the + /// allowlist, or an address on a non-public network. + #[error("URL blocked: {0}")] + UrlBlocked(String), + #[error("Encoding error: {0}")] Encoding(String), diff --git a/crates/markkit/src/extract.rs b/crates/markkit/src/extract.rs index 2d84a9d..06ce32b 100644 --- a/crates/markkit/src/extract.rs +++ b/crates/markkit/src/extract.rs @@ -235,25 +235,20 @@ impl Markkit { )) } - /// Extract markdown from a URL by fetching and converting. + /// Fetch a URL and extract markdown from the response. + /// + /// Treats the URL as untrusted (threat model T3): only `http`/`https`; + /// loopback, private, link-local and other non-public addresses are + /// refused with [`Error::UrlBlocked`] on every hop (including after DNS + /// resolution) unless [`MarkkitBuilder::allow_private_networks`] is set; + /// [`MarkkitBuilder::allow_host`] restricts hosts; redirects capped by + /// `max_redirects`; body capped by `max_input_size`; whole fetch capped + /// by `max_processing_secs` ([`Error::ResourceLimit`]). #[cfg(feature = "http_client")] pub async fn extract_url(&self, url: &str) -> crate::Result { - let response = reqwest::get(url) + let fetched = crate::fetch::fetch(url, &self.config).await?; + self.extract_bytes(&fetched.bytes, fetched.content_type.as_deref()) .await - .map_err(|e| Error::Http(e.to_string()))?; - - let content_type = response - .headers() - .get("content-type") - .and_then(|v| v.to_str().ok()) - .map(|s| s.to_string()); - - let bytes = response - .bytes() - .await - .map_err(|e| Error::Http(e.to_string()))?; - - self.extract_bytes(&bytes, content_type.as_deref()).await } /// PDF extraction is not implemented yet; report it explicitly rather than diff --git a/crates/markkit/src/fetch.rs b/crates/markkit/src/fetch.rs new file mode 100644 index 0000000..bc4a127 --- /dev/null +++ b/crates/markkit/src/fetch.rs @@ -0,0 +1,336 @@ +//! Hardened HTTP fetch behind [`Markkit::extract_url`](crate::Markkit::extract_url) +//! (threat model T3). +//! +//! Decisions: +//! - Default-deny non-public destinations. Three layers: (1) URL check before +//! any I/O (scheme, allowlist, IP literal, `localhost` names), (2) the same +//! check on every redirect hop, (3) a DNS resolver that drops non-public +//! addresses, so a public name resolving to 127.0.0.1 / 169.254.169.254 +//! (DNS rebinding) never connects. Layer 3 pins the addresses reqwest +//! connects to, so there is no check-then-resolve gap. +//! - With an HTTP(S) proxy from the environment, the proxy resolves names; +//! layers 1-2 still apply, layer 3 cannot. Documented in the threat model. +//! - Body bounded by `max_input_size` while streaming (declared +//! `Content-Length` rejected up front); whole fetch bounded by +//! `max_processing_secs`. +//! - Policy and resolver run inside reqwest and can only return opaque +//! errors, so they record the block reason in a per-call slot that is +//! turned into [`Error::UrlBlocked`] afterwards. + +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use reqwest::Url; +use reqwest::dns::{Addrs, Name, Resolve, Resolving}; + +use crate::config::MarkkitBuilder; +use crate::error::Error; + +/// Fetched body plus its declared content type. +pub(crate) struct Fetched { + pub bytes: Vec, + pub content_type: Option, +} + +/// Per-call slot for the first block reason seen inside reqwest. +type Blocked = Arc>>; + +fn record(slot: &Blocked, reason: String) -> String { + let mut s = slot.lock().unwrap_or_else(|e| e.into_inner()); + s.get_or_insert(reason).clone() +} + +pub(crate) async fn fetch(url: &str, cfg: &MarkkitBuilder) -> crate::Result { + let parsed = Url::parse(url).map_err(|e| Error::UrlBlocked(format!("invalid URL: {e}")))?; + check_url(&parsed, cfg).map_err(Error::UrlBlocked)?; + + let blocked: Blocked = Arc::default(); + let client = client(cfg, &blocked)?; + let limit = cfg.max_input_size; + + let run = async { + let mut response = client.get(parsed).send().await?; + if let Some(len) = response.content_length() + && len > limit as u64 + { + return Ok(Err(Error::ResourceLimit(format!( + "response Content-Length {len} exceeds max_input_size {limit}" + )))); + } + let content_type = response + .headers() + .get(reqwest::header::CONTENT_TYPE) + .and_then(|v| v.to_str().ok()) + .map(str::to_string); + let mut bytes = Vec::new(); + while let Some(chunk) = response.chunk().await? { + if bytes.len() + chunk.len() > limit { + return Ok(Err(Error::ResourceLimit(format!( + "response body exceeds max_input_size {limit}" + )))); + } + bytes.extend_from_slice(&chunk); + } + Ok(Ok(Fetched { + bytes, + content_type, + })) + }; + + let result: Result, reqwest::Error> = run.await; + match result { + Ok(r) => r, + Err(e) => { + if let Some(reason) = blocked.lock().unwrap_or_else(|p| p.into_inner()).take() { + return Err(Error::UrlBlocked(reason)); + } + if e.is_timeout() { + return Err(Error::ResourceLimit(format!( + "fetch exceeded max_processing_secs {}", + cfg.max_processing_secs + ))); + } + Err(Error::Http(e.to_string())) + } + } +} + +fn client(cfg: &MarkkitBuilder, blocked: &Blocked) -> crate::Result { + let max_redirects = cfg.max_redirects; + let policy_cfg = cfg.clone(); + let policy_slot = blocked.clone(); + let policy = reqwest::redirect::Policy::custom(move |attempt| { + if attempt.previous().len() > max_redirects { + let reason = format!("too many redirects (max {max_redirects})"); + return attempt.error(record(&policy_slot, reason)); + } + match check_url(attempt.url(), &policy_cfg) { + Ok(()) => attempt.follow(), + Err(reason) => { + let reason = record(&policy_slot, format!("redirect: {reason}")); + attempt.error(reason) + } + } + }); + + let secs = cfg.max_processing_secs.max(1); + let mut builder = reqwest::Client::builder() + .redirect(policy) + .timeout(Duration::from_secs(secs)) + .connect_timeout(Duration::from_secs(secs.min(10))); + if !cfg.allow_private_networks { + builder = builder.dns_resolver(Arc::new(PublicOnlyResolver { + blocked: blocked.clone(), + })); + } + builder + .build() + .map_err(|e| Error::Http(format!("client setup: {e}"))) +} + +/// Check scheme, allowlist and (unless private networks are allowed) IP +/// literals / `localhost` names. Pure: no DNS, no I/O. +fn check_url(url: &Url, cfg: &MarkkitBuilder) -> Result<(), String> { + match url.scheme() { + "http" | "https" => {} + s => return Err(format!("scheme '{s}' not allowed (http/https only)")), + } + let host = url + .host_str() + .ok_or_else(|| "URL has no host".to_string())?; + let host = normalize_host(host); + + if !cfg.allowed_hosts.is_empty() && !cfg.allowed_hosts.iter().any(|h| normalize_host(h) == host) + { + return Err(format!("host '{host}' not in allowed hosts")); + } + + if cfg.allow_private_networks { + return Ok(()); + } + if host == "localhost" || host.ends_with(".localhost") { + return Err(format!("host '{host}' is a loopback name")); + } + if let Ok(ip) = host.parse::() + && is_forbidden_ip(ip) + { + return Err(format!("address {ip} is not public")); + } + Ok(()) +} + +/// Lowercase, strip IPv6 brackets and a trailing root dot. +fn normalize_host(host: &str) -> String { + let h = host.trim_start_matches('[').trim_end_matches(']'); + h.strip_suffix('.').unwrap_or(h).to_ascii_lowercase() +} + +/// True for any address that is not globally routable unicast. +pub(crate) fn is_forbidden_ip(ip: IpAddr) -> bool { + match ip { + IpAddr::V4(v4) => is_forbidden_v4(v4), + IpAddr::V6(v6) => is_forbidden_v6(v6), + } +} + +fn is_forbidden_v4(ip: Ipv4Addr) -> bool { + let [a, b, c, _] = ip.octets(); + ip.is_unspecified() + || ip.is_loopback() + || ip.is_private() + || ip.is_link_local() + || ip.is_broadcast() + || ip.is_documentation() + || ip.is_multicast() + || a == 0 // 0.0.0.0/8 "this network" + || (a == 100 && (64..128).contains(&b)) // 100.64.0.0/10 CGNAT + || (a == 192 && b == 0 && c == 0) // 192.0.0.0/24 IETF protocol + || (a == 198 && (b == 18 || b == 19)) // 198.18.0.0/15 benchmarking + || a >= 240 // 240.0.0.0/4 reserved +} + +fn is_forbidden_v6(ip: Ipv6Addr) -> bool { + let seg = ip.segments(); + // Addresses that embed an IPv4 address are judged by it. + if let Some(v4) = ip.to_ipv4_mapped() { + return is_forbidden_v4(v4); + } + let embedded = |hi: u16, lo: u16| Ipv4Addr::from(((hi as u32) << 16) | lo as u32); + if seg[..6] == [0; 6] && !(ip.is_loopback() || ip.is_unspecified()) { + return is_forbidden_v4(embedded(seg[6], seg[7])); // ::a.b.c.d (deprecated compat) + } + if seg[..6] == [0x64, 0xff9b, 0, 0, 0, 0] { + return is_forbidden_v4(embedded(seg[6], seg[7])); // NAT64 64:ff9b::/96 + } + if seg[0] == 0x2002 { + return is_forbidden_v4(embedded(seg[1], seg[2])); // 6to4 2002::/16 + } + ip.is_loopback() + || ip.is_unspecified() + || ip.is_multicast() + || (seg[0] & 0xfe00) == 0xfc00 // fc00::/7 unique local + || (seg[0] & 0xffc0) == 0xfe80 // fe80::/10 link-local + || (seg[0] & 0xffc0) == 0xfec0 // fec0::/10 site-local (deprecated) + || seg[0] == 0x2001 && seg[1] == 0x0db8 // 2001:db8::/32 documentation +} + +/// Resolver that only hands reqwest public addresses. +struct PublicOnlyResolver { + blocked: Blocked, +} + +impl Resolve for PublicOnlyResolver { + fn resolve(&self, name: Name) -> Resolving { + let blocked = self.blocked.clone(); + let host = name.as_str().to_string(); + Box::pin(async move { + let addrs: Vec = + tokio::net::lookup_host((host.as_str(), 0)).await?.collect(); + let public: Vec = addrs + .iter() + .copied() + .filter(|a| !is_forbidden_ip(a.ip())) + .collect(); + if public.is_empty() { + let reason = if addrs.is_empty() { + format!("host '{host}' did not resolve") + } else { + format!("host '{host}' resolves only to non-public addresses") + }; + return Err(record(&blocked, reason).into()); + } + let addrs: Addrs = Box::new(public.into_iter()); + Ok(addrs) + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn forbidden(s: &str) -> bool { + is_forbidden_ip(s.parse().unwrap()) + } + + #[test] + fn non_public_addresses_forbidden() { + for ip in [ + "127.0.0.1", + "10.1.2.3", + "172.31.255.255", + "192.168.0.1", + "169.254.169.254", + "100.64.0.1", + "0.1.2.3", + "192.0.0.1", + "198.18.0.1", + "224.0.0.1", + "255.255.255.255", + "240.0.0.1", + "::1", + "::", + "::ffff:10.0.0.1", + "::127.0.0.1", + "64:ff9b::a9fe:a9fe", // NAT64 of 169.254.169.254 + "2002:7f00:1::", // 6to4 of 127.0.0.1 + "fd12::1", + "fe80::1", + "ff02::1", + "2001:db8::1", + ] { + assert!(forbidden(ip), "{ip} should be forbidden"); + } + } + + #[test] + fn public_addresses_allowed() { + for ip in [ + "93.184.216.34", + "8.8.8.8", + "100.128.0.1", + "172.32.0.1", + "2606:4700::1111", + "::ffff:8.8.8.8", + "64:ff9b::808:808", + "2002:808:808::", + ] { + assert!(!forbidden(ip), "{ip} should be allowed"); + } + } + + fn check(url: &str, cfg: &MarkkitBuilder) -> Result<(), String> { + check_url(&Url::parse(url).unwrap(), cfg) + } + + #[test] + fn check_url_default_policy() { + let cfg = MarkkitBuilder::default(); + assert!(check("https://example.com/a", &cfg).is_ok()); + assert!(check("http://8.8.8.8/", &cfg).is_ok()); + assert!(check("http://localhost/", &cfg).is_err()); + assert!(check("http://a.localhost./", &cfg).is_err()); + assert!(check("http://[::1]:8080/", &cfg).is_err()); + assert!(check("data:text/html,hi", &cfg).is_err()); + } + + #[test] + fn check_url_allowlist() { + let cfg = MarkkitBuilder::default().allow_host("Example.COM."); + assert!(check("https://example.com/", &cfg).is_ok()); + assert!(check("https://EXAMPLE.com./x", &cfg).is_ok()); + assert!(check("https://sub.example.com/", &cfg).is_err()); + assert!(check("https://example.com.evil.net/", &cfg).is_err()); + } + + #[test] + fn check_url_private_opt_in_keeps_scheme_and_allowlist() { + let cfg = MarkkitBuilder::default() + .allow_private_networks(true) + .allow_host("127.0.0.1"); + assert!(check("http://127.0.0.1:9/", &cfg).is_ok()); + assert!(check("http://10.0.0.1/", &cfg).is_err()); + assert!(check("file:///etc/passwd", &cfg).is_err()); + } +} diff --git a/crates/markkit/src/lib.rs b/crates/markkit/src/lib.rs index 3252a3d..d6453e2 100644 --- a/crates/markkit/src/lib.rs +++ b/crates/markkit/src/lib.rs @@ -47,6 +47,8 @@ pub mod config; mod docx; pub mod error; pub mod extract; +#[cfg(feature = "http_client")] +mod fetch; pub mod format; #[cfg(feature = "html")] pub mod html; diff --git a/crates/markkit/tests/url_security.rs b/crates/markkit/tests/url_security.rs new file mode 100644 index 0000000..a7657d4 --- /dev/null +++ b/crates/markkit/tests/url_security.rs @@ -0,0 +1,262 @@ +//! Security tests for `extract_url` (threat model T3 SSRF, T2/T4 via remote bodies). +//! +//! A raw TCP server on loopback stands in for the remote host. Loopback is +//! itself a forbidden destination by default, which the first tests assert; +//! the rest opt in with `allow_private_networks(true)` to exercise limits. + +#![cfg(feature = "http_client")] + +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::time::{Duration, Instant}; + +use markkit::Markkit; +use markkit::error::Error; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::net::TcpListener; + +/// Serve `respond(path)` to every connection; returns base URL and a hit counter. +async fn serve(respond: F) -> (String, Arc) +where + F: Fn(&str) -> Vec + Send + Sync + 'static, +{ + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let hits = Arc::new(AtomicUsize::new(0)); + let counter = hits.clone(); + let respond = Arc::new(respond); + tokio::spawn(async move { + loop { + let Ok((mut sock, _)) = listener.accept().await else { + return; + }; + counter.fetch_add(1, Ordering::SeqCst); + let respond = respond.clone(); + tokio::spawn(async move { + let mut buf = vec![0u8; 4096]; + let n = sock.read(&mut buf).await.unwrap_or(0); + let req = String::from_utf8_lossy(&buf[..n]); + let path = req.split_whitespace().nth(1).unwrap_or("/").to_string(); + let _ = sock.write_all(&respond(&path)).await; + let _ = sock.shutdown().await; + }); + } + }); + (format!("http://{addr}"), hits) +} + +fn html(body: &str) -> Vec { + format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ) + .into_bytes() +} + +fn redirect(to: &str) -> Vec { + format!( + "HTTP/1.1 302 Found\r\nLocation: {to}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + ) + .into_bytes() +} + +fn private_ok() -> Markkit { + Markkit::builder().allow_private_networks(true).build() +} + +fn assert_blocked(r: markkit::Result) { + match r { + Err(Error::UrlBlocked(_)) => {} + other => panic!("expected UrlBlocked, got {other:?}"), + } +} + +// ─── Destination checks (default policy) ───────────────────────────── + +#[tokio::test] +async fn loopback_ip_blocked_by_default_without_connecting() { + let (base, hits) = serve(|_| html("

secret

")).await; + assert_blocked(Markkit::builder().build().extract_url(&base).await); + assert_eq!( + hits.load(Ordering::SeqCst), + 0, + "no request may reach the host" + ); +} + +#[tokio::test] +async fn localhost_name_blocked_by_default_without_connecting() { + let (base, hits) = serve(|_| html("

secret

")).await; + let url = base.replace("127.0.0.1", "localhost"); + assert_blocked(Markkit::builder().build().extract_url(&url).await); + assert_eq!(hits.load(Ordering::SeqCst), 0); +} + +#[tokio::test] +async fn forbidden_ip_literals_blocked() { + let m = Markkit::builder().build(); + for url in [ + "http://169.254.169.254/latest/meta-data/", + "http://10.0.0.1/", + "http://192.168.1.1/", + "http://172.16.0.1/", + "http://100.64.0.1/", + "http://0.0.0.0/", + "http://[::1]/", + "http://[::ffff:127.0.0.1]/", + "http://[fd00::1]/", + "http://[fe80::1]/", + "http://2130706433/", // 127.0.0.1 as a decimal integer + "http://0x7f.0.0.1/", + ] { + assert_blocked(m.extract_url(url).await); + } +} + +#[tokio::test] +async fn non_http_schemes_blocked() { + let m = private_ok(); + for url in [ + "file:///etc/passwd", + "ftp://example.com/", + "gopher://127.0.0.1/", + ] { + assert_blocked(m.extract_url(url).await); + } +} + +#[tokio::test] +async fn invalid_url_is_error_not_panic() { + assert!( + Markkit::builder() + .build() + .extract_url("not a url") + .await + .is_err() + ); +} + +// ─── Opt-in private access ────────────────────────────────────────── + +#[tokio::test] +async fn private_networks_reachable_when_allowed() { + let (base, _) = serve(|_| html("

Hi

")).await; + assert_eq!(private_ok().extract_url(&base).await.unwrap(), "# Hi\n"); +} + +// ─── Host allowlist ───────────────────────────────────────────────── + +#[tokio::test] +async fn host_outside_allowlist_blocked() { + let (base, hits) = serve(|_| html("

x

")).await; + let m = Markkit::builder() + .allow_private_networks(true) + .allow_host("example.com") + .build(); + assert_blocked(m.extract_url(&base).await); + assert_eq!(hits.load(Ordering::SeqCst), 0); +} + +#[tokio::test] +async fn redirect_to_host_outside_allowlist_blocked() { + let (base, _) = serve(|path| { + if path == "/" { + redirect("http://localhost:9/") // different host than 127.0.0.1 + } else { + html("

x

") + } + }) + .await; + let m = Markkit::builder() + .allow_private_networks(true) + .allow_host("127.0.0.1") + .build(); + assert_blocked(m.extract_url(&base).await); +} + +#[tokio::test] +async fn allowlisted_host_matches_case_insensitively() { + let (base, _) = serve(|_| html("

ok

")).await; + let url = base.replace("127.0.0.1", "LOCALHOST"); + let m = Markkit::builder() + .allow_private_networks(true) + .allow_host("localhost") + .build(); + assert_eq!(m.extract_url(&url).await.unwrap(), "ok\n"); +} + +// ─── Redirects ────────────────────────────────────────────────────── + +#[tokio::test] +async fn redirect_loop_stops_at_limit() { + let (base, hits) = serve(|_| redirect("/again")).await; + let m = Markkit::builder() + .allow_private_networks(true) + .max_redirects(2) + .build(); + assert!(m.extract_url(&base).await.is_err()); + assert!(hits.load(Ordering::SeqCst) <= 3, "followed past the limit"); +} + +#[tokio::test] +async fn redirect_to_non_http_scheme_blocked() { + let (base, _) = serve(|_| redirect("file:///etc/passwd")).await; + assert!(private_ok().extract_url(&base).await.is_err()); +} + +// ─── Body size and time limits ────────────────────────────────────── + +#[tokio::test] +async fn declared_oversized_body_rejected() { + let (base, _) = serve(|_| { + b"HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nContent-Length: 100000000\r\nConnection: close\r\n\r\n

".to_vec() + }) + .await; + let m = Markkit::builder() + .allow_private_networks(true) + .max_input_size(1024) + .build(); + match m.extract_url(&base).await { + Err(Error::ResourceLimit(_)) => {} + other => panic!("expected ResourceLimit, got {other:?}"), + } +} + +#[tokio::test] +async fn undeclared_oversized_body_rejected_while_streaming() { + let (base, _) = serve(|_| { + let mut r = + b"HTTP/1.1 200 OK\r\nContent-Type: text/html\r\nConnection: close\r\n\r\n".to_vec(); + r.extend(std::iter::repeat_n(b'a', 64 * 1024)); + r + }) + .await; + let m = Markkit::builder() + .allow_private_networks(true) + .max_input_size(1024) + .build(); + match m.extract_url(&base).await { + Err(Error::ResourceLimit(_)) => {} + other => panic!("expected ResourceLimit, got {other:?}"), + } +} + +#[tokio::test] +async fn stalled_server_times_out() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + let (_sock, _) = listener.accept().await.unwrap(); + tokio::time::sleep(Duration::from_secs(30)).await; // never answer + }); + let m = Markkit::builder() + .allow_private_networks(true) + .max_processing_secs(1) + .build(); + let start = Instant::now(); + assert!(m.extract_url(&format!("http://{addr}/")).await.is_err()); + assert!( + start.elapsed() < Duration::from_secs(10), + "timeout not enforced" + ); +} diff --git a/knowledge/foundations/architecture.md b/knowledge/foundations/architecture.md index 1451347..9781979 100644 --- a/knowledge/foundations/architecture.md +++ b/knowledge/foundations/architecture.md @@ -53,6 +53,7 @@ Via `MarkkitBuilder`: - CSS selectors for include/exclude - Link and image preservation - Office-document limits (archive entries/sizes, sheet rows/cols, grid cells, hidden sheets) +- URL fetch policy (`allow_private_networks`, `allow_host`, `max_redirects`) ## See also diff --git a/knowledge/log.md b/knowledge/log.md index 1cb075e..1b2ff07 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -2,6 +2,7 @@ ## 2026-10-26 +* **URL fetch hardening**: T3 SSRF is mitigated. `extract_url` blocks non-public addresses by default (URL, resolver and redirect layers), enforces scheme, allowlist, redirect, body and time limits; new invariant S12 and `url_security` tests. Proxy-resolved names are the documented residual. See [Threat Model](security/threat-model.md) and [Security Model](security/security-model.md). * **Dependency trim**: unused dependencies removed (lockfile shrinks by ~460 lines; the `instant`/`paste` unmaintained advisories and their `deny.toml` ignores are gone with the stub bench deps). quick-xml moves to 0.42, one version shared with calamine; markkit's XML walkers now read `&str` names and values. See [Threat Model](security/threat-model.md) T6d and [Maintenance](operations/maintenance.md). * **Inter-block whitespace**: indented HTML lost blank lines between blocks and leaked a leading space into every following line, because whitespace-only text between blocks stayed in the block list as `Text(" ")`. The parser now normalizes block-context lists; golden fixtures, which had recorded the broken output, are regenerated. See [HTML Extraction](formats/html-extraction.md) and [IR Specification](foundations/ir.md). * **OKF migration**: `specs/` became this OKF v0.2 bundle, grouped into `foundations/`, `formats/`, `security/`, `runtimes/`, `operations/`. Every concept gained frontmatter and a See also section; `just check-okf` and CI enforce conformance. See [Knowledge Maintenance Contract](knowledge-contract.md). diff --git a/knowledge/operations/testing.md b/knowledge/operations/testing.md index e639d7f..49bdfba 100644 --- a/knowledge/operations/testing.md +++ b/knowledge/operations/testing.md @@ -52,7 +52,7 @@ each security test states exactly which bytes it feeds the parser. `cargo audit` + `cargo deny`, `cargo test --workspace` (default and all features), the `docx,spreadsheet` no-HTTP build with a reqwest leak check, and every example. `nightly.yml` repeats build + test on nightly Rust daily; `coverage.yml` uploads -tarpaulin coverage. All must pass before merge. +tarpaulin coverage (`--engine llvm`; ptrace segfaults on threaded socket tests). All must pass before merge. ## See also diff --git a/knowledge/security/security-model.md b/knowledge/security/security-model.md index ffdbc78..a5086a6 100644 --- a/knowledge/security/security-model.md +++ b/knowledge/security/security-model.md @@ -24,8 +24,9 @@ Markkit processes untrusted input from the internet. Key threats: - **Mitigation**: Nesting depth limits ### TM-SSRF: Server-side request forgery (URL fetching) -- **Status**: open, mitigations planned (see [Threat Model](threat-model.md) T3) -- **Planned**: URL allowlist, private IP range blocking, redirect limit, bounded response body +- **Mitigation**: Non-public addresses refused by default, after DNS resolution and on every redirect (opt in with `allow_private_networks`) +- **Mitigation**: http/https only, optional host allowlist, redirect limit, body bounded by `max_input_size`, fetch bounded by `max_processing_secs` +- **Residual**: an environment proxy resolves names itself; see [Threat Model](threat-model.md) T3 ### TM-DOC: Malicious office documents - **Mitigation**: Bounded ZIP decompression (entry count, per-entry and total size) diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 9d906ac..49d78f1 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -95,20 +95,44 @@ tags: **Likelihood**: Medium -- `extract_url()` exists whenever `http_client` is enabled, and `http_client` is a **default** feature. -**Status**: Open. `extract_url()` calls `reqwest::get` directly: reqwest's default -redirect policy (10 hops) applies, nothing else does. +**Status**: Mitigated (`crates/markkit/src/fetch.rs`). **Mitigations**: -- URL allowlist support (planned) -- Private IP range blocking (planned) -- Response body bounded by `max_input_size` before download completes (planned; - today `max_input_size` is checked only after the full body is buffered, see T4) -- Request timeout from `max_processing_secs` (planned) +- Scheme allowlist: `http`/`https` only, initial URL and every redirect hop +- Non-public destinations refused by default (`Error::UrlBlocked`): loopback, + RFC 1918, link-local / cloud metadata (169.254.0.0/16), CGNAT, `0.0.0.0/8`, + reserved, multicast, documentation, benchmarking; IPv6 loopback, ULA, + link-local, site-local, multicast; IPv4 embedded in IPv6 (mapped, + compatible, NAT64, 6to4) judged by the IPv4. `localhost` / `*.localhost` + refused by name. Non-canonical IPv4 (`2130706433`, `0x7f.0.0.1`) is + normalized by the URL parser before the check +- DNS rebinding: a custom resolver drops non-public addresses, and reqwest + connects only to what it returns (no check-then-resolve gap) +- Opt-in `allow_private_networks(true)` for trusted URLs (CLI / bindings use + the default, so they block too) +- Host allowlist `allow_host(..)`: exact, case-insensitive, trailing dot + ignored, checked on every hop (no subdomain wildcard) +- Redirects capped by `max_redirects` (default 5) +- Response body bounded by `max_input_size`: declared `Content-Length` rejected + before reading, undeclared bodies stopped while streaming (`ResourceLimit`) +- Whole fetch (connect, headers, body) bounded by `max_processing_secs` - Opt-out: embedders that fetch content themselves build with `default-features = false` and never link reqwest (CI asserts this for `docx,spreadsheet`) -**Test Coverage**: None yet. +**Residual risk**: with an `HTTP(S)_PROXY` in the environment the proxy +resolves names, so the resolver filter does not apply to proxied requests; +scheme, allowlist, IP-literal and redirect checks still do. Restrict egress at +the proxy, or use `allow_host`. + +**Test Coverage**: +- `url_security::loopback_ip_blocked_by_default_without_connecting`, `localhost_name_blocked_by_default_without_connecting` (no TCP hit) +- `url_security::forbidden_ip_literals_blocked` (metadata, RFC 1918, CGNAT, IPv6, mapped, decimal / hex IPv4) +- `url_security::non_http_schemes_blocked`, `redirect_to_non_http_scheme_blocked`, `invalid_url_is_error_not_panic` +- `url_security::host_outside_allowlist_blocked`, `redirect_to_host_outside_allowlist_blocked`, `allowlisted_host_matches_case_insensitively` +- `url_security::redirect_loop_stops_at_limit`, `private_networks_reachable_when_allowed` +- `url_security::declared_oversized_body_rejected`, `undeclared_oversized_body_rejected_while_streaming`, `stalled_server_times_out` +- `fetch::tests::*` (address classification and URL check units) ### T4: Memory Exhaustion @@ -212,7 +236,9 @@ process, or smuggling content past a human reviewer. | Resource | Default | Configurable | Enforcement Point | |----------|---------|--------------|-------------------| | Input size | 50 MB | `max_input_size(usize)` | Before parsing | -| Processing time | 60 s | `max_processing_secs(u64)` | Config field (enforcement TBD) | +| Processing time | 60 s | `max_processing_secs(u64)` | Whole `extract_url` fetch (parsing itself TBD) | +| Redirects (`extract_url`) | 5 | `max_redirects(usize)` | Every redirect hop | +| Fetched body (`extract_url`) | = input size | `max_input_size(usize)` | `Content-Length` and while streaming | | Nesting depth | 200 | Hardcoded `MAX_DEPTH` | During HTML tree walk | | XML nesting depth (DOCX) | 256 | Hardcoded `MAX_XML_DEPTH` | While building the XML tree | | ZIP entries | 10 000 | `max_archive_entries(usize)` | On archive open | @@ -258,6 +284,7 @@ process, or smuggling content past a human reviewer. | S9 | Malformed office documents produce errors, never panics | `document_security::truncated_fixtures_never_panic`, `*_never_panic` proptests | | S10 | Spreadsheet parsing never allocates a grid beyond `max_spreadsheet_cells` | `document_security::xls_*_rejected`, `ods_repeated_rows_and_columns_rejected` | | S11 | Unsupported formats fail explicitly (no empty success) | `document_detection::pdf_is_explicitly_unsupported_not_empty` | +| S12 | Default `extract_url` never connects to a non-public address | `url_security::*_blocked*`, `fetch::tests::non_public_addresses_forbidden` | ## See also