[Bindings] Cross-origin checks happen too late for overloaded methods
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Dec 2019 19:05:04 +0000 (19:05 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Dec 2019 19:05:04 +0000 (19:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205092

Reviewed by Sam Weinig.

LayoutTests/imported/w3c:

Rebaseline WPT test now that it is failing a bit later.

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

Source/WebCore:

Cross-origin checks happen too late for overloaded methods. We're supposed to do the security check
and then find the right overload to call [1]. In our bindings, we first find the right overload body
to call and then do the security check in the body of the chosen overload. This results in the wrong
exception being thrown in some cases (TypeError due to missing arguments instead of a SecurityError).

[1] https://heycam.github.io/webidl/#dfn-create-operation-function

No new tests, updated existing tests.

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateOperationBodyDefinition):
(GenerateOperationDefinition):

LayoutTests:

Extend layout test coverage.

* http/tests/security/cross-origin-window-property-access-expected.txt:
* http/tests/security/cross-origin-window-property-access.html:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/security/cross-origin-window-property-access-expected.txt
LayoutTests/http/tests/security/cross-origin-window-property-access.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
Source/WebCore/bindings/scripts/test/JS/JSTestActiveDOMObject.cpp
Source/WebCore/bindings/scripts/test/TestDomainSecurity.idl

index eee588b..f433675 100644 (file)
@@ -1,3 +1,15 @@
+2019-12-11  Chris Dumez  <cdumez@apple.com>
+
+        [Bindings] Cross-origin checks happen too late for overloaded methods
+        https://bugs.webkit.org/show_bug.cgi?id=205092
+
+        Reviewed by Sam Weinig.
+
+        Extend layout test coverage.
+
+        * http/tests/security/cross-origin-window-property-access-expected.txt:
+        * http/tests/security/cross-origin-window-property-access.html:
+
 2019-12-11  youenn fablet  <youenn@apple.com>
 
         (r252889) webrtc/peerconnection-page-cache.html is crashing on iOS Debug
index df7234d..dbd3b78 100644 (file)
@@ -9,6 +9,8 @@ PASS Object.getOwnPropertyDescriptor(window, "menubar").get.call(crossOriginWind
 PASS Object.getOwnPropertyDescriptor(window, "scrollbars").get.call(crossOriginWindow) threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
 PASS Object.getOwnPropertyDescriptor(window, "navigator").get.call(crossOriginWindow) threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
 PASS Object.getOwnPropertyDescriptor(window, "screenX").get.call(crossOriginWindow) threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
+PASS Calling createImageBitmap rejected promise with exception SecurityError.
+PASS Calling fetch rejected promise with exception SecurityError.
 PASS Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call(crossOriginWindow) threw exception TypeError: undefined is not an object (evaluating 'Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call').
 PASS Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call(crossOriginWindow.__proto__) threw exception TypeError: undefined is not an object (evaluating 'Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call').
 PASS crossOriginWindow.constructor threw exception SecurityError: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a cross-origin frame. Protocols, domains, and ports must match..
index f7893c1..57df377 100644 (file)
@@ -9,33 +9,33 @@
 description("Tests that using another window's property getter does not bypass cross-origin checks.");
 jsTestIsAsync = true;
 
-function shouldThrowOrReturnUndefined(expression)
+function callingFunctionShouldRejectPromiseWithErrorName(functionName, errorName)
 {
-    try {
-        result = eval(expression);
-    } catch (e) {
-        testPassed(expression + " threw exception " + e + ".");
-        return;
-    }
-    if (result === undefined)
-        testPassed(expression + " returned undefined.");
-    else
-        testFailed(expression + " returned " + result);
+    return Object.getOwnPropertyDescriptor(window, functionName).value.call(crossOriginWindow).then(() => {
+        testFailed("Calling " + functionName + " did not reject the promise");
+    }, (e) => {
+        if (e.name == errorName)
+           testPassed("Calling " + functionName + " rejected promise with exception " + errorName + ".");
+       else
+           testFailed("Calling " + functionName + " should reject promise with exception " + errorName + ". Rejected with exception " + e.name + " instead.");
+    });
 }
 
-function runTest()
+async function runTest()
 {
     crossOriginWindow = frames[0];
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window, "document").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window, "name").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window, "menubar").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window, "scrollbars").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window, "navigator").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window, "screenX").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call(crossOriginWindow)');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call(crossOriginWindow.__proto__)');
-    shouldThrowOrReturnUndefined('crossOriginWindow.constructor');
-    shouldThrowOrReturnUndefined('Object.getOwnPropertyDescriptor(crossOriginWindow.__proto__, "constructor").value');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window, "document").get.call(crossOriginWindow)', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window, "name").get.call(crossOriginWindow)', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window, "menubar").get.call(crossOriginWindow)', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window, "scrollbars").get.call(crossOriginWindow)', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window, "navigator").get.call(crossOriginWindow)', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window, "screenX").get.call(crossOriginWindow)', 'SecurityError');
+    await callingFunctionShouldRejectPromiseWithErrorName('createImageBitmap', 'SecurityError');
+    await callingFunctionShouldRejectPromiseWithErrorName('fetch', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call(crossOriginWindow)', 'TypeError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(window.__proto__, "constructor").get.call(crossOriginWindow.__proto__)', 'TypeError');
+    shouldThrowErrorName('crossOriginWindow.constructor', 'SecurityError');
+    shouldThrowErrorName('Object.getOwnPropertyDescriptor(crossOriginWindow.__proto__, "constructor").value', 'SecurityError');
     shouldBeTrue('Object.getOwnPropertyDescriptor(window, "location").get.call(crossOriginWindow) === crossOriginWindow.location');
     finishJSTest();
 }
index 47fd5e1..e4c3efa 100644 (file)
@@ -1,3 +1,14 @@
+2019-12-11  Chris Dumez  <cdumez@apple.com>
+
+        [Bindings] Cross-origin checks happen too late for overloaded methods
+        https://bugs.webkit.org/show_bug.cgi?id=205092
+
+        Reviewed by Sam Weinig.
+
+        Rebaseline WPT test now that it is failing a bit later.
+
+        * web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects-expected.txt:
+
 2019-12-10  Chris Dumez  <cdumez@apple.com>
 
         Re-sync imported/w3c/web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html from usptream
index 43a9b47..18dab6f 100644 (file)
@@ -21,8 +21,12 @@ FAIL Only whitelisted properties are accessible cross-origin (cross-origin) asse
 FAIL Only whitelisted properties are accessible cross-origin (same-origin + document.domain) assert_throws: Should throw when writing to toString on Location function "function () { win.location[prop] = undefined; }" did not throw
 FAIL Only whitelisted properties are accessible cross-origin (cross-site) assert_throws: Should throw when writing to toString on Location function "function () { win.location[prop] = undefined; }" did not throw
 FAIL Only whitelisted properties are usable as cross-origin this objects (cross-origin) promise_test: Unhandled rejection with value: object "SyntaxError: The string did not match the expected pattern."
-FAIL Only whitelisted properties are usable as cross-origin this objects (same-origin + document.domain) assert_throws: Should throw when calling window.createImageBitmap with cross-origin this object function "function () { throw e }" threw object "TypeError: Not enough arguments" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
-FAIL Only whitelisted properties are usable as cross-origin this objects (cross-site) assert_throws: Should throw when calling window.createImageBitmap with cross-origin this object function "function () { throw e }" threw object "TypeError: Not enough arguments" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
+FAIL Only whitelisted properties are usable as cross-origin this objects (same-origin + document.domain) assert_throws: Should throw when calling window.AudioContext with cross-origin this object function "function webkitAudioContext() {
+    [native code]
+}" threw object "TypeError: Constructor requires 'new' operator" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
+FAIL Only whitelisted properties are usable as cross-origin this objects (cross-site) assert_throws: Should throw when calling window.AudioContext with cross-origin this object function "function webkitAudioContext() {
+    [native code]
+}" threw object "TypeError: Constructor requires 'new' operator" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
 PASS [[GetPrototypeOf]] should return null (cross-origin) 
 PASS [[GetPrototypeOf]] should return null (same-origin + document.domain) 
 PASS [[GetPrototypeOf]] should return null (cross-site) 
index 05c9d43..01aee48 100644 (file)
@@ -1,3 +1,23 @@
+2019-12-11  Chris Dumez  <cdumez@apple.com>
+
+        [Bindings] Cross-origin checks happen too late for overloaded methods
+        https://bugs.webkit.org/show_bug.cgi?id=205092
+
+        Reviewed by Sam Weinig.
+
+        Cross-origin checks happen too late for overloaded methods. We're supposed to do the security check
+        and then find the right overload to call [1]. In our bindings, we first find the right overload body
+        to call and then do the security check in the body of the chosen overload. This results in the wrong
+        exception being thrown in some cases (TypeError due to missing arguments instead of a SecurityError).
+
+        [1] https://heycam.github.io/webidl/#dfn-create-operation-function
+
+        No new tests, updated existing tests.
+
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateOperationBodyDefinition):
+        (GenerateOperationDefinition):
+
 2019-12-11  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Make incremental builds faster after modifying DisplayListItems.h
index 2fc466e..cf20465 100644 (file)
@@ -5222,7 +5222,7 @@ sub GenerateOperationTrampolineDefinition
 
 sub GenerateOperationBodyDefinition
 {
-    my ($outputArray, $interface, $className, $operation, $functionName, $functionImplementationName, $functionBodyName, $generatingOverloadDispatcher) = @_;
+    my ($outputArray, $interface, $className, $operation, $functionName, $functionImplementationName, $functionBodyName, $isOverloaded, $generatingOverloadDispatcher) = @_;
 
     my $hasPromiseReturnType = $codeGenerator->IsPromiseType($operation->type);
     my $idlOperationType = $hasPromiseReturnType ? "IDLOperationReturningPromise" : "IDLOperation";
@@ -5241,9 +5241,11 @@ sub GenerateOperationBodyDefinition
     push(@$outputArray, "    UNUSED_PARAM(callFrame);\n");
     push(@$outputArray, "    UNUSED_PARAM(throwScope);\n");
 
-    if (!$generatingOverloadDispatcher) {
-        GenerateCustomElementReactionsStackIfNeeded($outputArray, $operation, "*lexicalGlobalObject");
+    GenerateCustomElementReactionsStackIfNeeded($outputArray, $operation, "*lexicalGlobalObject") unless $generatingOverloadDispatcher;
 
+    # For overloads, we generate the security check in the overload dispatcher, instead of the body of each overload, as per specification:
+    # https://heycam.github.io/webidl/#dfn-create-operation-function
+    if (!$isOverloaded || $generatingOverloadDispatcher) {
         if ($interface->extendedAttributes->{CheckSecurity} and !$operation->extendedAttributes->{DoNotCheckSecurity}) {
             assert("Security checks are not supported for static operations.") if $operation->isStatic;
             
@@ -5335,7 +5337,7 @@ sub GenerateOperationDefinition
     my $functionImplementationName = $operation->extendedAttributes->{ImplementedAs} || $codeGenerator->WK_lcfirst($operation->name);
     my $functionBodyName = ($isOverloaded ? $functionName . $operation->{overloadIndex} : $functionName) . "Body";
 
-    GenerateOperationBodyDefinition($outputArray, $interface, $className, $operation, $functionName, $functionImplementationName, $functionBodyName);
+    GenerateOperationBodyDefinition($outputArray, $interface, $className, $operation, $functionName, $functionImplementationName, $functionBodyName, $isOverloaded);
 
     # Overloaded operations don't generate a trampoline for each overload, and instead have a single dispatch trampoline
     # that gets generated after the last overload body has been generated.
@@ -5352,7 +5354,7 @@ sub GenerateOperationDefinition
         push(@$outputArray, "#if ${overloadsConditionalString}\n\n") if $overloadsConditionalString;
 
         my $overloadDispatcherFunctionBodyName = $functionName . "OverloadDispatcher";
-        GenerateOperationBodyDefinition($outputArray, $interface, $className, $operation, $functionName, $functionImplementationName, $overloadDispatcherFunctionBodyName, 1);
+        GenerateOperationBodyDefinition($outputArray, $interface, $className, $operation, $functionName, $functionImplementationName, $overloadDispatcherFunctionBodyName, $isOverloaded, 1);
         GenerateOperationTrampolineDefinition($outputArray, $interface, $className, $operation, $functionName, $overloadDispatcherFunctionBodyName);
     
         push(@$outputArray, "#endif\n\n") if $overloadsConditionalString;
index da53ff9..8cf1456 100644 (file)
@@ -22,6 +22,7 @@
 #include "JSTestActiveDOMObject.h"
 
 #include "ActiveDOMObject.h"
+#include "CustomElementReactionQueue.h"
 #include "JSDOMAttribute.h"
 #include "JSDOMBinding.h"
 #include "JSDOMBindingSecurity.h"
@@ -49,6 +50,7 @@ using namespace JSC;
 
 JSC::EncodedJSValue JSC_HOST_CALL jsTestActiveDOMObjectPrototypeFunctionExcitingFunction(JSC::JSGlobalObject*, JSC::CallFrame*);
 JSC::EncodedJSValue JSC_HOST_CALL jsTestActiveDOMObjectPrototypeFunctionPostMessage(JSC::JSGlobalObject*, JSC::CallFrame*);
+JSC::EncodedJSValue JSC_HOST_CALL jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod(JSC::JSGlobalObject*, JSC::CallFrame*);
 
 // Attributes
 
@@ -119,6 +121,7 @@ static const HashTableValue JSTestActiveDOMObjectPrototypeTableValues[] =
     { "constructor", static_cast<unsigned>(JSC::PropertyAttribute::DontEnum), NoIntrinsic, { (intptr_t)static_cast<PropertySlot::GetValueFunc>(jsTestActiveDOMObjectConstructor), (intptr_t) static_cast<PutPropertySlot::PutValueFunc>(setJSTestActiveDOMObjectConstructor) } },
     { "excitingFunction", static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { (intptr_t)static_cast<RawNativeFunction>(jsTestActiveDOMObjectPrototypeFunctionExcitingFunction), (intptr_t) (1) } },
     { "postMessage", static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { (intptr_t)static_cast<RawNativeFunction>(jsTestActiveDOMObjectPrototypeFunctionPostMessage), (intptr_t) (1) } },
+    { "overloadedMethod", static_cast<unsigned>(JSC::PropertyAttribute::Function), NoIntrinsic, { (intptr_t)static_cast<RawNativeFunction>(jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod), (intptr_t) (1) } },
 };
 
 const ClassInfo JSTestActiveDOMObjectPrototype::s_info = { "TestActiveDOMObjectPrototype", &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSTestActiveDOMObjectPrototype) };
@@ -255,6 +258,57 @@ EncodedJSValue JSC_HOST_CALL jsTestActiveDOMObjectPrototypeFunctionPostMessage(J
     return IDLOperation<JSTestActiveDOMObject>::call<jsTestActiveDOMObjectPrototypeFunctionPostMessageBody>(*lexicalGlobalObject, *callFrame, "postMessage");
 }
 
+static inline JSC::EncodedJSValue jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod1Body(JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame, typename IDLOperation<JSTestActiveDOMObject>::ClassParameter castedThis, JSC::ThrowScope& throwScope)
+{
+    UNUSED_PARAM(lexicalGlobalObject);
+    UNUSED_PARAM(callFrame);
+    UNUSED_PARAM(throwScope);
+    CustomElementReactionStack customElementReactionStack(*lexicalGlobalObject);
+    auto& impl = castedThis->wrapped();
+    auto param = convert<IDLDOMString>(*lexicalGlobalObject, callFrame->uncheckedArgument(0));
+    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
+    impl.overloadedMethod(WTFMove(param));
+    return JSValue::encode(jsUndefined());
+}
+
+static inline JSC::EncodedJSValue jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod2Body(JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame, typename IDLOperation<JSTestActiveDOMObject>::ClassParameter castedThis, JSC::ThrowScope& throwScope)
+{
+    UNUSED_PARAM(lexicalGlobalObject);
+    UNUSED_PARAM(callFrame);
+    UNUSED_PARAM(throwScope);
+    auto& impl = castedThis->wrapped();
+    auto param1 = convert<IDLInterface<Node>>(*lexicalGlobalObject, callFrame->uncheckedArgument(0), [](JSC::JSGlobalObject& lexicalGlobalObject, JSC::ThrowScope& scope) { throwArgumentTypeError(lexicalGlobalObject, scope, 0, "param1", "TestActiveDOMObject", "overloadedMethod", "Node"); });
+    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
+    auto param2 = convert<IDLInterface<Node>>(*lexicalGlobalObject, callFrame->uncheckedArgument(1), [](JSC::JSGlobalObject& lexicalGlobalObject, JSC::ThrowScope& scope) { throwArgumentTypeError(lexicalGlobalObject, scope, 1, "param2", "TestActiveDOMObject", "overloadedMethod", "Node"); });
+    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());
+    impl.overloadedMethod(*param1, *param2);
+    return JSValue::encode(jsUndefined());
+}
+
+static inline JSC::EncodedJSValue jsTestActiveDOMObjectPrototypeFunctionOverloadedMethodOverloadDispatcher(JSC::JSGlobalObject* lexicalGlobalObject, JSC::CallFrame* callFrame, typename IDLOperation<JSTestActiveDOMObject>::ClassParameter castedThis, JSC::ThrowScope& throwScope)
+{
+    UNUSED_PARAM(lexicalGlobalObject);
+    UNUSED_PARAM(callFrame);
+    UNUSED_PARAM(throwScope);
+    if (!BindingSecurity::shouldAllowAccessToDOMWindow(lexicalGlobalObject, castedThis->wrapped().window(), ThrowSecurityError))
+        return JSValue::encode(jsUndefined());
+    VM& vm = JSC::getVM(lexicalGlobalObject);
+    UNUSED_PARAM(vm);
+    size_t argsCount = std::min<size_t>(2, callFrame->argumentCount());
+    if (argsCount == 1) {
+        return jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod1Body(lexicalGlobalObject, callFrame, castedThis, throwScope);
+    }
+    if (argsCount == 2) {
+        return jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod2Body(lexicalGlobalObject, callFrame, castedThis, throwScope);
+    }
+    return argsCount < 1 ? throwVMError(lexicalGlobalObject, throwScope, createNotEnoughArgumentsError(lexicalGlobalObject)) : throwVMTypeError(lexicalGlobalObject, throwScope);
+}
+
+EncodedJSValue JSC_HOST_CALL jsTestActiveDOMObjectPrototypeFunctionOverloadedMethod(JSGlobalObject* lexicalGlobalObject, CallFrame* callFrame)
+{
+    return IDLOperation<JSTestActiveDOMObject>::call<jsTestActiveDOMObjectPrototypeFunctionOverloadedMethodOverloadDispatcher>(*lexicalGlobalObject, *callFrame, "overloadedMethod");
+}
+
 void JSTestActiveDOMObject::analyzeHeap(JSCell* cell, HeapAnalyzer& analyzer)
 {
     auto* thisObject = jsCast<JSTestActiveDOMObject*>(cell);
index 8af6c19..9fc24b1 100644 (file)
@@ -32,4 +32,7 @@
     readonly attribute long excitingAttr;
     void excitingFunction(Node nextChild);
     [DoNotCheckSecurity] void postMessage(DOMString message);
+
+    [CEReactions] void overloadedMethod(DOMString param);
+    void overloadedMethod(Node param1, Node param2);
 };