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

@@ -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 {

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("<!ENTITY") {
+        let rest = svg.get(at + "<!ENTITY".len()..).unwrap_or("").trim_start();
+        // A parameter entity (`%`) isn't referenced as `&name;`.
+        if rest.starts_with('%') {
+            continue;
+        }
+        let Some(end) = rest.find(char::is_whitespace) else { continue };
+        let (name, rest) = rest.split_at(end);
+        let rest = rest.trim_start();
+        let Some(quote) = rest.chars().next().filter(|q| matches!(q, '"' | '\'')) else { continue };
+        let Some(value) = rest.get(1..).and_then(|r| r.split_once(quote)).map(|(v, _)| v) else { continue };
+        map.entry(name).or_insert(value);
+    }
+    map
+}
+
+/// The names of the `&name;` references in `text`.
+fn references(text: &str) -> impl Iterator<Item = &str> {
+    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!("<!DOCTYPE svg [<!ENTITY a \"{big}\">]><svg xmlns=\"http://www.w3.org/2000/svg\" id=\"{refs}\"/>");
+        assert!(expansion_too_large(&bomb));
+        let nested = format!("<!DOCTYPE svg [<!ENTITY a \"{big}\"><!ENTITY b \"{}\">]><svg id=\"{}\"/>", "&a;".repeat(8), "&b;".repeat(8));
+        assert!(expansion_too_large(&nested));
+        let ns = "<!DOCTYPE svg [<!ENTITY ns_svg \"http://www.w3.org/2000/svg\">]><svg xmlns=\"&ns_svg;\"/>";
+        assert!(!expansion_too_large(ns));
+        assert!(!expansion_too_large("<svg a=\"&amp;&#10;\"/>"));
+    }
+}

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<String, usize>,
     used_ids: HashSet<String>,
+    /// The next suffix to try for a base name, so a repeated name doesn't probe the ones taken again.
+    next_suffix: HashMap<String, usize>,
+    /// The names in `linked`.
+    linked_names: HashSet<String>,
     names: HashMap<NodeId, String>,
     depth: usize,
     /// `<pattern>` 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

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<Glyph> {
+    // 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);

crates/svg/src/import.rs

@@ -34,6 +34,9 @@ mod vector_effect;
 
 /// The XML of `svg`.
 fn parse(svg: &str) -> Result<roxmltree::Document<'_>, 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> = 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();

crates/svg/src/import/fx.rs

@@ -83,7 +83,16 @@ enum V {
     Stack(Vec<V>),
 }
 
+/// 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<V>) -> Option<V> {
+        (layers.len() <= MAX_LAYERS).then_some(V::Stack(layers))
+    }
+
     fn layers(self) -> Vec<V> {
         match self {
             V::Stack(l) => l,
@@ -207,7 +216,7 @@ fn eval(f: &Filter, k: f64) -> Option<V> {
                 } 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<V> {
                         } 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<V> {
                 match layers.len() {
                     0 => return None,
                     1 => layers.remove(0),
-                    _ => V::Stack(layers),
+                    _ => V::stack(layers)?,
                 }
             }
             Kind::ColorMatrix(c) => match c.kind() {

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<Editing> {
     // (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() };