From 98d11ad66b832f8d8c60519613675a74e2f403d1 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Fri, 23 Oct 2026 16:41:28 -0400 Subject: [PATCH] Audit: cap SVG entity expansion, filter layer stacks and export id/companion probing; share compound-path appearances; escape-proof font feature tags --- crates/svg/src/css.rs | 7 ++- crates/svg/src/entities.rs | 92 +++++++++++++++++++++++++++++++++++++ crates/svg/src/export.rs | 21 ++++++--- crates/svg/src/glyph.rs | 4 ++ crates/svg/src/import.rs | 7 ++- crates/svg/src/import/fx.rs | 15 ++++-- crates/svg/src/lib.rs | 3 +- 7 files changed, 136 insertions(+), 13 deletions(-) create mode 100644 crates/svg/src/entities.rs diff --git a/crates/svg/src/css.rs b/crates/svg/src/css.rs index 52f3ab563..178528887 100644 --- a/crates/svg/src/css.rs +++ b/crates/svg/src/css.rs @@ -152,9 +152,12 @@ pub(crate) fn type_props(st: &CharStyle, len: &dyn Fn(f64) -> String) -> Props { .features .iter() .map(|t| match t.strip_prefix('-') { - Some(off) => format!("\"{off}\" 0"), - None => format!("\"{t}\" 1"), + Some(off) => (off, 0), + None => (t.as_str(), 1), }) + // A feature tag is four letters or digits: anything else could end the CSS string. + .filter(|(tag, _)| tag.len() == 4 && tag.bytes().all(|b| b.is_ascii_alphanumeric())) + .map(|(tag, on)| format!("\"{tag}\" {on}")) .collect(); // Proportional Metrics: `palt` (SVG writes vertical type as outlines). if st.proportional_metrics { diff --git a/crates/svg/src/entities.rs b/crates/svg/src/entities.rs new file mode 100644 index 000000000..6f2a814dc --- /dev/null +++ b/crates/svg/src/entities.rs @@ -0,0 +1,92 @@ +//! A cap on what the entities of an SVG's document type declaration expand to. The XML reader +//! replaces every `&name;` with the entity's text as it reads, with no total limit: a short file +//! that repeats a large entity in an attribute allocates far more than it holds. + +use std::collections::HashMap; + +/// The most text the references to entities may expand to in all (bytes). +const MAX_EXPANDED: usize = 16 << 20; + +/// How deep entities that refer to entities are followed (the reader's own limit is lower). +const MAX_NESTING: u32 = 16; + +/// The internal general entities declared in `svg`: name → value. +fn declared(svg: &str) -> HashMap<&str, &str> { + let mut map = HashMap::new(); + for (at, _) in svg.match_indices(" impl Iterator { + text.match_indices('&').filter_map(|(at, _)| { + let rest = text.get(at + 1..)?; + let name = rest.get(..rest.find(';')?)?; + (!name.is_empty() && name.len() <= 256 && !name.starts_with('#') && !name.contains(char::is_whitespace)).then_some(name) + }) +} + +/// What entity `name` expands to (bytes), references in its value followed. +fn expanded<'a>(name: &'a str, entities: &HashMap<&'a str, &'a str>, memo: &mut HashMap<&'a str, usize>, depth: u32) -> usize { + if let Some(n) = memo.get(name) { + return *n; + } + let Some(value) = entities.get(name) else { return 0 }; + if depth >= MAX_NESTING { + return usize::MAX; + } + let mut total = value.len(); + for inner in references(value) { + total = total.saturating_add(expanded(inner, entities, memo, depth + 1)); + } + memo.insert(name, total); + total +} + +/// Whether the references to the entities `svg` declares expand to more than [`MAX_EXPANDED`] +/// bytes in all. False for a file that declares none. +pub(crate) fn expansion_too_large(svg: &str) -> bool { + let entities = declared(svg); + if entities.is_empty() { + return false; + } + let mut memo = HashMap::new(); + let mut total = 0usize; + for name in references(svg) { + total = total.saturating_add(expanded(name, &entities, &mut memo, 0)); + if total > MAX_EXPANDED { + return true; + } + } + false +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_repeated_large_entity_is_too_large_and_a_namespace_entity_is_not() { + let big = "x".repeat(1 << 20); + let refs = "&a;".repeat(20); + let bomb = format!("]>"); + assert!(expansion_too_large(&bomb)); + let nested = format!("]>", "&a;".repeat(8), "&b;".repeat(8)); + assert!(expansion_too_large(&nested)); + let ns = "]>"; + assert!(!expansion_too_large(ns)); + assert!(!expansion_too_large("")); + } +} diff --git a/crates/svg/src/export.rs b/crates/svg/src/export.rs index e2dbbd338..bbc74676e 100644 --- a/crates/svg/src/export.rs +++ b/crates/svg/src/export.rs @@ -54,6 +54,8 @@ fn write(doc: &Document, opts: &ExportOptions, native: Option<&[u8]>, id_prefix: classes: Vec::new(), class_ids: HashMap::new(), used_ids: HashSet::new(), + next_suffix: HashMap::new(), + linked_names: HashSet::new(), names: HashMap::new(), depth: 1, patterns: HashMap::new(), @@ -304,6 +306,10 @@ struct Writer<'a> { /// 1-based class/entity number by declaration; avoids a linear scan for each style. class_ids: HashMap, used_ids: HashSet, + /// The next suffix to try for a base name, so a repeated name doesn't probe the ones taken again. + next_suffix: HashMap, + /// The names in `linked`. + linked_names: HashSet, names: HashMap, depth: usize, /// `` def ids by pattern name + placement. @@ -601,14 +607,17 @@ impl Writer<'_> { /// `base` (with the unique prefix) if free, else `base-2`, `base-3`… fn unique_id(&mut self, base: &str) -> String { let base = format!("{}{base}", self.id_prefix); - let mut cand = base.clone(); - for i in 2.. { + if self.used_ids.insert(base.clone()) { + return base; + } + let next = self.next_suffix.entry(base.clone()).or_insert(2); + loop { + let cand = format!("{base}-{next}"); + *next += 1; if self.used_ids.insert(cand.clone()) { - break; + return cand; } - cand = format!("{base}-{i}"); } - cand } /// Reserve ids for every named object up front so generated def ids never collide with them. @@ -1584,7 +1593,7 @@ impl Writer<'_> { return format!("data:{};base64,{}", b.mime, base64_encode(&b.bytes)); } let name = format!("{}.{}", b.content_key(), image_ext(&b.mime)); - if !self.linked.iter().any(|l| l.name == name) { + if self.linked_names.insert(name.clone()) { self.linked.push(LinkedImage { name: name.clone(), bytes: b.bytes.clone() }); } name diff --git a/crates/svg/src/glyph.rs b/crates/svg/src/glyph.rs index 2ee78b6c9..1d3c95ae9 100644 --- a/crates/svg/src/glyph.rs +++ b/crates/svg/src/glyph.rs @@ -90,6 +90,10 @@ struct Glyph { /// size (the em square is its frame) and in the text's colour `color`. `None` when it doesn't read /// or has no element `id`. fn glyph(svg: &str, id: &str, color: &str) -> Option { + // Before the XML reader: it nests by recursion and expands entities without a total limit. + if crate::import::xml_too_deep(svg) || crate::entities::expansion_too_large(svg) { + return None; + } let xml = roxmltree::Document::parse_with_options(svg, roxmltree::ParsingOptions { allow_dtd: true, ..Default::default() }).ok()?; let root = xml.root_element(); let whole = root.attribute("id") == Some(id); diff --git a/crates/svg/src/import.rs b/crates/svg/src/import.rs index 0ff620fd2..b9732794a 100644 --- a/crates/svg/src/import.rs +++ b/crates/svg/src/import.rs @@ -34,6 +34,9 @@ mod vector_effect; /// The XML of `svg`. fn parse(svg: &str) -> Result, SvgError> { + if crate::entities::expansion_too_large(svg) { + return Err(SvgError::Parse("the entities the document declares expand to too much text".into())); + } roxmltree::Document::parse_with_options(svg, roxmltree::ParsingOptions { allow_dtd: true, ..Default::default() }) .map_err(|e| SvgError::Parse(e.to_string())) } @@ -1512,6 +1515,8 @@ impl Importer { return None; } let (appearance, r) = self.appearance(p, acc, path.bounds()); + // Shared, so a compound path's subpaths don't each copy a gradient's stops. + let appearance: vectorcraft_doc::Shared = appearance.into(); let mut n = if path.subpaths.len() > 1 { // Multi-subpath SVG paths are compound paths (as in Illustrator). let children = path @@ -1520,7 +1525,7 @@ impl Importer { .map(|sp| { let id = self.doc.alloc_id(); let mut c = Node::new(id, NodeKind::Path { path: PathData::single(sp), rule: r, live: None, clipping: false, guide: false }); - c.appearance = appearance.clone().into(); + c.appearance = appearance.clone(); Arc::new(c) }) .collect(); diff --git a/crates/svg/src/import/fx.rs b/crates/svg/src/import/fx.rs index 0c4408f58..1b2f8ddb5 100644 --- a/crates/svg/src/import/fx.rs +++ b/crates/svg/src/import/fx.rs @@ -83,7 +83,16 @@ enum V { Stack(Vec), } +/// The most layers a filter result may stack: a result combined with itself again and again +/// would otherwise double each time. A filter past it is left as it is (not editable). +const MAX_LAYERS: usize = 64; + impl V { + /// `layers` stacked, `None` past [`MAX_LAYERS`]. + fn stack(layers: Vec) -> Option { + (layers.len() <= MAX_LAYERS).then_some(V::Stack(layers)) + } + fn layers(self) -> Vec { match self { V::Stack(l) => l, @@ -207,7 +216,7 @@ fn eval(f: &Filter, k: f64) -> Option { } else if transparent(&a) { b_in } else if b.mode() == usvg::BlendMode::Normal { - V::Stack(b_in.layers().into_iter().chain(a.layers()).collect()) + V::stack(b_in.layers().into_iter().chain(a.layers()).collect())? } else { return None; } @@ -223,7 +232,7 @@ fn eval(f: &Filter, k: f64) -> Option { } else if transparent(&a) { b } else { - V::Stack(b.layers().into_iter().chain(a.layers()).collect()) + V::stack(b.layers().into_iter().chain(a.layers()).collect())? } } _ => return None, @@ -242,7 +251,7 @@ fn eval(f: &Filter, k: f64) -> Option { match layers.len() { 0 => return None, 1 => layers.remove(0), - _ => V::Stack(layers), + _ => V::stack(layers)?, } } Kind::ColorMatrix(c) => match c.kind() { diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index e647fcbcc..b292ce53e 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -95,6 +95,7 @@ #![forbid(unsafe_code)] mod css; +mod entities; mod export; mod glyph; mod import; @@ -378,7 +379,7 @@ pub struct Editing { /// The native document an SVG carries, if any, and whether the SVG was edited since. pub fn editing(svg: &str) -> Option { // (An SVG nested too deep for the XML reader carries none: it isn't one of ours.) - if !svg.contains(EDITING_NS) || import::xml_too_deep(svg) { + if !svg.contains(EDITING_NS) || import::xml_too_deep(svg) || entities::expansion_too_large(svg) { return None; } let opts = usvg::roxmltree::ParsingOptions { allow_dtd: true, ..Default::default() };