New issue
Advanced search Search tips

Issue 694111 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Restructure binding security checks to be based on Window rather than Frame

Project Member Reported by dcheng@chromium.org, Feb 20 2017

Issue description

(Restricting view for now, since this is partially motivated by issue 693695)

Currently, binding security checks are all based around the concept of frame. However, this is problematic as when a DOMWindow is detached, the connection back to the frame is broken. This makes it impossible to perform security checks, so we must fail the access--but if we throw an exception, it incorrectly causes a bunch of same-origin detached accesses to also fail. See https://codereview.chromium.org/2705783004/ for an example.

Instead, we should change canAccessFrame() to canAccessWindow(). The biggest obstacle to doing this is the Location object, which is currently associated with Frame. However, the Location member of DOMWindow is reset on detach / when a new DOMWindow is installed to prevent it from using a stale frame... rather than doing that, we should just be associating it with the Window object itself. This is what the spec actually requires anyway.

From https://whatwg.org/C/browsers.html#the-location-interface:
Each Window object is associated with a unique instance of a Location object, allocated when the Window object is created.

http://ithildin.com/tests/window/different-location.html is a quick test to verify this. Edge, Firefox, and Chrome all fail this test: Firefox and Chrome don't keep the same Location object when the Window object is reused, while Edge always uses the same Location object.

Once Location is associated with Window, then we can update the security checks to be based on Window, and we can remove the exceptions for detached frames.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86ff27ab575772808e4aa02430c571f770d556be

commit 86ff27ab575772808e4aa02430c571f770d556be
Author: dcheng <dcheng@chromium.org>
Date: Tue Feb 21 11:25:56 2017

Simplify conditionals for blink::Location::isActive().

On navigation or detach, DOMWindow's back pointer to Frame is cleared,
so it's only necessary to check that the back pointer to Frame is
non-null.

BUG= 694111 

Review-Url: https://codereview.chromium.org/2704303002
Cr-Commit-Position: refs/heads/master@{#451732}

[modify] https://crrev.com/86ff27ab575772808e4aa02430c571f770d556be/third_party/WebKit/Source/core/frame/Location.cpp
[modify] https://crrev.com/86ff27ab575772808e4aa02430c571f770d556be/third_party/WebKit/Source/core/frame/Location.h

Project Member

Comment 3 by bugdroid1@chromium.org, May 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d357b6a46d70097aa28428b1f106284f7fd9990

commit 1d357b6a46d70097aa28428b1f106284f7fd9990
Author: dcheng <dcheng@chromium.org>
Date: Wed May 17 10:46:21 2017

Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. This is because Location had a pointer
to the Frame, not the DOMWindow. However, this means that checks that
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames, since a detached Window/Location has
a nulled out Frame pointer.

Now that Location is associated with a DOMWindow, the security checks
should use DOMWindow. This allows the access checks to distinguish
same origin and cross origin access on a detached window, correctly
throwing an exception on detached cross-origin access. This also
matches Firefox behavior.

BUG= 694111 

Review-Url: https://codereview.chromium.org/2706923002
Cr-Commit-Position: refs/heads/master@{#472412}

[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-navigated.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed.html
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/fast/dom/Window/resources/window-property-collector.js
[add] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/http/tests/security/detached-window-cross-origin-access-expected.txt
[add] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/LayoutTests/http/tests/security/detached-window-cross-origin-access.html
[delete] https://crrev.com/9e3c1f13fb959c98bdb6aa91aa510e51e635eac7/third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt
[delete] https://crrev.com/9e3c1f13fb959c98bdb6aa91aa510e51e635eac7/third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced-expected.txt
[delete] https://crrev.com/9e3c1f13fb959c98bdb6aa91aa510e51e635eac7/third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-expected.txt
[delete] https://crrev.com/9e3c1f13fb959c98bdb6aa91aa510e51e635eac7/third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/bindings/templates/interface_base.cpp.tmpl
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
[modify] https://crrev.com/1d357b6a46d70097aa28428b1f106284f7fd9990/third_party/WebKit/Source/core/frame/Location.h

Comment 4 by dcheng@chromium.org, Dec 26 2017

Status: Fixed (was: Started)
Project Member

Comment 5 by sheriffbot@chromium.org, Dec 26 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 6 by sheriffbot@chromium.org, Apr 3 2018

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment