2009-09-21 Adam Barth <abarth@webkit.org>
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Sep 2009 05:08:37 +0000 (05:08 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Sep 2009 05:08:37 +0000 (05:08 +0000)
        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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@48619 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/bindings/js/JSDOMWindowCustom.cpp
WebCore/bindings/js/JSLocationCustom.cpp
WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
WebCore/bindings/v8/custom/V8LocationCustom.cpp

index 457fb4a461e09551a53331cdeb82de14835a7b02..5c4b58f95501e5ae79e59437e22c345b9b902371 100644 (file)
@@ -1,3 +1,35 @@
+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):
+
 2009-09-21  Dumitru Daniliuc  <dumi@chromium.org>
 
         Reviewed by Eric Seidel.
index 7fe1dc551334ec93ad5e39b050ad4c03e97657ff..7217fc8249920a9b73101a55c8017fcd6c508ef7 100644 (file)
@@ -580,13 +580,13 @@ void JSDOMWindow::setLocation(ExecState* exec, JSValue value)
     Frame* frame = impl()->frame();
     ASSERT(frame);
 
-    if (!shouldAllowNavigation(exec, frame))
-        return;
-
     KURL url = completeURL(exec, value.toString(exec));
     if (url.isNull())
         return;
 
+    if (!shouldAllowNavigation(exec, frame))
+        return;
+
     if (!protocolIsJavaScript(url) || allowsAccessFrom(exec)) {
         // We want a new history item if this JS was called via a user gesture
         frame->loader()->scheduleLocationChange(url, lexicalFrame->loader()->outgoingReferrer(), !lexicalFrame->script()->anyPageIsProcessingUserGesture(), false, processingUserGesture(exec));
@@ -781,6 +781,10 @@ static Frame* createWindow(ExecState* exec, Frame* lexicalFrame, Frame* dynamicF
 
 JSValue JSDOMWindow::open(ExecState* exec, const ArgList& args)
 {
+    String urlString = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
+    AtomicString frameName = args.at(1).isUndefinedOrNull() ? "_blank" : AtomicString(args.at(1).toString(exec));
+    WindowFeatures windowFeatures(valueToStringWithUndefinedOrNullCheck(exec, args.at(2)));
+
     Frame* frame = impl()->frame();
     if (!frame)
         return jsUndefined();
@@ -793,9 +797,6 @@ JSValue JSDOMWindow::open(ExecState* exec, const ArgList& args)
 
     Page* page = frame->page();
 
-    String urlString = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
-    AtomicString frameName = args.at(1).isUndefinedOrNull() ? "_blank" : AtomicString(args.at(1).toString(exec));
-
     // Because FrameTree::find() returns true for empty strings, we must check for empty framenames.
     // Otherwise, illegitimate window.open() calls with no name will pass right through the popup blocker.
     if (!DOMWindow::allowPopUp(dynamicFrame) && (frameName.isEmpty() || !frame->tree()->find(frameName)))
@@ -813,13 +814,13 @@ JSValue JSDOMWindow::open(ExecState* exec, const ArgList& args)
         topOrParent = true;
     }
     if (topOrParent) {
-        if (!shouldAllowNavigation(exec, frame))
-            return jsUndefined();
-
         String completedURL;
         if (!urlString.isEmpty())
             completedURL = completeURL(exec, urlString).string();
 
+        if (!shouldAllowNavigation(exec, frame))
+            return jsUndefined();
+
         const JSDOMWindow* targetedWindow = toJSDOMWindow(frame);
         if (!completedURL.isEmpty() && (!protocolIsJavaScript(completedURL) || (targetedWindow && targetedWindow->allowsAccessFrom(exec)))) {
             bool userGesture = processingUserGesture(exec);
@@ -835,7 +836,6 @@ JSValue JSDOMWindow::open(ExecState* exec, const ArgList& args)
     }
 
     // In the case of a named frame or a new window, we'll use the createWindow() helper
-    WindowFeatures windowFeatures(valueToStringWithUndefinedOrNullCheck(exec, args.at(2)));
     FloatRect windowRect(windowFeatures.xSet ? windowFeatures.x : 0, windowFeatures.ySet ? windowFeatures.y : 0,
                          windowFeatures.widthSet ? windowFeatures.width : 0, windowFeatures.heightSet ? windowFeatures.height : 0);
     DOMWindow::adjustWindowRect(screenAvailableRect(page ? page->mainFrame()->view() : 0), windowRect, windowRect);
@@ -855,6 +855,10 @@ JSValue JSDOMWindow::open(ExecState* exec, const ArgList& args)
 
 JSValue JSDOMWindow::showModalDialog(ExecState* exec, const ArgList& args)
 {
+    String url = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
+    JSValue dialogArgs = args.at(1);
+    String featureArgs = valueToStringWithUndefinedOrNullCheck(exec, args.at(2));
+
     Frame* frame = impl()->frame();
     if (!frame)
         return jsUndefined();
@@ -868,10 +872,6 @@ JSValue JSDOMWindow::showModalDialog(ExecState* exec, const ArgList& args)
     if (!DOMWindow::canShowModalDialogNow(frame) || !DOMWindow::allowPopUp(dynamicFrame))
         return jsUndefined();
 
-    String url = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
-    JSValue dialogArgs = args.at(1);
-    String featureArgs = valueToStringWithUndefinedOrNullCheck(exec, args.at(2));
-
     HashMap<String, String> features;
     DOMWindow::parseModalDialogFeatures(featureArgs, features);
 
index 9cd942c6f4d7459ffb834c70aae5c2a842d99978..aecec5ea56d6f6cba51e7b338c90fd0cab027824 100644 (file)
@@ -204,13 +204,13 @@ void JSLocation::setHref(ExecState* exec, JSValue value)
     Frame* frame = impl()->frame();
     ASSERT(frame);
 
-    if (!shouldAllowNavigation(exec, frame))
-        return;
-
     KURL url = completeURL(exec, value.toString(exec));
     if (url.isNull())
         return;
 
+    if (!shouldAllowNavigation(exec, frame))
+        return;
+
     navigateIfAllowed(exec, frame, url, !frame->script()->anyPageIsProcessingUserGesture(), false);
 }
 
@@ -308,13 +308,13 @@ JSValue JSLocation::replace(ExecState* exec, const ArgList& args)
     if (!frame)
         return jsUndefined();
 
-    if (!shouldAllowNavigation(exec, frame))
-        return jsUndefined();
-
     KURL url = completeURL(exec, args.at(0).toString(exec));
     if (url.isNull())
         return jsUndefined();
 
+    if (!shouldAllowNavigation(exec, frame))
+        return jsUndefined();
+
     navigateIfAllowed(exec, frame, url, true, true);
     return jsUndefined();
 }
@@ -336,13 +336,13 @@ JSValue JSLocation::assign(ExecState* exec, const ArgList& args)
     if (!frame)
         return jsUndefined();
 
-    if (!shouldAllowNavigation(exec, frame))
-        return jsUndefined();
-
     KURL url = completeURL(exec, args.at(0).toString(exec));
     if (url.isNull())
         return jsUndefined();
 
+    if (!shouldAllowNavigation(exec, frame))
+        return jsUndefined();
+
     // We want a new history item if this JS was called via a user gesture
     navigateIfAllowed(exec, frame, url, !frame->script()->anyPageIsProcessingUserGesture(), false);
     return jsUndefined();
index 8a92e0aa703702b8e91a5bb4bbc9d69d86651b14..a4a7f9a1f732f25debe6d4d690084d18f6b10c14 100644 (file)
@@ -65,6 +65,27 @@ v8::Handle<v8::Value> V8Custom::WindowSetTimeoutImpl(const v8::Arguments& args,
     if (argumentCount < 1)
         return v8::Undefined();
 
+    v8::Handle<v8::Value> function = args[0];
+
+    WebCore::String functionString;
+    if (!function->IsFunction())
+        functionString = function->IsString() ? 
+            toWebCoreString(function) : toWebCoreString(function->ToString());
+
+        // Bail out if string conversion failed.
+        if (functionString.IsEmpty())
+            return v8::Undefined();
+
+        // Don't allow setting timeouts to run empty functions!
+        // (Bug 1009597)
+        if (functionString.length() == 0)
+            return v8::Undefined();
+    }
+
+    int32_t timeout = 0;
+    if (argumentCount >= 2)
+        timeout = args[1]->Int32Value();
+
     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
 
     if (!V8Proxy::canAccessFrame(imp->frame(), true))
@@ -75,12 +96,6 @@ v8::Handle<v8::Value> V8Custom::WindowSetTimeoutImpl(const v8::Arguments& args,
     if (!scriptContext)
         return v8::Undefined();
 
-    v8::Handle<v8::Value> function = args[0];
-
-    int32_t timeout = 0;
-    if (argumentCount >= 2)
-        timeout = args[1]->Int32Value();
-
     int id;
     if (function->IsFunction()) {
         int paramCount = argumentCount >= 2 ? argumentCount - 2 : 0;
@@ -99,20 +114,6 @@ v8::Handle<v8::Value> V8Custom::WindowSetTimeoutImpl(const v8::Arguments& args,
 
         id = DOMTimer::install(scriptContext, action, timeout, singleShot);
     } else {
-        if (!function->IsString()) {
-            function = function->ToString();
-            // Bail out if string conversion failed.
-            if (function.IsEmpty())
-                return v8::Undefined();
-        }
-
-        WebCore::String functionString = toWebCoreString(function);
-
-        // Don't allow setting timeouts to run empty functions!
-        // (Bug 1009597)
-        if (functionString.length() == 0)
-            return v8::Undefined();
-
         id = DOMTimer::install(scriptContext, new ScheduledAction(V8Proxy::context(imp->frame()), functionString), timeout, singleShot);
     }
 
@@ -227,6 +228,10 @@ ACCESSOR_SETTER(DOMWindowOpener)
 CALLBACK_FUNC_DECL(DOMWindowAddEventListener)
 {
     INC_STATS("DOM.DOMWindow.addEventListener()");
+
+    String eventType = toWebCoreString(args[0]);
+    bool useCapture = args[2]->BooleanValue();
+
     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
 
     if (!V8Proxy::canAccessFrame(imp->frame(), true))
@@ -244,11 +249,8 @@ CALLBACK_FUNC_DECL(DOMWindowAddEventListener)
 
     RefPtr<EventListener> listener = proxy->eventListeners()->findOrCreateWrapper<V8EventListener>(proxy->frame(), args[1], false);
 
-    if (listener) {
-        String eventType = toWebCoreString(args[0]);
-        bool useCapture = args[2]->BooleanValue();
+    if (listener)
         imp->addEventListener(eventType, listener, useCapture);
-    }
 
     return v8::Undefined();
 }
@@ -257,6 +259,10 @@ CALLBACK_FUNC_DECL(DOMWindowAddEventListener)
 CALLBACK_FUNC_DECL(DOMWindowRemoveEventListener)
 {
     INC_STATS("DOM.DOMWindow.removeEventListener()");
+
+    String eventType = toWebCoreString(args[0]);
+    bool useCapture = args[2]->BooleanValue();
+
     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
 
     if (!V8Proxy::canAccessFrame(imp->frame(), true))
@@ -273,11 +279,8 @@ CALLBACK_FUNC_DECL(DOMWindowRemoveEventListener)
 
     RefPtr<EventListener> listener = proxy->eventListeners()->findWrapper(args[1], false);
 
-    if (listener) {
-        String eventType = toWebCoreString(args[0]);
-        bool useCapture = args[2]->BooleanValue();
+    if (listener)
         imp->removeEventListener(eventType, listener.get(), useCapture);
-    }
 
     return v8::Undefined();
 }
@@ -318,6 +321,11 @@ CALLBACK_FUNC_DECL(DOMWindowPostMessage)
 CALLBACK_FUNC_DECL(DOMWindowAtob)
 {
     INC_STATS("DOM.DOMWindow.atob()");
+
+    if (args[0]->IsNull())
+        return v8String("");
+    String str = toWebCoreString(args[0]);
+
     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
 
     if (!V8Proxy::canAccessFrame(imp->frame(), true))
@@ -326,16 +334,17 @@ CALLBACK_FUNC_DECL(DOMWindowAtob)
     if (args.Length() < 1)
         return throwError("Not enough arguments", V8Proxy::SyntaxError);
 
-    if (args[0]->IsNull())
-        return v8String("");
-
-    String str = toWebCoreString(args[0]);
     return convertBase64(str, false);
 }
 
 CALLBACK_FUNC_DECL(DOMWindowBtoa)
 {
     INC_STATS("DOM.DOMWindow.btoa()");
+
+    if (args[0]->IsNull())
+        return v8String("");
+    String str = toWebCoreString(args[0]);
+
     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
 
     if (!V8Proxy::canAccessFrame(imp->frame(), true))
@@ -344,10 +353,6 @@ CALLBACK_FUNC_DECL(DOMWindowBtoa)
     if (args.Length() < 1)
         return throwError("Not enough arguments", V8Proxy::SyntaxError);
 
-    if (args[0]->IsNull())
-        return v8String("");
-
-    String str = toWebCoreString(args[0]);
     return convertBase64(str, true);
 }
 
@@ -563,6 +568,11 @@ static Frame* createWindow(Frame* callingFrame,
 CALLBACK_FUNC_DECL(DOMWindowShowModalDialog)
 {
     INC_STATS("DOM.DOMWindow.showModalDialog()");
+
+    String url = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
+    v8::Local<v8::Value> dialogArgs = args[1];
+    String featureArgs = toWebCoreStringWithNullOrUndefinedCheck(args[2]);
+
     DOMWindow* window = V8DOMWrapper::convertToNativeObject<DOMWindow>(
         V8ClassIndex::DOMWINDOW, args.Holder());
     Frame* frame = window->frame();
@@ -581,10 +591,6 @@ CALLBACK_FUNC_DECL(DOMWindowShowModalDialog)
     if (!canShowModalDialogNow(frame) || !allowPopUp())
         return v8::Undefined();
 
-    String url = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
-    v8::Local<v8::Value> dialogArgs = args[1];
-    String featureArgs = toWebCoreStringWithNullOrUndefinedCheck(args[2]);
-
     const HashMap<String, String> features = parseModalDialogFeatures(featureArgs);
 
     const bool trusted = false;
@@ -652,6 +658,10 @@ CALLBACK_FUNC_DECL(DOMWindowShowModalDialog)
 CALLBACK_FUNC_DECL(DOMWindowOpen)
 {
     INC_STATS("DOM.DOMWindow.open()");
+
+    String urlString = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
+    AtomicString frameName = (args[1]->IsUndefined() || args[1]->IsNull()) ? "_blank" : AtomicString(toWebCoreString(args[1]));
+
     DOMWindow* parent = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, args.Holder());
     Frame* frame = parent->frame();
 
@@ -670,9 +680,6 @@ CALLBACK_FUNC_DECL(DOMWindowOpen)
     if (!page)
         return v8::Undefined();
 
-    String urlString = toWebCoreStringWithNullOrUndefinedCheck(args[0]);
-    AtomicString frameName = (args[1]->IsUndefined() || args[1]->IsNull()) ? "_blank" : AtomicString(toWebCoreString(args[1]));
-
     // Because FrameTree::find() returns true for empty strings, we must check
     // for empty framenames. Otherwise, illegitimate window.open() calls with
     // no name will pass right through the popup blocker.
@@ -848,13 +855,13 @@ void V8Custom::WindowSetLocation(DOMWindow* window, const String& relativeURL)
     if (!frame)
         return;
 
-    if (!shouldAllowNavigation(frame))
-        return;
-
     KURL url = completeURL(relativeURL);
     if (url.isNull())
         return;
 
+    if (!shouldAllowNavigation(frame))
+        return;
+
     navigateIfAllowed(frame, url, false, false);
 }
 
@@ -875,6 +882,8 @@ CALLBACK_FUNC_DECL(DOMWindowSetInterval)
 
 void V8Custom::ClearTimeoutImpl(const v8::Arguments& args)
 {
+    int handle = toInt32(args[0]);
+
     v8::Handle<v8::Object> holder = args.Holder();
     DOMWindow* imp = V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, holder);
     if (!V8Proxy::canAccessFrame(imp->frame(), true))
@@ -882,7 +891,6 @@ void V8Custom::ClearTimeoutImpl(const v8::Arguments& args)
     ScriptExecutionContext* context = static_cast<ScriptExecutionContext*>(imp->document());
     if (!context)
         return;
-    int handle = toInt32(args[0]);
     DOMTimer::removeById(context, handle);
 }
 
index 3f3ff6b910f8e598699f17aac7219f98dad7cc35..17a5e7ce0f2c33ee063923f753d3db347643ec61 100644 (file)
@@ -128,13 +128,13 @@ ACCESSOR_SETTER(LocationHref)
     if (!frame)
         return;
 
-    if (!shouldAllowNavigation(frame))
-        return;
-
     KURL url = completeURL(toWebCoreString(value));
     if (url.isNull())
         return;
 
+    if (!shouldAllowNavigation(frame))
+        return;
+
     navigateIfAllowed(frame, url, false, false);
 }
 
@@ -288,13 +288,13 @@ CALLBACK_FUNC_DECL(LocationReplace)
     if (!frame)
         return v8::Undefined();
 
-    if (!shouldAllowNavigation(frame))
-        return v8::Undefined();
-
     KURL url = completeURL(toWebCoreString(args[0]));
     if (url.isNull())
         return v8::Undefined();
 
+    if (!shouldAllowNavigation(frame))
+        return v8::Undefined();
+
     navigateIfAllowed(frame, url, true, true);
     return v8::Undefined();
 }
@@ -309,13 +309,13 @@ CALLBACK_FUNC_DECL(LocationAssign)
     if (!frame)
         return v8::Undefined();
 
-    if (!shouldAllowNavigation(frame))
-        return v8::Undefined();
-
     KURL url = completeURL(toWebCoreString(args[0]));
     if (url.isNull())
         return v8::Undefined();
 
+    if (!shouldAllowNavigation(frame))
+        return v8::Undefined();
+
     navigateIfAllowed(frame, url, false, false);
     return v8::Undefined();
 }