Update V8 security token correctly when installing a new Document.

Update V8 security token correctly when installing a new Document.

BUG=583445
Committed: https://crrev.com/cdd9b1aa17694677c566e3a6ce6c620006dbb46a
Cr-Commit-Position: refs/heads/master@{#374871}

[dcheng, 2016-02-05 00:12:59]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, japhet@chromium.org

[dcheng, 2016-02-05 00:20:42]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org

[dcheng, 2016-02-05 00:20:42]

I had an explanation typed up because I thought I understood it... but I don't.
So I'm going to think about it some more and try to write up a description of
the test case.

In the mean time, this is the proposed fix in the linked bug, so I think it
should be safe. I've also run the test with and without my patch to verify that
it does fix the described issue.

[dcheng, 2016-02-05 00:22:06]

The CQ bit was checked by dcheng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-05 00:22:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675473002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675473002/1

[haraken, 2016-02-05 00:26:36]

LGTM on my side but let's have jochen confirm this.

I'm rather surprised that there are still many call sites that use
setSecurityOrigin. I guess they should be replaced with updateSecurityOrigin as
well (I haven't yet investigated the call sites though).

[dcheng, 2016-02-05 00:36:56]

On 2016/02/05 at 00:26:36, haraken wrote:
> LGTM on my side but let's have jochen confirm this.
> 
> I'm rather surprised that there are still many call sites that use
setSecurityOrigin. I guess they should be replaced with updateSecurityOrigin as
well (I haven't yet investigated the call sites though).

Yeah, that was my concern as well: that's why I tried to do
https://codereview.chromium.org/1655413002 first, but it wasn't successful:
there's a circular dependency there between creating the new Document and
installing it into the LocalFrame/LocalDOMWindow. So this is just a small fix
and hopefully we can figure out a more comprehensive way to do this later.

[haraken, 2016-02-05 00:44:04]

> > I'm rather surprised that there are still many call sites that use
> setSecurityOrigin. I guess they should be replaced with updateSecurityOrigin
as
> well (I haven't yet investigated the call sites though).
> 
> Yeah, that was my concern as well: that's why I tried to do
> https://codereview.chromium.org/1655413002 first, but it wasn't successful:
> there's a circular dependency there between creating the new Document and
> installing it into the LocalFrame/LocalDOMWindow. So this is just a small fix
> and hopefully we can figure out a more comprehensive way to do this later.

Makes sense.

[dcheng, 2016-02-05 00:48:08]

I added an explanation of the bug.

https://codereview.chromium.org/1675473002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/security/opened-document-security-origin-resets-on-navigation.html
(right):

https://codereview.chromium.org/1675473002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/opened-document-security-origin-resets-on-navigation.html:10:
frame.onload = function () {
After this loads, frame tree looks like this:

[ Main frame: origin 127.0.0.1:8000 ]
 `--[ Subframe A: origin localhost:8000 ]
     `--[Subframe B: origin doesn't matter yet ]

https://codereview.chromium.org/1675473002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/opened-document-security-origin-resets-on-navigation.html:12:
var blob = new Blob(["<script>(" + function () {
The blob URL has origin 127.0.0.1:8000. Once subframe A is finished loading, the
blob URL is loaded into subframe B. The frame tree + origins now look like this:

[ Main frame: origin 127.0.0.1:8000 ]
 `--[ Subframe A: origin localhost:8000 ]
     `--[Subframe B: 127.0.0.1:8000 (blob URL) ]

https://codereview.chromium.org/1675473002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/opened-document-security-origin-resets-on-navigation.html:13:
frame = document.documentElement.appendChild(document.createElement("iframe"));
After subframe C loads, frame tree looks like this:

[ Main frame: origin 127.0.0.1:8000 ]
 `--[ Subframe A: origin localhost:8000 ]
     `--[Subframe B: 127.0.0.1:8000 (blob URL) ]
         `--[Subframe C: 127.0.0.1:8000 ]

https://codereview.chromium.org/1675473002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/opened-document-security-origin-resets-on-navigation.html:14:
frame.contentWindow.setTimeout("parent.document.open()", 0);
document.open() apparently aliases the parent window's security origin:

Once this timer fires, the frame tree + origins look like this:
[ Main frame: origin 127.0.0.1:8000 ]
 `--[ Subframe A: origin localhost:8000 ]
     `--[Subframe B: origin localhost:8000 ]

Subframe C is detached due to the document.open() call, so it is no longer in
the frame tree.

https://codereview.chromium.org/1675473002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/security/opened-document-security-origin-resets-on-navigation.html:15:
setTimeout(function () {
This setTimeout is set on the context of Subframe B. But the document.open()
call does NOT cancel it.

So now we have:
[ Main frame: origin 127.0.0.1:8000 ]
 `--[ Subframe A: origin localhost:8000 ]
     `--[Subframe B: javascript URL: origin = ???? ]

The javascript URL should have origin 127.0.0.1:8000, because it originates from
the blob URL. However, due to the bug of calling setSecurityOrigin():
- The Document has the right security origin (127.0.0.1:8000)
- But the security token of the v8 Context is not updated and is still
localhost:8000.

So v8 allows parent.eval to proceed, even though parent (subframe A) is
cross-origin to subframe B.

[commit-bot: I haz the power, 2016-02-05 02:05:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-05 02:05:20]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-02-05 10:00:13]

I took another look at this after jochen@ asked me some questions.

The parent.document.open() call on line 14 of the test is critical, but I don't
completely understand why. Two weird things happen:

1. The security origin of [subframe B]'s document changes to match the security
origin of [subframe A]'s document. So [B]->document()->securityOrigin() ==
[A]->document()->securityOrigin() == localhost:8000. I'm still trying to figure
out when this origin change happens...

2. However, the security token of [B]'s context does NOT match the security
token of [A]'s context yet since [B]'s security token has not yet been updated.
So [A]'s security token is localhost:8000 and [B]'s security token is
127.0.0.1:8000.

Then we load the javascript: URL. Before we load the javascript: URL, we do a
security check in DOMWindow::isInsecureScriptAccess. This passes because
&callingWindow == this.

Now, we load the JS URL. The security token is first set by this path:
#0 0x7fe38ff4a12e base::debug::StackTrace::StackTrace()
#1 0x7fe3890b4b4b blink::WindowProxy::setSecurityToken()
#2 0x7fe3890b5bfa blink::WindowProxy::updateSecurityOrigin()
#3 0x7fe3890b49f1 blink::WindowProxy::updateDocument()
#4 0x7fe38902f8a0 blink::ScriptController::updateDocument()
#5 0x7fe389f007b7 blink::LocalDOMWindow::installNewDocument()
#6 0x7fe38a0bf448 blink::DocumentLoader::createWriterFor()
#7 0x7fe38a0bf1bf blink::DocumentLoader::ensureWriter()
#8 0x7fe38a0bdc80 blink::DocumentLoader::commitData()
#9 0x7fe38a0bf900 blink::DocumentLoader::processData()
#10 0x7fe38a0bf796 blink::DocumentLoader::dataReceived()
#11 0x7fe389e81a0e blink::RawResource::appendData()
#12 0x7fe389ea52bb blink::ResourceLoader::didReceiveData()
#13 0x7fe390dd7c78 content::WebURLLoaderImpl::Context::OnReceivedData()
#14 0x7fe390dd8713 content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
#15 0x7fe390d7272f content::ResourceDispatcher::OnReceivedData()

However, at this point, LocalFrame::document()'s SecurityOrigin ==
localhost:8000. So the security token for [B] is set to localhost:8000, and [B]
can now reach into [A] since the security tokens now match.


Without the call to parent.document.open(), the security for [B]'s document
remains 127.0.0.1:8000. So when we load the JS URL, the security token gets set
to 127.0.0.1:8000: since the security token does not match [A], [B] cannot reach
into [A].


The current patch prevents this: even though the security token of [B] initially
matches [A], it gets overwritten a few lines after the installNewDocument() call
to the correct security token (I guess this is the page of the Document that
initiated the load of the javascript: URL).


After looking at this again, I'm not sure if this is really the right fix: it
seems wrong that document.open() can cause the Document's origin to change like
that.

[dcheng, 2016-02-05 10:11:36]

Hm... but when I debug this, it looks weird: it looks like the security origin
for [B] is set to 127.0.0.1:8000 in parent.document.open().

Investigating more...

[dcheng, 2016-02-05 10:44:11]

OK, so I did a bunch more debugging, and this is why parent.document.open() is
crucial:

FrameLoader::replaceDocumentWhileExecutingJavaScriptURL creates a DocumentInit
using Document::url() from the previous Document.

Without the open() call, the previous Document's URL is just the blob URL.

With open() call, the previous Document's URL is about:blank. That means that
when we call Document::initSecurityContext(),
shouldInheritSecurityOriginFromOwner() incorrectly returns true. As a result, we
call setSecurityOrigin(initializer.owner()->securityOrigin()) here
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
Now, the Document for the JS URL incorrectly has the security origin of its
parent frame [A].

Then when LocalDOMWindow::installNewDocument() calls updateDocument(), it sets
the security token using the security origin of the new Document... but the new
Document incorrectly has a security origin of A.

So to sum up:
[Main]
`-[A]
  `-[B] (blob URL)
    `-[C] (about:blank)

After document.open(), [C]'s URL is cloned into [B]:
[Main]
`-[A]
  `-[B] (about:blank)

Then when we create the new Document for loading the JS URL into [B],
initialization of the security context incorrectly inherits the security origin
of [A] due to it seeing the about:blank URL.

[jochen (gone - plz use gerrit), 2016-02-05 12:12:10]

my reading of the spec is that we should be using the origin of the source
document, which would be the about:blank frame here. However, we don't pass it
from the FrameLoader down to the ScriptController. Let me try that.

[jochen (gone - plz use gerrit), 2016-02-05 13:59:48]

i think the source document is a red herring.

really, what happens is that we set the origin to the parent's origin because
the URL is about:blank, and then we set it to the right origin.

we shouldn't set it twice: https://codereview.chromium.org/1670173002

[dcheng, 2016-02-10 06:14:33]

jochen, what do you think about landing this patch as a temporary mitigation?

I have another experimental patch that takes another approach, but I think it
will be difficult to merge to stable: https://codereview.chromium.org/1685003002

[dcheng, 2016-02-10 22:31:07]

dcheng@chromium.org changed reviewers:
+ esprehn@chromium.org

[dcheng, 2016-02-10 22:31:07]

+esprehn to get some thoughts on this patch, as jochen is OOO.

[esprehn, 2016-02-11 03:29:07]

esprehn@chromium.org changed reviewers:
+ adamk@chromium.org

[esprehn, 2016-02-11 03:29:07]

adamk@ and I went through this patch, the surrounding code, and the history and
from what I can tell we already set the token inside installNewDocument(), in
frame()->script().updateDocument();, but we read the old securityOrigin() and
then call setSecurityOrigin() when we unwind the stack in createWriterFor which
is too late. This ordering bug is ancient and seems to date all the way back to
the original patch that added the if (ownerDocument) code. The only place this
is currently observable is inside the appendReplacingData(source); call in
replaceDocumentWhileExecutingJavaScriptURL which is exactly what this patch is
trying to fix.

The places that want to execute javascript: urls where I think you could notice
this are page level navigation, iframe.src = "javascript:", and <form
submit=javascript:>, so maybe if you do parent.document.open() and then
.submit() on a form in yourself this changes some behavior? I'm not sure I
totally understand what the implications are there, but seems obscure, and this
patch seems correct from a sanity perspective.

So yeah this code goes through the token updating SecurityToken twice, but I
don't think it should cause issues.

lgtm

[dcheng, 2016-02-11 07:28:53]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-02-11 07:28:54]

OK, going to go ahead and land this as a temporary measure: I have a better fix
in progress and the trybots/the problematic Google spreadsheet/the new test in
this patch are all pretty happy with it: it's just a bit too large to safely
merge back (and still has a few rough edges I'm still polishing off).

Thanks all!

[commit-bot: I haz the power, 2016-02-11 07:29:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675473002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675473002/1

[commit-bot: I haz the power, 2016-02-11 08:51:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-16 22:34:56]

Description was changed from

==========
Update V8 security token correctly when installing a new Document.

BUG=583445
==========

to

==========
Update V8 security token correctly when installing a new Document.

BUG=583445

Committed: https://crrev.com/cdd9b1aa17694677c566e3a6ce6c620006dbb46a
Cr-Commit-Position: refs/heads/master@{#374871}
==========

[commit-bot: I haz the power, 2016-02-16 22:34:57]

Patchset 1 (id:??) landed as
https://crrev.com/cdd9b1aa17694677c566e3a6ce6c620006dbb46a
Cr-Commit-Position: refs/heads/master@{#374871}