It seems like there should be a UXSS by getting the parent page to touch window.external and then doing:

(new window.external.__proto__.constructor.__proto__.constructor("alert(window.location.href)"))()

but something seems to be making that return undefined. Is there an origin check in "new Function" somehow?

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

commit f2026d03c0b50a54ca5d4845edb64f65a49deff7
Author: dcheng <dcheng@chromium.org>
Date: Thu Feb 02 08:26:34 2017

Make External per-window rather than a static Persistent.

Sharing wrappers between contexts is unsafe.

BUG= 687844 

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

[modify] https://crrev.com/f2026d03c0b50a54ca5d4845edb64f65a49deff7/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] https://crrev.com/f2026d03c0b50a54ca5d4845edb64f65a49deff7/third_party/WebKit/Source/core/frame/DOMWindow.h

If we have a test that uses window.external(), the memory leak should have been caught by the leak detector (I think).

Will introducing LocalDOMWindow::m_external solve the problem?

Yes, that's what the patch does. Originally I wanted to be clever and save memory for a rarely used attribute, but the resulting behavior is clearly wrong.

Didn't notice you've already fixed it :)

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

We could outlaw ScriptWrappable singletons via a static_assert() in DEFINE_STATIC_LOCAL().

Was the issue discovered by code inspection?

I believe this was discovered via code inspection.

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

commit dd371fbefb97e0cb80079b34a7ec69afe759bd29
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Feb 03 19:48:19 2017

Make External per-window rather than a static Persistent.

Sharing wrappers between contexts is unsafe.

BUG= 687844 

Review-Url: https://codereview.chromium.org/2671673002
Cr-Commit-Position: refs/heads/master@{#447724}
(cherry picked from commit f2026d03c0b50a54ca5d4845edb64f65a49deff7)

Review-Url: https://codereview.chromium.org/2670763009 .
Cr-Commit-Position: refs/branch-heads/2987@{#296}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/dd371fbefb97e0cb80079b34a7ec69afe759bd29/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] https://crrev.com/dd371fbefb97e0cb80079b34a7ec69afe759bd29/third_party/WebKit/Source/core/frame/DOMWindow.h

The InputDeviceCapabilities singletons could run into same, i think -- https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/InputDeviceCapabilities.cpp?l=19

Yeah those are wrong too.

Filed  issue 688569  to followup on the others, to make merges easier to track.

+awhalley for evaluating the severity of this.

Since we're already launched to Stable on M56 this may be too late, is this a recent regression or has window.external always persisted across navigations?

Since this was internally reported I'm ok picking it up in the next stable we ship (be that a 56 update, or 57)

#18, this wasn't a particularly recent regression, so it's definitely in M56. esprehn@ and I haven't been able to find an outright uxss using this, but what the code currently does is certainly various dangerous.

One mitigating factor is that you can "only" uxss the first origin that touches window.external in a given renderer... so if nothing ever touches it, it's "okay".

Approving for merge into M56

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

commit b2bd86444285956bcd1a516cb49dd1261270c5d9
Author: Daniel Cheng <dcheng@chromium.org>
Date: Thu Feb 09 21:10:32 2017

Make External per-window rather than a static Persistent.

Sharing wrappers between contexts is unsafe.

BUG= 687844 

Review-Url: https://codereview.chromium.org/2671673002
Cr-Commit-Position: refs/heads/master@{#447724}
(cherry picked from commit f2026d03c0b50a54ca5d4845edb64f65a49deff7)

Review-Url: https://codereview.chromium.org/2681233004 .
Cr-Commit-Position: refs/branch-heads/2924@{#905}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/b2bd86444285956bcd1a516cb49dd1261270c5d9/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] https://crrev.com/b2bd86444285956bcd1a516cb49dd1261270c5d9/third_party/WebKit/Source/core/frame/DOMWindow.h

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