[librsvg: 15/16] (#515) - Add a limit for the number of loaded elements

[Federico Mena Quintero <federico src gnome org>]

commit 572f95f739529b865e2717664d6fefcef9493135
Author: Federico Mena Quintero <federico gnome org>
Date:   Fri Oct 11 18:11:15 2019 -0500

    (#515) - Add a limit for the number of loaded elements
    
    This fixes the last part of #515, an enormous SVG file with millions
    of elements, which causes out-of-memory.
    
    To avoid unbounded memory consumption, we'll set a hard limit on the
    number of loaded elements.  The largest legitimate file I have is a
    map rendering with about 26K elements; here we set a limit of 200,000
    elements for good measure.
    
    Fixes https://gitlab.gnome.org/GNOME/librsvg/issues/515

 librsvg_crate/examples/proportional.rs           |  10 ++++++-
 rsvg_internals/src/limits.rs                     |  33 +++++++++++++++++------
 rsvg_internals/src/xml.rs                        |  30 ++++++++++++++++-----
 tests/errors.c                                   |  15 ++++++++---
 tests/fixtures/errors/515-too-many-elements.svgz | Bin 0 -> 40811 bytes
 5 files changed, 69 insertions(+), 19 deletions(-)
---
diff --git a/librsvg_crate/examples/proportional.rs b/librsvg_crate/examples/proportional.rs
index 9a4834ff..6bb474e9 100644
--- a/librsvg_crate/examples/proportional.rs
+++ b/librsvg_crate/examples/proportional.rs
@@ -28,7 +28,15 @@ fn main() {
 
     assert!(width > 0 && height > 0);
 
-    let handle = librsvg::Loader::new().read_path(input).unwrap();
+    let handle = match librsvg::Loader::new().read_path(input) {
+        Ok(handle) => handle,
+
+        Err(e) => {
+            eprintln!("loading error: {}", e);
+            process::exit(1);
+        }
+    };
+
     let renderer = librsvg::CairoRenderer::new(&handle);
 
     let surface = cairo::ImageSurface::create(cairo::Format::ARgb32, width, height).unwrap();
diff --git a/rsvg_internals/src/limits.rs b/rsvg_internals/src/limits.rs
index f5d58d95..c7ad667a 100644
--- a/rsvg_internals/src/limits.rs
+++ b/rsvg_internals/src/limits.rs
@@ -1,11 +1,28 @@
-/// This is a mitigation for the security-related bug
-/// https://gitlab.gnome.org/GNOME/librsvg/issues/323 - imagine
-/// the XML [billion laughs attack], but done by creating deeply
-/// nested groups of `<use>` elements.  The first one references
-/// the second one ten times, the second one references the third
-/// one ten times, and so on.  In the file given, this causes
-/// 10^17 objects to be rendered.  While this does not exhaust
-/// memory, it would take a really long time.
+/// This is a mitigation for the security-related bugs:
+/// https://gitlab.gnome.org/GNOME/librsvg/issues/323
+/// https://gitlab.gnome.org/GNOME/librsvg/issues/515
+///
+/// Imagine the XML [billion laughs attack], but done in SVG's terms:
+///
+/// - #323 above creates deeply nested groups of `<use>` elements.
+/// The first one references the second one ten times, the second one
+/// references the third one ten times, and so on.  In the file given,
+/// this causes 10^17 objects to be rendered.  While this does not
+/// exhaust memory, it would take a really long time.
+///
+/// - #515 has deeply nested references of `<pattern>` elements.  Each
+/// object inside each pattern has an attribute
+/// fill="url(#next_pattern)", so the number of final rendered objects
+/// grows exponentially.
+///
+/// We deal with both cases by placing a limit on how many references
+/// will be resolved during the SVG rendering process, that is,
+/// how many `url(#foo)` will be resolved.
 ///
 /// [billion laughs attack]: https://bitbucket.org/tiran/defusedxml
 pub const MAX_REFERENCED_ELEMENTS: usize = 500_000;
+
+/// This is a mitigation for SVG files which create millions of elements
+/// in an attempt to exhaust memory.  We don't allow loading more than
+/// this number of elements during the initial streaming load process.
+pub const MAX_LOADED_ELEMENTS: usize = 200_000;
diff --git a/rsvg_internals/src/xml.rs b/rsvg_internals/src/xml.rs
index 23264c25..2c6b8b41 100644
--- a/rsvg_internals/src/xml.rs
+++ b/rsvg_internals/src/xml.rs
@@ -14,6 +14,7 @@ use crate::css::CssRules;
 use crate::error::LoadingError;
 use crate::handle::LoadOptions;
 use crate::io::{self, get_input_stream_for_loading};
+use crate::limits::MAX_LOADED_ELEMENTS;
 use crate::node::{NodeData, NodeType, RsvgNode};
 use crate::property_bag::PropertyBag;
 use crate::style::NodeStyle;
@@ -68,6 +69,7 @@ extern "C" {
 /// what creates normal graphical elements.
 struct XmlStateInner {
     weak: Option<Weak<XmlState>>,
+    num_loaded_elements: usize,
     tree_root: Option<RsvgNode>,
     ids: Option<HashMap<String, RsvgNode>>,
     css_rules: Option<CssRules>,
@@ -107,6 +109,7 @@ impl XmlState {
         XmlState {
             inner: RefCell::new(XmlStateInner {
                 weak: None,
+                num_loaded_elements: 0,
                 tree_root: None,
                 ids: Some(HashMap::new()),
                 css_rules: Some(CssRules::default()),
@@ -153,13 +156,29 @@ impl XmlState {
         }
     }
 
+    fn check_limits(&self) -> Result<(), ()> {
+        if self.inner.borrow().num_loaded_elements > MAX_LOADED_ELEMENTS {
+            self.error(ParseFromStreamError::XmlParseError(format!(
+                "cannot load more than {} XML elements",
+                MAX_LOADED_ELEMENTS
+            )));
+            Err(())
+        } else {
+            Ok(())
+        }
+    }
+
     pub fn start_element(&self, name: &str, pbag: &PropertyBag) -> Result<(), ()> {
+        self.check_limits()?;
+
         let context = self.inner.borrow().context();
 
         if let Context::FatalError(_) = context {
             return Err(());
         }
 
+        self.inner.borrow_mut().num_loaded_elements += 1;
+
         // FIXME: we should deal with namespaces at some point
         let name = skip_namespace(name);
 
@@ -328,15 +347,12 @@ impl XmlState {
     }
 
     fn element_creation_characters(&self, text: &str) {
-        let inner = self.inner.borrow();
+        let mut current_node = self.inner.borrow().current_node.as_ref().unwrap().clone();
 
         if text.len() != 0 {
             // When the last child is a Chars node we can coalesce
             // the text and avoid screwing up the Pango layouts
-            let chars_node = if let Some(child) = inner
-                .current_node
-                .as_ref()
-                .unwrap()
+            let chars_node = if let Some(child) = current_node
                 .last_child()
                 .filter(|c| c.borrow().get_type() == NodeType::Chars)
             {
@@ -350,8 +366,8 @@ impl XmlState {
                     Box::new(NodeChars::new()),
                 ));
 
-                let mut node = inner.current_node.as_ref().unwrap().clone();
-                node.append(child.clone());
+                self.inner.borrow_mut().num_loaded_elements += 1;
+                current_node.append(child.clone());
 
                 child
             };
diff --git a/tests/errors.c b/tests/errors.c
index 7bf72485..ea1e3c2e 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -24,9 +24,10 @@ get_test_filename (const char *basename) {
 }
 
 static void
-test_non_svg_element (void)
+test_loading_error (gconstpointer data)
 {
-    char *filename = get_test_filename ("335-non-svg-element.svg");
+    const char *basename = data;
+    char *filename = get_test_filename (basename);
     RsvgHandle *handle;
     GError *error = NULL;
 
@@ -67,7 +68,10 @@ main (int argc, char **argv)
 {
     g_test_init (&argc, &argv, NULL);
 
-    g_test_add_func ("/errors/non_svg_element", test_non_svg_element);
+    g_test_add_data_func_full ("/errors/non_svg_element",
+                               "335-non-svg-element.svg",
+                               test_loading_error,
+                               NULL);
 
     g_test_add_data_func_full ("/errors/instancing_limit/323-nested-use.svg",
                                "323-nested-use.svg",
@@ -79,6 +83,11 @@ main (int argc, char **argv)
                                test_instancing_limit,
                                NULL);
 
+    g_test_add_data_func_full ("/errors/515-too-many-elements.svgz",
+                               "515-too-many-elements.svgz",
+                               test_loading_error,
+                               NULL);
+
 
     return g_test_run ();
 }
diff --git a/tests/fixtures/errors/515-too-many-elements.svgz 
b/tests/fixtures/errors/515-too-many-elements.svgz
new file mode 100644
index 00000000..a7f7cf67
Binary files /dev/null and b/tests/fixtures/errors/515-too-many-elements.svgz differ