<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[196628] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/196628">196628</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2016-02-16 00:11:47 -0800 (Tue, 16 Feb 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestshttptestssecuritycrossframeaccessputexpectedtxt">trunk/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorebindingsjsJSDOMWindowCustomcpp">trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestshttptestssecuritycrossoriginreifiedwindowlocationsettingexpectedtxt">trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt</a></li>
<li><a href="#trunkLayoutTestshttptestssecuritycrossoriginreifiedwindowlocationsettinghtml">trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (196627 => 196628)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2016-02-16 07:14:15 UTC (rev 196627)
+++ trunk/LayoutTests/ChangeLog 2016-02-16 08:11:47 UTC (rev 196628)
</span><span class="lines">@@ -1,3 +1,24 @@
</span><ins>+2016-02-16 Chris Dumez <cdumez@apple.com>
+
+ 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.
+
</ins><span class="cx"> 2016-02-15 Mark Lam <mark.lam@apple.com>
</span><span class="cx">
</span><span class="cx"> [ARMv7] Some JSC test fails due to exhausting the JIT code heap on the no LLINT test configuration.
</span></span></pre></div>
<a id="trunkLayoutTestshttptestssecuritycrossframeaccessputexpectedtxt"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/http/tests/security/cross-frame-access-put-expected.txt (196627 => 196628)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -271,13 +271,16 @@
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><ins>+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.
</ins><span class="cx"> 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.
</span><ins>+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.
</ins><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><ins>+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.
</ins><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="lines">@@ -317,6 +320,8 @@
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span><ins>+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.
</ins><span class="cx"> 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.
</span><span class="cx"> 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.
</span><span class="cx"> 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.
</span></span></pre></div>
<a id="trunkLayoutTestshttptestssecuritycrossoriginreifiedwindowlocationsettingexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting-expected.txt (0 => 196628)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,11 @@
</span><ins>+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
+
</ins></span></pre></div>
<a id="trunkLayoutTestshttptestssecuritycrossoriginreifiedwindowlocationsettinghtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/http/tests/security/cross-origin-reified-window-location-setting.html (0 => 196628)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,25 @@
</span><ins>+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body onload="runTest()">
+<iframe id="crossOriginFrame" src="http://localhost:8000/security/resources/reify-window.html"></iframe>
+<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="../../resources/js-test-post.js"></script>
+</html>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (196627 => 196628)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,33 @@
</span><ins>+2016-02-16 Chris Dumez <cdumez@apple.com>
+
+ 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):
+
</ins><span class="cx"> 2016-02-15 Brent Fulgham <bfulgham@apple.com>
</span><span class="cx">
</span><span class="cx"> [Mac] Gather some rudimentary statistics during resource load
</span></span></pre></div>
<a id="trunkSourceWebCorebindingsjsJSDOMWindowCustomcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp (196627 => 196628)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -347,14 +347,23 @@
</span><span class="cx">
</span><span class="cx"> void JSDOMWindow::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSValue value, PutPropertySlot& slot)
</span><span class="cx"> {
</span><del>- JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(cell);
</del><ins>+ auto* thisObject = jsCast<JSDOMWindow*>(cell);
</ins><span class="cx"> if (!thisObject->wrapped().frame())
</span><span class="cx"> return;
</span><span class="cx">
</span><ins>+ 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;
+ }
+
</ins><span class="cx"> // Optimization: access JavaScript global variables directly before involving the DOM.
</span><span class="cx"> if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, propertyName)) {
</span><del>- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- JSGlobalObject::put(thisObject, exec, propertyName, value, slot);
</del><ins>+ JSGlobalObject::put(thisObject, exec, propertyName, value, slot);
</ins><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -363,27 +372,22 @@
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- Base::put(thisObject, exec, propertyName, value, slot);
</del><ins>+ Base::put(thisObject, exec, propertyName, value, slot);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void JSDOMWindow::putByIndex(JSCell* cell, ExecState* exec, unsigned index, JSValue value, bool shouldThrow)
</span><span class="cx"> {
</span><del>- JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(cell);
- if (!thisObject->wrapped().frame())
</del><ins>+ auto* thisObject = jsCast<JSDOMWindow*>(cell);
+ if (!thisObject->wrapped().frame() || !BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
</ins><span class="cx"> return;
</span><span class="cx">
</span><del>- Identifier propertyName = Identifier::from(exec, index);
-
</del><span class="cx"> // Optimization: access JavaScript global variables directly before involving the DOM.
</span><del>- if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, propertyName)) {
- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- JSGlobalObject::putByIndex(thisObject, exec, index, value, shouldThrow);
</del><ins>+ if (thisObject->JSGlobalObject::hasOwnPropertyForWrite(exec, Identifier::from(exec, index))) {
+ JSGlobalObject::putByIndex(thisObject, exec, index, value, shouldThrow);
</ins><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- if (BindingSecurity::shouldAllowAccessToDOMWindow(exec, thisObject->wrapped()))
- Base::putByIndex(thisObject, exec, index, value, shouldThrow);
</del><ins>+ Base::putByIndex(thisObject, exec, index, value, shouldThrow);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> bool JSDOMWindow::deleteProperty(JSCell* cell, ExecState* exec, PropertyName propertyName)
</span></span></pre>
</div>
</div>
</body>
</html>