Merge pull request #1335 from storytold/codex/xml-dtd-review-hardening

fix(xml): address DTD validator review findings
This commit is contained in:
Dylan M. Taylor authored and GitHub committed 2026-11-14 14:41:44 -05:00
commit cbf6ea8584
1 file changed
+364 -49
+364 -49
View File
@@ -3,11 +3,19 @@
//! Document Type Definitions: element declarations (content models) and required attributes, and
//! validation of an XML document against them (Structure ▸ Validate).
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
/// How much text parameter entities may add during expansion (a pass count is not an output
/// bound: each pass replaces whole strings, so references can inflate multiplicatively).
const MAX_EXPANDED_BYTES: usize = 1024 * 1024;
const MAX_DTD_BYTES: usize = 1024 * 1024;
const MAX_VALIDATION_XML_BYTES: usize = 16 * 1024 * 1024;
const MAX_VALIDATION_NODES: usize = 100_000;
const MAX_VALIDATION_DEPTH: usize = 256;
const MAX_VALIDATION_ATTRIBUTES: usize = 1_000_000;
const MAX_MATCH_WORK: usize = 20_000_000;
const MAX_MATCH_POSITIONS: usize = 5_000_000;
const MAX_MATCH_STATES: usize = 100_000;
/// A content particle: a name, a sequence, a choice, or one of those repeated (`?`, `*`, `+`).
#[derive(Clone, Debug, PartialEq)]
@@ -54,32 +62,38 @@ impl Dtd {
}
/// Markup declarations (`<!…>`) with comments removed and quoted strings kept whole.
fn declarations(src: &str) -> Vec<String> {
fn declarations(src: &str) -> Result<Vec<String>, String> {
let mut out = Vec::new();
let mut rest = src;
while let Some(i) = rest.find("<!") {
rest = &rest[i..];
if rest.starts_with("<!--") {
rest = rest.find("-->").map_or("", |j| &rest[j + 3..]);
let Some(j) = rest.find("-->") else {
return Err("unterminated DTD comment".into());
};
rest = &rest[j + 3..];
continue;
}
let mut quote = None;
let mut end = rest.len();
let mut end = None;
for (k, c) in rest.char_indices().skip(2) {
match (quote, c) {
(Some(q), c) if c == q => quote = None,
(None, '"' | '\'') => quote = Some(c),
(None, '>') => {
end = k;
end = Some(k);
break;
}
_ => {}
}
}
let Some(end) = end else {
return Err("unterminated DTD declaration".into());
};
out.push(rest[2..end].to_string());
rest = rest.get(end + 1..).unwrap_or("");
rest = &rest[end + 1..];
}
out
Ok(out)
}
fn tokens(s: &str) -> Vec<String> {
@@ -159,8 +173,48 @@ fn parse_model(s: &str) -> Result<Model, String> {
_ => {}
}
if s.contains("#PCDATA") {
let t = tokens(&s.replace("#PCDATA", " #PCDATA "));
return Ok(Model::Mixed(t.into_iter().filter(|x| !"()|*".contains(x.as_str()) && x != "#PCDATA").collect()));
let t = tokens(s);
if t.first().map(String::as_str) != Some("(") || t.get(1).map(String::as_str) != Some("#PCDATA") {
return Err("malformed mixed content model".into());
}
let mut i = 2;
let mut allowed = Vec::new();
if t.get(i).map(String::as_str) == Some(")") {
i += 1;
if t.get(i).map(String::as_str) == Some("*") {
i += 1;
}
if i != t.len() {
return Err("unexpected token after mixed content model".into());
}
return Ok(Model::Mixed(allowed));
}
loop {
if t.get(i).map(String::as_str) != Some("|") {
return Err("expected `|` in mixed content model".into());
}
i += 1;
let Some(name) = t.get(i).filter(|name| !"(),|?*+".contains(name.as_str())) else {
return Err("expected element name in mixed content model".into());
};
if allowed.contains(name) {
return Err(format!("duplicate `{name}` in mixed content model"));
}
allowed.push(name.clone());
i += 1;
match t.get(i).map(String::as_str) {
Some("|") => continue,
Some(")") => {
i += 1;
break;
}
_ => return Err("expected `|` or `)` in mixed content model".into()),
}
}
if t.get(i).map(String::as_str) != Some("*") || i + 1 != t.len() {
return Err("mixed content with element names must end in `*`".into());
}
return Ok(Model::Mixed(allowed));
}
let t = tokens(s);
let mut i = 0;
@@ -196,7 +250,33 @@ fn expand_parameter_entities(src: &str, ents: &[(String, String)]) -> Result<Str
let mut out = String::with_capacity(text.len());
let mut rest = text.as_str();
let mut changed = false;
while let Some(i) = rest.find('%') {
loop {
let mut special = None;
for (i, c) in rest.char_indices() {
if c == '%' {
special = Some((i, false));
break;
}
if c == '<' && rest[i..].starts_with("<!--") {
special = Some((i, true));
break;
}
}
let Some((i, is_comment)) = special else {
append(&mut out, rest)?;
rest = "";
break;
};
if is_comment {
append(&mut out, &rest[..i])?;
let comment_tail = &rest[i..];
let Some(end) = comment_tail.find("-->") else {
return Err("unterminated DTD comment".into());
};
append(&mut out, &comment_tail[..end + 3])?;
rest = &comment_tail[end + 3..];
continue;
}
let (prefix, reference) = rest.split_at(i);
append(&mut out, prefix)?;
// `find` points to an ASCII `%`, so both split positions are UTF-8 boundaries.
@@ -231,9 +311,12 @@ fn expand_parameter_entities(src: &str, ents: &[(String, String)]) -> Result<Str
/// Parse a DTD (its element and attribute-list declarations; parameter entities are expanded).
pub fn parse(src: &str) -> Result<Dtd, String> {
if src.len() > MAX_DTD_BYTES {
return Err(format!("DTD exceeds the {MAX_DTD_BYTES} byte parsing limit"));
}
// Parameter entities first, then expand references and read the declarations again.
let mut ents: Vec<(String, String)> = Vec::new();
for d in declarations(src) {
for d in declarations(src)? {
if let Some(rest) = d.strip_prefix("ENTITY") {
let rest = rest.trim_start();
if let Some(rest) = rest.strip_prefix('%') {
@@ -249,7 +332,7 @@ pub fn parse(src: &str) -> Result<Dtd, String> {
}
let text = expand_parameter_entities(src, &ents)?;
let mut dtd = Dtd::default();
for d in declarations(&text) {
for d in declarations(&text)? {
if let Some(rest) = d.strip_prefix("ELEMENT") {
let rest = rest.trim_start();
let name: String = rest.chars().take_while(|c| !c.is_whitespace()).collect();
@@ -293,13 +376,31 @@ pub fn parse(src: &str) -> Result<Dtd, String> {
}
let Some((el, attrs)) = words.split_first() else { continue };
let mut k = 0;
while k + 2 < attrs.len() {
while k < attrs.len() {
let name = &attrs[k];
let default = attrs.get(k + 2).map(String::as_str).unwrap_or("");
let Some(ty) = attrs.get(k + 1) else {
return Err(format!("incomplete ATTLIST for `{el}`"));
};
k += 2;
// NOTATION is a two-token type: the keyword followed by its enumeration.
if ty == "NOTATION" {
if attrs.get(k).is_none_or(|group| !group.starts_with('(') || !group.ends_with(')')) {
return Err(format!("malformed NOTATION type for `{name}`"));
}
k += 1;
}
let Some(default) = attrs.get(k).map(String::as_str) else {
return Err(format!("missing default declaration for `{name}`"));
};
k += 1;
if default == "#REQUIRED" {
dtd.required.entry(el.clone()).or_default().push(name.clone());
} else if default == "#FIXED" {
if k >= attrs.len() {
return Err(format!("missing #FIXED value for `{name}`"));
}
k += 1;
}
k += if default == "#FIXED" { 4 } else { 3 };
}
}
}
@@ -313,18 +414,61 @@ pub fn parse(src: &str) -> Result<Dtd, String> {
/// address of its node, which lives as long as the model) from a position.
type EndsMemo = HashMap<(usize, usize), Vec<usize>>;
#[derive(Default)]
struct MatchBudget {
work: usize,
positions: usize,
states: usize,
exhausted: bool,
reported: bool,
}
impl MatchBudget {
fn spend(&mut self, work: usize, positions: usize) -> Result<(), &'static str> {
if self.exhausted {
return Err("content model matching exceeded its resource budget");
}
self.work = self.work.saturating_add(work);
self.positions = self.positions.saturating_add(positions);
if self.work > MAX_MATCH_WORK {
self.exhausted = true;
return Err("content model matching exceeded its work limit");
}
if self.positions > MAX_MATCH_POSITIONS {
self.exhausted = true;
return Err("content model matching exceeded its memory limit");
}
Ok(())
}
fn retain_state(&mut self) -> Result<(), &'static str> {
self.states = self.states.saturating_add(1);
if self.states > MAX_MATCH_STATES {
self.exhausted = true;
return Err("content model matching exceeded its state limit");
}
Ok(())
}
}
/// End positions after `cp` matches a prefix of `seq[i..]`.
///
/// The positions of each group are worked out once per start position (`memo`): nested repetitions
/// such as `((a*)*)*` ask the inner group for the same positions again and again, which is
/// exponential in their depth without it.
fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo, budget: &mut MatchBudget) -> Result<Vec<usize>, &'static str> {
budget.spend(1, 0)?;
if let Cp::Name(n) = cp {
return if seq.get(i) == Some(&n.as_str()) { vec![i + 1] } else { vec![] };
if seq.get(i) == Some(&n.as_str()) {
budget.spend(0, 1)?;
return Ok(vec![i + 1]);
}
return Ok(vec![]);
}
let key = (std::ptr::from_ref(cp) as usize, i);
if let Some(done) = memo.get(&key) {
return done.clone();
budget.spend(0, done.len())?;
return Ok(done.clone());
}
let mut out: Vec<usize> = match cp {
Cp::Name(_) => vec![],
@@ -334,7 +478,9 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
for c in items {
let mut next = Vec::new();
for p in at {
next.extend(ends(c, seq, p, memo));
let found = ends(c, seq, p, memo, budget)?;
budget.spend(found.len(), found.len())?;
next.extend(found);
}
next.sort_unstable();
next.dedup();
@@ -345,22 +491,41 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
Cp::Choice(items) => {
let mut all = Vec::new();
for c in items {
all.extend(ends(c, seq, i, memo));
let found = ends(c, seq, i, memo, budget)?;
budget.spend(found.len(), found.len())?;
all.extend(found);
}
all
}
Cp::Rep(c, r) => {
let mut all = if *r == '+' { vec![] } else { vec![i] };
let mut frontier = ends(c, seq, i, memo);
let mut seen: HashSet<usize> = all.iter().copied().collect();
let mut frontier = ends(c, seq, i, memo, budget)?;
budget.spend(frontier.len(), frontier.len())?;
if *r == '?' {
all.extend(frontier);
} else {
while !frontier.is_empty() {
let new: Vec<usize> = frontier.iter().copied().filter(|p| !all.contains(p)).collect();
let mut new = Vec::new();
for p in frontier.iter().copied() {
budget.spend(1, 0)?;
if seen.insert(p) {
budget.spend(0, 1)?;
new.push(p);
}
}
budget.spend(new.len(), new.len())?;
all.extend(&new);
let mut next = Vec::new();
let mut next_seen = HashSet::new();
for p in new.into_iter().filter(|&p| p > i) {
next.extend(ends(c, seq, p, memo).into_iter().filter(|p| !all.contains(p)));
for found in ends(c, seq, p, memo, budget)? {
budget.spend(1, 0)?;
if !seen.contains(&found) && next_seen.insert(found) {
budget.spend(0, 1)?;
next.push(found);
}
}
}
// Two paths to one position are one position: kept apart they multiply per round.
next.sort_unstable();
@@ -373,8 +538,10 @@ fn ends(cp: &Cp, seq: &[&str], i: usize, memo: &mut EndsMemo) -> Vec<usize> {
};
out.sort_unstable();
out.dedup();
budget.spend(out.len(), out.len())?;
budget.retain_state()?;
memo.insert(key, out.clone());
out
Ok(out)
}
fn show(cp: &Cp) -> String {
@@ -392,60 +559,130 @@ struct Node {
attrs: Vec<String>,
children: Vec<Node>,
text: bool,
content: bool,
}
fn bounded_node(e: &quick_xml::events::BytesStart<'_>, depth: usize, nodes: &mut usize, attributes: &mut usize) -> Result<Node, String> {
if depth >= MAX_VALIDATION_DEPTH {
return Err(format!("XML nesting exceeds the {MAX_VALIDATION_DEPTH} level validation limit"));
}
*nodes = nodes.saturating_add(1);
if *nodes > MAX_VALIDATION_NODES {
return Err(format!("XML exceeds the {MAX_VALIDATION_NODES} node validation limit"));
}
let mut attrs = Vec::new();
for attr in e.attributes().with_checks(true) {
let attr = attr.map_err(|err| format!("invalid attribute: {err}"))?;
attrs.push(String::from_utf8_lossy(attr.key.as_ref()).to_string());
}
*attributes = attributes.saturating_add(attrs.len());
if *attributes > MAX_VALIDATION_ATTRIBUTES {
return Err(format!("XML exceeds the {MAX_VALIDATION_ATTRIBUTES} attribute validation limit"));
}
Ok(Node { name: String::from_utf8_lossy(e.name().as_ref()).to_string(), attrs, children: vec![], text: false, content: false })
}
fn read_tree(xml: &str) -> Result<Node, String> {
use quick_xml::events::Event;
if xml.len() > MAX_VALIDATION_XML_BYTES {
return Err(format!("XML exceeds the {} byte validation limit", MAX_VALIDATION_XML_BYTES));
}
let mut r = quick_xml::Reader::from_str(xml);
let mut stack: Vec<Node> = Vec::new();
let node = |e: &quick_xml::events::BytesStart| Node {
name: String::from_utf8_lossy(e.name().as_ref()).to_string(),
attrs: e.attributes().flatten().map(|a| String::from_utf8_lossy(a.key.as_ref()).to_string()).collect(),
children: vec![],
text: false,
};
let mut root = None;
let mut nodes = 0usize;
let mut attributes = 0usize;
loop {
match r.read_event().map_err(|e| e.to_string())? {
Event::Start(e) => stack.push(node(&e)),
Event::Start(e) => {
if stack.is_empty() && root.is_some() {
return Err("multiple root elements".into());
}
let n = bounded_node(&e, stack.len(), &mut nodes, &mut attributes)?;
if let Some(parent) = stack.last_mut() {
parent.content = true;
}
stack.push(n);
}
Event::Empty(e) => {
let n = node(&e);
if stack.is_empty() && root.is_some() {
return Err("multiple root elements".into());
}
let n = bounded_node(&e, stack.len(), &mut nodes, &mut attributes)?;
match stack.last_mut() {
Some(p) => p.children.push(n),
None => return Ok(n),
Some(p) => {
p.content = true;
p.children.push(n);
}
None => root = Some(n),
}
}
Event::End(_) => {
Event::End(e) => {
let n = stack.pop().ok_or("unbalanced end tag")?;
if n.name.as_bytes() != e.name().as_ref() {
return Err("mismatched end tag".into());
}
match stack.last_mut() {
Some(p) => p.children.push(n),
None => return Ok(n),
None => root = Some(n),
}
}
Event::Text(t) => {
if let Some(p) = stack.last_mut()
&& !t.iter().all(u8::is_ascii_whitespace)
{
p.text = true;
if let Some(p) = stack.last_mut() {
p.content = true;
if !t.iter().all(u8::is_ascii_whitespace) {
p.text = true;
}
} else if !t.iter().all(u8::is_ascii_whitespace) {
return Err("text outside the root element".into());
}
}
Event::CData(_) | Event::GeneralRef(_) => {
Event::CData(_) => {
if let Some(p) = stack.last_mut() {
p.text = true;
p.content = true;
}
}
Event::Eof => return Err("no root element".into()),
Event::GeneralRef(reference) => {
let name = reference.decode().map_err(|e| e.to_string())?;
let accepted = if let Some(character) = reference.resolve_char_ref().map_err(|e| e.to_string())? {
crate::is_xml_char(character)
} else {
matches!(name.as_ref(), "amp" | "lt" | "gt" | "apos" | "quot")
};
if !accepted {
return Err(format!("unsupported general entity reference `&{name};`"));
}
if let Some(p) = stack.last_mut() {
p.text = true;
p.content = true;
} else {
return Err("reference outside the root element".into());
}
}
Event::Comment(_) | Event::PI(_) => {
if let Some(parent) = stack.last_mut() {
parent.content = true;
}
}
Event::Eof => {
if !stack.is_empty() {
return Err("unclosed element".into());
}
return root.ok_or_else(|| "no root element".into());
}
_ => {}
}
}
}
fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>) {
fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>, budget: &mut MatchBudget) {
let mut problem = |m: String| out.push(Problem { path: path.to_string(), message: m });
match dtd.model(&n.name) {
None => problem(format!("`{}` isn't declared in the DTD", n.name)),
Some(Model::Any) => {}
Some(Model::Empty) => {
if n.text || !n.children.is_empty() {
if n.content {
problem(format!("`{}` should be empty", n.name));
}
}
@@ -461,9 +698,19 @@ fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>) {
problem(format!("`{}` can't contain text", n.name));
}
let seq: Vec<&str> = n.children.iter().map(|c| c.name.as_str()).collect();
if !ends(cp, &seq, 0, &mut EndsMemo::new()).contains(&seq.len()) {
let got = if seq.is_empty() { "nothing".to_string() } else { seq.join(", ") };
problem(format!("`{}` contains {got}; the DTD asks for {}", n.name, show(cp)));
let matched = ends(cp, &seq, 0, &mut EndsMemo::new(), budget);
match matched {
Ok(positions) if positions.contains(&seq.len()) => {}
Ok(_) => {
let got = if seq.is_empty() { "nothing".to_string() } else { seq.join(", ") };
problem(format!("`{}` contains {got}; the DTD asks for {}", n.name, show(cp)));
}
Err(error) => {
if !budget.reported {
problem(format!("`{}`: {error}", n.name));
budget.reported = true;
}
}
}
}
}
@@ -476,17 +723,21 @@ fn check(dtd: &Dtd, n: &Node, path: &str, out: &mut Vec<Problem>) {
for c in &n.children {
let k = seen.entry(c.name.as_str()).or_default();
*k += 1;
check(dtd, c, &format!("{path}/{}[{k}]", c.name), out);
check(dtd, c, &format!("{path}/{}[{k}]", c.name), out, budget);
}
}
/// Validate `xml` against `dtd`; an empty list when it's valid.
///
/// Validation rejects input above the internal byte, depth, node, attribute and content-model
/// matching budgets. Only predefined entities and numeric character references are resolved; a
/// general entity reference is reported as unsupported rather than expanded or fetched.
pub fn validate(dtd: &Dtd, xml: &str) -> Vec<Problem> {
match read_tree(xml) {
Ok(root) => {
let mut out = Vec::new();
let path = root.name.clone();
check(dtd, &root, &path, &mut out);
check(dtd, &root, &path, &mut out, &mut MatchBudget::default());
out
}
Err(e) => vec![Problem { path: String::new(), message: format!("not well-formed XML: {e}") }],
@@ -745,4 +996,68 @@ mod tests {
assert_eq!(problems.len(), 1, "{problems:?}");
assert!(problems[0].message.starts_with("`r` contains a, a, a"), "{problems:?}");
}
#[test]
fn incomplete_declarations_and_malformed_mixed_models_are_rejected() {
assert!(parse("<!ELEMENT r EMPTY").is_err());
assert!(parse("<!-- unterminated").is_err());
for model in ["(#PCDATA | a)", "(#PCDATA, a)*", "(#PCDATA | )*", "(#PCDATA | a)* junk"] {
assert!(parse(&format!("<!ELEMENT r {model}>")).is_err(), "{model}");
}
assert!(parse("<!ELEMENT r (#PCDATA)>").is_ok());
assert!(parse("<!ELEMENT r (#PCDATA)*>").is_ok());
assert!(parse("<!ELEMENT r (#PCDATA | a)*>").is_ok());
}
#[test]
fn parameter_entities_are_not_expanded_inside_comments() {
let dtd = parse(
r#"<!ENTITY % e "--> <!ELEMENT r ANY> <!--">
<!-- %e; -->
<!ELEMENT r EMPTY>
<!ELEMENT a EMPTY>"#,
)
.unwrap();
assert_eq!(dtd.model("r"), Some(&Model::Empty));
assert!(!validate(&dtd, "<r><a/></r>").is_empty());
}
#[test]
fn notation_attribute_type_keeps_following_required_attribute_aligned() {
let dtd = parse("<!ELEMENT r ANY><!ATTLIST r kind NOTATION (png|jpeg) #IMPLIED id CDATA #REQUIRED>").unwrap();
assert!(validate(&dtd, "<r kind='png'/>").iter().any(|p| p.message.contains("required attribute `id`")));
assert!(validate(&dtd, "<r id='x'/>").is_empty());
}
#[test]
fn malformed_or_unbounded_xml_is_rejected_before_validation() {
let dtd = parse("<!ELEMENT r ANY><!ELEMENT a EMPTY>").unwrap();
for xml in ["<r/><broken", "<r></r><a/>", "<r a='1' a='2'/>"] {
assert!(!validate(&dtd, xml).is_empty(), "{xml}");
}
let deep = format!("{}{}", "<r>".repeat(MAX_VALIDATION_DEPTH + 1), "</r>".repeat(MAX_VALIDATION_DEPTH + 1));
let problems = validate(&dtd, &deep);
assert!(problems.iter().any(|p| p.message.contains("nesting exceeds")), "{problems:?}");
}
#[test]
fn references_must_be_declared_and_empty_elements_must_have_no_content() {
let dtd = parse("<!ELEMENT r ANY><!ELEMENT e EMPTY><!ENTITY known 'value'>").unwrap();
assert!(validate(&dtd, "<r>&missing;</r>").iter().any(|p| p.message.contains("unsupported general entity")));
assert!(validate(&dtd, "<r>&known;</r>").iter().any(|p| p.message.contains("unsupported general entity")));
assert!(validate(&dtd, "<r>&#x41;&amp;</r>").is_empty());
assert!(!validate(&dtd, "<r>&#0;</r>").is_empty());
for xml in ["<e> </e>", "<e><!--x--></e>", "<e><?work x?></e>", "<e>&amp;</e>"] {
assert!(validate(&dtd, xml).iter().any(|p| p.message.contains("should be empty")), "{xml}");
}
assert!(validate(&dtd, "<e/>").is_empty());
}
#[test]
fn ambiguous_content_matching_stops_at_its_budget() {
let dtd = parse("<!ELEMENT r ((a*)*)*><!ELEMENT a EMPTY>").unwrap();
let xml = format!("<r>{}</r>", "<a/>".repeat(10_000));
let problems = validate(&dtd, &xml);
assert!(problems.iter().any(|p| p.message.contains("matching exceeded its ")), "{}", problems.len());
}
}