diff --git a/Cargo.lock b/Cargo.lock index 8e1ceb1f3..9525d604e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5477,6 +5477,7 @@ dependencies = [ "config", "cookie", "criterion", + "cssparser 0.36.0", "derive_more", "ed25519-dalek", "edgezero-core", diff --git a/Cargo.toml b/Cargo.toml index 7faba7553..d6ecdb9ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -51,6 +51,7 @@ clap = { version = "4", features = ["derive"] } config = "0.15.19" cookie = "0.18.1" criterion = { version = "0.5", default-features = false, features = ["cargo_bench_support"] } +cssparser = "0.36" derive_more = { version = "2.0", features = ["display", "error"] } directories = "5" ed25519-dalek = { version = "2.2", features = ["rand_core"] } diff --git a/crates/trusted-server-core/Cargo.toml b/crates/trusted-server-core/Cargo.toml index e44d46f77..01780dd39 100644 --- a/crates/trusted-server-core/Cargo.toml +++ b/crates/trusted-server-core/Cargo.toml @@ -19,6 +19,7 @@ bytes = { workspace = true } chacha20poly1305 = { workspace = true } chrono = { workspace = true } cookie = { workspace = true } +cssparser = { workspace = true } derive_more = { workspace = true } ed25519-dalek = { workspace = true } edgezero-core = { workspace = true } diff --git a/crates/trusted-server-core/src/creative.rs b/crates/trusted-server-core/src/creative.rs index a4d641bdf..17dcc5754 100644 --- a/crates/trusted-server-core/src/creative.rs +++ b/crates/trusted-server-core/src/creative.rs @@ -31,8 +31,8 @@ //! - `split_srcset_candidates(&str) -> Vec<&str>`: Robust splitting that supports //! commas with or without spaces and avoids splitting the mediatype/data comma //! in a leading `data:` URL. -//! - `rewrite_css_body(&Settings, &str) -> String`: Rewrites url(...) occurrences -//! inside CSS bodies. +//! - `rewrite_css_body(&Settings, &str) -> Result`: +//! Rewrites url(...) occurrences inside CSS bodies. //! //! See the tests in this module for comprehensive cases, including irregular //! spacing, no-space commas, and `data:` handling. @@ -73,64 +73,440 @@ pub(super) fn to_abs(settings: &Settings, u: &str) -> Option { Some(absolute) } -// Helper: rewrite url(...) occurrences inside a CSS style string to first-party proxy. -// `base_origin` is prefixed onto the proxy path — empty for root-relative output, -// `https://` for absolute output (see [`build_proxy_url`]). +/// Maximum number of nested parser scopes [`rewrite_style_urls`] will enter. +/// +/// The walk recurses into blocks, functions and `@import` preludes, and the CSS +/// it reads is supplied by the upstream creative, so an input of nothing but +/// `{` would otherwise decide how deep the stack goes — and an overflow aborts +/// the guest, turning a 2 KB style attribute into a failed response. Measured on +/// `wasm32-wasip1`, the walk survives 1,000 nested blocks and overflows by +/// 1,100, so this leaves room for a stack an order of magnitude smaller than +/// the one measured. It is still far above any real stylesheet, where nesting +/// is a declaration list inside a handful of blocks. Entering this many +/// scopes costs one more stack frame than the bound, since the outermost walk +/// has entered none. +/// +/// An iterative walk would need no bound, but the tokenizer only exposes a +/// block through a closure, so each nested slice would be re-read from its +/// start — quadratic in the input, which [`MAX_REWRITABLE_BODY_SIZE`] allows +/// to be 10 MB. Recursing stays linear and bounds the stack instead. +/// +/// A scope is anything whose contents the walk reads by recursing: a block, a +/// function that may hold a value of its own (`image-set()`, `var()`), an +/// `@import` prelude. Reading the single string argument of a `url()` or +/// `src()` is not one — that grammar is terminal, so it costs no recursion and +/// is not charged a level. Without that exemption the bound would depend on +/// how a URL is spelled, admitting `url(https://…)` where it rejects the +/// identical `url("https://…")`, and rejection discards the whole stylesheet. +const MAX_CSS_NESTING_DEPTH: usize = 64; + +/// Rewrites URL references inside a CSS string to the first-party proxy. +/// +/// Covers every form the browser fetches: `url()` and `src()`, a bare string +/// candidate in `image-set()`, and an `@import` prelude string. +/// +/// `base_origin` is prefixed onto the proxy path — empty for root-relative +/// output, `https://` for absolute output (see [`build_proxy_url`]). +/// +/// Values are read with a CSS tokenizer rather than by scanning for quotes, so +/// the extent of a value comes from the grammar and escapes are already +/// resolved. That matters in both directions: a value whose escapes hide an +/// absolute URL (`url("https://t.example/\70 ixel.gif")`) is still proxied, and +/// a malformed value — which the tokenizer reports as a bad URL or bad string, +/// exactly what a browser discards — is left untouched rather than guessed at. +/// +/// A rewritten reference keeps the shape it was read in — `url()` as `url()`, +/// `src()` as `src()`, a bare string as a bare string — because the forms are +/// not interchangeable to a browser. Only the value inside is replaced, and +/// it is re-quoted, so the output is normalized in that respect rather than +/// byte-preserved. Anything not rewritten keeps its original bytes. +/// +/// A URL that only exists after custom-property substitution is out of reach: +/// `--c:"https://t.example/a.png"` used as `image-set(var(--c) 1x)` is a URL to +/// the browser, but the string and its use are separate declarations and +/// resolving one against the other is the cascade's job, not a rewriter's. The +/// inline fallback form, `image-set(var(--c, "https://t.example/a.png") 1x)`, +/// is substituted in place and is rewritten. A `url()` token in a custom +/// property is also rewritten, since it is a URL wherever it lands. +/// +/// CSS nested past [`MAX_CSS_NESTING_DEPTH`] is rejected outright (empty string +/// returned), matching [`MAX_CREATIVE_SIZE`]. The alternative — keeping the +/// rewrite of everything above the cap and passing the deeper bytes through — +/// turns the bound into a way around the rewrite: a `url()` placed below the +/// cap is never inspected and reaches the browser untouched, which is the leak +/// this exists to close. Rejecting costs the styling of CSS no real page +/// produces; passing through would cost the guarantee. +/// +/// This entry point serves markup, where the stylesheet is one part of a +/// document the rest of which is still rewritten, so a rejection drops that +/// part and is reported in the log. A whole CSS response has no such +/// remainder — see [`rewrite_css_body`], which reports the rejection to its +/// caller so the response carries it. pub(super) fn rewrite_style_urls(settings: &Settings, style: &str, base_origin: &str) -> String { - // naive url(...) rewrite for absolute/protocol-relative URLs - let lower = style.to_ascii_lowercase(); - let mut out = String::with_capacity(style.len() + 16); - let mut write_pos = 0_usize; - let mut scan = 0_usize; - while let Some(off) = lower[scan..].find("url(") { - let start = scan + off; - let open = start + 4; // after 'url(' - // write prefix including 'url(' - out.push_str(&style[write_pos..open]); - // find closing ')' - let close = if let Some(c) = lower[open..].find(')') { - open + c - } else { - out.push_str(&style[open..]); - return out; - }; - // trim spaces and quotes - let bytes = style.as_bytes(); - let mut s = open; - while s < close && bytes[s].is_ascii_whitespace() { - s += 1; + drop_if_rejected( + rewrite_style_urls_in_context(settings, style, base_origin, true), + "