From c3bdcd73b4c4ee4494ef35f37990668f0dc00fa4 Mon Sep 17 00:00:00 2001 From: Jeremy Yao Date: Wed, 14 Oct 2026 18:00:35 -0400 Subject: [PATCH] Harden against hostile-document DoS and non-web link export (#1235) * Harden against hostile-document DoS and non-web link export A security pass found several small files that make the app allocate or compute without bound when opened (GUI, wordcraft-cli, or the MCP server), and link targets leaving on export without the scheme check the HTML and Markdown writers already apply. None lets a document run code or read files; all are denial-of-service or credential-leak-on-click. Each fix bounds an attacker-controlled size and lands with a regression test, per the never-crash standard. - layout: a table repeats its header rows on every page it spills onto, so many tall header rows paginate to O(headers x pages) placed items. Cap the total header-row repeats per table (lib.rs). 2 KB docx: 1.1 GB -> 124 MB. - formats/model, html: an HTML table of many with large rowspan expanded to rows x 64 covered cells, and `model::table` pre-allocated a full rows x cols grid only to clear it. Cap rows at MAX_ROWS before insert_covered, bound its total work, and build the grid template with one row. 640 KB html: >1.5 GB -> 214 MB (a plain large table is faster too). - docx/read/freeform: a custom-geometry Bezier segment took any number of points, making de Casteljau flattening quadratic. Cap points per segment. 3 KB docx: 12 s -> 0.15 s. - docbin: footnote/header/text-box story tables and the piece table can name the same backing text many times. Hold the total characters materialised to twice the WordDocument stream size (a ceiling no real file reaches, since each char costs >=1 stream byte). 45 KB doc: >1.2 GB -> 14 MB. - render, pdf, formats/rtf: document pictures were decoded with no dimension limit, so a tiny file declaring a huge image could allocate before the size check. Decode through image::Limits with a side cap first. - doc/link, formats/rtf, odt, latex: route exported hyperlink targets through is_safe_href (as HTML/Markdown already do), and extend it to reject protocol-relative (//host) and UNC (\host) references. Keeps file:, javascript:, ms-msdt: and UNC targets from being written into exported RTF, ODT, LaTeX and PDF where a reader would follow them (UNC leaks NTLM on Windows). Co-Authored-By: Claude Opus 4.8 * Align picture-decode limits with the bounds already in force Review of the previous commit: `image::load_from_memory` and `load_from_memory_with_format` already applied `Limits::default()`, whose 512 MB allocation cap bounded every picture decode. The PDF change had set `max_alloc = None`, loosening that; restore the default. The render and RTF side caps (16 384 px) newly rejected wide-but-short pictures that fit in memory, so render now uses the pixmap's own `u16` side limit (what `premul_pixmap` enforced after decoding, applied before it instead) and the RTF re-encode keeps only the explicit allocation cap. Net effect: rejection moves to header time with no change to the sizes accepted. Also fix a comment: the header-row repeat cap is per table, not per document. Co-Authored-By: Claude Fable 5.1 --------- Co-authored-by: Claude Opus 4.8 --- crates/doc/src/link.rs | 24 +++++++++++++++---- crates/docbin/src/lib.rs | 21 +++++++++++++++++ crates/docx/src/read/freeform.rs | 39 ++++++++++++++++++++++++++++++- crates/formats/src/html.rs | 5 +++- crates/formats/src/latex.rs | 6 ++++- crates/formats/src/model.rs | 40 +++++++++++++++++++++++++++++++- crates/formats/src/odt.rs | 4 +++- crates/formats/src/rtf.rs | 10 +++++--- crates/layout/src/lib.rs | 15 ++++++++++-- crates/pdf/src/lib.rs | 10 +++++++- crates/render/src/lib.rs | 15 ++++++++++-- 11 files changed, 171 insertions(+), 18 deletions(-) diff --git a/crates/doc/src/link.rs b/crates/doc/src/link.rs index 73e4f5ea8..9bb08b90e 100644 --- a/crates/doc/src/link.rs +++ b/crates/doc/src/link.rs @@ -9,14 +9,23 @@ pub fn is_web_or_mail_link(url: &str) -> bool { ["http", "https", "mailto"].iter().any(|s| scheme.eq_ignore_ascii_case(s)) } -/// Whether a hyperlink may be written as an `href` (or Markdown destination) in an exported file: -/// a web or email link, a bookmark (`#name`) or a relative path. Scripts (`javascript:`, -/// `vbscript:`), `data:`, `file:` and every other scheme are refused, read the way a browser reads -/// a URL: ASCII control characters and spaces are dropped first, so `java\nscript:` is a script. +/// Whether a hyperlink may be written as an `href` (or Markdown destination, RTF `HYPERLINK` +/// field, ODT `xlink:href`, LaTeX `\href`) in an exported file: a web or email link, a bookmark +/// (`#name`) or a relative path. Scripts (`javascript:`, `vbscript:`), `data:`, `file:` and every +/// other scheme are refused, read the way a browser reads a URL: ASCII control characters and +/// spaces are dropped first, so `java\nscript:` is a script. A protocol-relative reference +/// (`//host/…`) or a UNC path (`\\host\…`, or `//host` once backslashes are read as separators) +/// names another machine — following it on Windows leaks the user's NTLM credentials — so it is +/// refused too. pub fn is_safe_href(url: &str) -> bool { + let cleaned: String = url.chars().filter(|c| !c.is_ascii_control() && *c != ' ').collect(); + // `//host` (protocol-relative) and `\\host` (UNC), with `\` read as `/` the way Windows does. + if cleaned.replace('\\', "/").starts_with("//") { + return false; + } match scheme(url) { Some(s) => matches!(s.as_str(), "http" | "https" | "mailto"), - None => !url.trim().is_empty(), + None => !cleaned.is_empty(), } } @@ -73,6 +82,11 @@ mod tests { "data:text/html,", "file:///etc/passwd", "ms-msdt:/id", + // Protocol-relative and UNC references name another machine (NTLM leak on Windows). + "//attacker.example/share/x", + "\\\\attacker.example\\share\\x", + "/\\attacker.example\\x", + " //attacker.example/x", "", ] { assert!(!is_safe_href(bad), "{bad:?}"); diff --git a/crates/docbin/src/lib.rs b/crates/docbin/src/lib.rs index 3da61275c..e076ec2be 100644 --- a/crates/docbin/src/lib.rs +++ b/crates/docbin/src/lib.rs @@ -44,6 +44,10 @@ use table::ParaOut; const MAX_STREAM: u64 = 1 << 30; /// Most header/footer parts we create (matches the docx reader's cap). const MAX_PARTS: usize = 50_000; +/// Hard ceiling on characters materialised across every story walk of one document (body, +/// headers, notes, text boxes), whatever the file's size. The working budget is tied to the +/// WordDocument stream (see [`read`]); this caps even a maximal stream. +const MAX_TOTAL_CHARS: u64 = 64 << 20; /// `sprmCPicLocation`: the fc of a picture in the Data stream. const C_PIC_LOCATION: u16 = 0x6A03; /// `sprmPIlvl` / `sprmPIlfo`: the paragraph's list level and list. @@ -158,6 +162,12 @@ pub fn read_with_password(bytes: &[u8], password: Option<&str>) -> Result) -> Result { /// Where sections end (main-document CPs, sorted): the section mark (0x0C) just before /// each ends the section's last paragraph, it isn't a page break inside it. sect_ends: &'a [u32], + /// Characters still allowed across every story walk of this document. Story ranges (headers, + /// footnotes, text boxes) are hostile and can each span a whole subdocument, so thousands of + /// them could otherwise materialise the same text thousands of times; this bounds the total. + budget: &'a std::cell::Cell, } /// State while assembling one paragraph from per-CP formatting. @@ -385,6 +400,12 @@ fn walk(ctx: &WalkCtx, cp_start: u32, cp_end: u32, pics: &mut media::Pictures) - continue; } }; + // Every real character counts against the document-wide budget; once it is spent, stop + // (a hostile file with thousands of story ranges over the same text can't multiply on). + if ctx.budget.get() == 0 { + break; + } + ctx.budget.set(ctx.budget.get() - 1); // A section mark (0x0C) at a section's end is that section's last paragraph mark. let section_mark = c == '\u{C}' && ctx.sect_ends.binary_search(&cp.saturating_add(1)).is_ok(); match c { diff --git a/crates/docx/src/read/freeform.rs b/crates/docx/src/read/freeform.rs index 95b224acc..d83521f53 100644 --- a/crates/docx/src/read/freeform.rs +++ b/crates/docx/src/read/freeform.rs @@ -7,6 +7,10 @@ use crate::xml::El; /// Straight pieces a Bézier curve is flattened into. const CURVE_STEPS: usize = 8; +/// Points read from one path segment. A conformant `a:lnTo`/`a:moveTo` has one, `a:quadBezTo` +/// two and `a:cubicBezTo` three; the cap bounds the de Casteljau control polygon so a segment +/// stuffed with points can't make curve flattening quadratic in the point count. +const MAX_SEG_POINTS: usize = 8; /// Largest path coordinate kept, in EMUs. const MAX_EMU: f64 = 1.0e12; @@ -41,7 +45,7 @@ pub fn cust_geom(sppr: &El, w: f32, h: f32) -> Option { if cur.len() >= MAX_POINTS || done.len() >= MAX_PATHS { break; } - let pts: Vec<[f64; 2]> = seg.children("a:pt").filter_map(pt).collect(); + let pts: Vec<[f64; 2]> = seg.children("a:pt").filter_map(pt).take(MAX_SEG_POINTS).collect(); match seg.local() { "moveTo" => { if !cur.is_empty() { @@ -118,3 +122,36 @@ fn bezier(c: &[[f64; 2]], t: f64) -> [f64; 2] { } p.first().copied().unwrap_or([0.0, 0.0]) } + +#[cfg(test)] +mod tests { + use super::*; + + fn sppr(seg: &str) -> crate::xml::El { + let xml = format!( + "\ + \ + {seg}" + ); + crate::xml::parse(xml.as_bytes()).expect("parse spPr") + } + + #[test] + fn a_bezier_segment_with_many_points_is_bounded() { + // One cubicBezTo stuffed with 50 000 points must not make de Casteljau quadratic: the + // control polygon is capped, so flattening stays a handful of points and returns at once. + let pts = "".repeat(50_000); + let sppr = sppr(&format!("{pts}")); + let f = cust_geom(&sppr, 100.0, 100.0).expect("a path"); + assert!(f.paths.iter().all(|p| p.pts.len() <= 1 + CURVE_STEPS), "{:?}", f.paths.iter().map(|p| p.pts.len()).collect::>()); + } + + #[test] + fn a_conformant_cubic_still_flattens_to_a_curve() { + let sppr = sppr(""); + let f = cust_geom(&sppr, 100.0, 100.0).expect("a path"); + assert_eq!(f.paths.len(), 1); + assert!(f.paths[0].pts.len() >= 2 && f.paths[0].pts.len() <= 1 + CURVE_STEPS); + } +} diff --git a/crates/formats/src/html.rs b/crates/formats/src/html.rs index 536a46542..0c6c975b1 100644 --- a/crates/formats/src/html.rs +++ b/crates/formats/src/html.rs @@ -824,7 +824,10 @@ impl<'r> Builder<'r> { t.rows.push(r); } let borderless = !t.bordered; - let mut ft = FTable { rows: t.rows.into_iter().filter(|r| !r.is_empty()).collect(), widths: Vec::new(), borderless }; + // Keep at most MAX_ROWS rows (the model drops the rest anyway): bounds the + // O(rows × rowspan) work of `insert_covered` on a table of millions of ``. + let rows: Vec<_> = t.rows.into_iter().filter(|r| !r.is_empty()).take(wordcraft_doc::table::MAX_ROWS).collect(); + let mut ft = FTable { rows, widths: Vec::new(), borderless }; if !ft.rows.is_empty() { ft.insert_covered(); self.container().push(FBlock::Table(ft)); diff --git a/crates/formats/src/latex.rs b/crates/formats/src/latex.rs index d9e799046..c7c555b85 100644 --- a/crates/formats/src/latex.rs +++ b/crates/formats/src/latex.rs @@ -317,8 +317,12 @@ impl Writer { } if let Some(l) = f.link.as_deref().filter(|l| !l.is_empty()) { s = match l.strip_prefix('#') { + // An internal bookmark reference, not a URL. Some(target) => format!("\\hyperlink{{{}}}{{{s}}}", escape_url(target)), - None => format!("\\href{{{}}}{{{s}}}", escape_url(l)), + // A script, `file:`, UNC or other non-web target becomes plain text: a `\href` + // compiles to a PDF `/URI` action the reader would otherwise follow on click. + None if wordcraft_doc::link::is_safe_href(l) => format!("\\href{{{}}}{{{s}}}", escape_url(l)), + None => s, }; } s diff --git a/crates/formats/src/model.rs b/crates/formats/src/model.rs index 38d187fdc..e23f13895 100644 --- a/crates/formats/src/model.rs +++ b/crates/formats/src/model.rs @@ -265,6 +265,12 @@ impl FTable { /// After an importer that gives `rowspan` but no placeholder cells (HTML): insert covered /// cells into the rows below each spanning cell. pub fn insert_covered(&mut self) { + // Each spanning cell touches up to `rowspan - 1` rows below it; a table of many rows each + // carrying a large rowspan would otherwise be O(rows × rowspan) covered cells, each of + // which becomes a document cell downstream. Cap the total touched so a small file can't + // multiply into a huge table, whatever the rowspans claim. + const MAX_COVERED_WORK: usize = 1 << 17; + let mut work = 0usize; let nrows = self.rows.len(); for r in 0..nrows { let mut g = 0usize; @@ -273,6 +279,10 @@ impl FTable { let (span, rs, covered) = (c.colspan.clamp(1, 63) as usize, c.rowspan.clamp(1, 1000) as usize, c.covered); if rs > 1 && !covered { for k in 1..rs { + work += 1; + if work > MAX_COVERED_WORK { + return; + } let Some(row) = self.rows.get_mut(r + k) else { break }; // Find the index in that row at grid column `g`. let mut x = 0usize; @@ -603,7 +613,10 @@ impl<'a> Builder<'a> { } let cols = t.cols().clamp(1, wordcraft_doc::table::MAX_COLS); let width = self.doc.last_section.text_width().max(72.0); - let mut tb = Table::new(rows.len(), cols, width); + // One template row for the grid widths and props; the real rows are built below. Sizing it + // to `rows.len()` would allocate a full rows × cols cell grid only to clear it, which a + // tall table (many rows, wide from merged cells) turns into a large transient spike. + let mut tb = Table::new(1, cols, width); if t.borderless { tb.props.style = None; } @@ -1021,6 +1034,21 @@ pub fn image_px(data: &[u8]) -> Option<(u32, u32)> { image::ImageReader::new(std::io::Cursor::new(data)).with_guessed_format().ok()?.into_dimensions().ok() } +/// Largest allocation one decoded picture may make when it is re-encoded on export: the `image` +/// crate's default, stated here so the bound doesn't depend on a library default that may change. +const MAX_DECODE_ALLOC: u64 = 512 << 20; + +/// Decode an encoded image with a bounded allocation, so a small file that declares huge +/// dimensions can't exhaust memory when a picture is re-encoded on export. `None` when it would +/// be too large or doesn't decode. +pub fn decode_bounded(data: &[u8]) -> Option { + let mut reader = image::ImageReader::new(std::io::Cursor::new(data)).with_guessed_format().ok()?; + let mut limits = image::Limits::default(); + limits.max_alloc = Some(MAX_DECODE_ALLOC); + reader.limits(limits); + reader.decode().ok() +} + /// A picture from bytes with an optional display size (points); the natural size (96 dpi) is /// used otherwise, shrunk to fit a 6.5" column. pub fn make_img(data: impl Into>>, w: Option, h: Option, alt: &str) -> Option { @@ -1225,6 +1253,16 @@ pub fn parse_color(s: &str) -> Option { mod tests { use super::*; + #[test] + fn insert_covered_is_bounded_on_hostile_rowspan() { + // Many rows each carrying the maximum rowspan would be O(rows × rowspan) covered cells; + // the work cap must keep insertion bounded (and quick) instead of exhausting memory. + let mut t = + FTable { rows: (0..20_000).map(|_| vec![Cell { rowspan: 1000, ..Default::default() }]).collect(), widths: Vec::new(), borderless: false }; + t.insert_covered(); + assert!(t.rows.iter().all(|r| r.len() <= 64), "a row grew past the column cap"); + } + #[test] fn base64_round_trip() { for data in [&b""[..], b"f", b"fo", b"foo", b"foob", b"fooba", b"foobar", &[0, 255, 128, 7]] { diff --git a/crates/formats/src/odt.rs b/crates/formats/src/odt.rs index 3c6491b03..4e315010d 100644 --- a/crates/formats/src/odt.rs +++ b/crates/formats/src/odt.rs @@ -357,7 +357,9 @@ impl Writer { flush(&mut body, &mut spaces, prev_space); prev_space = true; } - let link = f.link.as_ref(); + // A script, `file:`, UNC or other non-web target is written as plain text, as + // the HTML and Markdown exporters do (LibreOffice follows `file:`/UNC hrefs). + let link = f.link.as_deref().filter(|l| wordcraft_doc::link::is_safe_href(l)); if let Some(l) = link { out.push_str(&format!("", x(l))); } diff --git a/crates/formats/src/rtf.rs b/crates/formats/src/rtf.rs index 53dc89e04..d358179f4 100644 --- a/crates/formats/src/rtf.rs +++ b/crates/formats/src/rtf.rs @@ -173,7 +173,10 @@ impl Writer { ctl.push_str(&format!("\\chcbpat{i}")); } let body = if ctl.is_empty() { esc(t) } else { format!("{{{ctl} {}}}", esc(t)) }; - match &f.link { + // A script, `file:`, UNC or other non-web target is written as plain text, as the HTML and + // Markdown exporters do: Word follows a HYPERLINK field's target on click, and a UNC one + // leaks the reader's NTLM credentials. + match f.link.as_deref().filter(|l| wordcraft_doc::link::is_safe_href(l)) { Some(l) => { out.push_str(&format!("{{\\field{{\\*\\fldinst{{HYPERLINK \"{}\"}}}}{{\\fldrslt{{{}}}}}}}", esc(&l.replace('"', "%22")), body)); } @@ -186,8 +189,9 @@ impl Writer { "png" => (img.data.to_vec(), "\\pngblip"), "jpeg" | "jpg" => (img.data.to_vec(), "\\jpegblip"), _ => { - // Re-encode other formats as PNG. - let Ok(dec) = image::load_from_memory(&img.data) else { return }; + // Re-encode other formats as PNG, bounding the decode so a tiny file declaring + // huge dimensions can't exhaust memory on export. + let Some(dec) = model::decode_bounded(&img.data) else { return }; let mut buf = Vec::new(); if dec.write_to(&mut std::io::Cursor::new(&mut buf), image::ImageFormat::Png).is_err() { return; diff --git a/crates/layout/src/lib.rs b/crates/layout/src/lib.rs index 2278cfaae..efa2f3c5d 100644 --- a/crates/layout/src/lib.rs +++ b/crates/layout/src/lib.rs @@ -3456,6 +3456,13 @@ fn place_table(ctx: &mut Ctx, pb: &mut PageBuilder, t: &wordcraft_doc::Table, bl // A table in the text flow starts below the floating objects in its way. below_floats(pb, pb.col_x() + tl.x, pb.col_x() + tl.x + tl.width); let header_rows: Vec = (0..t.rows.len()).take_while(|r| t.rows.get(*r).is_some_and(|row| row.props.header)).collect(); + // Header rows are re-placed on every page a table spills onto, so a table with many header + // rows that each spill onto their own page is O(header rows × pages) placed items — a few KB + // of document can otherwise fill memory. Once this many header rows have been repeated across + // the whole table, later pages carry none; pagination still finishes. Far above any real + // table (e.g. 3 header rows over 30 000 pages). + const MAX_HEADER_REPEATS: usize = 200_000; + let mut header_repeats = 0usize; let place = |pb: &mut PageBuilder, row: &table::RowLayout| { let (x, y) = (pb.col_x() + tl.x, pb.y); if let Some(pg) = pb.page() { @@ -3497,11 +3504,15 @@ fn place_table(ctx: &mut Ctx, pb: &mut PageBuilder, t: &wordcraft_doc::Table, bl None => {} } pb.advance(block, body_top); - // Repeat header rows. - if !header_rows.contains(&ri) { + // Repeat header rows, up to this table's cap. + if !header_rows.contains(&ri) && header_repeats < MAX_HEADER_REPEATS { for hr in &header_rows { + if header_repeats >= MAX_HEADER_REPEATS { + break; + } if let Some(h) = tl.rows.get(*hr) { place(pb, h); + header_repeats += 1; } } } diff --git a/crates/pdf/src/lib.rs b/crates/pdf/src/lib.rs index 1cb097fc3..e73ccb8e5 100644 --- a/crates/pdf/src/lib.rs +++ b/crates/pdf/src/lib.rs @@ -1641,7 +1641,15 @@ fn path_from(b: &kurbo::BezPath) -> Option { /// can't fail the whole export). fn load_image(data: &Arc>) -> Option { let fmt = image::guess_format(data).ok()?; - let dec = image::load_from_memory_with_format(data, fmt).ok()?; + // The side check below used to run only after a full decode; applying it as a decode limit + // rejects a small file that declares a huge image at header time instead. The default + // allocation cap (512 MB, what `load_from_memory_with_format` applied) stays in force. + let mut reader = image::ImageReader::new(std::io::Cursor::new(data.as_slice())).with_guessed_format().ok()?; + let mut limits = image::Limits::default(); + limits.max_image_width = Some(MAX_IMAGE_SIDE); + limits.max_image_height = Some(MAX_IMAGE_SIDE); + reader.limits(limits); + let dec = reader.decode().ok()?; let (w, h) = (dec.width(), dec.height()); if w == 0 || h == 0 || w > MAX_IMAGE_SIDE || h > MAX_IMAGE_SIDE { return None; diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index bd7ef50b7..972c3c638 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -167,9 +167,20 @@ fn vectors() -> &'static Mutex>>> { C.get_or_init(|| Mutex::new(MediaCache { map: HashMap::new(), bytes: 0 })) } -/// Decode an encoded image to a premultiplied pixmap. +/// Largest side (pixels) a document picture may decode to: what a `Pixmap` can hold (`u16` +/// sides, which [`premul_pixmap`] enforces after decoding). Applying it as a decode limit rejects +/// a small file that declares a bigger image at header time, before anything is allocated. +const MAX_DECODE_SIDE: u32 = u16::MAX as u32; + +/// Decode an encoded image to a premultiplied pixmap. The dimension limit above and the default +/// allocation cap (512 MB, what `load_from_memory` applied) bound the decode during paint. pub fn decode_pixmap(bytes: &[u8]) -> Option { - let img = image::load_from_memory(bytes).ok()?.to_rgba8(); + let mut reader = image::ImageReader::new(std::io::Cursor::new(bytes)).with_guessed_format().ok()?; + let mut limits = image::Limits::default(); + limits.max_image_width = Some(MAX_DECODE_SIDE); + limits.max_image_height = Some(MAX_DECODE_SIDE); + reader.limits(limits); + let img = reader.decode().ok()?.to_rgba8(); let (w, h) = img.dimensions(); premul_pixmap(w, h, img.as_raw()) }