From a707004765e402ab5630521cbcd5b9523b266d2b Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Tue, 22 Sep 2026 11:05:51 +0200 Subject: [PATCH] fix(background): canonicalize the heropattern colour before templating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cfg.color` is a free-form `String` on `HeropatternConfig` (crates/rustmotion-core/src/schema/background.rs:216, `#[serde(default = "default_hero_color")]`) with no validation anywhere — `validate_animated_bg` (crates/rustmotion/src/include.rs:267-273) checks only that the pattern *name* resolves. The template lands inside a double-quoted attribute (`fill="{{color}}"`), so a colour containing `"` closes the attribute and injects arbitrary markup into the document usvg then parses. usvg 0.46's `Options::default()` keeps the default `ImageHrefResolver`, whose string resolver I read at usvg-0.46.0/src/parser/image.rs:85-112: it calls `std::fs::read(&path)` on any href that `path.exists()` accepts — with `resources_dir: None` an absolute path resolves to itself — so an injected `` is decoded and painted into the frame. The impact is bounded (a scenario can already read a local image legitimately via `{"type":"image","src":"/abs/path.png"}`, and there is no network resolver), so this is a hardening gap rather than a privilege escalation. The quieter half is availability: any malformed splice makes `Tree::from_data` fail and line 601 `return`s, so the background silently does not render with no diagnostic at all. Refs #220 --- .../src/engine/render/background.rs | 122 +++++++++++++++++- 1 file changed, 120 insertions(+), 2 deletions(-) diff --git a/crates/rustmotion/src/engine/render/background.rs b/crates/rustmotion/src/engine/render/background.rs index ef95320e..80dd1fa3 100644 --- a/crates/rustmotion/src/engine/render/background.rs +++ b/crates/rustmotion/src/engine/render/background.rs @@ -566,6 +566,48 @@ fn draw_bg_pixel_grid( } } +/// Re-emit a scenario-supplied heropattern colour as a canonical +/// `#rrggbbaa` before it is spliced into generated SVG source. +/// +/// `cfg.color` is free-form user input landing inside a double-quoted +/// `fill="{{color}}"` attribute of hand-built SVG text; routing it through +/// `color4f_from_hex` first guarantees the only characters that can ever +/// reach the SVG are hex digits and `#`, so a colour string can never close +/// the attribute and inject markup, whatever it contains. +/// `color4f_from_hex` is infallible: unresolvable input resolves to the +/// same opaque-magenta sentinel every other unresolved colour in this +/// engine does, rather than passing the raw string through. +fn canonical_hero_color(color: &str) -> String { + let c = color4f_from_hex(color); + format!( + "#{:02X}{:02X}{:02X}{:02X}", + (c.r.clamp(0.0, 1.0) * 255.0).round() as u8, + (c.g.clamp(0.0, 1.0) * 255.0).round() as u8, + (c.b.clamp(0.0, 1.0) * 255.0).round() as u8, + (c.a.clamp(0.0, 1.0) * 255.0).round() as u8, + ) +} + +/// `usvg::Options` for parsing a generated heropattern tile. +/// +/// Neutralises the default `image_href_resolver`'s string resolver, which +/// reads arbitrary files from disk for any `` it +/// encounters (usvg-0.44.0's `ImageHrefResolver::default_string_resolver`). +/// A heropattern tile never legitimately references an external image, so +/// an `` element reaching this parser can only be an injection — +/// `canonical_hero_color` closes the splice that could put one there in the +/// first place; this is the defence-in-depth half, for any other way one +/// could arrive. +fn heropattern_svg_options() -> usvg::Options<'static> { + usvg::Options { + image_href_resolver: usvg::ImageHrefResolver { + resolve_string: Box::new(|_, _| None), + ..usvg::ImageHrefResolver::default() + }, + ..usvg::Options::default() + } +} + /// Tiled heropattern background. /// /// The tile is rasterized once at `cfg.scale`, using `heropattern_raster_size` @@ -597,12 +639,16 @@ fn draw_bg_heropattern( def.width, def.height, def.svg_paths - .replace("{{color}}", &cfg.color) + .replace("{{color}}", &canonical_hero_color(&cfg.color)) .replace("{{opacity}}", &cfg.opacity.to_string()), ); - let opt = usvg::Options::default(); + let opt = heropattern_svg_options(); let Ok(tree) = usvg::Tree::from_data(svg_content.as_bytes(), &opt) else { + eprintln!( + "warning: heropattern '{}' (colour '{}') failed to parse as SVG — background not rendered", + cfg.pattern, cfg.color + ); return; }; @@ -1587,3 +1633,75 @@ mod heropattern_raster_tests { draw_bg_heropattern(surface.canvas(), &cfg, 0.0, 64.0, 64.0); } } + +#[cfg(test)] +mod heropattern_svg_injection_tests { + //! A scenario-supplied heropattern colour used to be spliced unescaped + //! into hand-built SVG source inside a double-quoted `fill="..."` + //! attribute, then parsed by usvg with its default (file-reading) + //! `image_href_resolver`. A colour containing a `"` could close the + //! attribute and inject arbitrary markup, including an `` the default resolver would read straight off disk. + + use super::*; + + #[test] + fn a_colour_containing_a_double_quote_cannot_inject_markup() { + let payload = r#""/>"#, + ) + .expect("scratch SVG written"); + + let opt = heropattern_svg_options(); + let resolved = + (opt.image_href_resolver.resolve_string)(scratch_path.to_str().unwrap(), &opt); + + let _ = std::fs::remove_file(&scratch_path); + + assert!( + resolved.is_none(), + "the string resolver must be neutralised, not read a real SVG file from disk" + ); + } + + #[test] + fn draw_bg_heropattern_survives_an_injection_attempt_without_panicking() { + let mut surface = skia_safe::surfaces::raster_n32_premul((32, 32)).expect("surface"); + let cfg = HeropatternConfig { + pattern: "aztec".to_string(), + color: r#""/>