Submitting a form can cause HTMLFormElement's associated elements vector to be mutate...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Sep 2017 01:49:05 +0000 (01:49 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Sep 2017 01:49:05 +0000 (01:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176368
<rdar://problem/34254998>

Reviewed by Ryosuke Niwa.

Source/WebCore:

In the process of iterating over form.associatedElements() during form submission in FormSubmission::create, the
page may cause us to clobber the vector of FormAssociatedElements* we're currently iterating over by inserting
new form controls beneath the form element we're in the process of submitting. This happens because
FormSubmission::create calls HTMLTextAreaElement::appendFormData, which requires layout to be up to date, which
in turn makes us updateLayout() and set focus, which fires a `change` event, upon which the page's JavaScript
inserts additonal DOM nodes into the form, modifying the vector of associated elements.

To mitigate this, instead of iterating over HTMLFormElement::associatedElements(), which returns a reference to
the HTMLFormElement's actual m_associatedElements vector, we iterate over a new vector of
Ref<FormAssociatedElement>s created from m_associatedElements.

This patch also removes an event dispatch assertion added in r212026. This assertion was added to catch any
other events dispatched in this scope, since dispatching events there would have had security implications, but
after making iteration over associated elements robust, this NoEventDispatchAssertion is no longer useful.

Test: fast/forms/append-children-during-form-submission.html

* loader/FormSubmission.cpp:
(WebCore::FormSubmission::create):

LayoutTests:

Adds a new test to make sure we don't crash when mutating a form's associated elements during form submission.

* fast/forms/append-children-during-form-submission-expected.txt: Added.
* fast/forms/append-children-during-form-submission.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222005 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/forms/append-children-during-form-submission-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/append-children-during-form-submission.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FormSubmission.cpp

index 6bd1e6e1056d8f37011f13ff0f041a2f14b16199..01d85e97b888ca6dd7d107134da95f5fb7044d5d 100644 (file)
@@ -1,3 +1,16 @@
+2017-09-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Submitting a form can cause HTMLFormElement's associated elements vector to be mutated during iteration
+        https://bugs.webkit.org/show_bug.cgi?id=176368
+        <rdar://problem/34254998>
+
+        Reviewed by Ryosuke Niwa.
+
+        Adds a new test to make sure we don't crash when mutating a form's associated elements during form submission.
+
+        * fast/forms/append-children-during-form-submission-expected.txt: Added.
+        * fast/forms/append-children-during-form-submission.html: Added.
+
 2017-09-13  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: Event Listeners section does not update when listeners are added/removed
diff --git a/LayoutTests/fast/forms/append-children-during-form-submission-expected.txt b/LayoutTests/fast/forms/append-children-during-form-submission-expected.txt
new file mode 100644 (file)
index 0000000..6020a9f
--- /dev/null
@@ -0,0 +1,2 @@
+     
+To manually test, load this page. The web process should not crash.
diff --git a/LayoutTests/fast/forms/append-children-during-form-submission.html b/LayoutTests/fast/forms/append-children-during-form-submission.html
new file mode 100644 (file)
index 0000000..fb13bb8
--- /dev/null
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<script>
+function loaded() {
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
+    area1.setRangeText("foo");
+    area1.name = "foo";
+    area2.autofocus = true;
+    form.insertBefore(area2, form.firstChild);
+    form.submit();
+}
+
+function changed() {
+    for (let i = 0; i < 100; i++)
+        form.appendChild(document.createElement("input"));
+}
+</script>
+
+<body onload=loaded()>
+<form id="form" onchange=changed()>
+    <textarea id="area1">a</textarea>
+    <object></object>
+    <textarea id="area2">b</textarea>
+</form>
+<p>To manually test, load this page. The web process should not crash.</p>
+</body>
+</html>
\ No newline at end of file
index b84aecbc3142fcc9a13c0f43c34aec2831f14c76..7331e6d0c05de2fe27fe1f196ef089046a88e2f8 100644 (file)
@@ -1,3 +1,31 @@
+2017-09-13  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Submitting a form can cause HTMLFormElement's associated elements vector to be mutated during iteration
+        https://bugs.webkit.org/show_bug.cgi?id=176368
+        <rdar://problem/34254998>
+
+        Reviewed by Ryosuke Niwa.
+
+        In the process of iterating over form.associatedElements() during form submission in FormSubmission::create, the
+        page may cause us to clobber the vector of FormAssociatedElements* we're currently iterating over by inserting
+        new form controls beneath the form element we're in the process of submitting. This happens because
+        FormSubmission::create calls HTMLTextAreaElement::appendFormData, which requires layout to be up to date, which
+        in turn makes us updateLayout() and set focus, which fires a `change` event, upon which the page's JavaScript
+        inserts additonal DOM nodes into the form, modifying the vector of associated elements.
+
+        To mitigate this, instead of iterating over HTMLFormElement::associatedElements(), which returns a reference to
+        the HTMLFormElement's actual m_associatedElements vector, we iterate over a new vector of
+        Ref<FormAssociatedElement>s created from m_associatedElements.
+
+        This patch also removes an event dispatch assertion added in r212026. This assertion was added to catch any
+        other events dispatched in this scope, since dispatching events there would have had security implications, but
+        after making iteration over associated elements robust, this NoEventDispatchAssertion is no longer useful.
+
+        Test: fast/forms/append-children-during-form-submission.html
+
+        * loader/FormSubmission.cpp:
+        (WebCore::FormSubmission::create):
+
 2017-09-13  Devin Rousso  <webkit@devinrousso.com>
 
         Web Inspector: Event Listeners section does not update when listeners are added/removed
index ee031269be123104ceb0ed349b678a4ab8b2fc07..c128a99ca41ede49e0a6857b88936e59d65d5bee 100644 (file)
@@ -191,22 +191,22 @@ Ref<FormSubmission> FormSubmission::create(HTMLFormElement& form, const Attribut
     StringPairVector formValues;
 
     bool containsPasswordData = false;
-    {
-        NoEventDispatchAssertion noEventDispatchAssertion;
-
-        for (auto& control : form.associatedElements()) {
-            auto& element = control->asHTMLElement();
-            if (!element.isDisabledFormControl())
-                control->appendFormData(domFormData, isMultiPartForm);
-            if (is<HTMLInputElement>(element)) {
-                auto& input = downcast<HTMLInputElement>(element);
-                if (input.isTextField()) {
-                    formValues.append({ input.name().string(), input.value() });
-                    input.addSearchResult();
-                }
-                if (input.isPasswordField() && !input.value().isEmpty())
-                    containsPasswordData = true;
+    auto protectedAssociatedElements = form.associatedElements().map([] (FormAssociatedElement* rawElement) -> Ref<FormAssociatedElement> {
+        return *rawElement;
+    });
+
+    for (auto& control : protectedAssociatedElements) {
+        auto& element = control->asHTMLElement();
+        if (!element.isDisabledFormControl())
+            control->appendFormData(domFormData, isMultiPartForm);
+        if (is<HTMLInputElement>(element)) {
+            auto& input = downcast<HTMLInputElement>(element);
+            if (input.isTextField()) {
+                formValues.append({ input.name(), input.value() });
+                input.addSearchResult();
             }
+            if (input.isPasswordField() && !input.value().isEmpty())
+                containsPasswordData = true;
         }
     }