New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment

Null out DOMWindow::m_frame as soon as the frame/window is detached

Project Member Reported by dcheng@chromium.org, Aug 27 2015 Back to list

Issue description

Currently, DOMWindow::m_frame is not cleared until the Frame it refers to is destroyed. This results in some subtleties: often times, code will have to call DOMWindow::isCurrentlyDisplayedInFrame() to make sure it's actually the active Window in the Frame before trying to do something.

Unfortunately, it's very easy to forget to do this. Forgetting to do this can lead to security bugs. In the past month, there have been two UXSS reported that were both the result of using DOMWindow::frame() when the DOMWindow was already detached: see  issue 522791  and  issue 524074 

I believe we should experiment with figuring out a way to null out this pointer when the DOMWindow is detached. This would have prevented both of these bugs. I believe abarth@ made an attempt at this many years ago in 
https://bugs.webkit.org/show_bug.cgi?id=62054

The problem is that doing this the naive way (by just nulling out the pointer when Frame::setDOMWindow is called) results in visible changes to the web. Given the HTML:

<iframe name="abc"/>

Without the naive way:
var ifr = document.querySelector('iframe');
var win = ifr.contentWindow;
ifr.parentNode.removeChild(ifr);
console.log(win.name);   // Prints "abc"

With the naive way:
var ifr = document.querySelector('iframe');
var win = ifr.contentWindow;
ifr.parentNode.removeChild(ifr);
console.log(win.name);   // Prints nothing.
 
Labels: Security_Severity-High Security_Impact-Stable
(Being a good little sheriff and assigning severity ratings for the security queue based on the two other bugs caused by the issue. dcheng, wanna be owner until you find someone else to take charge? :-P)
Project Member

Comment 2 by ClusterFuzz, Aug 27 2015

Labels: -Pri-2 Pri-1

Comment 3 by dcheng@chromium.org, Aug 27 2015

Owner: dcheng@chromium.org
Status: Assigned
Project Member

Comment 4 by ClusterFuzz, Sep 11 2015

Labels: Nag
dcheng@: Uh oh! This issue is still open and hasn't been updated in the last 14 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 5 by ClusterFuzz, Sep 25 2015

dcheng@: Uh oh! This issue is still open and hasn't been updated in the last 28 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz

Comment 6 by dcheng@chromium.org, Sep 28 2015

Status: Started

Comment 7 by jochen@chromium.org, Sep 28 2015

Cc: verwa...@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 30 2015

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

commit a55a83b15a84eb408cd5805c4fb4dcba17d6054a
Author: dcheng <dcheng@chromium.org>
Date: Wed Sep 30 05:04:51 2015

Null out LocalDOMWindow::frame() on navigation.

In Blink, LocalDOMWindow::frame() is only set to null when the
LocalFrame is destroyed. Multiple LocalDOMWindow objects can hold a
reference to the same LocalFrame.

It turns out that this is dangerous and a persistent source of XSS bugs.
Code that creates scriptable objects for a frame needs to remember to
call DOMWindow::isCurrentlyDisplayedInFrame() to verify that the
creating context is still active in the frame. If this check is left
out, the created object can often trigger XSS.

Instead of depending on developers to remember to add this check where
needed, Blink now nulls out LocalDOMWindow::frame() as soon as it
navigates away from a LocalDOMWindow. Code in Blink already handles the
null case, since this is already something that can happen. Code that
improperly handles this case will tend to crash (suboptimal but safe),
and in general, failures won't result in XSS, since a detached frame
cannot be reattached.

BUG= 525330 

Review URL: https://codereview.chromium.org/1374533002

Cr-Commit-Position: refs/heads/master@{#351496}

[add] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-in-closure-after-navigation-expected.txt
[add] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/dom/Window/property-access-in-closure-after-navigation.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/dom/Window/resources/dom-access-from-closure-iframe-child.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/dom/Window/resources/dom-access-from-closure-window-child.html
[add] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/dom/Window/resources/property-access-in-closure-after-navigation-child.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/dom/resources/javascript-url-crash-function-iframe.html
[rename] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/events/crash-on-querying-event-path-expected.txt
[rename] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/events/crash-on-querying-event-path.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/fast/parser/resources/set-parent-to-javascript-url.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/http/tests/security/frameNavigation/resources/popup-ready-to-navigate-child.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/LayoutTests/http/tests/security/resources/xss-eval2.html
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] http://crrev.com/a55a83b15a84eb408cd5805c4fb4dcba17d6054a/third_party/WebKit/Source/core/frame/LocalDOMWindow.h

Comment 9 by dcheng@chromium.org, Sep 30 2015

Status: Fixed
Project Member

Comment 10 by ClusterFuzz, Sep 30 2015

Labels: -Restrict-View-SecurityTeam Merge-Triage M-46 Restrict-View-SecurityNotify M-45
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

Your fix is very close to the branch point. After the branch happens, please make sure to check if your fix is in.

- Your friendly ClusterFuzz
Cc: timwillis@chromium.org
Labels: -M-45
@dcheng - Will this play nicely if pushed in a patch release to M-46 stable or should we let this roll in with M-47? Based on your description in #8, seems more like a stability concern rather than a security one, but please let me know what your preference is for M46.  
For this patch, it's probably safer to let it roll in M47.
Labels: -Merge-Triage Release-0-M47 Merge-NA
Project Member

Comment 14 by ClusterFuzz, Jan 6 2016

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 15 by sheriffbot@chromium.org, Oct 1 2016

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
Project Member

Comment 16 by sheriffbot@chromium.org, Oct 2 2016

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
Labels: allpublic

Sign in to add a comment