CVE-2021-22540: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Bad validation logic in the Dart SDK versions prior to 2.12.3 allow an attacker to use an XSS attack via DOM clobbering. The validation logic in dart:html for creating DOM nodes from text did not sanitize properly when it came across template tags.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 6.1
Published: 2021-04-22T15:15Z
Repository: https://github.com/dart-lang/sdk
Language: Other
Files changed: sdk/lib/html/dart2js/html_dart2js.dart, tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart, tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart, tools/dom/src/Validators.dart
Commit date: 2021-04-13 23:29:08 +0000
Commit message: [dart:html] Fix sanitization for HTML templates

Bug: b/143778164

Resolves an issue where sanitization wasn't properly handled
when templates were involved.

Change-Id: Ic8f6f28036e18981eb934c2b39c2c0cd4e6f1a96
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/195056
Reviewed-by: Sigmund Cherem <sigmund@google.com>

Diff: diff --git a/sdk/lib/html/dart2js/html_dart2js.dart b/sdk/lib/html/dart2js/html_dart2js.dart
index 977a16ceaf2..14477694c68 100644
--- a/sdk/lib/html/dart2js/html_dart2js.dart
+++ b/sdk/lib/html/dart2js/html_dart2js.dart
@@ -40992,12 +40992,12 @@ class _ThrowsNodeValidator implements NodeValidator {
  * validator and removes any nodes or attributes which are not allowed.
  */
 class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
   NodeValidator validator;
 
-  /// Did we modify the tree by removing anything.
-  bool modifiedTree = false;
+  /// Number of tree modifications this instance has made.
+  int numTreeModifications = 0;
   _ValidatingTreeSanitizer(this.validator) {}
 
   void sanitizeTree(Node node) {
     void walk(Node node, Node? parent) {
       sanitizeNode(node, parent);
@@ -41024,24 +41024,24 @@ class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
         if (child != null) walk(child, node);
         child = nextChild;
       }
     }
 
-    modifiedTree = false;
-    walk(node, null);
-    while (modifiedTree) {
-      modifiedTree = false;
+    // Walk the tree until no new modifications are added to the tree.
+    var previousTreeModifications;
+    do {
+      previousTreeModifications = numTreeModifications;
       walk(node, null);
-    }
+    } while (previousTreeModifications != numTreeModifications);
   }
 
   /// Aggressively try to remove node.
   void _removeNode(Node node, Node? parent) {
     // If we have the parent, it's presumably already passed more sanitization
     // or is the fragment, so ask it to remove the child. And if that fails
     // try to set the outer html.
-    modifiedTree = true;
+    numTreeModifications++;
     if (parent == null || parent != node.parentNode) {
       node.remove();
     } else {
       parent._removeChild(node);
     }
diff --git a/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart b/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
index 5a3d0f28b51..2d7205ca279 100644
--- a/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
+++ b/tests/lib/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
@@ -451,10 +451,24 @@ main() {
         "<form><input name='children'><input name='children'>"
             "<input id='children' name='lastChild'>"
             "<input id='bad' onmouseover='alert(1)'>",
         "");
 
+    // Walking templates triggers a recursive sanitization call, which shouldn't
+    // invalidate the information collected from the previous visit of the later
+    // nodes in the walk.
+    testHtml(
+        'DOM clobbering with recursive sanitize call using templates',
+        validator,
+        "<form><div>"
+            "<input id=childNodes />"
+            "<template></template>"
+            "<input id=childNodes name=lastChild />"
+            "<img id=exploitImg src=0 onerror='alert(1)' />"
+            "</div></form>",
+        "");
+
     test('tagName makes containing form invalid', () {
       var fragment = document.body!.createFragment(
           "<form onmouseover='alert(2)'><input name='tagName'>",
           validator: validator);
       var form = fragment.lastChild as FormElement?;
diff --git a/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart b/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
index 1b3ea601bb6..ee318684692 100644
--- a/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
+++ b/tests/lib_2/html/node_validator_important_if_you_suppress_make_the_bug_critical_test.dart
@@ -476,10 +476,24 @@ main() {
         "<form><input name='children'><input name='children'>"
         "<input id='children' name='lastChild'>"
         "<input id='bad' onmouseover='alert(1)'>",
         "");
 
+    // Walking templates triggers a recursive sanitization call, which shouldn't
+    // invalidate the information collected from the previous visit of the later
+    // nodes in the walk.
+    testHtml(
+        'DOM clobbering with recursive sanitize call using templates',
+        validator,
+        "<form><div>"
+          "<input id=childNodes />"
+          "<template></template>"
+          "<input id=childNodes name=lastChild />"
+          "<img id=exploitImg src=0 onerror='alert(1)' />"
+          "</div></form>",
+        "");
+
     test('tagName makes containing form invalid', () {
       var fragment = document.body.createFragment(
           "<form onmouseover='alert(2)'><input name='tagName'>",
           validator: validator);
       var form = fragment.lastChild as FormElement;
diff --git a/tools/dom/src/Validators.dart b/tools/dom/src/Validators.dart
index 0c45b8c8600..1fbafbd9bc5 100644
--- a/tools/dom/src/Validators.dart
+++ b/tools/dom/src/Validators.dart
@@ -156,12 +156,12 @@ class _ThrowsNodeValidator implements NodeValidator {
  * validator and removes any nodes or attributes which are not allowed.
  */
 class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
   NodeValidator validator;
 
-  /// Did we modify the tree by removing anything.
-  bool modifiedTree = false;
+  /// Number of tree modifications this instance has made.
+  int numTreeModifications = 0;
   _ValidatingTreeSanitizer(this.validator) {}
 
   void sanitizeTree(Node node) {
     void walk(Node node, Node? parent) {
       sanitizeNode(node, parent);
@@ -188,24 +188,24 @@ class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
         if (child != null) walk(child, node);
         child = nextChild;
       }
     }
 
-    modifiedTree = false;
-    walk(node, null);
-    while (modifiedTree) {
-      modifiedTree = false;
+    // Walk the tree until no new modifications are added to the tree.
+    var previousTreeModifications;
+    do {
+      previousTreeModifications = numTreeModifications;
       walk(node, null);
-    }
+    } while (previousTreeModifications != numTreeModifications);
   }
 
   /// Aggressively try to remove node.
   void _removeNode(Node node, Node? parent) {
     // If we have the parent, it's presumably already passed more sanitization
     // or is the fragment, so ask it to remove the child. And if that fails
     // try to set the outer html.
-    modifiedTree = true;
+    numTreeModifications++;
     if (parent == null || parent != node.parentNode) {
       node.remove();
     } else {
       parent._removeChild(node);
     }

Vulnerable code:   /// Did we modify the tree by removing anything.
  bool modifiedTree = false;
    modifiedTree = false;
    walk(node, null);
    while (modifiedTree) {
      modifiedTree = false;
    }
    modifiedTree = true;
  /// Did we modify the tree by removing anything.
  bool modifiedTree = false;
    modifiedTree = false;
    walk(node, null);
    while (modifiedTree) {
      modifiedTree = false;
    }
    modifiedTree = true;
Fixed code:   /// Number of tree modifications this instance has made.
  int numTreeModifications = 0;
    // Walk the tree until no new modifications are added to the tree.
    var previousTreeModifications;
    do {
      previousTreeModifications = numTreeModifications;
    } while (previousTreeModifications != numTreeModifications);
    numTreeModifications++;
    // Walking templates triggers a recursive sanitization call, which shouldn't
    // invalidate the information collected from the previous visit of the later
    // nodes in the walk.
    testHtml(
        'DOM clobbering with recursive sanitize call using templates',
        validator,
        "<form><div>"
            "<input id=childNodes />"
            "<template></template>"
            "<input id=childNodes name=lastChild />"
            "<img id=exploitImg src=0 onerror='alert(1)' />"
            "</div></form>",
        "");

    // Walking templates triggers a recursive sanitization call, which shouldn't
    // invalidate the information collected from the previous visit of the later
    // nodes in the walk.
    testHtml(
        'DOM clobbering with recursive sanitize call using templates',
        validator,
        "<form><div>"
          "<input id=childNodes />"
          "<template></template>"
          "<input id=childNodes name=lastChild />"
          "<img id=exploitImg src=0 onerror='alert(1)' />"
          "</div></form>",
        "");

  /// Number of tree modifications this instance has made.
  int numTreeModifications = 0;
    // Walk the tree until no new modifications are added to the tree.
    var previousTreeModifications;
    do {
      previousTreeModifications = numTreeModifications;
    } while (previousTreeModifications != numTreeModifications);
    numTreeModifications++;