Deny cross-origin access to 'window.history'.

Deny cross-origin access to 'window.history'.

Currently, we allow cross-origin access to certain methods on
'window.history'. This is contrary to both the spec, and the existing
behavior of both Gecko and IE[1]. This patch drops the custom
security checks on the history object, denying cross-origin access
to the object entirely.

[1]: https://bug839867.bugzilla.mozilla.org/attachment.cgi?id=712247

Original patch from Adam Barth <abarth@chromium.org>; on
http://webkit.org/b/106641.

BUG=237080
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149846

[Mike West, 2013-05-01 04:08:21]

Hi Adam and Eric. I've ported over Adam's stab at a patch to deny cross-origin
access to 'window.history' from https://bugs.webkit.org/show_bug.cgi?id=106641.
I think the IDL changes are straightforward, but I'd really like some feedback
on the tests.

It seems like we're more or less covered by the change to
LayoutTests/http/tests/security/cross-frame-access-get.html, and I think most of
the other tests are superfluous. That said, I'd appreciate you taking a close
look to see if there are edge cases we'd be missing by throwing away the
cross-origin access tests.

Additionally, Adam suggested on the original bug that we might want to measure
usage of cross-origin history access before making this change. Given the spec,
Gecko, and IE are all denying access, it seems that any applications relying on
this behavior will already require workarounds. Anecdotally, the only
cross-origin history access I've seen in the wild is malicious
(http://homakov.blogspot.de/2013/03/pwning-your-privacy-in-all-browsers.html for
example). I'd suggest that measurement isn't necessary, given this background,
but if there's any doubt in your minds, I'm happy to drop a line to blink-dev
and go through the deprecation process formally.

WDYT?

-mike

[eseidel, 2013-05-01 04:17:23]

Seems reasonable to me.

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/fast/frames/sa...
File LayoutTests/fast/frames/sandboxed-iframe-history-denied-expected.txt
(right):

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/fast/frames/sa...
LayoutTests/fast/frames/sandboxed-iframe-history-denied-expected.txt:1: CONSOLE
MESSAGE: Sandbox access violation: Blocked a frame at "null" from accessing a
frame at "".  The frame requesting access is sandboxed and lacks the
"allow-same-origin" flag.
Seems like a rather lacking message. :)

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/http/tests/sec...
File
LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt
(right):

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/http/tests/sec...
LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt:355:
*** FAIL: canGetDescriptor(targetWindow, 'length') should be 'false' but instead
is true. ***
Are these expected?

[Use mkwst_at_chromium.org plz., 2013-05-01 04:45:18]

Thanks, Eric.

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/fast/frames/sa...
File LayoutTests/fast/frames/sandboxed-iframe-history-denied-expected.txt
(right):

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/fast/frames/sa...
LayoutTests/fast/frames/sandboxed-iframe-history-denied-expected.txt:1: CONSOLE
MESSAGE: Sandbox access violation: Blocked a frame at "null" from accessing a
frame at "".  The frame requesting access is sandboxed and lacks the
"allow-same-origin" flag.
On 2013/05/01 04:17:23, Eric Seidel (Google) wrote:
> Seems like a rather lacking message. :)

Yeah. The message isn't wrong, but it's certainly not helpful. I need to update
this to handle 'data:' URLs more gracefully. Filed crbug.com/237085

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/http/tests/sec...
File
LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt
(right):

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/http/tests/sec...
LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt:355:
*** FAIL: canGetDescriptor(targetWindow, 'length') should be 'false' but instead
is true. ***
On 2013/05/01 04:17:23, Eric Seidel (Google) wrote:
> Are these expected?

The history bits are my oversight; I'll drop that section of the test, as it's
no longer relevant.

The other buts (cross-domain location and 'closed'/'frames' above) are failing
on ToT, and apparently have been for some time:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

[Use mkwst_at_chromium.org plz., 2013-05-01 04:50:23]

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/http/tests/sec...
File
LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt
(right):

https://codereview.chromium.org/14198015/diff/7001/LayoutTests/http/tests/sec...
LayoutTests/http/tests/security/cross-frame-access-getOwnPropertyDescriptor-expected.txt:355:
*** FAIL: canGetDescriptor(targetWindow, 'length') should be 'false' but instead
is true. ***
On 2013/05/01 04:45:19, mkwst wrote:
> On 2013/05/01 04:17:23, Eric Seidel (Google) wrote:
> > Are these expected?
> 
> The history bits are my oversight; I'll drop that section of the test, as it's
> no longer relevant.
> 
> The other buts (cross-domain location and 'closed'/'frames' above) are failing
> on ToT, and apparently have been for some time:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

Actually, I'm wrong: all of these failures are currently present on ToT. I'll
drop this rebaseline from the patch and skip the test in a separate patch: filed
http://crbug.com/237086 to cover the test.

[abarth-chromium, 2013-05-06 16:49:58]

LGTM

There's likely some more code you can remove.  For example, the V8History clause
in findFrame in V8Initializer.cpp.

[abarth-chromium, 2013-05-06 16:51:00]

https://codereview.chromium.org/14198015/diff/7005/Source/core/page/History.idl
File Source/core/page/History.idl (left):

https://codereview.chromium.org/14198015/diff/7005/Source/core/page/History.i...
Source/core/page/History.idl:30: CustomEnumerateProperty,
I would have expected that we'd need to delete these custom bindings, but I
couldn't find them anywhere.

[Mike West, 2013-05-06 17:59:53]

On 2013/05/06 16:49:58, abarth wrote:
> LGTM
> 
> There's likely some more code you can remove.  For example, the V8History
clause
> in findFrame in V8Initializer.cpp.

I'll look at that tomorrow morning and either add it to this patch or create
another.

Thanks for the review!

[abarth-chromium, 2013-05-06 19:43:49]

Might as well try to land this CL and iterate in a followup CL.

[commit-bot: I haz the power, 2013-05-06 19:44:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/14198015/7005

[commit-bot: I haz the power, 2013-05-06 20:23:57]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-07 03:38:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/14198015/25001

[commit-bot: I haz the power, 2013-05-07 04:03:34]

Change committed as 149846