Changeset 248494 in webkit


Ignore:
Timestamp:
Aug 9, 2019 6:20:22 PM (5 years ago)
Author:
ysuzuki@apple.com
Message:

Universal XSS in JSObject::putInlineSlow and JSValue::putToPrimitive
​https://bugs.webkit.org/show_bug.cgi?id=199864

Reviewed by Saam Barati.

Source/JavaScriptCore:

Our JSObject::put implementation is not correct in term of the spec. Our Put? implementation is something like this.

JSObject::put(object):

if (can-do-fast-path(object))

return fast-path(object);

slow-path
do {

object-put-check-and-setter-calls(object); (1)
object = object->prototype;

} while (is-object(object));
return do-put(object);

Since JSObject::put is registered in the methodTable, the derived classes can override it. Some of classes are adding
extra checks to this put.

Derived::put(object):

if (do-extra-check(object))

fail

return JSObject::put(object)

The problem is that Derived::put is only called when the |this| object is the Derived class. When traversing Prototype? in
JSObject::put, at (1), we do not perform the extra checks added in Derived::put even if object is Derived one. This means that
we skip the check.

Currently, JSObject::put and WebCore checking mechanism are broken. JSObject::put should call getOwnPropertySlot at (1) to
perform the additional checks. This behavior is matching against the spec. However, currently, our JSObject::getOwnPropertySlot
does not propagate setter information. This is required to cache cacheable Put? at (1) for CustomValue, CustomAccessor, and
Accessors. We also need to reconsider how to integrate static property setters to this mechanism. So, basically, this involves
large refactoring to renew our JSObject::put and JSObject::getOwnPropertySlot.

To work-around for now, we add a new TypeInfo flag, HasPutPropertySecurityCheck . And adding this flag to DOM objects
that implements the addition checks. We also add doPutPropertySecurityCheck method hook to perform the check in JSObject.
When we found this flag at (1), we perform doPutPropertySecurityCheck to properly perform the checks.

Since our JSObject::put code is old and it does not match against the spec now, we should refactor it largely. This is tracked separately in [1].

[1]: ​https://bugs.webkit.org/show_bug.cgi?id=200562

  • runtime/ClassInfo.h:
  • runtime/JSCJSValue.cpp:

(JSC::JSValue::putToPrimitive):

  • runtime/JSCell.cpp:

(JSC::JSCell::doPutPropertySecurityCheck):

  • runtime/JSCell.h:
  • runtime/JSObject.cpp:

(JSC::JSObject::putInlineSlow):
(JSC::JSObject::getOwnPropertyDescriptor):

  • runtime/JSObject.h:

(JSC::JSObject::doPutPropertySecurityCheck):

  • runtime/JSTypeInfo.h:

(JSC::TypeInfo::hasPutPropertySecurityCheck const):

Source/WebCore:

Test: http/tests/security/cross-frame-access-object-put-optimization.html

  • bindings/js/JSDOMWindowCustom.cpp:

(WebCore::JSDOMWindow::doPutPropertySecurityCheck):

  • bindings/js/JSLocationCustom.cpp:

(WebCore::JSLocation::doPutPropertySecurityCheck):

  • bindings/scripts/CodeGeneratorJS.pm:

(GenerateHeader):

  • bindings/scripts/test/JS/JSTestActiveDOMObject.h:

LayoutTests:

  • http/tests/security/cross-frame-access-object-put-optimization-expected.txt: Added.
  • http/tests/security/cross-frame-access-object-put-optimization.html: Added.
  • http/tests/security/resources/cross-frame-iframe-for-object-put-optimization-test.html: Added.
Location:
trunk
Files:
3 added
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r248491 r248494  
     12019-08-09  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        Universal XSS in JSObject::putInlineSlow and JSValue::putToPrimitive
     4        https://bugs.webkit.org/show_bug.cgi?id=199864
     5
     6        Reviewed by Saam Barati.
     7
     8        * http/tests/security/cross-frame-access-object-put-optimization-expected.txt: Added.
     9        * http/tests/security/cross-frame-access-object-put-optimization.html: Added.
     10        * http/tests/security/resources/cross-frame-iframe-for-object-put-optimization-test.html: Added.
     11
    1122019-08-09  Ali Juma  <ajuma@chromium.org>
    213

  • trunk/Source/JavaScriptCore/ChangeLog

    r248462 r248494  
     12019-08-09  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        Universal XSS in JSObject::putInlineSlow and JSValue::putToPrimitive
     4        https://bugs.webkit.org/show_bug.cgi?id=199864
     5
     6        Reviewed by Saam Barati.
     7
     8        Our JSObject::put implementation is not correct in term of the spec. Our [[Put]] implementation is something like this.
     9
     10            JSObject::put(object):
     11                if (can-do-fast-path(object))
     12                    return fast-path(object);
     13                // slow-path
     14                do {
     15                    object-put-check-and-setter-calls(object); // (1)
     16                    object = object->prototype;
     17                } while (is-object(object));
     18                return do-put(object);
     19
     20        Since JSObject::put is registered in the methodTable, the derived classes can override it. Some of classes are adding
     21        extra checks to this put.
     22
     23            Derived::put(object):
     24                if (do-extra-check(object))
     25                    fail
     26                return JSObject::put(object)
     27
     28        The problem is that Derived::put is only called when the |this| object is the Derived class. When traversing [[Prototype]] in
     29        JSObject::put, at (1), we do not perform the extra checks added in Derived::put even if `object` is Derived one. This means that
     30        we skip the check.
     31
     32        Currently, JSObject::put and WebCore checking mechanism are broken. JSObject::put should call getOwnPropertySlot at (1) to
     33        perform the additional checks. This behavior is matching against the spec. However, currently, our JSObject::getOwnPropertySlot
     34        does not propagate setter information. This is required to cache cacheable [[Put]] at (1) for CustomValue, CustomAccessor, and
     35        Accessors. We also need to reconsider how to integrate static property setters to this mechanism. So, basically, this involves
     36        large refactoring to renew our JSObject::put and JSObject::getOwnPropertySlot.
     37
     38        To work-around for now, we add a new TypeInfo flag, HasPutPropertySecurityCheck . And adding this flag to DOM objects
     39        that implements the addition checks. We also add doPutPropertySecurityCheck method hook to perform the check in JSObject.
     40        When we found this flag at (1), we perform doPutPropertySecurityCheck to properly perform the checks.
     41
     42        Since our JSObject::put code is old and it does not match against the spec now, we should refactor it largely. This is tracked separately in [1].
     43
     44        [1]: https://bugs.webkit.org/show_bug.cgi?id=200562
     45
     46        * runtime/ClassInfo.h:
     47        * runtime/JSCJSValue.cpp:
     48        (JSC::JSValue::putToPrimitive):
     49        * runtime/JSCell.cpp:
     50        (JSC::JSCell::doPutPropertySecurityCheck):
     51        * runtime/JSCell.h:
     52        * runtime/JSObject.cpp:
     53        (JSC::JSObject::putInlineSlow):
     54        (JSC::JSObject::getOwnPropertyDescriptor):
     55        * runtime/JSObject.h:
     56        (JSC::JSObject::doPutPropertySecurityCheck):
     57        * runtime/JSTypeInfo.h:
     58        (JSC::TypeInfo::hasPutPropertySecurityCheck const):
     59
    1602019-08-08  Per Arne Vollan  <pvollan@apple.com>
    261

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

    r248192 r248494  
    7272    using GetOwnPropertySlotByIndexFunctionPtr = bool (*)(JSObject*, ExecState*, unsigned, PropertySlot&);
    7373    GetOwnPropertySlotByIndexFunctionPtr METHOD_TABLE_ENTRY(getOwnPropertySlotByIndex);
     74
     75    using DoPutPropertySecurityCheckFunctionPtr = void (*)(JSObject*, ExecState*, PropertyName, PutPropertySlot&);
     76    DoPutPropertySecurityCheckFunctionPtr METHOD_TABLE_ENTRY(doPutPropertySecurityCheck);
    7477
    7578    using ToThisFunctionPtr = JSValue (*)(JSCell*, ExecState*, ECMAMode);
    … …  
    161164        &ClassName::getOwnPropertySlot, \
    162165        &ClassName::getOwnPropertySlotByIndex, \
     166        &ClassName::doPutPropertySecurityCheck, \
    163167        &ClassName::toThis, \
    164168        &ClassName::defaultValue, \

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

    r246490 r248494  
    163163    JSValue prototype;
    164164    if (propertyName != vm.propertyNames->underscoreProto) {
    165         for (; !obj->structure(vm)->hasReadOnlyOrGetterSetterPropertiesExcludingProto(); obj = asObject(prototype)) {
     165        while (true) {
     166            Structure* structure = obj->structure(vm);
     167            if (structure->hasReadOnlyOrGetterSetterPropertiesExcludingProto() || structure->typeInfo().hasPutPropertySecurityCheck())
     168                break;
    166169            prototype = obj->getPrototype(vm, exec);
    167170            RETURN_IF_EXCEPTION(scope, false);
    … …  
    169172            if (prototype.isNull())
    170173                return typeError(exec, scope, slot.isStrictMode(), ReadonlyPropertyWriteError);
     174            obj = asObject(prototype);
    171175        }
    172176    }
    173177
    174178    for (; ; obj = asObject(prototype)) {
     179        Structure* structure = obj->structure(vm);
     180        if (UNLIKELY(structure->typeInfo().hasPutPropertySecurityCheck())) {
     181            obj->methodTable(vm)->doPutPropertySecurityCheck(obj, exec, propertyName, slot);
     182            RETURN_IF_EXCEPTION(scope, false);
     183        }
    175184        unsigned attributes;
    176         PropertyOffset offset = obj->structure(vm)->get(vm, propertyName, attributes);
     185        PropertyOffset offset = structure->get(vm, propertyName, attributes);
    177186        if (offset != invalidOffset) {
    178187            if (attributes & PropertyAttribute::ReadOnly)

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

    r233765 r248494  
    213213}
    214214
     215void JSCell::doPutPropertySecurityCheck(JSObject*, ExecState*, PropertyName, PutPropertySlot&)
     216{
     217    RELEASE_ASSERT_NOT_REACHED();
     218}
     219
    215220void JSCell::getOwnPropertyNames(JSObject*, ExecState*, PropertyNameArray&, EnumerationMode)
    216221{

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

    r240965 r248494  
    266266    static bool getOwnPropertySlot(JSObject*, ExecState*, PropertyName, PropertySlot&);
    267267    static bool getOwnPropertySlotByIndex(JSObject*, ExecState*, unsigned propertyName, PropertySlot&);
     268    static NO_RETURN_DUE_TO_CRASH void doPutPropertySecurityCheck(JSObject*, ExecState*, PropertyName, PutPropertySlot&);
    268269
    269270private:

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

    r246084 r248494  
    791791    JSObject* obj = this;
    792792    for (;;) {
     793        Structure* structure = obj->structure(vm);
     794        if (UNLIKELY(structure->typeInfo().hasPutPropertySecurityCheck())) {
     795            obj->methodTable(vm)->doPutPropertySecurityCheck(obj, exec, propertyName, slot);
     796            RETURN_IF_EXCEPTION(scope, false);
     797        }
    793798        unsigned attributes;
    794         PropertyOffset offset = obj->structure(vm)->get(vm, propertyName, attributes);
     799        PropertyOffset offset = structure->get(vm, propertyName, attributes);
    795800        if (isValidOffset(offset)) {
    796801            if (attributes & PropertyAttribute::ReadOnly) {
    … …  
    802807            if (gs.isGetterSetter()) {
    803808                // We need to make sure that we decide to cache this property before we potentially execute aribitrary JS.
    804                 if (!structure(vm)->isDictionary())
     809                if (!this->structure(vm)->isDictionary())
    805810                    slot.setCacheableSetter(obj, offset);
    806811
    … …  
    34693474        return false;
    34703475
     3476
     3477    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=200560
     3478    // This breaks the assumption that getOwnPropertySlot should return "own" property.
     3479    // We should fix DebuggerScope, ProxyObject etc. to remove this.
     3480    //
    34713481    // DebuggerScope::getOwnPropertySlot() (and possibly others) may return attributes from the prototype chain
    34723482    // but getOwnPropertyDescriptor() should only work for 'own' properties so we exit early if we detect that

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

    r247386 r248494  
    169169    JS_EXPORT_PRIVATE static bool getOwnPropertySlotByIndex(JSObject*, ExecState*, unsigned propertyName, PropertySlot&);
    170170    bool getOwnPropertySlotInline(ExecState*, PropertyName, PropertySlot&);
     171    static void doPutPropertySecurityCheck(JSObject*, ExecState*, PropertyName, PutPropertySlot&);
    171172
    172173    // The key difference between this and getOwnPropertySlot is that getOwnPropertySlot
    … …  
    14051406}
    14061407
     1408ALWAYS_INLINE void JSObject::doPutPropertySecurityCheck(JSObject*, ExecState*, PropertyName, PutPropertySlot&)
     1409{
     1410}
     1411
    14071412// It may seem crazy to inline a function this large but it makes a big difference
    14081413// since this is function very hot in variable lookup

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

    r240951 r248494  
    5757static const unsigned InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero = 1 << 15;
    5858static const unsigned StructureIsImmortal = 1 << 16;
     59static const unsigned HasPutPropertySecurityCheck = 1 << 17;
    5960
    6061class TypeInfo {
    … …  
    9798    bool getOwnPropertySlotIsImpure() const { return isSetOnFlags2(GetOwnPropertySlotIsImpure); }
    9899    bool getOwnPropertySlotIsImpureForPropertyAbsence() const { return isSetOnFlags2(GetOwnPropertySlotIsImpureForPropertyAbsence); }
     100    bool hasPutPropertySecurityCheck() const { return isSetOnFlags2(HasPutPropertySecurityCheck); }
    99101    bool newImpurePropertyFiresWatchpoints() const { return isSetOnFlags2(NewImpurePropertyFiresWatchpoints); }
    100102    bool isImmutablePrototypeExoticObject() const { return isSetOnFlags2(IsImmutablePrototypeExoticObject); }

  • trunk/Source/WebCore/ChangeLog

    r248493 r248494  
     12019-08-09  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        Universal XSS in JSObject::putInlineSlow and JSValue::putToPrimitive
     4        https://bugs.webkit.org/show_bug.cgi?id=199864
     5
     6        Reviewed by Saam Barati.
     7
     8        Test: http/tests/security/cross-frame-access-object-put-optimization.html
     9
     10        * bindings/js/JSDOMWindowCustom.cpp:
     11        (WebCore::JSDOMWindow::doPutPropertySecurityCheck):
     12        * bindings/js/JSLocationCustom.cpp:
     13        (WebCore::JSLocation::doPutPropertySecurityCheck):
     14        * bindings/scripts/CodeGeneratorJS.pm:
     15        (GenerateHeader):
     16        * bindings/scripts/test/JS/JSTestActiveDOMObject.h:
     17
    1182019-08-09  Saam Barati  <sbarati@apple.com>
    219

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

    r248155 r248494  
    267267}
    268268
     269void JSDOMWindow::doPutPropertySecurityCheck(JSObject* cell, ExecState* state, PropertyName propertyName, PutPropertySlot&)
     270{
     271    VM& vm = state->vm();
     272    auto scope = DECLARE_THROW_SCOPE(vm);
     273
     274    auto* thisObject = jsCast<JSDOMWindow*>(cell);
     275    if (!thisObject->wrapped().frame())
     276        return;
     277
     278    String errorMessage;
     279    if (!BindingSecurity::shouldAllowAccessToDOMWindow(*state, thisObject->wrapped(), errorMessage)) {
     280        // We only allow setting "location" attribute cross-origin.
     281        if (propertyName == static_cast<JSVMClientData*>(vm.clientData)->builtinNames().locationPublicName())
     282            return;
     283        throwSecurityError(*state, scope, errorMessage);
     284        return;
     285    }
     286}
     287
    269288bool JSDOMWindow::put(JSCell* cell, ExecState* state, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
    270289{

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

    r245249 r248494  
    127127}
    128128
     129void JSLocation::doPutPropertySecurityCheck(JSObject* object, ExecState* state, PropertyName propertyName, PutPropertySlot&)
     130{
     131    auto* thisObject = jsCast<JSLocation*>(object);
     132    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     133
     134    VM& vm = state->vm();
     135
     136    // Always allow assigning to the whole location.
     137    // However, alllowing assigning of pieces might inadvertently disclose parts of the original location.
     138    // So fall through to the access check for those.
     139    if (propertyName == static_cast<JSVMClientData*>(vm.clientData)->builtinNames().hrefPublicName())
     140        return;
     141
     142    // Block access and throw if there is a security error.
     143    BindingSecurity::shouldAllowAccessToDOMWindow(state, thisObject->wrapped().window(), ThrowSecurityError);
     144}
     145
    129146bool JSLocation::put(JSCell* cell, ExecState* state, PropertyName propertyName, JSValue value, PutPropertySlot& putPropertySlot)
    130147{

  • trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm

    r248276 r248494  
    26552655        push(@headerContent, "    static bool getOwnPropertySlotByIndex(JSC::JSObject*, JSC::ExecState*, unsigned propertyName, JSC::PropertySlot&);\n");
    26562656        $structureFlags{"JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero"} = 1;
     2657    }
     2658
     2659    if ($interface->extendedAttributes->{CheckSecurity}) {
     2660        push(@headerContent, "    static void doPutPropertySecurityCheck(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PutPropertySlot&);\n");
     2661        $structureFlags{"JSC::HasPutPropertySecurityCheck"} = 1;
    26572662    }
    26582663   

  • trunk/Source/WebCore/bindings/scripts/test/JS/JSTestActiveDOMObject.h

    r239191 r248494  
    4040    static JSC::JSObject* prototype(JSC::VM&, JSDOMGlobalObject&);
    4141    static TestActiveDOMObject* toWrapped(JSC::VM&, JSC::JSValue);
     42    static void doPutPropertySecurityCheck(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PutPropertySlot&);
    4243    static void destroy(JSC::JSCell*);
    4344
    … …  
    5253    static void heapSnapshot(JSCell*, JSC::HeapSnapshotBuilder&);
    5354public:
    54     static const unsigned StructureFlags = Base::StructureFlags | JSC::HasStaticPropertyTable;
     55    static const unsigned StructureFlags = Base::StructureFlags | JSC::HasPutPropertySecurityCheck | JSC::HasStaticPropertyTable;
    5556protected:
    5657    JSTestActiveDOMObject(JSC::Structure*, JSDOMGlobalObject&, Ref<TestActiveDOMObject>&&);
Note: See TracChangeset for help on using the changeset viewer.