Merge r183064 - Use ASSERT_WITH_SECURITY_IMPLICATION() for NoEventDis…

…patchAssertion https://bugs.webkit.org/show_bug.cgi?id=143971 Reviewed by Darin Adler. Use ASSERT_WITH_SECURITY_IMPLICATION() for NoEventDispatchAssertion as firing JS events can cause arbitrary JS execution which often leads to security bugs when event firing is forbidden. For e.g. firing events from ActiveDOMObject::suspend() means JS can construct or destroy ActiveDOMObjects while we are iterating over them. * dom/ContainerNode.cpp: (WebCore::dispatchChildInsertionEvents): (WebCore::dispatchChildRemovalEvents): * dom/ContainerNodeAlgorithms.h: (WebCore::ChildNodeInsertionNotifier::notify): * dom/Document.cpp: (WebCore::Document::dispatchWindowEvent): (WebCore::Document::dispatchWindowLoadEvent): * dom/Element.cpp: (WebCore::Element::dispatchFocusInEvent): (WebCore::Element::dispatchFocusOutEvent): * dom/EventDispatcher.cpp: (WebCore::EventDispatcher::dispatchEvent): * dom/EventTarget.cpp: (WebCore::EventTarget::fireEventListeners): * dom/Node.cpp: (WebCore::Node::dispatchSubtreeModifiedEvent): (WebCore::Node::dispatchDOMActivateEvent): * dom/ScriptExecutionContext.cpp: (WebCore::ScriptExecutionContext::canSuspendActiveDOMObjectsForPageCache): (WebCore::ScriptExecutionContext::suspendActiveDOMObjects): (WebCore::ScriptExecutionContext::resumeActiveDOMObjects): (WebCore::ScriptExecutionContext::stopActiveDOMObjects): (WebCore::ScriptExecutionContext::willDestroyActiveDOMObject): * dom/WebKitNamedFlow.cpp: (WebCore::WebKitNamedFlow::dispatchRegionOversetChangeEvent):
WebKit · May 11, 2015 · 8a1f16d · 8a1f16d
1 parent 3c5c6c6
commit 8a1f16d
Show file tree

Hide file tree

Showing 10 changed files with 59 additions and 18 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,43 @@
+2015-04-21  Chris Dumez  <cdumez@apple.com>
+
+        Use ASSERT_WITH_SECURITY_IMPLICATION() for NoEventDispatchAssertion
+        https://bugs.webkit.org/show_bug.cgi?id=143971
+
+        Reviewed by Darin Adler.
+
+        Use ASSERT_WITH_SECURITY_IMPLICATION() for NoEventDispatchAssertion as
+        firing JS events can cause arbitrary JS execution which often leads to
+        security bugs when event firing is forbidden. For e.g. firing events
+        from ActiveDOMObject::suspend() means JS can construct or destroy
+        ActiveDOMObjects while we are iterating over them.
+
+        * dom/ContainerNode.cpp:
+        (WebCore::dispatchChildInsertionEvents):
+        (WebCore::dispatchChildRemovalEvents):
+        * dom/ContainerNodeAlgorithms.h:
+        (WebCore::ChildNodeInsertionNotifier::notify):
+        * dom/Document.cpp:
+        (WebCore::Document::dispatchWindowEvent):
+        (WebCore::Document::dispatchWindowLoadEvent):
+        * dom/Element.cpp:
+        (WebCore::Element::dispatchFocusInEvent):
+        (WebCore::Element::dispatchFocusOutEvent):
+        * dom/EventDispatcher.cpp:
+        (WebCore::EventDispatcher::dispatchEvent):
+        * dom/EventTarget.cpp:
+        (WebCore::EventTarget::fireEventListeners):
+        * dom/Node.cpp:
+        (WebCore::Node::dispatchSubtreeModifiedEvent):
+        (WebCore::Node::dispatchDOMActivateEvent):
+        * dom/ScriptExecutionContext.cpp:
+        (WebCore::ScriptExecutionContext::canSuspendActiveDOMObjectsForPageCache):
+        (WebCore::ScriptExecutionContext::suspendActiveDOMObjects):
+        (WebCore::ScriptExecutionContext::resumeActiveDOMObjects):
+        (WebCore::ScriptExecutionContext::stopActiveDOMObjects):
+        (WebCore::ScriptExecutionContext::willDestroyActiveDOMObject):
+        * dom/WebKitNamedFlow.cpp:
+        (WebCore::WebKitNamedFlow::dispatchRegionOversetChangeEvent):
+
 2015-04-20  Simon Fraser  <simon.fraser@apple.com>
 
         REGRESSION (r177494): -webkit-mask-image: with data URI fails on non-local files

diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
@@ -799,7 +799,7 @@ static void dispatchChildInsertionEvents(Node& child)
     if (child.isInShadowTree())
         return;
 
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
 
     RefPtr<Node> c = &child;
     Ref<Document> document(child.document());
@@ -821,7 +821,7 @@ static void dispatchChildRemovalEvents(Node& child)
         return;
     }
 
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
 
     willCreatePossiblyOrphanedTreeByRemoval(&child);
     InspectorInstrumentation::willRemoveDOMNode(child.document(), child);

diff --git a/Source/WebCore/dom/ContainerNodeAlgorithms.h b/Source/WebCore/dom/ContainerNodeAlgorithms.h
@@ -214,7 +214,7 @@ inline void ChildNodeInsertionNotifier::notifyNodeInsertedIntoTree(ContainerNode
 
 inline void ChildNodeInsertionNotifier::notify(Node& node)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
 
     InspectorInstrumentation::didInsertDOMNode(node.document(), node);
 

diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
@@ -3810,15 +3810,15 @@ EventListener* Document::getWindowAttributeEventListener(const AtomicString& eve
 
 void Document::dispatchWindowEvent(PassRefPtr<Event> event,  PassRefPtr<EventTarget> target)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     if (!m_domWindow)
         return;
     m_domWindow->dispatchEvent(event, target);
 }
 
 void Document::dispatchWindowLoadEvent()
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     if (!m_domWindow)
         return;
     m_domWindow->dispatchLoadEvent();

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -2071,14 +2071,14 @@ void Element::blur()
 
 void Element::dispatchFocusInEvent(const AtomicString& eventType, RefPtr<Element>&& oldFocusedElement)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     ASSERT(eventType == eventNames().focusinEvent || eventType == eventNames().DOMFocusInEvent);
     dispatchScopedEvent(FocusEvent::create(eventType, true, false, document().defaultView(), 0, WTF::move(oldFocusedElement)));
 }
 
 void Element::dispatchFocusOutEvent(const AtomicString& eventType, RefPtr<Element>&& newFocusedElement)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     ASSERT(eventType == eventNames().focusoutEvent || eventType == eventNames().DOMFocusOutEvent);
     dispatchScopedEvent(FocusEvent::create(eventType, true, false, document().defaultView(), 0, WTF::move(newFocusedElement)));
 }

diff --git a/Source/WebCore/dom/EventDispatcher.cpp b/Source/WebCore/dom/EventDispatcher.cpp
@@ -319,7 +319,7 @@ static void dispatchEventInDOM(Event& event, const EventPath& path, WindowEventC
 
 bool EventDispatcher::dispatchEvent(Node* origin, PassRefPtr<Event> prpEvent)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     if (!prpEvent)
         return true;
 
@@ -341,7 +341,7 @@ bool EventDispatcher::dispatchEvent(Node* origin, PassRefPtr<Event> prpEvent)
     ChildNodesLazySnapshot::takeChildNodesLazySnapshot();
 
     event->setTarget(&eventTargetRespectingTargetRules(*node));
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     ASSERT(event->target());
     WindowEventContext windowEventContext(node.get(), eventPath.lastContextIfExists());
 

diff --git a/Source/WebCore/dom/EventTarget.cpp b/Source/WebCore/dom/EventTarget.cpp
@@ -189,7 +189,7 @@ static const AtomicString& legacyType(const Event* event)
 
 bool EventTarget::fireEventListeners(Event* event)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     ASSERT(event && !event->type().isEmpty());
 
     EventTargetData* d = eventTargetData();

diff --git a/Source/WebCore/dom/Node.cpp b/Source/WebCore/dom/Node.cpp
@@ -2022,7 +2022,7 @@ void Node::dispatchSubtreeModifiedEvent()
     if (isInShadowTree())
         return;
 
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
 
     if (!document().hasListenerType(Document::DOMSUBTREEMODIFIED_LISTENER))
         return;
@@ -2035,7 +2035,7 @@ void Node::dispatchSubtreeModifiedEvent()
 
 bool Node::dispatchDOMActivateEvent(int detail, PassRefPtr<Event> underlyingEvent)
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
     RefPtr<UIEvent> event = UIEvent::create(eventNames().DOMActivateEvent, true, true, document().defaultView(), detail);
     event->setUnderlyingEvent(underlyingEvent);
     dispatchScopedEvent(event);

diff --git a/Source/WebCore/dom/ScriptExecutionContext.cpp b/Source/WebCore/dom/ScriptExecutionContext.cpp
@@ -187,8 +187,9 @@ bool ScriptExecutionContext::canSuspendActiveDOMObjects(Vector<ActiveDOMObject*>
 
     // We assume that m_activeDOMObjects will not change during iteration: canSuspend
     // functions should not add new active DOM objects, nor execute arbitrary JavaScript.
-    // An ASSERT or RELEASE_ASSERT will fire if this happens, but it's important to code
+    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
     // canSuspend functions so it will not happen!
+    NoEventDispatchAssertion assertNoEventDispatch;
     for (auto* activeDOMObject : m_activeDOMObjects) {
         if (!activeDOMObject->canSuspend()) {
             canSuspend = false;
@@ -225,7 +226,7 @@ void ScriptExecutionContext::suspendActiveDOMObjects(ActiveDOMObject::ReasonForS
 
     // We assume that m_activeDOMObjects will not change during iteration: suspend
     // functions should not add new active DOM objects, nor execute arbitrary JavaScript.
-    // An ASSERT or RELEASE_ASSERT will fire if this happens, but it's important to code
+    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
     // suspend functions so it will not happen!
     for (auto* activeDOMObject : m_activeDOMObjects)
         activeDOMObject->suspend(why);
@@ -254,7 +255,7 @@ void ScriptExecutionContext::resumeActiveDOMObjects(ActiveDOMObject::ReasonForSu
 
     // We assume that m_activeDOMObjects will not change during iteration: resume
     // functions should not add new active DOM objects, nor execute arbitrary JavaScript.
-    // An ASSERT or RELEASE_ASSERT will fire if this happens, but it's important to code
+    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code
     // resume functions so it will not happen!
     for (auto* activeDOMObject : m_activeDOMObjects)
         activeDOMObject->resume();
@@ -281,7 +282,7 @@ void ScriptExecutionContext::stopActiveDOMObjects()
 
     // We assume that new objects will not be added to m_activeDOMObjects during iteration:
     // stop functions should not add new active DOM objects, nor execute arbitrary JavaScript.
-    // A RELEASE_ASSERT will fire if this happens, but it's important to code stop functions
+    // An ASSERT_WITH_SECURITY_IMPLICATION or RELEASE_ASSERT will fire if this happens, but it's important to code stop functions
     // so it will not happen!
     for (auto* activeDOMObject : possibleActiveDOMObjects) {
         // Check if this object was deleted already. If so, just skip it.
@@ -323,7 +324,7 @@ void ScriptExecutionContext::didCreateActiveDOMObject(ActiveDOMObject& activeDOM
 
 void ScriptExecutionContext::willDestroyActiveDOMObject(ActiveDOMObject& activeDOMObject)
 {
-    ASSERT(!m_activeDOMObjectRemovalForbidden);
+    ASSERT_WITH_SECURITY_IMPLICATION(!m_activeDOMObjectRemovalForbidden);
     m_activeDOMObjects.remove(&activeDOMObject);
 }
 

diff --git a/Source/WebCore/dom/WebKitNamedFlow.cpp b/Source/WebCore/dom/WebKitNamedFlow.cpp
@@ -210,7 +210,7 @@ void WebKitNamedFlow::setRenderer(RenderNamedFlowThread* parentFlowThread)
 
 void WebKitNamedFlow::dispatchRegionOversetChangeEvent()
 {
-    ASSERT(!NoEventDispatchAssertion::isEventDispatchForbidden());
+    ASSERT_WITH_SECURITY_IMPLICATION(!NoEventDispatchAssertion::isEventDispatchForbidden());
 
     // If the flow is in the "NULL" state the event should not be dispatched any more.
     if (flowState() == FlowStateNull)