Changeset 196628 in webkit


Ignore:
Timestamp:
Feb 16, 2016 12:11:47 AM (8 years ago)
Author:
Chris Dumez
Message:

Do security checks early in JSDOMWindow::put*()
​https://bugs.webkit.org/show_bug.cgi?id=154270

Reviewed by Gavin Barraclough.

Source/WebCore:

Do security checks early in JSDOMWindow::put() / JSDOMWindow::putByIndex()
and return as soon as possible. This makes it less error-prone as we need
to do the security check only once, at the top of the function.

Also lock down the security further by calling lookupPut() only if the
property name is "location". The "location" property is the only one that
can be set cross-origin. Previously, trying to set a property such as
"name" (which cannot be set cross-origin) relied on the attribute setter
doing the security check when getting called. The new check is less error
prone and will correctly prevent overriding window's method cross-origin
once these move down from the prototype (Bug 154120).

Finally, the previous code was failing to set the "location" property
cross-origin after the window has been reified. This patch fixes the
issue by always calling the original "location" property setter from the
static table in the cross-origin case.

Test: http/tests/security/cross-origin-reified-window-location-setting.html

  • bindings/js/JSDOMWindowCustom.cpp:

(WebCore::JSDOMWindow::put):
(WebCore::JSDOMWindow::putByIndex):

LayoutTests:

  • http/tests/security/cross-frame-access-put-expected.txt:

Rebaseline. The extra security warnings are for the following properties:
closed, crypto, frameElement, pageXOffset and pageYOffset.
All these properties are read-only and therefore cannot be set (cross-origin
or not). The previous code was not doing an explicit check and ended up
trying to set these properties. However, since they are read-only, we would
silently fail to set them. The new code does the explicit check and therefore
will warn and NOT attempt to set.

  • http/tests/security/cross-origin-reified-window-location-setting-expected.txt: Added.
  • http/tests/security/cross-origin-reified-window-location-setting.html: Added.

Add test to check that setting window.location cross-origin still works after the
window object has been reified.

Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r196625 r196628  
     12016-02-16  Chris Dumez  <cdumez@apple.com>
     2
     3        Do security checks early in JSDOMWindow::put*()
     4        https://bugs.webkit.org/show_bug.cgi?id=154270
     5
     6        Reviewed by Gavin Barraclough.
     7
     8        * http/tests/security/cross-frame-access-put-expected.txt:
     9        Rebaseline. The extra security warnings are for the following properties:
     10        closed, crypto, frameElement, pageXOffset and pageYOffset.
     11        All these properties are read-only and therefore cannot be set (cross-origin
     12        or not). The previous code was not doing an explicit check and ended up
     13        trying to set these properties. However, since they are read-only, we would
     14        silently fail to set them. The new code does the explicit check and therefore
     15        will warn and NOT attempt to set.
     16
     17        * http/tests/security/cross-origin-reified-window-location-setting-expected.txt: Added.
     18        * http/tests/security/cross-origin-reified-window-location-setting.html: Added.
     19        Add test to check that setting window.location cross-origin still works after the
     20        window object has been reified.
     21
    1222016-02-15  Mark Lam  <mark.lam@apple.com>
    223

  • trunk/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt

    r196392 r196628  
    272272CONSOLE MESSAGE: line 122: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    273273CONSOLE MESSAGE: line 131: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
     274CONSOLE MESSAGE: line 132: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    274275CONSOLE MESSAGE: line 133: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
     276CONSOLE MESSAGE: line 134: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    275277CONSOLE MESSAGE: line 135: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    276278CONSOLE MESSAGE: line 136: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    … …  
    279281CONSOLE MESSAGE: line 139: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    280282CONSOLE MESSAGE: line 140: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
     283CONSOLE MESSAGE: line 141: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    281284CONSOLE MESSAGE: line 142: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    282285CONSOLE MESSAGE: line 143: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    … …  
    318321CONSOLE MESSAGE: line 179: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    319322CONSOLE MESSAGE: line 180: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
     323CONSOLE MESSAGE: line 181: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
     324CONSOLE MESSAGE: line 182: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    320325CONSOLE MESSAGE: line 183: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
    321326CONSOLE MESSAGE: line 184: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.

  • trunk/Source/WebCore/ChangeLog

    r196622 r196628  
     12016-02-16  Chris Dumez  <cdumez@apple.com>
     2
     3        Do security checks early in JSDOMWindow::put*()
     4        https://bugs.webkit.org/show_bug.cgi?id=154270
     5
     6        Reviewed by Gavin Barraclough.
     7
     8        Do security checks early in JSDOMWindow::put() / JSDOMWindow::putByIndex()
     9        and return as soon as possible. This makes it less error-prone as we need
     10        to do the security check only once, at the top of the function.
     11
     12        Also lock down the security further by calling lookupPut() only if the
     13        property name is "location". The "location" property is the only one that
     14        can be set cross-origin. Previously, trying to set a property such as
     15        "name" (which cannot be set cross-origin) relied on the attribute setter
     16        doing the security check when getting called. The new check is less error
     17        prone and will correctly prevent overriding window's method cross-origin
     18        once these move down from the prototype (Bug 154120).
     19
     20        Finally, the previous code was failing to set the "location" property
     21        cross-origin after the window has been reified. This patch fixes the
     22        issue by always calling the original "location" property setter from the
     23        static table in the cross-origin case.
     24
     25        Test: http/tests/security/cross-origin-reified-window-location-setting.html
     26
     27        * bindings/js/JSDOMWindowCustom.cpp:
     28        (WebCore::JSDOMWindow::put):
     29        (WebCore::JSDOMWindow::putByIndex):
     30
    1312016-02-15  Brent Fulgham  <bfulgham@apple.com>
    232

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

    r196583 r196628  
    348348void JSDOMWindow::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
    349349{
    350     JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(cell);
     350    auto* thisObject = jsCast<JSDOMWindow*>(cell);
    351351    if (!thisObject->wrapped().frame())
    352352        return;
     353
     354    String errorMessage;
     355    if (!shouldAllowAccessToDOMWindow(exec, thisObject->wrapped(), errorMessage)) {
     356        // We only allow setting "location" attribute cross-origin.
     357        if (propertyName == exec->propertyNames().location)
     358            lookupPut(exec, propertyName, thisObject, value, *s_info.staticPropHashTable, slot);
     359        else
     360            thisObject->printErrorMessage(errorMessage);
     361        return;
     362    }
    353363
    354364    // Optimization: access JavaScript global variables directly before involving the DOM.
    355365    if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, propertyName)) {
    356         if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
    357             JSGlobalObject::put(thisObject, exec, propertyName, value, slot);
     366        JSGlobalObject::put(thisObject, exec, propertyName, value, slot);
    358367        return;
    359368    }
    … …  
    364373    }
    365374
    366     if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
    367         Base::put(thisObject, exec, propertyName, value, slot);
     375    Base::put(thisObject, exec, propertyName, value, slot);
    368376}
    369377
    370378void JSDOMWindow::putByIndex(JSCell* cell, ExecState* exec, unsigned index, JSValue value, bool shouldThrow)
    371379{
    372     JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(cell);
    373     if (!thisObject->wrapped().frame())
     380    auto* thisObject = jsCast<JSDOMWindow*>(cell);
     381    if (!thisObject->wrapped().frame() || !BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
    374382        return;
    375383   
    376     Identifier propertyName = Identifier::from(exec, index);
    377 
    378384    // Optimization: access JavaScript global variables directly before involving the DOM.
    379     if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, propertyName)) {
    380         if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
    381             JSGlobalObject::putByIndex(thisObject, exec, index, value, shouldThrow);
     385    if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, Identifier::from(exec, index))) {
     386        JSGlobalObject::putByIndex(thisObject, exec, index, value, shouldThrow);
    382387        return;
    383388    }
    384389   
    385     if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
    386         Base::putByIndex(thisObject, exec, index, value, shouldThrow);
     390    Base::putByIndex(thisObject, exec, index, value, shouldThrow);
    387391}
    388392
Note: See TracChangeset for help on using the changeset viewer.