cc'ing myself since I didn't received mail notifications.

Thanks for the report. Does this work on any other platforms, or any other versions?

creis, nasko, could you please take a look, or help with assigning this to the right person?

This is working fine on Mac Chrome Stable channel 58.0.3029.96.
I don't have access to other platforms.

Ouch, I got this to work in a chrome dev build on Windows (60.0.3080.5) (doesn't seem to repro on Beta).

I can't repro this on Linux on either 60.0.3080.5, or beta or stable.

I'm going to slap on OS-All out of caution.

Thanks-- looks like a regression of the defense in  issue 9682 .  I wonder if we're not getting the same notification from V8 about the access to the initial blank page as before (RenderFrameImpl::DidAccessInitialDocument()).

Can someone bisect?  CC'ing dcheng@, in case he knows of any recent changes in that area that might have regressed it (though I'm surprised tests didn't catch it).

Attaching the spoof page, in case the test page isn't live in the future.

Deleted: spoof.html 1.4 KB

spoof.html 1.4 KB View Download

ochang@: Why Security_Impact-Stable?  Your comment 5 says this doesn't repro on Beta or Stable.

In a debugger, it does look like we're not getting to FrameLoader::DidAccessInitialDocument(), which is a problem.

I don't have access to a Mac to confirm, but #4 says this is working on stable.

srikanthg@: Can you clarify?  Does "working fine on Mac Chrome Stable channel 58.0.3029.96" mean the URL spoof occurs or does not occur there?

Does NOT repro on stable.
about:blank is displayed in stable.

I just tested on Mac canary and stable, found the same thing.

Thanks for clarifying.

Thanks!  I think we'll need a bisect to find when we stopped reporting these initial document accesses.

You are probably looking for a change made after 465483 (known good), but no later than 465484 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
  https://chromium.googlesource.com/chromium/src/+log/0f47d06919be1dfd8a307f2dca899612dcb68f9a..8267f48654eb20e9d6453b53f2da310953d244f1

That's a V8 roll.  Is there a way to bisect further?

There's not much in there:
207d5c3 Version 6.0.21 by v8-autoroll · 2 weeks ago 6.0.21 6.0.21
cd76322 Add flag to make __defineGetter__ & co. behave as strict functions by Adam Klein · 2 weeks ago
54271c2 [inspector] move console to builtins by kozyatinskiy · 2 weeks ago

adamk@ or kozyatinskiy@: Could either of your CLs have affected this (via  CanAccessFrameInternal in BindingSecurity or EvaluateScriptInMainWorld in ScriptController)?

I'm not sure that my change can be related - it just move console implementation and as long as test page doesn't use console - behavior should be the same. 

I'll try to reproduce it locally and revert suspected CLs locally later tomorrow.

If there are no calls to __defineGetter__ or __defineSetter__, I can't see how my patch could have been related. From looking at the spoof page in #7, I don't see any such thing.

Thanks.  I'll try as well, though it looks like kozyatinskiy@'s CL doesn't revert cleanly due to some dependent CLs.

The bug seems specific to document.write.  We have a test for that in ParameterizedWebFrameTest.DidWriteToInitialDocumentBeforeModalDialog (in WebFrameTest.cpp), so I'm not sure why it isn't failing.  I'll keep looking at that aspect.

Hmm, I couldn't compile after reverting cd76322.  Not sure how to proceed-- is there a better way to revert a V8 CL in a Chrome checkout than using git revert within the v8 directory?

Marking this as a stable blocker.

Since I haven't been able to compile when reverting the V8 CLs, I'm not sure yet which one is to blame.  kozyatinskiy@: I'll assign this to you for seeing which V8 CL is the cause.  Let me know if you have any questions about it, though.  Thanks!

As for tests, we also have RenderFrameHostManagerTest.ShowLoadingURLUntilSpoof, but that uses document.body.innerHTML instead of document.write, which isn't affected.  It also uses a 204 response followed by window.open, which is a different unaffected path.  I'll add a similar test for a slowly loading URL that uses document.write to try to get content_browsertest coverage for this.  (Still not sure why the WebFrameTest didn't catch it.)

I reverted adamk's CL locally and it still repros. I'll try reverting the other one now.

I can confirm that reverting the change to making console a builtin fixes this. I don't actually understand why yet though.

OK, I think our DidAccessInitialDocument check might just be completely broken =(

Here's the stack for how it gets called with the console changes reverted:
>	blink_web.dll!blink::LocalFrameClientImpl::DidAccessInitialDocument()	C++
 	blink_core.dll!blink::FrameLoader::DidAccessInitialDocument() Line 1007	C++
 	blink_core.dll!blink::`anonymous namespace'::CanAccessFrameInternal(const blink::LocalDOMWindow * accessing_window, const blink::SecurityOrigin * target_frame_origin, const blink::DOMWindow * target_window) Line 72	C++
 	blink_core.dll!blink::`anonymous namespace'::CanAccessFrame(const blink::LocalDOMWindow * accessing_window, blink::SecurityOrigin * target_frame_origin, const blink::DOMWindow * target_window, blink::BindingSecurity::ErrorReportOption reporting_option) Line 96	C++
 	blink_core.dll!blink::V8Window::namedPropertyGetterCustom(const v8::PropertyCallbackInfo<v8::Value> & name) Line 348	C++
 	blink_core.dll!blink::V8Window::namedPropertyGetterCallback(const v8::PropertyCallbackInfo<v8::Value> & info) Line 7522	C++
 	v8.dll!v8::internal::PropertyCallbackArguments::Call(void(*)(v8::Local<v8::Name>, const v8::PropertyCallbackInfo<v8::Value> &) f, v8::internal::Handle<v8::internal::Name>) Line 41	C++
 	v8.dll!v8::internal::`anonymous namespace'::GetPropertyAttributesWithInterceptorInternal(v8::internal::LookupIterator * it, v8::internal::Handle<v8::internal::InterceptorInfo>) Line 1697	C++
 	v8.dll!v8::internal::JSObject::GetPropertyAttributesWithInterceptor() Line 5568	C++
 	v8.dll!v8::internal::Object::SetPropertyInternal(v8::internal::Handle<v8::internal::Object> it, v8::internal::LanguageMode language_mode, v8::internal::Object::StoreFromKeyed store_mode, bool * found) Line 4348	C++
 	v8.dll!v8::internal::Object::SetProperty(v8::internal::Handle<v8::internal::Object> it, v8::internal::LanguageMode language_mode, v8::internal::Object::StoreFromKeyed store_mode) Line 4404	C++
 	v8.dll!v8::internal::Runtime::SetObjectProperty(v8::internal::Handle<v8::internal::Object> isolate, v8::internal::Handle<v8::internal::Object> language_mode, v8::internal::Handle<v8::internal::Object>) Line 349	C++
 	v8.dll!v8::Object::Set(v8::Local<v8::Context>) Line 4204	C++
 	v8.dll!v8_inspector::InspectedContext::InspectedContext(v8_inspector::V8InspectorImpl * inspector, const v8_inspector::V8ContextInfo & info, int contextId) Line 37	C++
 	v8.dll!v8_inspector::V8InspectorImpl::contextCreated(const v8_inspector::V8ContextInfo & info) Line 197	C++
 	blink_core.dll!blink::MainThreadDebugger::ContextCreated(blink::ScriptState * script_state, blink::LocalFrame * frame, blink::SecurityOrigin * origin) Line 165	C++
 	blink_core.dll!blink::LocalWindowProxy::Initialize() Line 160	C++
 	blink_core.dll!blink::Frame::GetWindowProxy(blink::DOMWrapperWorld & world) Line 393	C++
 	blink_core.dll!blink::ToV8(blink::DOMWindow * window, v8::Local<v8::Object> isolate, v8::Isolate *) Line 33	C++
 	blink_core.dll!blink::V8Window::openMethodCustom() Line 291	C++
 	v8.dll!v8::internal::FunctionCallbackArguments::Call(void(*)(const v8::FunctionCallbackInfo<v8::Value> &) f) Line 25	C++
 	v8.dll!v8::internal::`anonymous namespace'::HandleApiCallHelper<0>(v8::internal::Isolate * isolate, v8::internal::Handle<v8::internal::HeapObject> args, v8::internal::Handle<v8::internal::HeapObject>) Line 112	C++
 	v8.dll!v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments args, v8::internal::Isolate * isolate) Line 142	C++
 	v8.dll!v8::internal::Builtin_HandleApiCall(int args_length, v8::internal::Object * * args_object, v8::internal::Isolate * isolate) Line 130	C++
 	[External Code]	

The stack has absolutely nothing to do with actually accessing the document: it's getting called incidentally as part of installing the console object on the global...

https://chromium.googlesource.com/chromium/src/+log/f352827c452f7d6b493cd03cc367221368912879..b660c6450af8e7af4eb7401c7491d290730bbd81 is the revision range where we started always invoked the DidAccesssInitialDocument() callback. This is almost certainly the result of https://codereview.chromium.org/2209303002. I've filed issue 719122 to improve our test coverage in this area.

In the meantime, I'm going to go ahead and assign this bug to yukishiino@ since this regression is really not related to inspector.

Thanks a lot, I was about to start reverting my CLs but you already found the root. If we need workaround quick we can at the same place of inspector code just get console and set it to trigger DidAccessInitialDocument.

It was a test on android: testOnPageFinishedCalledOnDomModificationAfterNavigation this test timeouts with my this console CL and with previous attempts to align console property with spec for a long time. And after a lot of different attempts to reproduce it locally, discussing with test author and fact that test is flaky, I decided to disable it for now and investigate later. So it's partially my bad.

I think it's not really related to the console work. It just happened that the way the console was previously implemented hid the fact that we had this bug.

OK, I understand what's going on. I'll write a patch and add a test for this.

This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

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

 Issue 719295  has been merged into this issue.

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

commit 3bd0eead2eb6d83f7ccbbd128692731522a1d1f6
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue May 09 03:39:52 2017

Use the default security token for the initial empty Document.

This was incorrectly removed in r412736, but this bug was not noticed
due to an implementation detail of devtools that would trigger the
DidAccessInitialDocument() call via a different code path. When the
devtools changed in r465484, this revealed the bug.

This also improves the tests to try to catch any future instances of
tests that incorrectly pass when they shouldn't by adding baseline
tests that assert that DidAccessInitialDocument() isn't called when
it shouldn't be.

Bug:  718946 , 719122
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Ie2fb74f858ee5563788c8c74a01d1f63776798b3
Reviewed-on: https://chromium-review.googlesource.com/497934
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Charles Reis <creis@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#470152}
[modify] https://crrev.com/3bd0eead2eb6d83f7ccbbd128692731522a1d1f6/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/3bd0eead2eb6d83f7ccbbd128692731522a1d1f6/content/test/data/click-nocontent-link.html
[modify] https://crrev.com/3bd0eead2eb6d83f7ccbbd128692731522a1d1f6/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
[modify] https://crrev.com/3bd0eead2eb6d83f7ccbbd128692731522a1d1f6/third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp
[modify] https://crrev.com/3bd0eead2eb6d83f7ccbbd128692731522a1d1f6/third_party/WebKit/Source/web/tests/FrameTestHelpers.h
[modify] https://crrev.com/3bd0eead2eb6d83f7ccbbd128692731522a1d1f6/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

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

commit 0eef43880ae8ff6f4accd8d138a4a94197255651
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue May 09 06:42:38 2017

Revert "Use the default security token for the initial empty Document."

This reverts commit 3bd0eead2eb6d83f7ccbbd128692731522a1d1f6.

Reason for revert: updated and new tests are failing on Cast Audio
Linux and Cast Audio bots.

Original change's description:
> Use the default security token for the initial empty Document.
> 
> This was incorrectly removed in r412736, but this bug was not noticed
> due to an implementation detail of devtools that would trigger the
> DidAccessInitialDocument() call via a different code path. When the
> devtools changed in r465484, this revealed the bug.
> 
> This also improves the tests to try to catch any future instances of
> tests that incorrectly pass when they shouldn't by adding baseline
> tests that assert that DidAccessInitialDocument() isn't called when
> it shouldn't be.
> 
> Bug:  718946 , 719122
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
> Change-Id: Ie2fb74f858ee5563788c8c74a01d1f63776798b3
> Reviewed-on: https://chromium-review.googlesource.com/497934
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Charles Reis <creis@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#470152}

TBR=dcheng@chromium.org,creis@chromium.org,haraken@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Bug:  718946 , 719122

Change-Id: I8a9f637690f2e77a2f25dacbc50fbe2bb0980fad
Reviewed-on: https://chromium-review.googlesource.com/500087
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#470252}
[modify] https://crrev.com/0eef43880ae8ff6f4accd8d138a4a94197255651/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/0eef43880ae8ff6f4accd8d138a4a94197255651/content/test/data/click-nocontent-link.html
[modify] https://crrev.com/0eef43880ae8ff6f4accd8d138a4a94197255651/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
[modify] https://crrev.com/0eef43880ae8ff6f4accd8d138a4a94197255651/third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp
[modify] https://crrev.com/0eef43880ae8ff6f4accd8d138a4a94197255651/third_party/WebKit/Source/web/tests/FrameTestHelpers.h
[modify] https://crrev.com/0eef43880ae8ff6f4accd8d138a4a94197255651/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

For some reason, it succeeds on the trybots:
https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/363060

But fails on the waterfall:
https://build.chromium.org/p/chromium.linux/builders/Cast%20Linux/builds/40602

The GN configs are pretty similar. For the trybot:
dcheck_always_on = true
goma_dir = "/b/c/goma_client"
is_chromecast = true
is_component_build = false
is_debug = false
strip_absolute_paths_from_debug_symbols = true
symbol_level = 1
use_goma = true

For the waterfall:
goma_dir = "/b/c/goma_client"
is_chromecast = true
is_component_build = false
is_debug = false
use_goma = true

So it's not 100% clear why it's failing on the waterfall but not on the trybot runs...

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

commit 6f9445e94f65449f392ea05167979c053b1826cd
Author: Daniel Cheng <dcheng@chromium.org>
Date: Thu May 11 19:26:21 2017

Use the default security token for the initial empty Document.

This was incorrectly removed in r412736, but this bug was not noticed
due to an implementation detail of devtools that would trigger the
DidAccessInitialDocument() call via a different code path. When the
devtools changed in r465484, this revealed the bug.

This also improves the tests to try to catch any future instances of
tests that incorrectly pass when they shouldn't by adding baseline
tests that assert that DidAccessInitialDocument() isn't called when
it shouldn't be.

Bug:  718946 , 719122
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I3262f5f4367eeaeae1d24f145136773263610f77
Reviewed-on: https://chromium-review.googlesource.com/502347
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#471040}
[modify] https://crrev.com/6f9445e94f65449f392ea05167979c053b1826cd/content/browser/frame_host/render_frame_host_manager_browsertest.cc
[modify] https://crrev.com/6f9445e94f65449f392ea05167979c053b1826cd/content/test/data/click-nocontent-link.html
[modify] https://crrev.com/6f9445e94f65449f392ea05167979c053b1826cd/third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp
[modify] https://crrev.com/6f9445e94f65449f392ea05167979c053b1826cd/third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp
[modify] https://crrev.com/6f9445e94f65449f392ea05167979c053b1826cd/third_party/WebKit/Source/web/tests/FrameTestHelpers.h
[modify] https://crrev.com/6f9445e94f65449f392ea05167979c053b1826cd/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Based on https://storage.googleapis.com/chromium-find-releases-static/826.html#8267f48654eb20e9d6453b53f2da310953d244f1, I think there's no need to merge anything, right?

Correct-- the vulnerability doesn't seem to affect earlier branches than M59.  (As you noted, there's a functional bug in earlier branches where we don't show the pending URL in window.open cases that it's safe to, but I don't think we need to merge anything for that.)

Verified in Chrome Version 60.0.3104.0 (Official Build) canary (64-bit)
about:blank is displayed when running the testcase as mentioned in my initial bug report.

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