Changeset 206221 in webkit


Ignore:
Timestamp:
Sep 21, 2016 11:23:33 AM (8 years ago)
Author:
Chris Dumez
Message:

Object.getOwnPropertyDescriptor() does not work correctly cross origin
​https://bugs.webkit.org/show_bug.cgi?id=162311

Reviewed by Gavin Barraclough.

LayoutTests/imported/w3c:

Rebaseline W3C test now that more checks are passing.

  • web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:

Source/JavaScriptCore:

Add a CustomGetterSetter field to PropertySlot that gets populated
by getOwnPropertySlot() and use it in getOwnPropertyDescriptor()
to properly populate the descriptor. We used to rely on reifying
the properties and then call getDirect() in order to get the
CustomGetterSetter. However, this hack was insufficient to support
the cross-origin case because we need to control more precisely
the visibility of the getter and the setter. For example, Location's
href property has both a getter and a setter in the same origin
case but only has a setter in the cross-origin case.

In the future, we can extend the use of PropertySlot's
customGetterSetter field to the same origin case and get rid of the
reification + getDirect() hack in getOwnPropertyDescriptor().

  • runtime/JSObject.cpp:

(JSC::JSObject::getOwnPropertyDescriptor):

  • runtime/PropertySlot.cpp:

(JSC::PropertySlot::customAccessorGetter):

  • runtime/PropertySlot.h:

Source/WebCore:

Object.getOwnPropertyDescriptor() does not work correctly cross origin. In particular:

  • We return value descriptors for attributes instead of getter/setter descriptors
  • attributes / operations are wrongly marked as non-configurable

Corresponding specification:

Test: http/tests/security/cross-origin-descriptors.html

  • bindings/js/JSDOMWindowCustom.cpp:

(WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):

  • bindings/js/JSLocationCustom.cpp:

(WebCore::JSLocation::getOwnPropertySlotDelegate):

LayoutTests:

Add layout test coverage.

  • http/tests/security/cross-origin-descriptors-expected.txt: Added.
  • http/tests/security/cross-origin-descriptors.html: Added.
Location:
trunk
Files:
2 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r206217 r206221  
     12016-09-21  Chris Dumez  <cdumez@apple.com>
     2
     3        Object.getOwnPropertyDescriptor() does not work correctly cross origin
     4        https://bugs.webkit.org/show_bug.cgi?id=162311
     5
     6        Reviewed by Gavin Barraclough.
     7
     8        Add layout test coverage.
     9
     10        * http/tests/security/cross-origin-descriptors-expected.txt: Added.
     11        * http/tests/security/cross-origin-descriptors.html: Added.
     12
    1132016-09-21  Daniel Bates  <dabates@apple.com>
    214

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r206214 r206221  
     12016-09-21  Chris Dumez  <cdumez@apple.com>
     2
     3        Object.getOwnPropertyDescriptor() does not work correctly cross origin
     4        https://bugs.webkit.org/show_bug.cgi?id=162311
     5
     6        Reviewed by Gavin Barraclough.
     7
     8        Rebaseline W3C test now that more checks are passing.
     9
     10        * web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:
     11
    1122016-09-21  Jer Noble  <jer.noble@apple.com>
    213

  • trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt

    r205743 r206221  
    88PASS [[IsExtensible]] should return true for cross-origin objects
    99PASS [[PreventExtensions]] should throw for cross-origin objects
    10 FAIL [[GetOwnProperty]] - Properties on cross-origin objects should be reported |own| Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "http://127.0.0.1:8800". Protocols, domains, and ports must match.
    11 FAIL [[GetOwnProperty]] - Property descriptors for cross-origin properties should be set up correctly assert_equals: property descriptor for location should be configurable expected true but got false
     10PASS [[GetOwnProperty]] - Properties on cross-origin objects should be reported |own|
     11PASS [[GetOwnProperty]] - Property descriptors for cross-origin properties should be set up correctly
    1212PASS [[Delete]] Should throw on cross-origin objects
    1313PASS [[DefineOwnProperty]] Should throw for cross-origin objects
    14 FAIL [[Enumerate]] should return an empty iterator Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "http://127.0.0.1:8800". Protocols, domains, and ports must match.
     14FAIL [[Enumerate]] should return an empty iterator assert_unreached: Shouldn't have been able to enumerate href on cross-origin Location Reached unreachable code
    1515PASS [[OwnPropertyKeys]] should return all properties from cross-origin objects
    1616PASS A and B jointly observe the same identity for cross-origin Window and Location
    1717PASS Cross-origin functions get local Function.prototype
    18 FAIL Cross-origin Window accessors get local Function.prototype undefined is not an object (evaluating 'f.name')
     18PASS Cross-origin Window accessors get local Function.prototype
    1919PASS Same-origin observers get different functions for cross-origin objects
    20 FAIL Same-origin obsevers get different accessors for cross-origin Window assert_true: different Window accessors per-incumbent script settings object expected true got false
    21 FAIL Same-origin observers get different accessors for cross-origin Location Blocked a frame with origin "http://localhost:8800" from accessing a frame with origin "http://127.0.0.1:8800". Protocols, domains, and ports must match.
     20PASS Same-origin obsevers get different accessors for cross-origin Window
     21PASS Same-origin observers get different accessors for cross-origin Location
    2222 

  • trunk/Source/JavaScriptCore/ChangeLog

    r206212 r206221  
     12016-09-21  Chris Dumez  <cdumez@apple.com>
     2
     3        Object.getOwnPropertyDescriptor() does not work correctly cross origin
     4        https://bugs.webkit.org/show_bug.cgi?id=162311
     5
     6        Reviewed by Gavin Barraclough.
     7
     8        Add a CustomGetterSetter field to PropertySlot that gets populated
     9        by getOwnPropertySlot() and use it in getOwnPropertyDescriptor()
     10        to properly populate the descriptor. We used to rely on reifying
     11        the properties and then call getDirect() in order to get the
     12        CustomGetterSetter. However, this hack was insufficient to support
     13        the cross-origin case because we need to control more precisely
     14        the visibility of the getter and the setter. For example, Location's
     15        href property has both a getter and a setter in the same origin
     16        case but only has a setter in the cross-origin case.
     17
     18        In the future, we can extend the use of PropertySlot's
     19        customGetterSetter field to the same origin case and get rid of the
     20        reification + getDirect() hack in getOwnPropertyDescriptor().
     21
     22        * runtime/JSObject.cpp:
     23        (JSC::JSObject::getOwnPropertyDescriptor):
     24        * runtime/PropertySlot.cpp:
     25        (JSC::PropertySlot::customAccessorGetter):
     26        * runtime/PropertySlot.h:
     27
    1282016-09-21  Michael Saboff  <msaboff@apple.com>
    229

  • trunk/Source/JavaScriptCore/runtime/JSObject.cpp

    r206154 r206221  
    28432843            thisObject = proxy->target();
    28442844
    2845         JSValue maybeGetterSetter = thisObject->getDirect(exec->vm(), propertyName);
    2846         if (!maybeGetterSetter) {
    2847             thisObject->reifyAllStaticProperties(exec);
    2848             maybeGetterSetter = thisObject->getDirect(exec->vm(), propertyName);
    2849         }
    2850 
    2851         ASSERT(maybeGetterSetter);
    2852         auto* getterSetter = jsCast<CustomGetterSetter*>(maybeGetterSetter);
     2845        CustomGetterSetter* getterSetter;
     2846        if (slot.isCustomAccessor())
     2847            getterSetter = slot.customGetterSetter();
     2848        else {
     2849            JSValue maybeGetterSetter = thisObject->getDirect(exec->vm(), propertyName);
     2850            if (!maybeGetterSetter) {
     2851                thisObject->reifyAllStaticProperties(exec);
     2852                maybeGetterSetter = thisObject->getDirect(exec->vm(), propertyName);
     2853            }
     2854
     2855            ASSERT(maybeGetterSetter);
     2856            getterSetter = jsCast<CustomGetterSetter*>(maybeGetterSetter);
     2857        }
    28532858        if (getterSetter->getter())
    28542859            descriptor.setGetter(getCustomGetterSetterFunctionForGetterSetter(exec, propertyName, getterSetter, JSCustomGetterSetterFunction::Type::Getter));

  • trunk/Source/JavaScriptCore/runtime/PropertySlot.cpp

    r204248 r206221  
    4242}
    4343
     44JSValue PropertySlot::customAccessorGetter(ExecState* exec, PropertyName propertyName) const
     45{
     46    if (!m_data.customAccessor.getterSetter->getter())
     47        return jsUndefined();
     48    return JSValue::decode(m_data.customAccessor.getterSetter->getter()(exec, JSValue::encode(m_thisValue), propertyName));
     49}
     50
    4451JSValue PropertySlot::getPureResult() const
    4552{

  • trunk/Source/JavaScriptCore/runtime/PropertySlot.h

    r204248 r206221  
    7272        TypeValue,
    7373        TypeGetter,
    74         TypeCustom
     74        TypeCustom,
     75        TypeCustomAccessor,
    7576    };
    7677
    … …  
    108109    bool isAccessor() const { return m_propertyType == TypeGetter; }
    109110    bool isCustom() const { return m_propertyType == TypeCustom; }
     111    bool isCustomAccessor() const { return m_propertyType == TypeCustomAccessor; }
    110112    bool isCacheableValue() const { return isCacheable() && isValue(); }
    111113    bool isCacheableGetter() const { return isCacheable() && isAccessor(); }
    112114    bool isCacheableCustom() const { return isCacheable() && isCustom(); }
     115    bool isCacheableCustomAccessor() const { return isCacheable() && isCustomAccessor(); }
    113116    void setIsTaintedByOpaqueObject() { m_isTaintedByOpaqueObject = true; }
    114117    bool isTaintedByOpaqueObject() const { return m_isTaintedByOpaqueObject; }
    … …  
    141144    }
    142145
     146    CustomGetterSetter* customGetterSetter() const
     147    {
     148        ASSERT(isCustomAccessor());
     149        return m_data.customAccessor.getterSetter;
     150    }
     151
    143152    JSObject* slotBase() const
    144153    {
    … …  
    216225        m_slotBase = slotBase;
    217226        m_propertyType = TypeCustom;
     227        m_offset = !invalidOffset;
     228    }
     229
     230    void setCustomGetterSetter(JSObject* slotBase, unsigned attributes, CustomGetterSetter* getterSetter)
     231    {
     232        ASSERT(attributes == attributesForStructure(attributes));
     233
     234        ASSERT(getterSetter);
     235        m_data.customAccessor.getterSetter = getterSetter;
     236        m_attributes = attributes;
     237
     238        ASSERT(slotBase);
     239        m_slotBase = slotBase;
     240        m_propertyType = TypeCustomAccessor;
     241        m_offset = invalidOffset;
     242    }
     243
     244    void setCacheableCustomGetterSetter(JSObject* slotBase, unsigned attributes, CustomGetterSetter* getterSetter)
     245    {
     246        ASSERT(attributes == attributesForStructure(attributes));
     247
     248        ASSERT(getterSetter);
     249        m_data.customAccessor.getterSetter = getterSetter;
     250        m_attributes = attributes;
     251
     252        ASSERT(slotBase);
     253        m_slotBase = slotBase;
     254        m_propertyType = TypeCustomAccessor;
    218255        m_offset = !invalidOffset;
    219256    }
    … …  
    275312    JS_EXPORT_PRIVATE JSValue functionGetter(ExecState*) const;
    276313    JS_EXPORT_PRIVATE JSValue customGetter(ExecState*, PropertyName) const;
     314    JS_EXPORT_PRIVATE JSValue customAccessorGetter(ExecState*, PropertyName) const;
    277315
    278316    unsigned m_attributes;
    … …  
    285323            GetValueFunc getValue;
    286324        } custom;
     325        struct {
     326            CustomGetterSetter* getterSetter;
     327        } customAccessor;
    287328    } m_data;
    288329
    … …  
    303344    if (m_propertyType == TypeGetter)
    304345        return functionGetter(exec);
     346    if (m_propertyType == TypeCustomAccessor)
     347        return customAccessorGetter(exec, propertyName);
    305348    return customGetter(exec, propertyName);
    306349}

  • trunk/Source/WebCore/ChangeLog

    r206219 r206221  
     12016-09-21  Chris Dumez  <cdumez@apple.com>
     2
     3        Object.getOwnPropertyDescriptor() does not work correctly cross origin
     4        https://bugs.webkit.org/show_bug.cgi?id=162311
     5
     6        Reviewed by Gavin Barraclough.
     7
     8        Object.getOwnPropertyDescriptor() does not work correctly cross origin. In particular:
     9        - We return value descriptors for attributes instead of getter/setter descriptors
     10        - attributes / operations are wrongly marked as non-configurable
     11
     12        Corresponding specification:
     13        - https://html.spec.whatwg.org/#crossoriginproperties-(-o-)
     14        - https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-)
     15
     16        Test: http/tests/security/cross-origin-descriptors.html
     17
     18        * bindings/js/JSDOMWindowCustom.cpp:
     19        (WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
     20        * bindings/js/JSLocationCustom.cpp:
     21        (WebCore::JSLocation::getOwnPropertySlotDelegate):
     22
    1232016-09-21  Alex Christensen  <achristensen@webkit.org>
    224

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

    r205939 r206221  
    102102    // Always provide the original function, on a fresh uncached function object.
    103103    if (propertyName == exec->propertyNames().blur) {
    104         slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionBlur, 0>);
     104        slot.setCustom(thisObject, ReadOnly | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionBlur, 0>);
    105105        return true;
    106106    }
    107107    if (propertyName == exec->propertyNames().close) {
    108         slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionClose, 0>);
     108        slot.setCustom(thisObject, ReadOnly | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionClose, 0>);
    109109        return true;
    110110    }
    111111    if (propertyName == exec->propertyNames().focus) {
    112         slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionFocus, 0>);
     112        slot.setCustom(thisObject, ReadOnly | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionFocus, 0>);
    113113        return true;
    114114    }
    115115    if (propertyName == exec->propertyNames().postMessage) {
    116         slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionPostMessage, 2>);
     116        slot.setCustom(thisObject, ReadOnly | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionPostMessage, 2>);
    117117        return true;
    118118    }
    … …  
    132132            || propertyName == exec->propertyNames().parent
    133133            || propertyName == exec->propertyNames().top) {
    134             slot.setCacheableCustom(thisObject, ReadOnly | DontDelete | DontEnum, entry->propertyGetter());
     134            bool shouldExposeSetter = propertyName == exec->propertyNames().location;
     135            CustomGetterSetter* customGetterSetter = CustomGetterSetter::create(vm, entry->propertyGetter(), shouldExposeSetter ? entry->propertyPutter() : nullptr);
     136            slot.setCacheableCustomGetterSetter(thisObject, DontEnum | CustomAccessor, customGetterSetter);
    135137            return true;
    136138        }

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

    r205670 r206221  
    5353
    5454    // We only allow access to Location.replace() cross origin.
    55     // Make it read-only / non-configurable to prevent writes via defineProperty.
    5655    if (propertyName == exec->propertyNames().replace) {
    57         slot.setCustom(this, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsLocationInstanceFunctionReplace, 1>);
     56        slot.setCustom(this, ReadOnly | DontEnum, nonCachingStaticFunctionGetter<jsLocationInstanceFunctionReplace, 1>);
     57        return true;
     58    }
     59
     60    // Getting location.href cross origin needs to throw. However, getOwnPropertyDescriptor() needs to return
     61    // a descriptor that has a setter but no getter.
     62    if (slot.internalMethodType() == PropertySlot::InternalMethodType::GetOwnProperty && propertyName == exec->propertyNames().href) {
     63        auto* entry = JSLocation::info()->staticPropHashTable->entry(propertyName);
     64        CustomGetterSetter* customGetterSetter = CustomGetterSetter::create(vm, nullptr, entry->propertyPutter());
     65        slot.setCacheableCustomGetterSetter(this, DontEnum | CustomAccessor, customGetterSetter);
    5866        return true;
    5967    }
Note: See TracChangeset for help on using the changeset viewer.