Only do security checks on javascript: URLs for frames for loading

Only do security checks on javascript: URLs for frames for loading

During layout, we don't know what origin set the URL, so we can't do the
check. On the other hand, assert that we can actually do the check for
loading.

BUG=628942, 618138
R=dcheng@chromium.org,dominicc@chromium.org

[jochen (gone - plz use gerrit), 2016-07-27 09:17:59]

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

[commit-bot: I haz the power, 2016-07-27 09:18:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 12:06:23]

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

[commit-bot: I haz the power, 2016-07-27 12:06:24]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-27 15:15:58]

Incidentally, I think this would also address
https://bugs.chromium.org/p/chromium/issues/detail?id=618138

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:60: if (reason ==
WillLoadURL && protocolIsJavaScript(completeURL)) {
I wonder if we should just cache the result of this and have the various checks
consult the saved bit? It's not super intuitive (IMO) to callers what the right
enum value to pass should be, and I worry that if we add new calls, it'd be easy
to get it wrong.

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:64: if
(!ScriptController::canAccessFromCurrentOrigin(toIsolate(&document()),
contentFrame()))
I wonder if we should remove the isolate->InContext() branch in
canAccessFromCurrentOrigin()?

[jochen (gone - plz use gerrit), 2016-07-27 15:19:35]

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

[jochen (gone - plz use gerrit), 2016-07-27 15:19:37]

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:60: if (reason ==
WillLoadURL && protocolIsJavaScript(completeURL)) {
On 2016/07/27 at 15:15:58, dcheng wrote:
> I wonder if we should just cache the result of this and have the various
checks consult the saved bit? It's not super intuitive (IMO) to callers what the
right enum value to pass should be, and I worry that if we add new calls, it'd
be easy to get it wrong.

well, if you want to do layout and get it wrong, you'll crash in the
SECURITY_CHECK below.

If you want to load and use the layout check instead, we might be toast anyways,
if there is no cached value yet :-/

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:64: if
(!ScriptController::canAccessFromCurrentOrigin(toIsolate(&document()),
contentFrame()))
On 2016/07/27 at 15:15:58, dcheng wrote:
> I wonder if we should remove the isolate->InContext() branch in
canAccessFromCurrentOrigin()?

done

[commit-bot: I haz the power, 2016-07-27 15:19:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2016-07-27 15:20:07]

Description was changed from

==========
Only do security checks on javascript: URLs for frames for loading

During layout, we don't know what origin set the URL, so we can't do the
check. On the other hand, assert that we can actually do the check for
loading.

BUG=628942
R=dcheng@chromium.org,dominicc@chromium.org
==========

to

==========
Only do security checks on javascript: URLs for frames for loading

During layout, we don't know what origin set the URL, so we can't do the
check. On the other hand, assert that we can actually do the check for
loading.

BUG=628942,618138
R=dcheng@chromium.org,dominicc@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-07-27 15:20:22]

also referenced your bug from the CL desc

[dcheng, 2016-07-27 15:39:06]

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:60: if (reason ==
WillLoadURL && protocolIsJavaScript(completeURL)) {
On 2016/07/27 15:19:37, jochen wrote:
> On 2016/07/27 at 15:15:58, dcheng wrote:
> > I wonder if we should just cache the result of this and have the various
> checks consult the saved bit? It's not super intuitive (IMO) to callers what
the
> right enum value to pass should be, and I worry that if we add new calls, it'd
> be easy to get it wrong.
> 
> well, if you want to do layout and get it wrong, you'll crash in the
> SECURITY_CHECK below.
> 
> If you want to load and use the layout check instead, we might be toast
anyways,
> if there is no cached value yet :-/

I'm kind of thinking we should just reset this to false in insertedInto and
removedFrom: that would solve that issue right?

[commit-bot: I haz the power, 2016-07-27 16:42:15]

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

[commit-bot: I haz the power, 2016-07-27 16:42:16]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-07-27 18:31:09]

On 2016/07/27 at 15:39:06, dcheng wrote:
>
https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):
> 
>
https://codereview.chromium.org/2183423002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:60: if (reason ==
WillLoadURL && protocolIsJavaScript(completeURL)) {
> On 2016/07/27 15:19:37, jochen wrote:
> > On 2016/07/27 at 15:15:58, dcheng wrote:
> > > I wonder if we should just cache the result of this and have the various
> > checks consult the saved bit? It's not super intuitive (IMO) to callers what
the
> > right enum value to pass should be, and I worry that if we add new calls,
it'd
> > be easy to get it wrong.
> > 
> > well, if you want to do layout and get it wrong, you'll crash in the
> > SECURITY_CHECK below.
> > 
> > If you want to load and use the layout check instead, we might be toast
anyways,
> > if there is no cached value yet :-/
> 
> I'm kind of thinking we should just reset this to false in insertedInto and
removedFrom: that would solve that issue right?

I dunno... too much magic IMO.

[dominicc (has gone to gerrit), 2016-07-28 01:22:57]

Naive question: Would it be better to just always give non-display: none;
iframes layout objects, even if they have invalid URLs, and decouple layout from
URL validity checks? What's the benefit of iframes with invalid URLs not having
a layout object?

[jochen (gone - plz use gerrit), 2016-07-28 06:55:48]

On 2016/07/28 at 01:22:57, dominicc wrote:
> Naive question: Would it be better to just always give non-display: none;
iframes layout objects, even if they have invalid URLs, and decouple layout from
URL validity checks? What's the benefit of iframes with invalid URLs not having
a layout object?

I think that would be indeed better, but I don't know why this check is there -
looking at the history, it's from khtml

[dcheng, 2016-07-28 06:57:56]

On 2016/07/28 06:55:48, jochen wrote:
> On 2016/07/28 at 01:22:57, dominicc wrote:
> > Naive question: Would it be better to just always give non-display: none;
> iframes layout objects, even if they have invalid URLs, and decouple layout
from
> URL validity checks? What's the benefit of iframes with invalid URLs not
having
> a layout object?
> 
> I think that would be indeed better, but I don't know why this check is there
-
> looking at the history, it's from khtml

OK, how about we just cache and calculate it once in
HTMLFrameElementBase::setLocation() then? That seems like the most reasonable
place to do it, as that's the central location for trigger location changes.

[jochen (gone - plz use gerrit), 2016-07-28 07:09:02]

On 2016/07/28 at 06:57:56, dcheng wrote:
> On 2016/07/28 06:55:48, jochen wrote:
> > On 2016/07/28 at 01:22:57, dominicc wrote:
> > > Naive question: Would it be better to just always give non-display: none;
> > iframes layout objects, even if they have invalid URLs, and decouple layout
from
> > URL validity checks? What's the benefit of iframes with invalid URLs not
having
> > a layout object?
> > 
> > I think that would be indeed better, but I don't know why this check is
there -
> > looking at the history, it's from khtml
> 
> OK, how about we just cache and calculate it once in
HTMLFrameElementBase::setLocation() then? That seems like the most reasonable
place to do it, as that's the central location for trigger location changes.

could it happen that we don't have a contentFrame() in setLocation but have one
later when we get inserted into a tree?

[jochen (gone - plz use gerrit), 2016-07-28 07:16:28]

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

[jochen (gone - plz use gerrit), 2016-07-28 07:16:33]

updated

[commit-bot: I haz the power, 2016-07-28 07:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 08:43:04]

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

[commit-bot: I haz the power, 2016-07-28 08:43:05]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-07-28 08:55:03]

https://codereview.chromium.org/2183423002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):

https://codereview.chromium.org/2183423002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:192: if
(!m_URL.isEmpty()) {
OK, so I thought this would be better, but the more I'm looking at it, the more
confused I am.

setLocation() can be called directly by the parser, right? How come we don't
crash on SECURITY_CHECK(isolate->InContext())? I wouldn't expect us to
(normally) be in a v8 context when parsing a page like this:

<body>
<iframe src="javascript:42"></iframe>
</body>

So how does that work?

[jochen (gone - plz use gerrit), 2016-07-28 08:56:13]

On 2016/07/28 at 08:55:03, dcheng wrote:
>
https://codereview.chromium.org/2183423002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):
> 
>
https://codereview.chromium.org/2183423002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:192: if
(!m_URL.isEmpty()) {
> OK, so I thought this would be better, but the more I'm looking at it, the
more confused I am.
> 
> setLocation() can be called directly by the parser, right? How come we don't
crash on SECURITY_CHECK(isolate->InContext())? I wouldn't expect us to
(normally) be in a v8 context when parsing a page like this:
> 
> <body>
> <iframe src="javascript:42"></iframe>
> </body>
> 
> So how does that work?

either zero test coverage, or we are in context :)

[dcheng, 2016-07-29 08:41:47]

On 2016/07/28 08:56:13, jochen wrote:
> On 2016/07/28 at 08:55:03, dcheng wrote:
> >
>
https://codereview.chromium.org/2183423002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2183423002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLFrameElementBase.cpp:192: if
> (!m_URL.isEmpty()) {
> > OK, so I thought this would be better, but the more I'm looking at it, the
> more confused I am.
> > 
> > setLocation() can be called directly by the parser, right? How come we don't
> crash on SECURITY_CHECK(isolate->InContext())? I wouldn't expect us to
> (normally) be in a v8 context when parsing a page like this:
> > 
> > <body>
> > <iframe src="javascript:42"></iframe>
> > </body>
> > 
> > So how does that work?
> 
> either zero test coverage, or we are in context :)


OK, so the answer to this mystery is we missed an entry point to openURL:
setNameAndOpenURL also calls openURL. It seems like this is actually callable
from script as well:

<!DOCTYPE html>
<body>
<div>
</div>
<script>
document.querySelector('div').innerHTML = '<iframe src="javascript:\'hello
world\'"></iframe>';
</script>
</body>

In this case, I /think/ it might actually be safe to skip the check: since it is
only called when inserting new elements, the iframe must be same-origin, so
skipping the check is always safe. At the same time, it makes me pretty nervous
to have the check not cover this path...

The problem is if we extend the check to cover this path, then we have to make
the exception for isolate->InContext() once again...

[jochen (gone - plz use gerrit), 2016-08-01 12:20:25]

so back to WillLoadURL?