Changeset 48619 in webkit


Ignore:
Timestamp:
Sep 21, 2009 10:08:37 PM (15 years ago)
Author:
abarth@webkit.org
Message:

2009-09-21 Adam Barth <​abarth@webkit.org>

Reviewed by Sam Weinig.

Don't re-enter JavaScript after performing access checks
​https://bugs.webkit.org/show_bug.cgi?id=29531

Moved the access check slightly later in this functions to avoid
re-entering the JavaScript interpreter (typically via toString)
after performing the access check.

I can't really think of a meaningful test for this change. It's more
security hygiene.

  • bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::setLocation): (WebCore::JSDOMWindow::open): (WebCore::JSDOMWindow::showModalDialog):
  • bindings/js/JSLocationCustom.cpp: (WebCore::JSLocation::setHref): (WebCore::JSLocation::replace): (WebCore::JSLocation::assign):
  • bindings/v8/custom/V8DOMWindowCustom.cpp: (WebCore::V8Custom::WindowSetTimeoutImpl): (WebCore::if): (CALLBACK_FUNC_DECL): (V8Custom::WindowSetLocation): (V8Custom::ClearTimeoutImpl):
  • bindings/v8/custom/V8LocationCustom.cpp: (WebCore::ACCESSOR_SETTER): (WebCore::CALLBACK_FUNC_DECL):
Location:
trunk/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebCore/ChangeLog

    r48617 r48619  
     12009-09-21  Adam Barth  <abarth@webkit.org>
     2
     3        Reviewed by Sam Weinig.
     4
     5        Don't re-enter JavaScript after performing access checks
     6        https://bugs.webkit.org/show_bug.cgi?id=29531
     7
     8        Moved the access check slightly later in this functions to avoid
     9        re-entering the JavaScript interpreter (typically via toString)
     10        after performing the access check.
     11
     12        I can't really think of a meaningful test for this change.  It's more
     13        security hygiene.
     14
     15        * bindings/js/JSDOMWindowCustom.cpp:
     16        (WebCore::JSDOMWindow::setLocation):
     17        (WebCore::JSDOMWindow::open):
     18        (WebCore::JSDOMWindow::showModalDialog):
     19        * bindings/js/JSLocationCustom.cpp:
     20        (WebCore::JSLocation::setHref):
     21        (WebCore::JSLocation::replace):
     22        (WebCore::JSLocation::assign):
     23        * bindings/v8/custom/V8DOMWindowCustom.cpp:
     24        (WebCore::V8Custom::WindowSetTimeoutImpl):
     25        (WebCore::if):
     26        (CALLBACK_FUNC_DECL):
     27        (V8Custom::WindowSetLocation):
     28        (V8Custom::ClearTimeoutImpl):
     29        * bindings/v8/custom/V8LocationCustom.cpp:
     30        (WebCore::ACCESSOR_SETTER):
     31        (WebCore::CALLBACK_FUNC_DECL):
     32
    1332009-09-21  Dumitru Daniliuc  <dumi@chromium.org>
    234

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

    r48542 r48619  
    581581    ASSERT(frame);
    582582
    583     if (!shouldAllowNavigation(exec, frame))
    584         return;
    585 
    586583    KURL url = completeURL(exec, value.toString(exec));
    587584    if (url.isNull())
     585        return;
     586
     587    if (!shouldAllowNavigation(exec, frame))
    588588        return;
    589589
    … …  
    782782JSValue JSDOMWindow::open(ExecState* exec, const ArgList& args)
    783783{
     784    String urlString = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
     785    AtomicString frameName = args.at(1).isUndefinedOrNull() ? "_blank" : AtomicString(args.at(1).toString(exec));
     786    WindowFeatures windowFeatures(valueToStringWithUndefinedOrNullCheck(exec, args.at(2)));
     787
    784788    Frame* frame = impl()->frame();
    785789    if (!frame)
    … …  
    793797
    794798    Page* page = frame->page();
    795 
    796     String urlString = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
    797     AtomicString frameName = args.at(1).isUndefinedOrNull() ? "_blank" : AtomicString(args.at(1).toString(exec));
    798799
    799800    // Because FrameTree::find() returns true for empty strings, we must check for empty framenames.
    … …  
    814815    }
    815816    if (topOrParent) {
    816         if (!shouldAllowNavigation(exec, frame))
    817             return jsUndefined();
    818 
    819817        String completedURL;
    820818        if (!urlString.isEmpty())
    821819            completedURL = completeURL(exec, urlString).string();
     820
     821        if (!shouldAllowNavigation(exec, frame))
     822            return jsUndefined();
    822823
    823824        const JSDOMWindow* targetedWindow = toJSDOMWindow(frame);
    … …  
    836837
    837838    // In the case of a named frame or a new window, we'll use the createWindow() helper
    838     WindowFeatures windowFeatures(valueToStringWithUndefinedOrNullCheck(exec, args.at(2)));
    839839    FloatRect windowRect(windowFeatures.xSet ? windowFeatures.x : 0, windowFeatures.ySet ? windowFeatures.y : 0,
    840840                         windowFeatures.widthSet ? windowFeatures.width : 0, windowFeatures.heightSet ? windowFeatures.height : 0);
    … …  
    856856JSValue JSDOMWindow::showModalDialog(ExecState* exec, const ArgList& args)
    857857{
    858     Frame* frame = impl()->frame();
    859     if (!frame)
    860         return jsUndefined();
    861     Frame* lexicalFrame = toLexicalFrame(exec);
    862     if (!lexicalFrame)
    863         return jsUndefined();
    864     Frame* dynamicFrame = toDynamicFrame(exec);
    865     if (!dynamicFrame)
    866         return jsUndefined();
    867 
    868     if (!DOMWindow::canShowModalDialogNow(frame) || !DOMWindow::allowPopUp(dynamicFrame))
    869         return jsUndefined();
    870 
    871858    String url = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
    872859    JSValue dialogArgs = args.at(1);
    873860    String featureArgs = valueToStringWithUndefinedOrNullCheck(exec, args.at(2));
     861
     862    Frame* frame = impl()->frame();
     863    if (!frame)
     864        return jsUndefined();
     865    Frame* lexicalFrame = toLexicalFrame(exec);
     866    if (!lexicalFrame)
     867        return jsUndefined();
     868    Frame* dynamicFrame = toDynamicFrame(exec);
     869    if (!dynamicFrame)
     870        return jsUndefined();
     871
     872    if (!DOMWindow::canShowModalDialogNow(frame) || !DOMWindow::allowPopUp(dynamicFrame))
     873        return jsUndefined();
    874874
    875875    HashMap<String, String> features;

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

    r48542 r48619  
    205205    ASSERT(frame);
    206206
    207     if (!shouldAllowNavigation(exec, frame))
    208         return;
    209 
    210207    KURL url = completeURL(exec, value.toString(exec));
    211208    if (url.isNull())
     209        return;
     210
     211    if (!shouldAllowNavigation(exec, frame))
    212212        return;
    213213
    … …  
    309309        return jsUndefined();
    310310
    311     if (!shouldAllowNavigation(exec, frame))
    312         return jsUndefined();
    313 
    314311    KURL url = completeURL(exec, args.at(0).toString(exec));
    315312    if (url.isNull())
     313        return jsUndefined();
     314
     315    if (!shouldAllowNavigation(exec, frame))
    316316        return jsUndefined();
    317317
    … …  
    337337        return jsUndefined();
    338338
    339     if (!shouldAllowNavigation(exec, frame))
    340         return jsUndefined();
    341 
    342339    KURL url = completeURL(exec, args.at(0).toString(exec));
    343340    if (url.isNull())
    344341        return jsUndefined();
    345342
     343    if (!shouldAllowNavigation(exec, frame))
     344        return jsUndefined();
     345
    346346    // We want a new history item if this JS was called via a user gesture
    347347    navigateIfAllowed(exec, frame, url, !frame->script()->anyPageIsProcessingUserGesture(), false);

  • trunk/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp

    r48455 r48619  
    6666        return v8::Undefined();
    6767
    68     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
    69 
    70     if (!V8Proxy::canAccessFrame(imp->frame(), true))
    71         return v8::Undefined();
    72 
    73     ScriptExecutionContext* scriptContext = static_cast<ScriptExecutionContext*>(imp->document());
    74 
    75     if (!scriptContext)
    76         return v8::Undefined();
    77 
    7868    v8::Handle<v8::Value> function = args[0];
     69
     70    WebCore::String functionString;
     71    if (!function->IsFunction())
     72        functionString = function->IsString() ?
     73            toWebCoreString(function) : toWebCoreString(function->ToString());
     74
     75        // Bail out if string conversion failed.
     76        if (functionString.IsEmpty())
     77            return v8::Undefined();
     78
     79        // Don't allow setting timeouts to run empty functions!
     80        // (Bug 1009597)
     81        if (functionString.length() == 0)
     82            return v8::Undefined();
     83    }
    7984
    8085    int32_t timeout = 0;
    8186    if (argumentCount >= 2)
    8287        timeout = args[1]->Int32Value();
     88
     89    DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
     90
     91    if (!V8Proxy::canAccessFrame(imp->frame(), true))
     92        return v8::Undefined();
     93
     94    ScriptExecutionContext* scriptContext = static_cast<ScriptExecutionContext*>(imp->document());
     95
     96    if (!scriptContext)
     97        return v8::Undefined();
    8398
    8499    int id;
    … …  
    100115        id = DOMTimer::install(scriptContext, action, timeout, singleShot);
    101116    } else {
    102         if (!function->IsString()) {
    103             function = function->ToString();
    104             // Bail out if string conversion failed.
    105             if (function.IsEmpty())
    106                 return v8::Undefined();
    107         }
    108 
    109         WebCore::String functionString = toWebCoreString(function);
    110 
    111         // Don't allow setting timeouts to run empty functions!
    112         // (Bug 1009597)
    113         if (functionString.length() == 0)
    114             return v8::Undefined();
    115 
    116117        id = DOMTimer::install(scriptContext, new ScheduledAction(V8Proxy::context(imp->frame()), functionString), timeout, singleShot);
    117118    }
    … …  
    228229{
    229230    INC_STATS("DOM.DOMWindow.addEventListener()");
     231
     232    String eventType = toWebCoreString(args[0]);
     233    bool useCapture = args[2]->BooleanValue();
     234
    230235    DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
    231236
    … …  
    245250    RefPtr<EventListener> listener = proxy->eventListeners()->findOrCreateWrapper<V8EventListener>(proxy->frame(), args[1], false);
    246251
    247     if (listener) {
    248         String eventType = toWebCoreString(args[0]);
    249         bool useCapture = args[2]->BooleanValue();
     252    if (listener)
    250253        imp->addEventListener(eventType, listener, useCapture);
    251     }
    252254
    253255    return v8::Undefined();
    … …  
    258260{
    259261    INC_STATS("DOM.DOMWindow.removeEventListener()");
     262
     263    String eventType = toWebCoreString(args[0]);
     264    bool useCapture = args[2]->BooleanValue();
     265
    260266    DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
    261267
    … …  
    274280    RefPtr<EventListener> listener = proxy->eventListeners()->findWrapper(args[1], false);
    275281
    276     if (listener) {
    277         String eventType = toWebCoreString(args[0]);
    278         bool useCapture = args[2]->BooleanValue();
     282    if (listener)
    279283        imp->removeEventListener(eventType, listener.get(), useCapture);
    280     }
    281284
    282285    return v8::Undefined();
    … …  
    319322{
    320323    INC_STATS("DOM.DOMWindow.atob()");
     324
     325    if (args[0]->IsNull())
     326        return v8String("");
     327    String str = toWebCoreString(args[0]);
     328
    321329    DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
    322330
    … …  
    327335        return throwError("Not enough arguments", V8Proxy::SyntaxError);
    328336
     337    return convertBase64(str, false);
     338}
     339
     340CALLBACK_FUNC_DECL(DOMWindowBtoa)
     341{
     342    INC_STATS("DOM.DOMWindow.btoa()");
     343
    329344    if (args[0]->IsNull())
    330345        return v8String("");
    331 
    332346    String str = toWebCoreString(args[0]);
    333     return convertBase64(str, false);
    334 }
    335 
    336 CALLBACK_FUNC_DECL(DOMWindowBtoa)
    337 {
    338     INC_STATS("DOM.DOMWindow.btoa()");
     347
    339348    DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
    340349
    … …  
    345354        return throwError("Not enough arguments", V8Proxy::SyntaxError);
    346355
    347     if (args[0]->IsNull())
    348         return v8String("");
    349 
    350     String str = toWebCoreString(args[0]);
    351356    return convertBase64(str, true);
    352357}
    … …  
    564569{
    565570    INC_STATS("DOM.DOMWindow.showModalDialog()");
     571
     572    String url = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
     573    v8::Local<v8::Value> dialogArgs = args[1];
     574    String featureArgs = toWebCoreStringWithNullOrUndefinedCheck(args[2]);
     575
    566576    DOMWindow* window = V8DOMWrapper::convertToNativeObject<DOMWindow>(
    567577        V8ClassIndex::DOMWINDOW, args.Holder());
    … …  
    581591    if (!canShowModalDialogNow(frame) || !allowPopUp())
    582592        return v8::Undefined();
    583 
    584     String url = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
    585     v8::Local<v8::Value> dialogArgs = args[1];
    586     String featureArgs = toWebCoreStringWithNullOrUndefinedCheck(args[2]);
    587593
    588594    const HashMap<String, String> features = parseModalDialogFeatures(featureArgs);
    … …  
    653659{
    654660    INC_STATS("DOM.DOMWindow.open()");
     661
     662    String urlString = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
     663    AtomicString frameName = (args[1]->IsUndefined() || args[1]->IsNull()) ? "_blank" : AtomicString(toWebCoreString(args[1]));
     664
    655665    DOMWindow* parent = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
    656666    Frame* frame = parent->frame();
    … …  
    670680    if (!page)
    671681        return v8::Undefined();
    672 
    673     String urlString = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
    674     AtomicString frameName = (args[1]->IsUndefined() || args[1]->IsNull()) ? "_blank" : AtomicString(toWebCoreString(args[1]));
    675682
    676683    // Because FrameTree::find() returns true for empty strings, we must check
    … …  
    849856        return;
    850857
    851     if (!shouldAllowNavigation(frame))
    852         return;
    853 
    854858    KURL url = completeURL(relativeURL);
    855859    if (url.isNull())
    856860        return;
    857861
     862    if (!shouldAllowNavigation(frame))
     863        return;
     864
    858865    navigateIfAllowed(frame, url, false, false);
    859866}
    … …  
    876883void V8Custom::ClearTimeoutImpl(const v8::Arguments& args)
    877884{
     885    int handle = toInt32(args[0]);
     886
    878887    v8::Handle<v8::Object> holder = args.Holder();
    879888    DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, holder);
    … …  
    883892    if (!context)
    884893        return;
    885     int handle = toInt32(args[0]);
    886894    DOMTimer::removeById(context, handle);
    887895}

  • trunk/WebCore/bindings/v8/custom/V8LocationCustom.cpp

    r46993 r48619  
    129129        return;
    130130
    131     if (!shouldAllowNavigation(frame))
    132         return;
    133 
    134131    KURL url = completeURL(toWebCoreString(value));
    135132    if (url.isNull())
     133        return;
     134
     135    if (!shouldAllowNavigation(frame))
    136136        return;
    137137
    … …  
    289289        return v8::Undefined();
    290290
    291     if (!shouldAllowNavigation(frame))
    292         return v8::Undefined();
    293 
    294291    KURL url = completeURL(toWebCoreString(args[0]));
    295292    if (url.isNull())
    296293        return v8::Undefined();
    297294
     295    if (!shouldAllowNavigation(frame))
     296        return v8::Undefined();
     297
    298298    navigateIfAllowed(frame, url, true, true);
    299299    return v8::Undefined();
    … …  
    310310        return v8::Undefined();
    311311
    312     if (!shouldAllowNavigation(frame))
    313         return v8::Undefined();
    314 
    315312    KURL url = completeURL(toWebCoreString(args[0]));
    316313    if (url.isNull())
     314        return v8::Undefined();
     315
     316    if (!shouldAllowNavigation(frame))
    317317        return v8::Undefined();
    318318
Note: See TracChangeset for help on using the changeset viewer.