- Revision
- 196628
- Author
- [email protected]
- Date
- 2016-02-16 00:11:47 -0800 (Tue, 16 Feb 2016)
Log Message
Do security checks early in JSDOMWindow::put*()
https://bugs.webkit.org/show_bug.cgi?id=154270
Reviewed by Gavin Barraclough.
Source/WebCore:
Do security checks early in JSDOMWindow::put() / JSDOMWindow::putByIndex()
and return as soon as possible. This makes it less error-prone as we need
to do the security check only once, at the top of the function.
Also lock down the security further by calling lookupPut() only if the
property name is "location". The "location" property is the only one that
can be set cross-origin. Previously, trying to set a property such as
"name" (which cannot be set cross-origin) relied on the attribute setter
doing the security check when getting called. The new check is less error
prone and will correctly prevent overriding window's method cross-origin
once these move down from the prototype (Bug 154120).
Finally, the previous code was failing to set the "location" property
cross-origin after the window has been reified. This patch fixes the
issue by always calling the original "location" property setter from the
static table in the cross-origin case.
Test: http/tests/security/cross-origin-reified-window-location-setting.html
* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::put):
(WebCore::JSDOMWindow::putByIndex):
LayoutTests:
* http/tests/security/cross-frame-access-put-expected.txt:
Rebaseline. The extra security warnings are for the following properties:
closed, crypto, frameElement, pageXOffset and pageYOffset.
All these properties are read-only and therefore cannot be set (cross-origin
or not). The previous code was not doing an explicit check and ended up
trying to set these properties. However, since they are read-only, we would
silently fail to set them. The new code does the explicit check and therefore
will warn and NOT attempt to set.
* http/tests/security/cross-origin-reified-window-location-setting-expected.txt: Added.
* http/tests/security/cross-origin-reified-window-location-setting.html: Added.
Add test to check that setting window.location cross-origin still works after the
window object has been reified.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (196627 => 196628)
--- trunk/LayoutTests/ChangeLog 2016-02-16 07:14:15 UTC (rev 196627)
+++ trunk/LayoutTests/ChangeLog 2016-02-16 08:11:47 UTC (rev 196628)
@@ -1,3 +1,24 @@
+2016-02-16 Chris Dumez <[email protected]>
+
+ Do security checks early in JSDOMWindow::put*()
+ https://bugs.webkit.org/show_bug.cgi?id=154270
+
+ Reviewed by Gavin Barraclough.
+
+ * http/tests/security/cross-frame-access-put-expected.txt:
+ Rebaseline. The extra security warnings are for the following properties:
+ closed, crypto, frameElement, pageXOffset and pageYOffset.
+ All these properties are read-only and therefore cannot be set (cross-origin
+ or not). The previous code was not doing an explicit check and ended up
+ trying to set these properties. However, since they are read-only, we would
+ silently fail to set them. The new code does the explicit check and therefore
+ will warn and NOT attempt to set.
+
+ * http/tests/security/cross-origin-reified-window-location-setting-expected.txt: Added.
+ * http/tests/security/cross-origin-reified-window-location-setting.html: Added.
+ Add test to check that setting window.location cross-origin still works after the
+ window object has been reified.
+
2016-02-15 Mark Lam <[email protected]>
[ARMv7] Some JSC test fails due to exhausting the JIT code heap on the no LLINT test configuration.
Modified: trunk/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt (196627 => 196628)
--- trunk/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt 2016-02-16 07:14:15 UTC (rev 196627)
+++ trunk/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt 2016-02-16 08:11:47 UTC (rev 196628)
@@ -271,13 +271,16 @@
CONSOLE MESSAGE: line 121: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 122: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 131: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
+CONSOLE MESSAGE: line 132: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 133: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
+CONSOLE MESSAGE: line 134: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 135: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 136: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 137: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 138: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 139: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 140: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
+CONSOLE MESSAGE: line 141: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 142: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 143: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 144: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
@@ -317,6 +320,8 @@
CONSOLE MESSAGE: line 178: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 179: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 180: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
+CONSOLE MESSAGE: line 181: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
+CONSOLE MESSAGE: line 182: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 183: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 184: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
CONSOLE MESSAGE: line 185: Blocked a frame with origin "http://127.0.0.1:8000" from accessing a frame with origin "http://localhost:8000". Protocols, domains, and ports must match.
Added: trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt (0 => 196628)
--- trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt 2016-02-16 08:11:47 UTC (rev 196628)
@@ -0,0 +1,11 @@
+Tests that window.location can be set cross-origin even if the window object is reified.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS crossOriginWindow.location = 'about:blank' did not throw exception.
+PASS crossOriginWindow.location.href is "about:blank"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html (0 => 196628)
--- trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html (rev 0)
+++ trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html 2016-02-16 08:11:47 UTC (rev 196628)
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+</head>
+<body _onload_="runTest()">
+<iframe id="crossOriginFrame" src=""
+<script>
+description("Tests that window.location can be set cross-origin even if the window object is reified.");
+jsTestIsAsync = true;
+
+function runTest()
+{
+ crossOriginWindow = crossOriginFrame.window;
+ shouldNotThrow("crossOriginWindow.location = 'about:blank'");
+
+ setTimeout(function() {
+ shouldBeEqualToString("crossOriginWindow.location.href", "about:blank");
+ finishJSTest();
+ }, 0);
+}
+</script>
+</body>
+<script src=""
+</html>
Modified: trunk/Source/WebCore/ChangeLog (196627 => 196628)
--- trunk/Source/WebCore/ChangeLog 2016-02-16 07:14:15 UTC (rev 196627)
+++ trunk/Source/WebCore/ChangeLog 2016-02-16 08:11:47 UTC (rev 196628)
@@ -1,3 +1,33 @@
+2016-02-16 Chris Dumez <[email protected]>
+
+ Do security checks early in JSDOMWindow::put*()
+ https://bugs.webkit.org/show_bug.cgi?id=154270
+
+ Reviewed by Gavin Barraclough.
+
+ Do security checks early in JSDOMWindow::put() / JSDOMWindow::putByIndex()
+ and return as soon as possible. This makes it less error-prone as we need
+ to do the security check only once, at the top of the function.
+
+ Also lock down the security further by calling lookupPut() only if the
+ property name is "location". The "location" property is the only one that
+ can be set cross-origin. Previously, trying to set a property such as
+ "name" (which cannot be set cross-origin) relied on the attribute setter
+ doing the security check when getting called. The new check is less error
+ prone and will correctly prevent overriding window's method cross-origin
+ once these move down from the prototype (Bug 154120).
+
+ Finally, the previous code was failing to set the "location" property
+ cross-origin after the window has been reified. This patch fixes the
+ issue by always calling the original "location" property setter from the
+ static table in the cross-origin case.
+
+ Test: http/tests/security/cross-origin-reified-window-location-setting.html
+
+ * bindings/js/JSDOMWindowCustom.cpp:
+ (WebCore::JSDOMWindow::put):
+ (WebCore::JSDOMWindow::putByIndex):
+
2016-02-15 Brent Fulgham <[email protected]>
[Mac] Gather some rudimentary statistics during resource load
Modified: trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp (196627 => 196628)
--- trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp 2016-02-16 07:14:15 UTC (rev 196627)
+++ trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp 2016-02-16 08:11:47 UTC (rev 196628)
@@ -347,14 +347,23 @@
void JSDOMWindow::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
{
- JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(cell);
+ auto* thisObject = jsCast<JSDOMWindow*>(cell);
if (!thisObject->wrapped().frame())
return;
+ String errorMessage;
+ if (!shouldAllowAccessToDOMWindow(exec, thisObject->wrapped(), errorMessage)) {
+ // We only allow setting "location" attribute cross-origin.
+ if (propertyName == exec->propertyNames().location)
+ lookupPut(exec, propertyName, thisObject, value, *s_info.staticPropHashTable, slot);
+ else
+ thisObject->printErrorMessage(errorMessage);
+ return;
+ }
+
// Optimization: access _javascript_ global variables directly before involving the DOM.
if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, propertyName)) {
- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- JSGlobalObject::put(thisObject, exec, propertyName, value, slot);
+ JSGlobalObject::put(thisObject, exec, propertyName, value, slot);
return;
}
@@ -363,27 +372,22 @@
return;
}
- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- Base::put(thisObject, exec, propertyName, value, slot);
+ Base::put(thisObject, exec, propertyName, value, slot);
}
void JSDOMWindow::putByIndex(JSCell* cell, ExecState* exec, unsigned index, JSValue value, bool shouldThrow)
{
- JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(cell);
- if (!thisObject->wrapped().frame())
+ auto* thisObject = jsCast<JSDOMWindow*>(cell);
+ if (!thisObject->wrapped().frame() || !BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
return;
- Identifier propertyName = Identifier::from(exec, index);
-
// Optimization: access _javascript_ global variables directly before involving the DOM.
- if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, propertyName)) {
- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- JSGlobalObject::putByIndex(thisObject, exec, index, value, shouldThrow);
+ if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, Identifier::from(exec, index))) {
+ JSGlobalObject::putByIndex(thisObject, exec, index, value, shouldThrow);
return;
}
- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- Base::putByIndex(thisObject, exec, index, value, shouldThrow);
+ Base::putByIndex(thisObject, exec, index, value, shouldThrow);
}
bool JSDOMWindow::deleteProperty(JSCell* cell, ExecState* exec, PropertyName propertyName)
