collective
Browse / github.com/storytold/photocraft / patch
SVG: a DTD entity can't hide nesting from the depth check (#9669)
SVG: a DTD entity can hide nesting from the depth check and crash the app TL;DR: an SVG with markup in a DTD entity gets past the nesting-depth check and crashes the app with a stack overflow. Opening it should give an import error instead. ## What happens The nesting cap (512, from #1901) counts tags in the raw bytes. But the SVG parser runs with DTDs enabled and expands internal entities **after** that count. The counter treats a whole `<!ENTITY …>` as one tag, so nesting declared inside an entity is invisible to it: ```xml <!DOCTYPE svg [<!ENTITY a "<g><g><g>…(20 000)…</g></g></g>">]> <svg xmlns="http://www.w3.org/2000/svg" width="8" height="8">&a;</svg> ``` A
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- davidmann4 (imported from pull request #9670, open)
- Base
7ac7b8b106acccf6826d45ed5c9e481dfe341035- Size
- +123 −5 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
67efb32cf93788e01d0efa2f
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/tests/it/svg_smart_object.rs crates/io/src/svg.rs crates/io/tests/it/svg.rs
@@ -62,3 +62,32 @@ fn re_placing_at_another_scale_re_renders_the_vector_source() { assert!(alpha(&surf, 268, 60) > 0.99 && alpha(&surf, 271, 60) < 0.01); assert!(red(&surf, 150, 81) < 0.02 && red(&surf, 150, 75) > 0.98 && red(&surf, 150, 87) > 0.98); }++/// A `.pcraft` opens from its cached pixels and parses an embedded SVG source only on a later+/// edit. A source whose DTD entity hides 20 000 nested groups from the depth check used to open+/// clean and then overflow the stack on Image Size or a PSD save; both are an error or a fallback+/// now. Run on a 2 MB stack, as the app's import and command threads are.+#[test]+fn a_pcraft_smart_object_with_a_hostile_svg_does_not_abort_a_later_edit() {+ let deep = format!("{}{}", "<g>".repeat(20_000), "</g>".repeat(20_000));+ let hostile = format!("<!DOCTYPE svg [<!ENTITY a \"{deep}\">]><svg xmlns=\"http://www.w3.org/2000/svg\" width=\"20\" height=\"10\">&a;</svg>");+ std::thread::Builder::new()+ .stack_size(2 * 1024 * 1024)+ .spawn(move || {+ let mut s = Session::new();+ s.execute("file.new", json!({"width": 100, "height": 100, "background": "white"})).unwrap();+ photocraft_engine::file_cmds::place_bytes(&mut s, "logo.svg", SVG.as_bytes().to_vec(), None, &json!({"scale": 100})).unwrap();+ let mut doc = (*s.active().unwrap().doc).clone();+ let LayerContent::Smart(sm) = &mut doc.layers.last_mut().unwrap().content else { panic!("smart object") };+ sm.source = photocraft_doc::SmartSource::Embedded { file_name: "logo.svg".into(), bytes: std::sync::Arc::new(hostile.into_bytes()) };+ let carrier = photocraft_format::save_to_bytes(&doc, &photocraft_format::SaveOptions::default()).unwrap();++ photocraft_engine::file_cmds::open_bytes_as(&mut s, "carrier.pcraft", &carrier, None, None).unwrap();+ let _ = s.execute("image.imageSize", json!({"width": 50, "height": 50}));+ let dir = tempfile::tempdir().unwrap();+ let _ = s.execute("file.saveACopy", json!({"path": dir.path().join("copy.psd").to_string_lossy()}));+ })+ .unwrap()+ .join()+ .unwrap();+}@@ -344,7 +344,7 @@ fn parse(bytes: &[u8]) -> Result<Parsed, IoError> { /// [`parse`] with the font database `fonts` supplies (the tests hand in an empty one). fn parse_with(bytes: &[u8], fonts: fn() -> Arc<usvg::fontdb::Database>) -> Result<Parsed, IoError> { let xml = inflate(bytes)?;- if nesting_depth(&xml, MAX_XML_DEPTH) > MAX_XML_DEPTH {+ if nesting_depth(&xml, MAX_XML_DEPTH)? > MAX_XML_DEPTH { return Err(IoError::Svg(format!("elements are nested more than {MAX_XML_DEPTH} levels deep"))); } let has_text = xml.windows(4).any(|w| w == b"text");@@ -378,8 +378,10 @@ fn inflate(bytes: &[u8]) -> Result<std::borrow::Cow<'_, [u8]>, IoError> { /// The deepest element nesting in `xml`, stopping as soon as it passes `limit` (the result is /// then `limit + 1`). Comments, CDATA sections, processing instructions and the DOCTYPE add /// nothing; quoted attribute values may hold `>` or `/>`. Malformed markup is only counted-/// loosely: the parser rejects it afterwards.-fn nesting_depth(xml: &[u8], limit: usize) -> usize {+/// loosely: the parser rejects it afterwards. The parser expands the DTD's entities after this+/// count, so an entity holding markup (a `<`) is an error: it could nest deeper than anything+/// counted here. A `<` there stays text.+fn nesting_depth(xml: &[u8], limit: usize) -> Result<usize, IoError> { let after = |from: usize, pat: &[u8]| -> usize { xml.get(from..).and_then(|s| s.windows(pat.len()).position(|w| w == pat)).map_or(xml.len(), |p| from.saturating_add(p).saturating_add(pat.len())) };@@ -401,6 +403,44 @@ fn nesting_depth(xml: &[u8], limit: usize) -> usize { } (xml.len(), false) };+ // The byte after the DOCTYPE starting at `from`: its internal subset `[...]` is walked+ // declaration by declaration, so a quote in a comment there can't hide the markup after it.+ let doctype_end = |from: usize| -> Result<usize, IoError> {+ let (mut quote, mut j) = (None, from);+ loop {+ let Some(&b) = xml.get(j) else { return Ok(xml.len()) };+ j = j.saturating_add(1);+ match quote {+ Some(q) if b == q => quote = None,+ Some(_) => {}+ None if b == b'"' || b == b'\'' => quote = Some(b),+ None if b == b'>' => return Ok(j),+ None if b == b'[' => break,+ None => {}+ }+ }+ while let Some(rest) = xml.get(j..).filter(|r| !r.is_empty()) {+ j = if rest.starts_with(b"<!--") {+ after(j.saturating_add(4), b"-->")+ } else if rest.starts_with(b"<?") {+ after(j.saturating_add(2), b"?>")+ } else if rest.starts_with(b"<!ENTITY") {+ let end = tag_end(j.saturating_add(2)).0;+ if xml.get(j.saturating_add(2)..end).is_some_and(|decl| decl.contains(&b'<')) {+ return Err(IoError::Svg("the SVG's DTD declares an entity holding markup, which isn't supported".into()));+ }+ end+ } else if rest.starts_with(b"<!") {+ // The parser ends ELEMENT, ATTLIST and NOTATION at the first `>`, quotes or not.+ after(j.saturating_add(2), b">")+ } else if rest.starts_with(b"]") {+ return Ok(tag_end(j.saturating_add(1)).0);+ } else {+ j.saturating_add(1)+ };+ }+ Ok(xml.len())+ }; let (mut depth, mut deepest, mut i) = (0usize, 0usize, 0usize); while let Some(p) = xml.get(i..).and_then(|s| s.iter().position(|&b| b == b'<')) { let at = i.saturating_add(p);@@ -411,6 +451,8 @@ fn nesting_depth(xml: &[u8], limit: usize) -> usize { after(at.saturating_add(9), b"]]>") } else if rest.starts_with(b"<?") { after(at.saturating_add(2), b"?>")+ } else if rest.starts_with(b"<!DOCTYPE") {+ doctype_end(at.saturating_add(9))? } else if rest.starts_with(b"<!") { tag_end(at.saturating_add(2)).0 } else if rest.starts_with(b"</") {@@ -422,13 +464,13 @@ fn nesting_depth(xml: &[u8], limit: usize) -> usize { depth = depth.saturating_add(1); deepest = deepest.max(depth); if deepest > limit {- return deepest;+ return Ok(deepest); } } end }; }- deepest+ Ok(deepest) } /// The document canvas for a drawing: its size in px, scaled down to [`MAX_SIDE`] when needed.@@ -351,6 +351,53 @@ fn deeply_nested_markup_is_an_error_not_a_stack_overflow() { assert_eq!(r.document.layers.len(), 1); } +/// The parser expands a DTD's internal entities after the depth check: an entity holding+/// markup, or a quote in a DTD comment or declaration that made the check skip ahead, hid 20 000 nested groups+/// from it and overflowed the stack again. Both are errors now; same small stack as above.+#[test]+fn a_dtd_cannot_hide_nesting_from_the_depth_check() {+ let deep = |open: &str, close: &str| format!("{}{}", open.repeat(20_000), close.repeat(20_000));+ let with_dtd = |subset: &str, body: &str| format!("<!DOCTYPE svg [{subset}]><svg {NS} width=\"8\" height=\"8\">{body}</svg>");+ let cases = [+ ("entity", with_dtd(&format!("<!ENTITY a \"{}\">", deep("<g>", "</g>")), "&a;")),+ ("single-quoted entity", with_dtd(&format!("<!ENTITY a '{}'>", deep("<g>", "</g>")), "&a;")),+ ("quote in a comment", with_dtd("<!-- ' -->", &format!("{}<desc>'</desc>", deep("<g>", "</g>")))),+ ("quote in an element declaration", with_dtd("<!ELEMENT x '>", &format!("{}<desc>'</desc>", deep("<g>", "</g>")))),+ (+ "entity between quoted attribute lists",+ with_dtd(&format!("<!ATTLIST svg x CDATA '><!ENTITY a \"{}\"><!ATTLIST svg y CDATA '>", deep("<g>", "</g>")), "&a;"),+ ),+ ("entity between quoted notations", with_dtd(&format!("<!NOTATION n SYSTEM '><!ENTITY a \"{}\"><!NOTATION m SYSTEM '>", deep("<g>", "</g>")), "&a;")),+ ];+ std::thread::Builder::new()+ .stack_size(512 * 1024)+ .spawn(move || {+ for (name, svg) in &cases {+ let gz = gzip(svg.as_bytes());+ for bytes in [svg.as_bytes(), gz.as_slice()] {+ let e = import("deep.svg", bytes).err().unwrap_or_else(|| panic!("{name} opened"));+ assert!(matches!(&e, IoError::Svg(_)), "{name}: {e}");+ }+ }+ })+ .unwrap()+ .join()+ .unwrap();+}++/// Text-only entities, as Illustrator declares its namespaces, still open, `<` included:+/// the parser keeps a character reference in an entity as text.+#[test]+fn text_entities_in_a_dtd_still_open() {+ let r = open(+ "<?xml version=\"1.0\"?><!DOCTYPE svg PUBLIC \"-//W3C//DTD SVG 1.1//EN\" \"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd\" [\+ <!ENTITY ns_svg \"http://www.w3.org/2000/svg\"> <!-- a ' and a <g> --> <?pi > ?> <!ATTLIST svg data-x CDATA '<'> <!ENTITY red '#ff0000'> <!ENTITY c \"© <g>\"> ]>\+ <svg xmlns=\"&ns_svg;\" width=\"8\" height=\"8\"><title>&c;</title><rect width=\"8\" height=\"8\" fill=\"&red;\"/></svg>",+ );+ assert_eq!(r.document.layers.len(), 1);+ assert!(close(px(&r.document, 4, 4), [1.0, 0.0, 0.0, 1.0]));+}+ /// The depth check reads the markup, not the parser's tree: it must not count `<` in comments, /// CDATA, processing instructions or the DOCTYPE, nor end a tag at a `>` inside quotes. #[test]