Changeset 224534 in webkit


Ignore:
Timestamp:
Nov 7, 2017 9:41:05 AM (6 years ago)
Author:
rniwa@webkit.org
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.

Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r224533 r224534  
     12017-11-07  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Release-assert NoEventDispatchAssertion in canExecute, updateLayout, and updateStyle
     4        https://bugs.webkit.org/show_bug.cgi?id=179281
     5        <rdar://problem/35008993>
     6
     7        Reviewed by Antti Koivisto.
     8
     9        Surgically enable NoEventDispatchAssertion::InMainThread::isEventAllowed() in release builds to prevent
     10        against insecure execution of author scripts.
     11
     12        No new tests since there should be no behavioral changes (other than preventing potential security bugs
     13        from being exploited).
     14
     15        * bindings/js/ScriptController.cpp:
     16        (WebCore::ScriptController::canExecuteScripts): Use the release assert here. This function is consulted
     17        whenever author scripts are executed in event handler, script element, etc... in the main thread so
     18        enabling the release assert here should basically prevent all unwanted script executions protected by
     19        NoEventDispatchAssertion.
     20        * dom/ContainerNode.cpp:
     21        (NoEventDispatchAssertion::s_count): Now always compiled.
     22        * dom/Document.cpp:
     23        (WebCore::Document::updateStyleIfNeeded): Use the release assert here. This assertion would prevent
     24        unwanted style updating. This part of the change can be reverted if it turns out to be too crashy since
     25        just updating the style would not directly introduce a security vulnerability.
     26        (WebCore::Document::updateLayout): Ditto for updating the layout.
     27        * dom/NoEventDispatchAssertion.h:
     28        (WebCore::NoEventDispatchAssertion::NoEventDispatchAssertion): Enabled this in release builds.
     29        (WebCore::NoEventDispatchAssertion::~NoEventDispatchAssertion): Ditto.
     30        (WebCore::NoEventDispatchAssertion::isEventAllowedInMainThread): Ditto.
     31        (WebCore::NoEventDispatchAssertion::InMainThread::InMainThread): Ditto.
     32        (WebCore::NoEventDispatchAssertion::InMainThread::~InMainThread): Ditto.
     33        (WebCore::NoEventDispatchAssertion::InMainThread::isEventDispatchAllowedInSubtree): We still don't enable
     34        this assertion because this check requires O(n) operation. Added a comment to that end.
     35        (WebCore::NoEventDispatchAssertion::InMainThread::isEventAllowed): Enabled this in release builds.
     36        (WebCore::NoEventDispatchAssertion::DisableAssertionsInScope): Ditto.
     37        * dom/ScriptElement.cpp:
     38        (WebCore::ScriptElement::executeClassicScript): Use the release assert here. This is the function used by
     39        the HTML parser to run scripts via HTMLScriptRunner::executePendingScriptAndDispatchEvent. Having a release
     40        assertion here should prevent the rest of the unwanted script executions in the HTML parser not caught by
     41        canExecuteScripts.
     42
    1432017-11-07  Adrian Perez de Castro  <aperez@igalia.com>
    244

  • trunk/Source/WebCore/bindings/js/ScriptController.cpp

    r224356 r224534  
    679679{
    680680    if (reason == AboutToExecuteScript)
    681         ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());
     681        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());
    682682
    683683    if (m_frame.document() && m_frame.document()->isSandboxed(SandboxScripts)) {

  • trunk/Source/WebCore/dom/ContainerNode.cpp

    r224378 r224534  
    7171ChildNodesLazySnapshot* ChildNodesLazySnapshot::latestSnapshot;
    7272
     73unsigned NoEventDispatchAssertion::s_count = 0;
    7374#if !ASSERT_DISABLED
    74 unsigned NoEventDispatchAssertion::s_count = 0;
    7575NoEventDispatchAssertion::EventAllowedScope* NoEventDispatchAssertion::EventAllowedScope::s_currentScope = nullptr;
    7676#endif

  • trunk/Source/WebCore/dom/Document.cpp

    r224472 r224534  
    19401940
    19411941    // The early exit for needsStyleRecalc() is needed when updateWidgetPositions() is called in runOrScheduleAsynchronousTasks().
    1942     ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
     1942    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
    19431943
    19441944    resolveStyle();
    … …  
    19571957        return;
    19581958    }
    1959     ASSERT(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
     1959    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()));
    19601960
    19611961

  • trunk/Source/WebCore/dom/NoEventDispatchAssertion.h

    r224378 r224534  
    3131class NoEventDispatchAssertion {
    3232public:
     33    // This variant is expensive. Use NoEventDispatchAssertion::InMainThread whenever possible.
    3334    NoEventDispatchAssertion()
    3435    {
    35 #if !ASSERT_DISABLED
    3636        if (!isMainThread())
    3737            return;
    3838        ++s_count;
    39 #endif
    4039    }
    4140
    … …  
    4746    ~NoEventDispatchAssertion()
    4847    {
    49 #if !ASSERT_DISABLED
    5048        if (!isMainThread())
    5149            return;
    5250        ASSERT(s_count);
    5351        s_count--;
    54 #endif
    5552    }
    5653
    5754    static bool isEventAllowedInMainThread()
    5855    {
    59 #if ASSERT_DISABLED
    60         return true;
    61 #else
    6256        return !isMainThread() || !s_count;
    63 #endif
    6457    }
    6558
    … …  
    6962        {
    7063            ASSERT(isMainThread());
    71 #if !ASSERT_DISABLED
    7264            ++s_count;
    73 #endif
    7465        }
    7566
    … …  
    7768        {
    7869            ASSERT(isMainThread());
    79 #if !ASSERT_DISABLED
    8070            ASSERT(s_count);
    8171            --s_count;
    82 #endif
    8372        }
    8473
     74        // Don't enable this assertion in release since it's O(n).
     75        // Release asserts in canExecuteScript should be sufficient for security defense purposes.
    8576        static bool isEventDispatchAllowedInSubtree(Node& node)
    8677        {
     78#if !ASSERT_DISABLED || ENABLE(SECURITY_ASSERTIONS)
    8779            return isEventAllowed() || EventAllowedScope::isAllowedNode(node);
     80#else
     81            UNUSED_PARAM(node);
     82            return true;
     83#endif
    8884        }
    8985
    … …  
    9187        {
    9288            ASSERT(isMainThread());
    93 #if !ASSERT_DISABLED
    9489            return !s_count;
    95 #else
    96             return true;
    97 #endif
    9890        }
    9991    };
    … …  
    138130#endif
    139131
    140 #if !ASSERT_DISABLED
    141132    // FIXME: Remove this class once the sync layout inside SVGImage::draw is removed.
    142133    class DisableAssertionsInScope {
    … …  
    155146        unsigned m_originalCount { 0 };
    156147    };
    157 #else
    158     class DisableAssertionsInScope {
    159     public:
    160         DisableAssertionsInScope() { }
    161     };
    162 #endif
    163148
    164 #if !ASSERT_DISABLED
    165149private:
    166150    WEBCORE_EXPORT static unsigned s_count;
    167 #endif
    168151};
    169152

  • trunk/Source/WebCore/dom/ScriptElement.cpp

    r223628 r224534  
    362362void ScriptElement::executeClassicScript(const ScriptSourceCode& sourceCode)
    363363{
    364     ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::isEventAllowedInMainThread());
     364    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());
    365365    ASSERT(m_alreadyStarted);
    366366
Note: See TracChangeset for help on using the changeset viewer.