diff --git a/crates/svg/src/glyph.rs b/crates/svg/src/glyph.rs index 601ca2e63..9c0ac5d5a 100644 --- a/crates/svg/src/glyph.rs +++ b/crates/svg/src/glyph.rs @@ -43,7 +43,7 @@ pub fn glyph_art(document: &[u8], gid: u32, upem: f64, fill: Color, stroke: Opti let coloured = ["currentColor", "currentcolor", "context-fill", "context-stroke"].iter().any(|k| svg.contains(k)); // usvg resolves `context-fill` and `context-stroke` only for markers. let svg = svg.replace("context-fill", &hex(fill)).replace("context-stroke", &stroke.map_or_else(|| "none".into(), hex)); - let (doc, _) = crate::import::import(&svg, &ImportOptions::default()).ok()?; + let (doc, _) = crate::import::import(&svg, &ImportOptions::default(), crate::MAX_NODES).ok()?; let mut art = if whole { Node::group(NodeId(0), doc.layers.iter().map(|l| Arc::new(Node::group(NodeId(0), l.children().cloned().unwrap_or_default()))).collect()) } else { diff --git a/crates/svg/src/import.rs b/crates/svg/src/import.rs index fea237c61..ff2e63c35 100644 --- a/crates/svg/src/import.rs +++ b/crates/svg/src/import.rs @@ -20,8 +20,13 @@ use text::TextSlots; pub use depth::MAX_ELEMENT_DEPTH; pub(crate) use depth::xml_too_deep; +pub use files::MAX_NESTED_SVG_IMAGES; pub(crate) use fx::BLEND; +/// Most objects an imported SVG may expand to: a guard against `` and nested-image +/// expansion bombs (#3564). +pub const MAX_NODES: usize = if cfg!(target_arch = "wasm32") { 500_000 } else { 2_000_000 }; + mod convert; mod importer; mod layers; @@ -58,7 +63,7 @@ fn parse(svg: &str) -> Result, SvgError> { } /// The document SVG text `svg` holds, read with `opts`, and the warnings about what it couldn't keep. -pub(crate) fn import(svg: &str, opts: &ImportOptions) -> Result<(Document, Vec), SvgError> { +pub(crate) fn import(svg: &str, opts: &ImportOptions, max_nodes: usize) -> Result<(Document, Vec), SvgError> { // Before anything reads the tree: the XML reader, usvg, and what is made of its tree, all by // recursion. let too_deep = || SvgError::TooDeep(depth::MAX_ELEMENT_DEPTH); @@ -116,6 +121,10 @@ pub(crate) fn import(svg: &str, opts: &ImportOptions) -> Result<(Document, Vec Result<(Document, Vec false, }; - let id = im.doc.alloc_id(); + let Some(id) = im.alloc_id() else { + return Err(im.error.unwrap_or(SvgError::TooLarge(im.max_nodes))); + }; let preset = (im.doc.layers.len() % vectorcraft_doc::LAYER_COLORS.len()) as u8; let mut l = Node::layer(id, &name, LayerColor::Preset(preset)); l.set_clips(clip); @@ -197,10 +208,16 @@ pub(crate) fn import(svg: &str, opts: &ImportOptions) -> Result<(Document, Vec Result<(Document, Vec, + /// How many nested SVG images were converted so far. + nested_svg_images: usize, } diff --git a/crates/svg/src/import/files.rs b/crates/svg/src/import/files.rs index 1ee025db8..722d043d4 100644 --- a/crates/svg/src/import/files.rs +++ b/crates/svg/src/import/files.rs @@ -15,6 +15,9 @@ use vectorcraft_doc::LinkInfo; use super::{Edits, depth, href}; use crate::{ImportOptions, MAX_ELEMENT_DEPTH, MAX_SVGZ, SvgError}; +/// Most nested SVG images an imported SVG may decode (#3564). +pub const MAX_NESTED_SVG_IMAGES: usize = 256; + /// The files an SVG's images link to. #[derive(Default)] pub(in crate::import) struct Files { @@ -36,6 +39,10 @@ pub(in crate::import) struct Files { too_deep: AtomicBool, /// An SVGZ image was left out for unpacking past what the SVGZ images may. too_big: AtomicBool, + /// How many nested SVG images were decoded so far. + svg_images: AtomicUsize, + /// SVG images were left out for exceeding [`MAX_NESTED_SVG_IMAGES`]. + too_many_svg_images: AtomicBool, } /// How usvg holds the bytes of a raster of one format (`ImageKind::PNG`…). @@ -214,6 +221,9 @@ impl Files { if self.too_big.load(Ordering::Relaxed) { notes.push(format!("SVGZ images were left out: together they unpack to more than {} MB", MAX_SVGZ >> 20)); } + if self.too_many_svg_images.load(Ordering::Relaxed) { + notes.push(format!("SVG images were left out: more than {MAX_NESTED_SVG_IMAGES} nested SVG images")); + } notes } @@ -268,6 +278,9 @@ impl Files { /// can't read, when it isn't SVG or when its elements nest deeper than the SVGs around it /// leave: usvg reads it while it reads them, on the same stack. fn svg(&self, text: &str, opts: &usvg::Options) -> Option { + if self.svg_images.fetch_add(1, Ordering::Relaxed) >= MAX_NESTED_SVG_IMAGES { + return leave_out(&self.too_many_svg_images, true); + } // Measured as the SVG holding it is (`import`): the text before the XML reader. let used = self.depth.load(Ordering::Relaxed); let left = MAX_ELEMENT_DEPTH.saturating_sub(used); diff --git a/crates/svg/src/import/importer.rs b/crates/svg/src/import/importer.rs index 82cbfd7da..5b96ea5bd 100644 --- a/crates/svg/src/import/importer.rs +++ b/crates/svg/src/import/importer.rs @@ -41,12 +41,27 @@ impl Importer { } pub(super) fn children(&mut self, g: &usvg::Group, acc: Affine) -> Vec> { - g.children().iter().filter_map(|c| self.node(c, acc)).map(Arc::new).collect() + if self.error.is_some() { + return vec![]; + } + let mut out = Vec::new(); + for c in g.children() { + if self.error.is_some() { + break; + } + if let Some(node) = self.node(c, acc) { + out.push(Arc::new(node)); + } + } + out } /// The object usvg node `n` makes under transform `acc`, with what the file kept on its element /// (data, event and other applications' attributes); `None` for what imports as nothing. pub(super) fn node(&mut self, n: &usvg::Node, acc: Affine) -> Option { + if self.error.is_some() { + return None; + } let mut out = match n { usvg::Node::Group(g) => self.group(g, acc), usvg::Node::Path(p) => self.path(p, acc), @@ -97,17 +112,33 @@ impl Importer { self.labels.get(id).map_or(id, String::as_str) } + /// Allocate a fresh node id within [`Importer::max_nodes`]. + pub(super) fn alloc_id(&mut self) -> Option { + if self.error.is_some() { + return None; + } + if self.converted_nodes >= self.max_nodes { + self.error = Some(SvgError::TooLarge(self.max_nodes)); + return None; + } + self.converted_nodes += 1; + Some(self.doc.alloc_id()) + } + /// A new object named after element `id` (unless it has none or a made-up one). - fn named(&mut self, id: &str, kind: NodeKind) -> Node { - let nid = self.doc.alloc_id(); + fn named(&mut self, id: &str, kind: NodeKind) -> Option { + let nid = self.alloc_id()?; let mut n = Node::new(nid, kind); if !id.is_empty() && !made_up(id) { n.name = Some(self.name_of(id).to_string()); } - n + Some(n) } fn group(&mut self, g: &usvg::Group, acc: Affine) -> Option { + if self.error.is_some() { + return None; + } let url = self.links.get(g.id()).cloned(); let mut n = match self.instance(g, acc) { Some(n) => n, @@ -143,7 +174,7 @@ impl Importer { let art = match kids.len() { 0 => return None, 1 => Arc::unwrap_or_clone(kids.pop()?), - _ => self.named("", NodeKind::Group { children: kids, clip: false }), + _ => self.named("", NodeKind::Group { children: kids, clip: false })?, }; // usvg clips a `` of a symbol to its viewport: that only counts when it cuts the art. let whole = g.clip_path().is_none_or(|cp| uncut(cp, ts, &art, xf)); @@ -164,7 +195,7 @@ impl Importer { }; let label = if made_up(g.id()) { "a symbol instance".to_string() } else { format!("'{}'", g.id()) }; let mask = g.mask().and_then(|m| self.opacity_mask(m, ts, &label)); - let mut n = self.named(g.id(), NodeKind::SymbolInstance { symbol: name, xf }); + let mut n = self.named(g.id(), NodeKind::SymbolInstance { symbol: name, xf })?; n.opacity = g.opacity().get(); n.blend = blend(g.blend_mode()); n.isolate = g.isolate(); @@ -227,7 +258,7 @@ impl Importer { ap }); text::fold_scale(&mut obj, grow); - let mut n = self.named(&name, NodeKind::Text(Box::new(obj))); + let mut n = self.named(&name, NodeKind::Text(Box::new(obj)))?; if let Some(ap) = under { n.appearance = ap.into(); } @@ -277,12 +308,12 @@ impl Importer { let mut n = if let Some(cp) = g.clip_path() { // SVG filters before it clips: the effects go on the art inside the clip. if !effects.is_empty() { - let mut inner = self.named("", NodeKind::Group { children, clip: false }); + let mut inner = self.named("", NodeKind::Group { children, clip: false })?; inner.appearance.effects = effects.clone(); children = vec![Arc::new(inner)]; } let children = self.clipped(cp, ts, children, &label)?; - self.named(id, NodeKind::Group { children, clip: true }) + self.named(id, NodeKind::Group { children, clip: true })? } else { if children.is_empty() { return None; @@ -310,7 +341,7 @@ impl Importer { c.appearance.effects.extend(effects); return Some(c); } - let mut n = self.named(id, NodeKind::Group { children, clip: false }); + let mut n = self.named(id, NodeKind::Group { children, clip: false })?; n.appearance.effects = effects; n }; @@ -358,7 +389,7 @@ impl Importer { // over when clipped, white when not, inverted when the mask is). let art = match children.len() { 1 => Arc::unwrap_or_clone(children.pop()?), - _ => self.named("", NodeKind::Group { children, clip: false }), + _ => self.named("", NodeKind::Group { children, clip: false })?, }; let mut mask = vectorcraft_doc::OpacityMask::new(art, clip); mask.invert = invert; @@ -384,7 +415,7 @@ impl Importer { for clip in clips { let mut ch = vec![Arc::new(clip)]; ch.extend(children); - children = vec![Arc::new(self.named("", NodeKind::Group { children: ch, clip: true }))]; + children = vec![Arc::new(self.named("", NodeKind::Group { children: ch, clip: true })?)]; } let mut ch = vec![Arc::new(outer)]; ch.extend(children); @@ -400,7 +431,9 @@ impl Importer { if let NodeKind::Layer { children, clip: clip @ true, .. } = &mut l.kind { *clip = false; let clipped = std::mem::take(children); - children.push(Arc::new(self.named("", NodeKind::Group { children: clipped, clip: true }))); + if let Some(group) = self.named("", NodeKind::Group { children: clipped, clip: true }) { + children.push(Arc::new(group)); + } } if let Some(ch) = l.children_mut() { let i = index.min(ch.len()); @@ -421,16 +454,15 @@ impl Importer { let id = cp.id().to_string(); if subs.len() == 1 { let path = PathData::new(subs); - return Some(self.named(&id, NodeKind::Path { path, rule: r, live: None, clipping: true, guide: false })); + return self.named(&id, NodeKind::Path { path, rule: r, live: None, clipping: true, guide: false }); } - let children = subs - .into_iter() - .map(|sp| { - let nid = self.doc.alloc_id(); - Arc::new(Node::new(nid, NodeKind::Path { path: PathData::single(sp), rule: r, live: None, clipping: false, guide: false })) - }) - .collect(); - Some(self.named(&id, NodeKind::Compound { children, rule: r })) + let mut children = Vec::with_capacity(subs.len()); + for sp in subs { + let nid = self.alloc_id()?; + children + .push(Arc::new(Node::new(nid, NodeKind::Path { path: PathData::single(sp), rule: r, live: None, clipping: false, guide: false }))); + } + self.named(&id, NodeKind::Compound { children, rule: r }) } /// A usvg paint mapped by `m`, for art covering `area` (after `m`: where a reflected or @@ -502,9 +534,11 @@ impl Importer { } let clip = self.named("", NodeKind::Path { path: shapes::rectangle(tile), rule: FillRule::NonZero, live: None, clipping: true, guide: false }); + let Some(clip) = clip else { return Paint::None }; let mut children = vec![Arc::new(clip)]; children.extend(content); let art = self.named("", NodeKind::Group { children, clip: true }); + let Some(art) = art else { return Paint::None }; let base = if pt.id().is_empty() { "Pattern" } else { pt.id() }; let name = unique_name(base, |n| self.doc.pattern(n).is_some()); let mut def = PatternDef::new(&name, vec![Arc::new(art)]); @@ -575,19 +609,16 @@ impl Importer { 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 - .subpaths - .into_iter() - .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(); - Arc::new(c) - }) - .collect(); - self.named(p.id(), NodeKind::Compound { children, rule: r }) + let mut children = Vec::with_capacity(path.subpaths.len()); + for sp in path.subpaths { + let id = self.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(); + children.push(Arc::new(c)); + } + self.named(p.id(), NodeKind::Compound { children, rule: r })? } else { - self.named(p.id(), NodeKind::Path { path, rule: r, live: None, clipping: false, guide: false }) + self.named(p.id(), NodeKind::Path { path, rule: r, live: None, clipping: false, guide: false })? }; n.appearance = appearance; Some(n) @@ -620,15 +651,23 @@ impl Importer { if link.is_some() && !missing { blob.with_proxy() } else { blob } }); let xf = acc * Affine::scale_non_uniform(size.width() as f64 / pw as f64, size.height() as f64 / ph as f64); - Some(self.named( + self.named( i.id(), NodeKind::Image(Box::new(ImageObject { key, width: pw, height: ph, xf, link, placement: Default::default(), pixels: None })), - )) + ) } /// An SVG shown as an image (a data URL or a linked file) → its art, `acc` mapping its size. /// Its ids are its own: they mean nothing in the SVG around it. fn svg_image(&mut self, tree: &usvg::Tree, acc: Affine) -> Option { + if self.error.is_some() { + return None; + } + if self.nested_svg_images >= crate::MAX_NESTED_SVG_IMAGES { + self.error = Some(SvgError::TooLarge(self.max_nodes)); + return None; + } + self.nested_svg_images += 1; let outer = ( std::mem::take(&mut self.links), std::mem::take(&mut self.hidden), diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index b292ce53e..7a0c5ddf9 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -333,7 +333,7 @@ pub struct LinkedImage { pub bytes: std::sync::Arc>, } -#[derive(Debug, thiserror::Error)] +#[derive(Clone, Debug, thiserror::Error)] pub enum SvgError { #[error("SVG parse error: {0}")] Parse(String), @@ -343,6 +343,10 @@ pub enum SvgError { /// SVGZ data unpacks to more than the reader takes (a guard against decompression bombs). #[error("SVG parse error: {0}")] TooBig(String), + /// The SVG expands to more objects than the reader takes (a guard against `` and + /// nested-image expansion bombs, #3564). + #[error("the SVG expands to more than {0} objects")] + TooLarge(usize), } /// Export a document as an SVG string (linked images are not returned: see [`export_full`]). @@ -541,13 +545,18 @@ pub fn import_with_report(svg: &str) -> Result<(Document, Vec), SvgError } pub use glyph::glyph_art; -pub use import::{MAX_ELEMENT_DEPTH, MAX_FOREIGN, MAX_FOREIGN_VALUE}; +pub use import::{MAX_ELEMENT_DEPTH, MAX_FOREIGN, MAX_FOREIGN_VALUE, MAX_NESTED_SVG_IMAGES, MAX_NODES}; /// Import an SVG document reading the files it links to as `opts` says, also returning warnings. pub fn import_with(svg: &str, opts: &ImportOptions) -> Result<(Document, Vec), SvgError> { + import_within(svg, opts, MAX_NODES) +} + +/// [`import_with`], capping the objects the document may expand to at `max_nodes`. +pub fn import_within(svg: &str, opts: &ImportOptions, max_nodes: usize) -> Result<(Document, Vec), SvgError> { // usvg converts the tree by recursion, in large stack frames: the nesting it is given is // capped (`import::depth`), and it gets room for that cap whatever thread asks. - vectorcraft_doc::depth::with_deep_stack(|| import::import(svg, opts)) + vectorcraft_doc::depth::with_deep_stack(|| import::import(svg, opts, max_nodes)) } /// Reads a linked file: its bytes and the link to it (absolute path, size, time and hash), or diff --git a/crates/svg/tests/it/import_details.rs b/crates/svg/tests/it/import_details.rs index 9db57336d..8b3645860 100644 --- a/crates/svg/tests/it/import_details.rs +++ b/crates/svg/tests/it/import_details.rs @@ -6,7 +6,7 @@ use std::path::{Path, PathBuf}; use vectorcraft_doc::{Document, ImageObject, LinkInfo, Node, NodeKind}; use vectorcraft_geom::Rect; -use vectorcraft_svg::{ImportOptions, import_with, import_with_report}; +use vectorcraft_svg::{ImportOptions, MAX_NESTED_SVG_IMAGES, SvgError, import_with, import_with_report, import_within}; use vectorcraft_testkit::raster::{Image, assert_similar, render_artboard}; fn svg(body: &str) -> String { @@ -372,3 +372,54 @@ fn inkscape_pages_become_artboards() { fn svg_doc() -> String { svg(r#""#) } + +/// An ordinary nested SVG image imports within budget, while ``-of-nested-image fan-out +/// past the node budget is refused with `SvgError::TooLarge` (#3564). +#[test] +fn nested_image_within_budget_imports_and_use_fan_out_is_refused() { + let inner = r#""#; + let nested = format!("data:image/svg+xml;base64,{}", vectorcraft_svg::base64_encode(inner.as_bytes())); + let single = format!(r#""#); + // An ordinary nested image imports within a small budget (3 objects: Layer 1, Group, Rect). + let (d, _) = import_within(&single, &ImportOptions::default(), 10).unwrap(); + assert!(!d.layers[0].children().unwrap().is_empty()); + + // Fan-out duplicating the nested image via exceeds the budget and is refused. + let uses = "".repeat(20); + let fanout = format!( + r#" + + {uses} + "# + ); + let err = import_within(&fanout, &ImportOptions::default(), 5).unwrap_err(); + assert!(matches!(err, SvgError::TooLarge(5)), "{err:?}"); + assert_eq!(err.to_string(), "the SVG expands to more than 5 objects"); + + // Highly amplified multi-level tree is refused without exploding memory. + let multi_level = format!( + r##" + + + + + + + + + "## + ); + let err = import_within(&multi_level, &ImportOptions::default(), 15).unwrap_err(); + assert!(matches!(err, SvgError::TooLarge(15)), "{err:?}"); +} + +/// Nested SVG images exceeding `MAX_NESTED_SVG_IMAGES` are left out with a warning note (#3564). +#[test] +fn nested_svg_images_past_count_cap_are_left_out() { + let inner = r#""#; + let nested = format!("data:image/svg+xml;base64,{}", vectorcraft_svg::base64_encode(inner.as_bytes())); + let images = format!(r#""#).repeat(MAX_NESTED_SVG_IMAGES + 10); + let svg = format!(r#"{images}"#); + let (_d, warnings) = import_with(&svg, &ImportOptions::default()).unwrap(); + assert!(warnings.iter().any(|w| w.contains(&format!("more than {MAX_NESTED_SVG_IMAGES} nested SVG images")))); +}