Title: [224534] trunk/Source/WebCore
- Revision
- 224534
- Author
- [email protected]
- Date
- 2017-11-07 09:41:05 -0800 (Tue, 07 Nov 2017)
Log Message
Release-assert NoEventDispatchAssertion in canExecute, updateLayout, and updateStyle
https://bugs.webkit.org/show_bug.cgi?id=179281
<rdar://problem/35008993>
Reviewed by Antti Koivisto.
Surgically enable NoEventDispatchAssertion::InMainThread::isEventAllowed() in release builds to prevent
against insecure execution of author scripts.
No new tests since there should be no behavioral changes (other than preventing potential security bugs
from being exploited).
* bindings/js/ScriptController.cpp:
(WebCore::ScriptController::canExecuteScripts): Use the release assert here. This function is consulted
whenever author scripts are executed in event handler, script element, etc... in the main thread so
enabling the release assert here should basically prevent all unwanted script executions protected by
NoEventDispatchAssertion.
* dom/ContainerNode.cpp:
(NoEventDispatchAssertion::s_count): Now always compiled.
* dom/Document.cpp:
(WebCore::Document::updateStyleIfNeeded): Use the release assert here. This assertion would prevent
unwanted style updating. This part of the change can be reverted if it turns out to be too crashy since
just updating the style would not directly introduce a security vulnerability.
(WebCore::Document::updateLayout): Ditto for updating the layout.
* dom/NoEventDispatchAssertion.h:
(WebCore::NoEventDispatchAssertion::NoEventDispatchAssertion): Enabled this in release builds.
(WebCore::NoEventDispatchAssertion::~NoEventDispatchAssertion): Ditto.
(WebCore::NoEventDispatchAssertion::isEventAllowedInMainThread): Ditto.
(WebCore::NoEventDispatchAssertion::InMainThread::InMainThread): Ditto.
(WebCore::NoEventDispatchAssertion::InMainThread::~InMainThread): Ditto.
(WebCore::NoEventDispatchAssertion::InMainThread::isEventDispatchAllowedInSubtree): We still don't enable
this assertion because this check requires O(n) operation. Added a comment to that end.
(WebCore::NoEventDispatchAssertion::InMainThread::isEventAllowed): Enabled this in release builds.
(WebCore::NoEventDispatchAssertion::DisableAssertionsInScope): Ditto.
* dom/ScriptElement.cpp:
(WebCore::ScriptElement::executeClassicScript): Use the release assert here. This is the function used by
the HTML parser to run scripts via HTMLScriptRunner::executePendingScriptAndDispatchEvent. Having a release
assertion here should prevent the rest of the unwanted script executions in the HTML parser not caught by
canExecuteScripts.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (224533 => 224534)
--- trunk/Source/WebCore/ChangeLog 2017-11-07 16:28:24 UTC (rev 224533)
+++ trunk/Source/WebCore/ChangeLog 2017-11-07 17:41:05 UTC (rev 224534)
@@ -1,3 +1,45 @@
+2017-11-07 Ryosuke Niwa <[email protected]>
+
+ Release-assert NoEventDispatchAssertion in canExecute, updateLayout, and updateStyle
+ https://bugs.webkit.org/show_bug.cgi?id=179281
+ <rdar://problem/35008993>
+
+ Reviewed by Antti Koivisto.
+
+ Surgically enable NoEventDispatchAssertion::InMainThread::isEventAllowed() in release builds to prevent
+ against insecure execution of author scripts.
+
+ No new tests since there should be no behavioral changes (other than preventing potential security bugs
+ from being exploited).
+
+ * bindings/js/ScriptController.cpp:
+ (WebCore::ScriptController::canExecuteScripts): Use the release assert here. This function is consulted
+ whenever author scripts are executed in event handler, script element, etc... in the main thread so
+ enabling the release assert here should basically prevent all unwanted script executions protected by
+ NoEventDispatchAssertion.
+ * dom/ContainerNode.cpp:
+ (NoEventDispatchAssertion::s_count): Now always compiled.
+ * dom/Document.cpp:
+ (WebCore::Document::updateStyleIfNeeded): Use the release assert here. This assertion would prevent
+ unwanted style updating. This part of the change can be reverted if it turns out to be too crashy since
+ just updating the style would not directly introduce a security vulnerability.
+ (WebCore::Document::updateLayout): Ditto for updating the layout.
+ * dom/NoEventDispatchAssertion.h:
+ (WebCore::NoEventDispatchAssertion::NoEventDispatchAssertion): Enabled this in release builds.
+ (WebCore::NoEventDispatchAssertion::~NoEventDispatchAssertion): Ditto.
+ (WebCore::NoEventDispatchAssertion::isEventAllowedInMainThread): Ditto.
+ (WebCore::NoEventDispatchAssertion::InMainThread::InMainThread): Ditto.
+ (WebCore::NoEventDispatchAssertion::InMainThread::~InMainThread): Ditto.
+ (WebCore::NoEventDispatchAssertion::InMainThread::isEventDispatchAllowedInSubtree): We still don't enable
+ this assertion because this check requires O(n) operation. Added a comment to that end.
+ (WebCore::NoEventDispatchAssertion::InMainThread::isEventAllowed): Enabled this in release builds.
+ (WebCore::NoEventDispatchAssertion::DisableAssertionsInScope): Ditto.
+ * dom/ScriptElement.cpp:
+ (WebCore::ScriptElement::executeClassicScript): Use the release assert here. This is the function used by
+ the HTML parser to run scripts via HTMLScriptRunner::executePendingScriptAndDispatchEvent. Having a release
+ assertion here should prevent the rest of the unwanted script executions in the HTML parser not caught by
+ canExecuteScripts.
+
2017-11-07 Adrian Perez de Castro <[email protected]>
[WPE][GTK] Building with ENABLE_VIDEO=OFF fails to find AudioTrack.idl
Modified: trunk/Source/WebCore/bindings/js/ScriptController.cpp (224533 => 224534)
--- trunk/Source/WebCore/bindings/js/ScriptController.cpp 2017-11-07 16:28:24 UTC (rev 224533)
+++ trunk/Source/WebCore/bindings/js/ScriptController.cpp 2017-11-07 17:41:05 UTC (rev 224534)
@@ -678,7 +678,7 @@
bool ScriptController::canExecuteScripts(ReasonForCallingCanExecuteScripts reason)
{
if (reason == AboutToExecuteScript)
- ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());
+ RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());
if (m_frame.document() && m_frame.document()->isSandboxed(SandboxScripts)) {
// FIXME: This message should be moved off the console once a solution to https://bugs.webkit.org/show_bug.cgi?id=103274 exists.
Modified: trunk/Source/WebCore/dom/ContainerNode.cpp (224533 => 224534)
--- trunk/Source/WebCore/dom/ContainerNode.cpp 2017-11-07 16:28:24 UTC (rev 224533)
+++ trunk/Source/WebCore/dom/ContainerNode.cpp 2017-11-07 17:41:05 UTC (rev 224534)
@@ -70,8 +70,8 @@
ChildNodesLazySnapshot* ChildNodesLazySnapshot::latestSnapshot;
+unsigned NoEventDispatchAssertion::s_count = 0;
#if !ASSERT_DISABLED
-unsigned NoEventDispatchAssertion::s_count = 0;
NoEventDispatchAssertion::EventAllowedScope* NoEventDispatchAssertion::EventAllowedScope::s_currentScope = nullptr;
#endif
Modified: trunk/Source/WebCore/dom/Document.cpp (224533 => 224534)
--- trunk/Source/WebCore/dom/Document.cpp 2017-11-07 16:28:24 UTC (rev 224533)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-11-07 17:41:05 UTC (rev 224534)
@@ -1939,7 +1939,7 @@
}
// The early exit for needsStyleRecalc() is needed when updateWidgetPositions() is called in runOrScheduleAsynchronousTasks().
- ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
+ RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
resolveStyle();
return true;
@@ -1956,7 +1956,7 @@
ASSERT_NOT_REACHED();
return;
}
- ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
+ RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
RenderView::RepaintRegionAccumulator repaintRegionAccumulator(renderView());
Modified: trunk/Source/WebCore/dom/NoEventDispatchAssertion.h (224533 => 224534)
--- trunk/Source/WebCore/dom/NoEventDispatchAssertion.h 2017-11-07 16:28:24 UTC (rev 224533)
+++ trunk/Source/WebCore/dom/NoEventDispatchAssertion.h 2017-11-07 17:41:05 UTC (rev 224534)
@@ -30,13 +30,12 @@
class NoEventDispatchAssertion {
public:
+ // This variant is expensive. Use NoEventDispatchAssertion::InMainThread whenever possible.
NoEventDispatchAssertion()
{
-#if !ASSERT_DISABLED
if (!isMainThread())
return;
++s_count;
-#endif
}
NoEventDispatchAssertion(const NoEventDispatchAssertion&)
@@ -46,21 +45,15 @@
~NoEventDispatchAssertion()
{
-#if !ASSERT_DISABLED
if (!isMainThread())
return;
ASSERT(s_count);
s_count--;
-#endif
}
static bool isEventAllowedInMainThread()
{
-#if ASSERT_DISABLED
- return true;
-#else
return !isMainThread() || !s_count;
-#endif
}
class InMainThread {
@@ -68,33 +61,32 @@
InMainThread()
{
ASSERT(isMainThread());
-#if !ASSERT_DISABLED
++s_count;
-#endif
}
~InMainThread()
{
ASSERT(isMainThread());
-#if !ASSERT_DISABLED
ASSERT(s_count);
--s_count;
-#endif
}
+ // Don't enable this assertion in release since it's O(n).
+ // Release asserts in canExecuteScript should be sufficient for security defense purposes.
static bool isEventDispatchAllowedInSubtree(Node& node)
{
+#if !ASSERT_DISABLED || ENABLE(SECURITY_ASSERTIONS)
return isEventAllowed() || EventAllowedScope::isAllowedNode(node);
+#else
+ UNUSED_PARAM(node);
+ return true;
+#endif
}
static bool isEventAllowed()
{
ASSERT(isMainThread());
-#if !ASSERT_DISABLED
return !s_count;
-#else
- return true;
-#endif
}
};
@@ -137,7 +129,6 @@
};
#endif
-#if !ASSERT_DISABLED
// FIXME: Remove this class once the sync layout inside SVGImage::draw is removed.
class DisableAssertionsInScope {
public:
@@ -154,17 +145,9 @@
private:
unsigned m_originalCount { 0 };
};
-#else
- class DisableAssertionsInScope {
- public:
- DisableAssertionsInScope() { }
- };
-#endif
-#if !ASSERT_DISABLED
private:
WEBCORE_EXPORT static unsigned s_count;
-#endif
};
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/ScriptElement.cpp (224533 => 224534)
--- trunk/Source/WebCore/dom/ScriptElement.cpp 2017-11-07 16:28:24 UTC (rev 224533)
+++ trunk/Source/WebCore/dom/ScriptElement.cpp 2017-11-07 17:41:05 UTC (rev 224534)
@@ -361,7 +361,7 @@
void ScriptElement::executeClassicScript(const ScriptSourceCode& sourceCode)
{
- ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
+ RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());
ASSERT(m_alreadyStarted);
if (sourceCode.isEmpty())
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
