diff --git a/crates/engine/tests/it/svg_smart_object.rs b/crates/engine/tests/it/svg_smart_object.rs --- a/crates/engine/tests/it/svg_smart_object.rs +++ b/crates/engine/tests/it/svg_smart_object.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!("{}{}", "".repeat(20_000), "".repeat(20_000)); + let hostile = format!("]>&a;"); + 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(); +} diff --git a/crates/io/src/svg.rs b/crates/io/src/svg.rs --- a/crates/io/src/svg.rs +++ b/crates/io/src/svg.rs @@ -344,7 +344,7 @@ fn parse(bytes: &[u8]) -> Result { /// [`parse`] with the font database `fonts` supplies (the tests hand in an empty one). fn parse_with(bytes: &[u8], fonts: fn() -> Arc) -> Result { 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, 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 { 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 { + 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"") + } else if rest.starts_with(b"") + } else if rest.starts_with(b"`, 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"") + } else if rest.starts_with(b" 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. diff --git a/crates/io/tests/it/svg.rs b/crates/io/tests/it/svg.rs --- a/crates/io/tests/it/svg.rs +++ b/crates/io/tests/it/svg.rs @@ -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!("{body}"); + let cases = [ + ("entity", with_dtd(&format!("", deep("", "")), "&a;")), + ("single-quoted entity", with_dtd(&format!("", deep("", "")), "&a;")), + ("quote in a comment", with_dtd("", &format!("{}'", deep("", "")))), + ("quote in an element declaration", with_dtd("", &format!("{}'", deep("", "")))), + ( + "entity between quoted attribute lists", + with_dtd(&format!("", deep("", "")), "&a;"), + ), + ("entity between quoted notations", with_dtd(&format!("", deep("", "")), "&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( + " ?> ]>\ + &c;", + ); + 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]