collective
Browse / github.com/storytold/deckcraft / patch
PPTX: parse each tags part once and cap the tags an import copies (#1164)
[Audit] PPTX tags part is re-parsed and copied per reference <!-- deckcraft-audit:F-03 --> Read-only source audit of upstream `main` at [1dc69720db64](https://github.com/storytold/deckcraft/commit/1dc69720db64323370dda061226becb09ad79fd4). **Severity:** High **Category:** DoS / amplification **Confidence:** Confirmed by code reading, memory figure estimated **Audit ID:** F-03 **Evidence.** [`crates/pptx/src/read/mod.rs:193-205`](https://github.com/storytold/deckcraft/blob/1dc69720db64323370dda061226becb09ad79fd4/crates/pptx/src/read/mod.rs#L193-L205), called for every shape (`read/shapes.rs:169`) and every slide (`mod.rs:1100`): ```ru
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- Naseq (imported from pull request #1373, open)
- Base
ee875abffa1a31cb760c843bb29e79391a90a766- Size
- +128 −6 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
d1f2117ffd40ffd005c0c559
How it went
No other operator has reported using it yet.
Diff
raw · crates/pptx/src/lib.rs crates/pptx/src/read.rs crates/pptx/tests/it/tags.rs
@@ -14,6 +14,7 @@ mod maps; mod opc; mod png; pub(crate) mod read;+pub use read::MAX_TAG_BYTES; mod tables; mod write; mod xml;@@ -40,6 +40,12 @@ pub struct Imp<'a> { pub kept_sidecar_bytes: usize, /// Cached kept sidecars by normalized chart part name. pub kept_charts: HashMap<String, deckcraft_model::chart::ChartKept>,+ /// Add-in tags parts, parsed once each, by normalized part name.+ pub tags_by_part: HashMap<String, Arc<[(String, String)]>>,+ /// Bytes of tags copied into the model so far (see [`MAX_TAG_BYTES`]).+ pub tag_bytes: usize,+ /// The tag budget ran out and the warning was given.+ pub tags_capped: bool, } impl Imp<'_> {@@ -196,16 +202,54 @@ fn choice_has_model3d(el: &El, rels: &Rels) -> bool { rec(el, rels, 0) } +/// Bytes of tags one import copies into the model, every tags part and reference together: a+/// small compressed tags part listed by many shapes, or many times by one, must not grow without+/// bound (#1164). The package budgets (`opc::MAX_XML_PART`, `MAX_TOTAL`, #1283) bound what is+/// parsed; this bounds what is copied from it.+pub const MAX_TAG_BYTES: usize = 64 << 20;+/// What each copied pair costs besides its text, so empty pairs count too.+const TAG_PAIR_COST: usize = std::mem::size_of::<(String, String)>();+ /// Add-in tags named by `holder`'s `p:custDataLst`: the name/value pairs of each tags part.-pub fn tags(imp: &Imp, part: &Part, holder: &El) -> Vec<(String, String)> {+/// Each part is parsed once per import and counted once per holder.+pub fn tags(imp: &mut Imp, part: &Part, holder: &El) -> Vec<(String, String)> { let Some(list) = holder.child("custDataLst") else { return vec![] }; let mut out = Vec::new();+ let mut seen = HashSet::new(); for t in list.children_named("tags").take(100) {- let Some(doc) = t.attr("r:id").and_then(|id| part.rels.get(id)).and_then(|r| imp.pkg.xml(&r.target)) else { continue };- for tag in doc.root.children_named("tag").take(10_000) {- if let Some(name) = tag.attr("name") {- out.push((name.to_string(), tag.attr("val").unwrap_or("").to_string()));+ let Some(target) = t.attr("r:id").and_then(|id| part.rels.get(id)).map(|r| opc::norm(&r.target)) else { continue };+ if !seen.insert(target.clone()) {+ continue;+ }+ let parsed = match imp.tags_by_part.get(&target) {+ Some(p) => p.clone(),+ None => {+ let p: Arc<[(String, String)]> = imp+ .pkg+ .xml(&target)+ .map(|doc| {+ doc.root+ .children_named("tag")+ .take(10_000)+ .filter_map(|tag| Some((tag.attr("name")?.to_string(), tag.attr("val").unwrap_or("").to_string())))+ .collect()+ })+ .unwrap_or_default();+ imp.tags_by_part.insert(target, p.clone());+ p+ }+ };+ for (name, val) in parsed.iter() {+ let n = imp.tag_bytes.saturating_add(TAG_PAIR_COST + name.len() + val.len());+ if n > MAX_TAG_BYTES {+ if !imp.tags_capped {+ log::warn!("pptx: add-in tags over {MAX_TAG_BYTES} bytes; the rest are dropped");+ imp.tags_capped = true;+ }+ return out; }+ imp.tag_bytes = n;+ out.push((name.clone(), val.clone())); } } out@@ -389,6 +433,9 @@ pub fn import_theme(bytes: &[u8]) -> Result<(deckcraft_model::theme::Theme, Vec< vml_previews: HashMap::new(), kept_sidecar_bytes: 0, kept_charts: HashMap::new(),+ tags_by_part: HashMap::new(),+ tag_bytes: 0,+ tags_capped: false, }; let part = Part::new(&pkg, &theme_path, &doc); let theme = theme::read_theme(&mut imp, &part, &doc.root);@@ -470,6 +517,9 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE vml_previews: HashMap::new(), kept_sidecar_bytes: 0, kept_charts: HashMap::new(),+ tags_by_part: HashMap::new(),+ tag_bytes: 0,+ tags_capped: false, }; // Slide ids first so hyperlinks anywhere can point at slides. let mut slide_ids: Vec<(u32, String, SlideId)> = vec![];@@ -598,7 +648,7 @@ pub fn import_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>), PptxE p.raw_ext = Some(kept); } }- p.tags = tags(&imp, &pres_part, pres_el);+ p.tags = tags(&mut imp, &pres_part, pres_el); // Custom shows. if let Some(l) = pres_el.child("custShowLst") { for cs in l.children_named("custShow") {@@ -36,3 +36,74 @@ fn tags_survive_round_trip() { assert_eq!(first.tags, vec![t("THINKCELLSHAPEDONOTDELETE", "t!abc")]); assert!(back.slides.iter().skip(1).all(|s| s.tags.is_empty())); }++/// #1164: the same tags part listed over and over is read once, and the tags one import copies+/// into the model are capped however often a shape lists them.+#[test]+fn a_tags_part_listed_many_times_is_copied_once_and_capped() {+ let mut s = Session::new();+ deckcraft_engine::sample::open_sample(&mut s).expect("sample");+ for _ in 0..6 {+ s.execute("shape.insert", &serde_json::json!({"preset": "rect", "rect": [10, 10, 50, 50]})).expect("shape");+ }+ let mut p = s.doc().expect("doc").doc.as_ref().clone();+ let slide = Arc::make_mut(&mut p.slides[0]);+ // A quarter of the budget with its name and pair cost: the fifth shape listing it goes over.+ let pair = std::mem::size_of::<(String, String)>();+ slide.shapes[0].tags = vec![t("BIG", &"x".repeat(deckcraft_pptx::MAX_TAG_BYTES / 4 - 3 - pair))];+ let bytes = deckcraft_pptx::export(&p).expect("export");++ // Every shape on the first slide lists that one tags part 100 times.+ let mut zip = zip::ZipArchive::new(Cursor::new(bytes)).expect("zip");+ let mut xml = String::new();+ zip.by_name("ppt/slides/slide1.xml").expect("slide").read_to_string(&mut xml).expect("read");+ let start = xml.find("<p:custDataLst>").expect("tags listed");+ let end = xml[start..].find("</p:custDataLst>").expect("list ends") + start + "</p:custDataLst>".len();+ let list = &xml[start..end];+ let inner = list.trim_start_matches("<p:custDataLst>").trim_end_matches("</p:custDataLst>");+ let many = format!("<p:custDataLst>{}</p:custDataLst>", inner.repeat(100));+ let xml = xml.replace(list, "").replace("<p:nvPr/>", "<p:nvPr></p:nvPr>").replace("<p:nvPr>", &format!("<p:nvPr>{many}"));+ assert!(xml.matches(&many).count() >= 5, "enough shapes to go over the budget");+ let mut out = zip::ZipWriter::new(Cursor::new(Vec::new()));+ for i in 0..zip.len() {+ let mut f = zip.by_index(i).expect("entry");+ let name = f.name().to_string();+ let mut data = vec![];+ f.read_to_end(&mut data).expect("read");+ out.start_file(&name, zip::write::SimpleFileOptions::default()).expect("start");+ std::io::Write::write_all(&mut out, if name == "ppt/slides/slide1.xml" { xml.as_bytes() } else { &data }).expect("write");+ }+ let bytes = out.finish().expect("finish").into_inner();++ let back = deckcraft_pptx::import(&bytes).expect("import");+ let tagged: Vec<_> = back.slides[0].shapes.iter().filter(|x| !x.tags.is_empty()).collect();+ assert_eq!(tagged.len(), 4, "four copies fit the budget");+ assert!(tagged.iter().all(|x| x.tags.len() == 1), "each shape keeps the part's tag once, not 100 times");+}++/// #1164: empty tags cost something too, or a part of 10,000 empty pairs listed by every shape+/// would be copied without ever reaching the budget.+#[test]+fn empty_tags_count_against_the_budget() {+ let pair = std::mem::size_of::<(String, String)>();+ let per_shape = 10_000 * pair;+ let shapes = deckcraft_pptx::MAX_TAG_BYTES / per_shape + 2;+ let mut s = Session::new();+ deckcraft_engine::sample::open_sample(&mut s).expect("sample");+ let mut p = s.doc().expect("doc").doc.as_ref().clone();+ let slide = Arc::make_mut(&mut p.slides[0]);+ let template = slide.shapes[0].clone();+ slide.shapes.clear();+ for i in 0..shapes {+ let mut sh = template.clone();+ sh.id = deckcraft_model::ShapeId(10_000 + i as u32);+ sh.ph = None;+ sh.tags = vec![t("", ""); 10_000];+ slide.shapes.push(sh);+ }+ let bytes = deckcraft_pptx::export(&p).expect("export");+ let back = deckcraft_pptx::import(&bytes).expect("import");+ let pairs: usize = back.slides[0].shapes.iter().map(|x| x.tags.len()).sum();+ assert!(pairs * pair <= deckcraft_pptx::MAX_TAG_BYTES, "{pairs} pairs copied");+ assert!(pairs < shapes * 10_000, "the budget stopped the copying");+}