Merge pull request #5406 from storytold/fix/sec-svg-entity-estimate

SVG: the entity expansion cap counts every declaration and refuses names it can't follow

crates/svg/src/entities.rs

@@ -10,9 +10,14 @@ const MAX_EXPANDED: usize = 16 << 20;
 /// How deep entities that refer to entities are followed (the reader's own limit is lower).
 const MAX_NESTING: u32 = 16;
 
-/// The internal general entities declared in `svg`: name → value.
-fn declared(svg: &str) -> HashMap<&str, &str> {
-    let mut map = HashMap::new();
+/// The longest entity name a reference is looked for with ([`references`]).
+const MAX_NAME: usize = 256;
+
+/// The internal general entities declared in `svg`: name → every value declared for it. Text that
+/// only looks like a declaration (in a comment, or in another entity's value) is kept too: the
+/// largest value counts, so it can only make the estimate larger.
+fn declared(svg: &str) -> HashMap<&str, Vec<&str>> {
+    let mut map: HashMap<&str, Vec<&str>> = HashMap::new();
     for (at, _) in svg.match_indices("<!ENTITY") {
         let rest = svg.get(at + "<!ENTITY".len()..).unwrap_or("").trim_start();
         // A parameter entity (`%`) isn't referenced as `&name;`.
@@ -24,7 +29,7 @@ fn declared(svg: &str) -> HashMap<&str, &str> {
         let rest = rest.trim_start();
         let Some(quote) = rest.chars().next().filter(|q| matches!(q, '"' | '\'')) else { continue };
         let Some(value) = rest.get(1..).and_then(|r| r.split_once(quote)).map(|(v, _)| v) else { continue };
-        map.entry(name).or_insert(value);
+        map.entry(name).or_default().push(value);
     }
     map
 }
@@ -34,27 +39,31 @@ fn references(text: &str) -> impl Iterator<Item = &str> {
     text.match_indices('&').filter_map(|(at, _)| {
         let rest = text.get(at + 1..)?;
         // Look no further than the longest name: a file of `&`s without `;` would be read quadratically.
-        let end = rest.bytes().take(257).position(|b| b == b';')?;
+        let end = rest.bytes().take(MAX_NAME + 1).position(|b| b == b';')?;
         let name = rest.get(..end)?;
-        (!name.is_empty() && name.len() <= 256 && !name.starts_with('#') && !name.contains(char::is_whitespace)).then_some(name)
+        (!name.is_empty() && name.len() <= MAX_NAME && !name.starts_with('#') && !name.contains(char::is_whitespace)).then_some(name)
     })
 }
 
 /// What entity `name` expands to (bytes), references in its value followed.
-fn expanded<'a>(name: &'a str, entities: &HashMap<&'a str, &'a str>, memo: &mut HashMap<&'a str, usize>, depth: u32) -> usize {
+fn expanded<'a>(name: &'a str, entities: &HashMap<&'a str, Vec<&'a str>>, memo: &mut HashMap<&'a str, usize>, depth: u32) -> usize {
     if let Some(n) = memo.get(name) {
         return *n;
     }
-    let Some(value) = entities.get(name) else { return 0 };
+    let Some(values) = entities.get(name) else { return 0 };
     if depth >= MAX_NESTING {
         return usize::MAX;
     }
-    let mut total = value.len();
-    for inner in references(value) {
-        total = total.saturating_add(expanded(inner, entities, memo, depth + 1));
+    let mut most = 0usize;
+    for value in values {
+        let mut total = value.len();
+        for inner in references(value) {
+            total = total.saturating_add(expanded(inner, entities, memo, depth + 1));
+        }
+        most = most.max(total);
     }
-    memo.insert(name, total);
-    total
+    memo.insert(name, most);
+    most
 }
 
 /// Whether the references to the entities `svg` declares expand to more than [`MAX_EXPANDED`]
@@ -64,6 +73,10 @@ pub(crate) fn expansion_too_large(svg: &str) -> bool {
     if entities.is_empty() {
         return false;
     }
+    // A reference to a longer name isn't looked for: refuse such a declaration instead.
+    if entities.keys().any(|name| name.len() > MAX_NAME) {
+        return true;
+    }
     let mut memo = HashMap::new();
     let mut total = 0usize;
     for name in references(svg) {
@@ -96,4 +109,17 @@ mod tests {
         assert!(!expansion_too_large(&ampersands));
         assert!(start.elapsed() < std::time::Duration::from_secs(5), "{:?}", start.elapsed());
     }
+
+    #[test]
+    fn declarations_in_comments_and_long_names_cannot_hide_a_large_entity() {
+        let big = "x".repeat(1 << 20);
+        let refs = "&a;".repeat(20);
+        let commented = format!("<!DOCTYPE svg [<!-- <!ENTITY a \"x\"> --><!ENTITY a \"{big}\">]><svg id=\"{refs}\"/>");
+        assert!(expansion_too_large(&commented));
+        let in_value = format!("<!DOCTYPE svg [<!ENTITY c \"<!ENTITY a 'x'>\"><!ENTITY a \"{big}\">]><svg id=\"{refs}\"/>");
+        assert!(expansion_too_large(&in_value));
+        let long = "n".repeat(MAX_NAME + 1);
+        let long_name = format!("<!DOCTYPE svg [<!ENTITY {long} \"x\">]><svg id=\"&{long};\"/>");
+        assert!(expansion_too_large(&long_name));
+    }
 }