Improve sanitizer in parse_html_subset.js

This change improves sanitizer in parse_html_subset.js by using
allow-list of tags and attributes.

Bug: 1086318
Change-Id: I56b94350d49bbb9e218336c9292897ac2e619316
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2222115Reviewed-by: dpapad <dpapad@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Jun Kokatsu <Jun.Kokatsu@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#779556}

chrome/browser/resources/print_preview/ui/destination_dialog.js

@@ -129,12 +129,7 @@ Polymer({
     this.$$('.promo-text').innerHTML =
         this.i18nAdvanced('cloudPrintPromotion', {
           substitutions: ['<a is="action-link" class="sign-in">', '</a>'],
-          attrs: {
-            'is': (node, v) => v === 'action-link',
-            'class': (node, v) => v === 'sign-in',
-            'tabindex': (node, v) => v === '0',
-            'role': (node, v) => v === 'link',
-          },
+          attrs: ['is', 'class', 'tabindex', 'role'],
         });
   },
 

chrome/browser/resources/user_manager/create_profile.js

@@ -202,15 +202,7 @@ Polymer({
    * @private
    */
   i18nAllowIDAttr_(id) {
-    const opts = {
-      'attrs': {
-        'id'(node, value) {
-          return node.tagName == 'A';
-        }
-      }
-    };
-
-    return this.i18nAdvanced(id, opts);
+    return this.i18nAdvanced(id, {attrs: ['id']});
   },
 
   /**

chrome/test/data/webui/i18n_behavior_test.html

@@ -11,7 +11,7 @@ function setUpPage() {
   loadTimeData.data = {
     'allowedByDefault': allowedByDefault,
     'customAttr': '<a is="action-link">Take action!</a>',
-    'customTag': "<x-foo>I'm an X, foo!</x-foo>",
+    'optionalTag': '<img>',
     'javascriptHref': '<a href="javascript:alert(1)">teh hax</a>',
     'script': '<script>alert(/xss/)</scr' + 'ipt>',
     'text': text,
@@ -24,7 +24,7 @@ function testI18n() {
   assertEquals(nonBreakingSpace, I18nBehavior.i18n('nonBreakingSpace'));
 
   assertThrows(function() { I18nBehavior.i18n('customAttr'); });
-  assertThrows(function() { I18nBehavior.i18n('customTag'); });
+  assertThrows(function() { I18nBehavior.i18n('optionalTag'); });
   assertThrows(function() { I18nBehavior.i18n('javascriptHref'); });
   assertThrows(function() { I18nBehavior.i18n('script'); });
 }
@@ -33,14 +33,8 @@ function testI18nAdvanced() {
   assertEquals(
       allowedByDefault,
       I18nBehavior.i18nAdvanced('allowedByDefault'));
-  I18nBehavior.i18nAdvanced('customAttr', {
-    attrs: {
-      is: function(el, val) {
-        return el.tagName == 'A' && val == 'action-link';
-      },
-    },
-  });
-  I18nBehavior.i18nAdvanced('customTag', {tags: ['X-FOO']});
+  I18nBehavior.i18nAdvanced('customAttr', {attrs: ['is']});
+  I18nBehavior.i18nAdvanced('optionalTag', {tags: ['img']});
 }
 
 function testI18nDynamic() {

chrome/test/data/webui/js/i18n_behavior_test.js

@@ -13,7 +13,7 @@ suite('I18nBehaviorModuleTest', function() {
     window.loadTimeData.data = {
       'allowedByDefault': allowedByDefault,
       'customAttr': '<a is="action-link">Take action!</a>',
-      'customTag': '<x-foo>I\'m an X, foo!</x-foo>',
+      'optionalTag': '<img>',
       'javascriptHref': '<a href="javascript:alert(1)">teh hax</a>',
       'script': '<script>alert(/xss/)</scr' +
           'ipt>',
@@ -30,7 +30,7 @@ suite('I18nBehaviorModuleTest', function() {
       I18nBehavior.i18n('customAttr');
     });
     assertThrows(function() {
-      I18nBehavior.i18n('customTag');
+      I18nBehavior.i18n('optionalTag');
     });
     assertThrows(function() {
       I18nBehavior.i18n('javascriptHref');
@@ -43,14 +43,8 @@ suite('I18nBehaviorModuleTest', function() {
   test('i18n advanced', function() {
     assertEquals(
         allowedByDefault, I18nBehavior.i18nAdvanced('allowedByDefault'));
-    I18nBehavior.i18nAdvanced('customAttr', {
-      attrs: {
-        is: function(el, val) {
-          return el.tagName === 'A' && val === 'action-link';
-        },
-      },
-    });
-    I18nBehavior.i18nAdvanced('customTag', {tags: ['X-FOO']});
+    I18nBehavior.i18nAdvanced('customAttr', {attrs: ['is']});
+    I18nBehavior.i18nAdvanced('optionalTag', {tags: ['img']});
   });
 
   test('i18n dynamic', function() {

chrome/test/data/webui/js/load_time_data_test.js

@@ -15,11 +15,9 @@ suite('LoadTimeDataModuleTest', function() {
         '<a href="chrome://foo"></a>',
         loadTimeData.sanitizeInnerHtml('<a href="chrome://foo"></a>'));
     assertThrows(() => {
-      loadTimeData.sanitizeInnerHtml('<div></div>');
-    }, 'DIV is not supported');
-    assertEquals(
-        '<div></div>',
-        loadTimeData.sanitizeInnerHtml('<div></div>', {tags: ['div']}));
+      loadTimeData.sanitizeInnerHtml('<iframe></iframe>');
+    }, 'IFRAME is not supported');
+    assertEquals('<div></div>', loadTimeData.sanitizeInnerHtml('<div></div>'));
   });
 
   test('getStringPieces', function() {

chrome/test/data/webui/js/parse_html_subset_test.js

@@ -25,11 +25,13 @@ suite('ParseHtmlSubsetModuleTest', function() {
     parseHtmlSubset('<B>bold</B>');
     parseHtmlSubset('Some <B>bold</B> text');
     parseHtmlSubset('Some <STRONG>strong</STRONG> text');
+    parseHtmlSubset('<PRE>pre</PRE><BR>');
+    parseHtmlSubset('Some <PRE>pre</PRE><BR> text', ['BR']);
   });
 
   test('invalid tags', function() {
     parseAndAssertThrows('<unknown_tag>x</unknown_tag>');
-    parseAndAssertThrows('<img>');
+    parseAndAssertThrows('<style>*{color:red;}</style>');
     parseAndAssertThrows(
         '<script>alert(1)<' +
         '/script>');
@@ -71,40 +73,45 @@ suite('ParseHtmlSubsetModuleTest', function() {
   });
 
   test('invalid target', function() {
-    parseAndAssertThrows('<form target="_evil">', ['form']);
-    parseAndAssertThrows('<iframe target="_evil">', ['iframe']);
     parseAndAssertThrows(
         '<a href="https://google.com" target="foo">Google</a>');
   });
 
-  test('custom tags', function() {
-    parseHtmlSubset('yo <I>ho</i><bR>yo <EM>ho</em>', ['i', 'EM', 'Br']);
+  test('supported optional tags', function() {
+    parseHtmlSubset('<img>Some <b>bold</b> text', ['img']);
   });
 
-  test('invalid custom tags', function() {
+  test('supported optional tags without the argument', function() {
+    parseAndAssertThrows('<img>');
+  });
+
+  test('invalid optional tags', function() {
     parseAndAssertThrows(
-        'a pirate\'s<script>lifeForMe();<' +
+        'a pirate\'s<script>alert();<' +
             '/script>',
-        ['br']);
+        ['script']);
   });
 
-  test('custom attributes', function() {
-    const returnsTruthy = function(node, value) {
-      assertEquals('A', node.tagName);
-      assertEquals('fancy', value);
-      return true;
-    };
-    parseHtmlSubset(
-        '<a class="fancy">I\'m fancy!</a>', null, {class: returnsTruthy});
+  test('supported optional attributes', function() {
+    let result = parseHtmlSubset('<a role="link">link</a>', null, ['role']);
+    assertEquals('link', result.firstChild.getAttribute('role'));
+    result =
+        parseHtmlSubset('<img src="chrome://favicon2/">', ['img'], ['src']);
+    assertEquals('chrome://favicon2/', result.firstChild.getAttribute('src'));
   });
 
-  test('invalid custom attributes', function() {
-    const returnsFalsey = function() {
-      return false;
-    };
-    parseAndAssertThrows(
-        '<a class="fancy">I\'m fancy!</a>', null, {class: returnsFalsey});
-    parseAndAssertThrows('<a class="fancy">I\'m fancy!</a>');
+  test('supported optional attributes without the argument', function() {
+    parseAndAssertThrows('<img src="chrome://favicon2/">', ['img']);
+    parseAndAssertThrows('<a id="test">link</a>');
+  });
+
+  test('invalid optional attributes', function() {
+    parseAndAssertThrows('<a test="fancy">I\'m fancy!</a>', null, ['test']);
+    parseAndAssertThrows('<a name="fancy">I\'m fancy!</a>');
+  });
+
+  test('invalid optional attribute\'s value', function() {
+    parseAndAssertThrows('<a is="xss-link">link</a>', null, ['is']);
   });
 
   test('on error async', function(done) {

chrome/test/data/webui/load_time_data_browsertest.js

@@ -25,11 +25,9 @@ TEST_F('LoadTimeDataTest', 'sanitizeInnerHtml', function() {
       '<a href="chrome://foo"></a>',
       loadTimeData.sanitizeInnerHtml('<a href="chrome://foo"></a>'));
   assertThrows(() => {
-    loadTimeData.sanitizeInnerHtml('<div></div>');
-  }, 'DIV is not supported');
-  assertEquals(
-      '<div></div>',
-      loadTimeData.sanitizeInnerHtml('<div></div>', {tags: ['div']}));
+    loadTimeData.sanitizeInnerHtml('<iframe></iframe>');
+  }, 'IFRAME is not supported');
+  assertEquals('<div></div>', loadTimeData.sanitizeInnerHtml('<div></div>'));
 });
 
 TEST_F('LoadTimeDataTest', 'getStringPieces', function() {

chrome/test/data/webui/parse_html_subset_test.html

@@ -30,7 +30,7 @@ function testSupportedTags() {
 
 function testInvalidTags() {
   parseAndAssertThrows('<unknown_tag>x</unknown_tag>');
-  parseAndAssertThrows('<img>');
+  parseAndAssertThrows('<style>*{color:red;}</style>');
   parseAndAssertThrows('<script>alert(1)<' + '/script>');
 }
 
@@ -68,35 +68,23 @@ function testAnchorTarget() {
 }
 
 function testInvalidTarget() {
-  parseAndAssertThrows('<form target="_evil">', ['form']);
-  parseAndAssertThrows('<iframe target="_evil">', ['iframe']);
   parseAndAssertThrows('<a href="https://google.com" target="foo">Google</a>');
 }
 
 function testCustomTags() {
-  parseHtmlSubset('yo <I>ho</i><bR>yo <EM>ho</em>', ['i', 'EM', 'Br']);
+  parseHtmlSubset('<img>', ['IMG']);
 }
 
 function testInvalidCustomTags() {
-  parseAndAssertThrows("a pirate's<script>lifeForMe();<" + '/script>', ['br']);
+  parseAndAssertThrows('a pirate\'s<script>alert();<' + '/script>', ['script']);
 }
 
 function testCustomAttributes() {
-  function returnsTruthy(node, value) {
-    assertEquals('A', node.tagName);
-    assertEquals('fancy', value);
-    return true;
-  }
   parseHtmlSubset('<a class="fancy">I\'m fancy!</a>', null,
-      {class: returnsTruthy});
+      ['class']);
 }
 
 function testInvalidCustomAttributes() {
-  function returnsFalsey() {
-    return false;
-  }
-  parseAndAssertThrows('<a class="fancy">I\'m fancy!</a>', null,
-      {class: returnsFalsey});
   parseAndAssertThrows('<a class="fancy">I\'m fancy!</a>');
 }
 

ui/webui/resources/cr_components/chromeos/multidevice_setup/setup_succeeded_page.js

@@ -36,10 +36,7 @@ Polymer({
 
   /** @private */
   getMessageHtml_() {
-    const validNodeFn = (node, value) => node.tagName === 'A';
-    return this.i18nAdvanced(
-        'setupSucceededPageMessage',
-        {attrs: {'id': validNodeFn, 'href': validNodeFn}});
+    return this.i18nAdvanced('setupSucceededPageMessage', {attrs: ['id']});
   },
 
   /** @override */

ui/webui/resources/js/load_time_data.js

@@ -19,9 +19,9 @@
 
 /**
  * @typedef {{
- *   substitutions: (Array<string>|undefined),
- *   attrs: (Object<function(Node, string):boolean>|undefined),
- *   tags: (Array<string>|undefined),
+ *   substitutions: (!Array<string>|undefined),
+ *   attrs: (!Array<string>|undefined),
+ *   tags: (!Array<string>|undefined),
  * }}
  */
 /* #export */ let SanitizeInnerHtmlOpts;

ui/webui/resources/js/parse_html_subset.js

@@ -3,11 +3,11 @@
 // found in the LICENSE file.
 
 /**
- * Parses a very small subset of HTML.  This ensures that insecure HTML /
- * javascript cannot be injected into the new tab page.
+ * Parses a very small subset of HTML. This ensures that insecure HTML /
+ * javascript cannot be injected into WebUI.
  * @param {string} s The string to parse.
- * @param {Array<string>=} opt_extraTags Optional extra allowed tags.
- * @param {Object<function(Node, string):boolean>=} opt_extraAttrs
+ * @param {!Array<string>=} opt_extraTags Optional extra allowed tags.
+ * @param {!Array<string>=} opt_extraAttrs
  *     Optional extra allowed attributes (all tags are run through these).
  * @throws {Error} In case of non supported markup.
  * @return {DocumentFragment} A document fragment containing the DOM tree.
@@ -15,29 +15,73 @@
 /* #export */ const parseHtmlSubset = (function() {
   'use strict';
 
-  const allowedAttributes = {
-    'href'(node, value) {
-      // Only allow a[href] starting with chrome:// and https://
-      return node.tagName === 'A' &&
-          (value.startsWith('chrome://') || value.startsWith('https://'));
-    },
-    'target'(node, value) {
-      // Only allow a[target='_blank'].
-      // TODO(dbeam): are there valid use cases for target !== '_blank'?
-      return node.tagName === 'A' && value === '_blank';
-    },
-  };
+  /** @typedef {function(!Node, string):boolean} */
+  let AllowFunction;
+
+  /** @type {!AllowFunction} */
+  const allowAttribute = (node, value) => true;
+
+  /**
+   * Allow-list of attributes in parseHtmlSubset.
+   * @type {!Map<string, !AllowFunction>}
+   * @const
+   */
+  const allowedAttributes = new Map([
+    [
+      'href',
+      (node, value) => {
+        // Only allow a[href] starting with chrome:// and https://
+        return node.tagName === 'A' &&
+            (value.startsWith('chrome://') || value.startsWith('https://'));
+      }
+    ],
+    [
+      'target',
+      (node, value) => {
+        // Only allow a[target='_blank'].
+        // TODO(dbeam): are there valid use cases for target !== '_blank'?
+        return node.tagName === 'A' && value === '_blank';
+      }
+    ],
+  ]);
+
+  /**
+   * Allow-list of optional attributes in parseHtmlSubset.
+   * @type {!Map<string, !AllowFunction>}
+   * @const
+   */
+  const allowedOptionalAttributes = new Map([
+    ['class', allowAttribute],
+    ['id', allowAttribute],
+    ['is', (node, value) => value === 'action-link' || value === ''],
+    ['role', (node, value) => value === 'link'],
+    [
+      'src',
+      (node, value) => {
+        // Only allow img[src] starting with chrome://
+        return node.tagName === 'IMG' && value.startsWith('chrome://');
+      }
+    ],
+    ['tabindex', allowAttribute],
+  ]);
+
+  /**
+   * Allow-list of tag names in parseHtmlSubset.
+   * @type {!Set<string>}
+   * @const
+   */
+  const allowedTags =
+      new Set(['A', 'B', 'BR', 'DIV', 'P', 'PRE', 'SPAN', 'STRONG']);
 
   /**
-   * Whitelist of tag names allowed in parseHtmlSubset.
-   * @type {!Array<string>}
+   * Allow-list of optional tag names in parseHtmlSubset.
+   * @type {!Set<string>}
    * @const
    */
-  const allowedTags = ['A', 'B', 'SPAN', 'STRONG'];
+  const allowedOptionalTags = new Set(['IMG']);
 
   /**
    * This is used to create TrustedHTML.
-   *
    * @type {TrustedTypePolicy|undefined}
    */
   let untrustedHTMLPolicy;
@@ -47,24 +91,38 @@
         // This is safe because the untrusted HTML will be sanitized
         // later in this function. We are adding this so that
         // the sanitization will not cause a Trusted Types violation.
-        // TODO(crbug.com/1086318): Improve parseHtmlSubset sanitizer
         return untrustedHTML;
       },
     });
   }
 
-  /** @param {...Object} var_args Objects to merge. */
-  function merge(var_args) {
-    const clone = {};
-    for (let i = 0; i < arguments.length; ++i) {
-      if (typeof arguments[i] === 'object') {
-        for (const key in arguments[i]) {
-          if (arguments[i].hasOwnProperty(key)) {
-            clone[key] = arguments[i][key];
-          }
-        }
+  /**
+   * @param {!Array<string>} optTags an Array to merge.
+   * @return {!Set<string>} Set of allowed tags.
+   */
+  function mergeTags(optTags) {
+    const clone = new Set(allowedTags);
+    optTags.forEach(str => {
+      const tag = str.toUpperCase();
+      if (allowedOptionalTags.has(tag)) {
+        clone.add(tag);
       }
-    }
+    });
+    return clone;
+  }
+
+  /**
+   * @param {!Array<string>} optAttrs an Array to merge.
+   * @return {!Map<string, !AllowFunction>} Map of allowed
+   *     attributes.
+   */
+  function mergeAttrs(optAttrs) {
+    const clone = new Map([...allowedAttributes]);
+    optAttrs.forEach(key => {
+      if (allowedOptionalAttributes.has(key)) {
+        clone.set(key, allowedOptionalAttributes.get(key));
+      }
+    });
     return clone;
   }
 
@@ -76,7 +134,7 @@
   }
 
   function assertElement(tags, node) {
-    if (tags.indexOf(node.tagName) === -1) {
+    if (!tags.has(node.tagName)) {
       throw Error(node.tagName + ' is not supported');
     }
   }
@@ -84,17 +142,15 @@
   function assertAttribute(attrs, attrNode, node) {
     const n = attrNode.nodeName;
     const v = attrNode.nodeValue;
-    if (!attrs.hasOwnProperty(n) || !attrs[n](node, v)) {
+    if (!attrs.has(n) || !attrs.get(n)(node, v)) {
       throw Error(node.tagName + '[' + n + '="' + v + '"] is not supported');
     }
   }
 
   return function(s, opt_extraTags, opt_extraAttrs) {
-    const extraTags = (opt_extraTags || []).map(function(str) {
-      return str.toUpperCase();
-    });
-    const tags = allowedTags.concat(extraTags);
-    const attrs = merge(allowedAttributes, opt_extraAttrs || {});
+    const tags = opt_extraTags ? mergeTags(opt_extraTags) : allowedTags;
+    const attrs =
+        opt_extraAttrs ? mergeAttrs(opt_extraAttrs) : allowedAttributes;
 
     const doc = document.implementation.createHTMLDocument('');
     const r = doc.createRange();