Skip to content

fix(background): canonicalize the heropattern colour before templating - #250

Merged
LeadcodeDev merged 1 commit into
chantier/audit-2026-09from
fix/heropattern-colour-escape
Sep 22, 2026
Merged

LeadcodeDev merged 1 commit into
chantier/audit-2026-09from
fix/heropattern-colour-escape

Conversation

@LeadcodeDev

Copy link
Copy Markdown
Owner

Severity Low, category security. Location: crates/rustmotion/src/engine/render/background.rs:595

Impact

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 <image href="/absolute/path.png"> 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 returns, so the background silently does not render with no diagnostic at all.

Fix

XML-escape cfg.color before substitution (at minimum reject or strip ", <, &), or better, parse it with the existing rustmotion_core::engine::renderer::color4f_from_hex / paint_from_hex and re-emit a canonical #rrggbb. Set usvg::Options { image_href_resolver: ImageHrefResolver { resolve_string: Box::new(|_, _| None), .. }, .. } since a heropattern never legitimately references an external image. Replace the silent return at 601-603 with a warning naming the pattern and the colour.

Evidence the audit read

let svg_content = format!(
        r#"<svg xmlns="http://www.w3.org/2000/svg" width="{}" height="{}" viewBox="0 0 {} {}">{}</svg>"#,
        def.width, def.height, def.width, def.height,
        def.svg_paths
            .replace("{{color}}", &cfg.color)
            .replace("{{opacity}}", &cfg.opacity.to_string()),
    );

    let opt = usvg::Options::default();
    let Ok(tree) = usvg::Tree::from_data(svg_content.as_bytes(), &opt) else {
        return;
    };

Stacked on refactor/decorative-paint-path, which carries the previous finding of this workstream. GitHub shows only this finding's diff; merge in order.

Part of the September 2026 audit remediation chantier. Refs #220 (RM-45).

@LeadcodeDev LeadcodeDev added the bug Something isn't working label Sep 21, 2026
@LeadcodeDev LeadcodeDev self-assigned this Sep 21, 2026
@LeadcodeDev
LeadcodeDev force-pushed the refactor/decorative-paint-path branch from 3dcfc14 to afbca7b Compare September 22, 2026 06:10
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-colour-escape branch from 11ed92d to 14f8d7e Compare September 22, 2026 06:11
@LeadcodeDev
LeadcodeDev force-pushed the refactor/decorative-paint-path branch from afbca7b to 6a2f28e Compare September 22, 2026 08:35
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-colour-escape branch from 14f8d7e to 3163f12 Compare September 22, 2026 08:35
@LeadcodeDev
LeadcodeDev force-pushed the refactor/decorative-paint-path branch from 6a2f28e to e5699b7 Compare September 22, 2026 08:45
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-colour-escape branch from 3163f12 to c760058 Compare September 22, 2026 08:45
@LeadcodeDev
LeadcodeDev changed the base branch from refactor/decorative-paint-path to chantier/audit-2026-09 September 22, 2026 08:53
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-colour-escape branch from c760058 to 1f3ac28 Compare September 22, 2026 09:01
`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 `<image href="/absolute/path.png">` 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
@LeadcodeDev
LeadcodeDev force-pushed the fix/heropattern-colour-escape branch from 1f3ac28 to a707004 Compare September 22, 2026 09:05
@LeadcodeDev
LeadcodeDev merged commit 477618f into chantier/audit-2026-09 Sep 22, 2026
LeadcodeDev added a commit that referenced this pull request Sep 22, 2026
#250)

`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 `<image href="/absolute/path.png">` 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant