Bug 1510862 - Prevent exponential blowup of custom properties. r=jwatt

Put a hard cap on the value length instead of counting substitutions, because it
works best, see the comment.

Differential Revision: https://phabricator.services.mozilla.com/D13352

servo/components/style/custom_properties.rs

@@ -283,18 +283,32 @@ impl VariableValue {
         }
     }
 
-    fn push(
+    fn push<'i>(
         &mut self,
+        input: &Parser<'i, '_>,
         css: &str,
         css_first_token_type: TokenSerializationType,
         css_last_token_type: TokenSerializationType,
-    ) {
+    ) -> Result<(), ParseError<'i>> {
+        /// Prevent values from getting terribly big since you can use custom
+        /// properties exponentially.
+        ///
+        /// This number (1MB) is somewhat arbitrary, but silly enough that no
+        /// sane page would hit it. We could limit by number of total
+        /// substitutions, but that was very easy to work around in practice
+        /// (just choose a larger initial value and boom).
+        const MAX_VALUE_LENGTH_IN_BYTES: usize = 1024 * 1024;
+
+        if self.css.len() + css.len() > MAX_VALUE_LENGTH_IN_BYTES {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+        }
+
         // This happens e.g. between two subsequent var() functions:
         // `var(--a)var(--b)`.
         //
         // In that case, css_*_token_type is nonsensical.
         if css.is_empty() {
-            return;
+            return Ok(());
         }
 
         self.first_token_type.set_if_nothing(css_first_token_type);
@@ -307,21 +321,32 @@ impl VariableValue {
             self.css.push_str("/**/")
         }
         self.css.push_str(css);
-        self.last_token_type = css_last_token_type
+        self.last_token_type = css_last_token_type;
+        Ok(())
     }
 
-    fn push_from(
+    fn push_from<'i>(
         &mut self,
+        input: &Parser<'i, '_>,
         position: (SourcePosition, TokenSerializationType),
-        input: &Parser,
         last_token_type: TokenSerializationType,
-    ) {
-        self.push(input.slice_from(position.0), position.1, last_token_type)
+    ) -> Result<(), ParseError<'i>> {
+        self.push(
+            input,
+            input.slice_from(position.0),
+            position.1,
+            last_token_type,
+        )
     }
 
-    fn push_variable(&mut self, variable: &ComputedValue) {
+    fn push_variable<'i>(
+        &mut self,
+        input: &Parser<'i, '_>,
+        variable: &ComputedValue,
+    ) -> Result<(), ParseError<'i>> {
         debug_assert!(variable.references.is_empty());
         self.push(
+            input,
             &variable.css,
             variable.first_token_type,
             variable.last_token_type,
@@ -727,6 +752,7 @@ fn substitute_all(custom_properties_map: &mut CustomPropertiesMap, environment:
     // title=Tarjan%27s_strongly_connected_components_algorithm&oldid=801728495
 
     /// Struct recording necessary information for each variable.
+    #[derive(Debug)]
     struct VarInfo {
         /// The name of the variable. It will be taken to save addref
         /// when the corresponding variable is popped from the stack.
@@ -741,6 +767,7 @@ fn substitute_all(custom_properties_map: &mut CustomPropertiesMap, environment:
     }
     /// Context struct for traversing the variable graph, so that we can
     /// avoid referencing all the fields multiple times.
+    #[derive(Debug)]
     struct Context<'a> {
         /// Number of variables visited. This is used as the order index
         /// when we visit a new unresolved variable.
@@ -941,7 +968,7 @@ fn substitute_references_in_value<'i>(
         environment,
     )?;
 
-    computed_value.push_from(position, &input, last_token_type);
+    computed_value.push_from(&input, position, last_token_type)?;
     Ok(computed_value)
 }
 
@@ -955,8 +982,8 @@ fn substitute_references_in_value<'i>(
 ///
 /// Return `Err(())` if `input` is invalid at computed-value time.
 /// or `Ok(last_token_type that was pushed to partial_computed_value)` otherwise.
-fn substitute_block<'i, 't>(
-    input: &mut Parser<'i, 't>,
+fn substitute_block<'i>(
+    input: &mut Parser<'i, '_>,
     position: &mut (SourcePosition, TokenSerializationType),
     partial_computed_value: &mut ComputedValue,
     custom_properties: &CustomPropertiesMap,
@@ -991,10 +1018,11 @@ fn substitute_block<'i, 't>(
                 let is_env = name.eq_ignore_ascii_case("env");
 
                 partial_computed_value.push(
+                    input,
                     input.slice(position.0..before_this_token),
                     position.1,
                     last_token_type,
-                );
+                )?;
                 input.parse_nested_block(|input| {
                     // parse_var_function() / parse_env_function() ensure neither .unwrap() will fail.
                     let name = {
@@ -1014,7 +1042,7 @@ fn substitute_block<'i, 't>(
 
                     if let Some(v) = value {
                         last_token_type = v.last_token_type;
-                        partial_computed_value.push_variable(v);
+                        partial_computed_value.push_variable(input, v)?;
                         // Skip over the fallback, as `parse_nested_block` would return `Err`
                         // if we don't consume all of `input`.
                         // FIXME: Add a specialized method to cssparser to do this with less work.
@@ -1036,7 +1064,7 @@ fn substitute_block<'i, 't>(
                             custom_properties,
                             env,
                         )?;
-                        partial_computed_value.push_from(position, input, last_token_type);
+                        partial_computed_value.push_from(input, position, last_token_type)?;
                     }
                     Ok(())
                 })?;
@@ -1096,6 +1124,6 @@ pub fn substitute<'i>(
         &custom_properties,
         env,
     )?;
-    substituted.push_from(position, &input, last_token_type);
+    substituted.push_from(&input, position, last_token_type)?;
     Ok(substituted.css)
 }

testing/web-platform/tests/css/css-variables/variable-exponential-blowup.html

@@ -0,0 +1,28 @@
+<!doctype html>
+<title>CSS Variables Test: Exponential blowup doesn't crash</title>
+<meta charset="UTF-8">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">
+<link rel="author" href="https://mozilla.org" title="Mozilla">
+<link rel="help" href="https://drafts.csswg.org/css-variables/">
+<script>
+  let css = `
+    --v0: "Something really really really long";
+  `;
+  for (let i = 0; i < 30; ++i)
+    css += `--v${i + 1}: var(--v${i}), var(--v${i});`;
+  let s = document.createElement("style");
+  s.innerHTML = `
+    :root { ${css}; }
+    :root::before { content: var(--v31); }
+  `;
+  document.head.appendChild(s);
+</script>
+PASS if doesn't crash
+<script>
+  test(function() {
+    getComputedStyle(document.documentElement, "::before").content;
+    assert_true(true, "Didn't crash");
+  });
+</script>