Make mixed content checking and CSP aware of RemoteFrames

Make mixed content checking and CSP aware of RemoteFrames

This CL is stubbing Blink security features that operate on
frames across origins, so that they can deal with RemoteFrames
being in the FrameTree. They will not yet work with cross-process
frames because there is not enough information in most cases, but
this allows us to put the frame infrastructure in place before
we implement cross-process information sharing to correct the
relevant behavior.

BUG=346764
R=mkwst@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175084

[kenrb, 2014-05-28 17:57:59]

Tom: this is an intermediate change for making CSP and mixed content checking
work with out of process iframes. I don't have a plan for how this will really
be done, but we need some stubs to deal with RemoteFrames here so that we can
get all the RemoteFrame infrastructure working properly (in particular, putting
RemoteFrames in the FrameTree). Can you please have a look and let me know if
you have any concerns?

Daniel: Owner review, please?

[Tom Sepez, 2014-05-28 21:12:59]

+mike

[dcheng, 2014-05-28 22:37:19]

Given that we need SandboxFlags and SecurityOrigin, I wonder if it makes sense
to just make Frame an ExecutionContext...

[Mike West, 2014-05-29 09:47:41]

This looks pretty reasonable, thanks!

What web-visible impact will it have today? Do we already create RemoteFrames in
some situations, or will that not be happening for a while? All the FIXMEs about
things not working yet worry me a little. :)

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2943: if (!isSandboxed(SandboxTopNavigation) &&
targetFrame == m_frame->tree().top())
Sandboxing flags will be moving up to Frame as well, correct?

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2986: // If targetFrame is a RemoteFrame then it is
a different origin.
True, but that doesn't mean that canAccessAncestor would return false today.
SecurityOrigins can be flagged with "universal access", for example, which would
make the 'canAccess' fall in 'canAccessAncestor' true.

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:3001: // RemoteFrames always have different
origins.
Same comment regarding canAccess as above; different origins doesn't always mean
no access.

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:3246: // Srcdoc documents must be local within the
containing frame.
Perhaps you could add `ASSERT(frame->isLocal())` to document this precondition?

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4850: if (m_frame && m_frame->tree().parent() &&
m_frame->tree().parent()->isLocalFrame() &&
(shouldInheritSecurityOriginFromOwner(m_url) || isPluginDocument()))
Why does my parent need to be a local frame for this inheritance to work?

https://codereview.chromium.org/303793003/diff/1/Source/core/frame/csp/CSPDir...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/303793003/diff/1/Source/core/frame/csp/CSPDir...
Source/core/frame/csp/CSPDirectiveList.cpp:97: if (!current->isLocalFrame() ||
!directive->allows(toLocalFrame(current)->document()->url()))
This will block loading any page that sets a frame-ancestor directive if it's
somewhere nested in a chain that includes a RemoteFrame. Do we create those
today, or will those only pop up in an OOP world?

[kenrb, 2014-05-29 13:38:40]

Thanks for the review, Mike.

In the current state of things, RemoteFrames are not instantiated under any
circumstance, because we don't have enough code infrastructure in place for
things to interact with them without crashing. We are hoping to change that
soon, in which case they will exist when running with the --site-per-process
flag. Until then, out-of-process iframes are implemented using LocalFrames with
blank documents.

Right now every feature that operates across cross-origin iframes is broken when
we run with --site-per-process, and that won't change when we get RemoteFrames
in use, but at that point we can start working with feature owners to get these
fixed. The FIXMEs are intended to call out the specific things that we are
missing. Any help you might want to give to getting these resolved over the next
2-3 months would be appreciated. :)

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2943: if (!isSandboxed(SandboxTopNavigation) &&
targetFrame == m_frame->tree().top())
On 2014/05/29 09:47:41, Mike West wrote:
> Sandboxing flags will be moving up to Frame as well, correct?

Those come off of the HTMLFrameOwnerElement. dcheng@ has a CL that is
refactoring that a little bit to make it easier to 'export' the parts of it that
will be needed by child frames in another process. That includes sandbox flags.

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:2986: // If targetFrame is a RemoteFrame then it is
a different origin.
On 2014/05/29 09:47:41, Mike West wrote:
> True, but that doesn't mean that canAccessAncestor would return false today.
> SecurityOrigins can be flagged with "universal access", for example, which
would
> make the 'canAccess' fall in 'canAccessAncestor' true.

Good point. I have added a TODO to reflect that.

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:3001: // RemoteFrames always have different
origins.
On 2014/05/29 09:47:41, Mike West wrote:
> Same comment regarding canAccess as above; different origins doesn't always
mean
> no access.

Comment changed.

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:3246: // Srcdoc documents must be local within the
containing frame.
On 2014/05/29 09:47:41, Mike West wrote:
> Perhaps you could add `ASSERT(frame->isLocal())` to document this
precondition?

That will implicitly happen in the last stage of this change. Right now
frame->tree().parent() still returns a LocalFrame pointer, because too many call
sites still assume that. The cast macros don't allow casting a LocalFrame
pointer to a LocalFrame pointer (i.e. toLocalFrame(localFrame)), so I can't add
casts to lines like this yet. The cast macros does contain an ASSERT for type
correctness.

https://codereview.chromium.org/303793003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:4850: if (m_frame && m_frame->tree().parent() &&
m_frame->tree().parent()->isLocalFrame() &&
(shouldInheritSecurityOriginFromOwner(m_url) || isPluginDocument()))
On 2014/05/29 09:47:41, Mike West wrote:
> Why does my parent need to be a local frame for this inheritance to work?

I probably should have mentioned this when I put the CL up, but the same
explanation as above. The check is there because m_frame->tree().parent() will
soon need to have a toLocalFrame() cast, because RemoteFrames don't have
documents. I don't know how we are going to do CSP inheritance across processes
yet.

https://codereview.chromium.org/303793003/diff/1/Source/core/frame/csp/CSPDir...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/303793003/diff/1/Source/core/frame/csp/CSPDir...
Source/core/frame/csp/CSPDirectiveList.cpp:97: if (!current->isLocalFrame() ||
!directive->allows(toLocalFrame(current)->document()->url()))
On 2014/05/29 09:47:41, Mike West wrote:
> This will block loading any page that sets a frame-ancestor directive if it's
> somewhere nested in a chain that includes a RemoteFrame. Do we create those
> today, or will those only pop up in an OOP world?

RemoteFrames will only be used behind a flag until we are ready to launch this
feature.

[Mike West, 2014-05-29 15:31:06]

Got it. Sounds reasonable; the FIXMEs scare me a little bit, but I'm TOTALLY
SURE that we have tests for everything they effect. *cough* :)

Thanks for updating the comments, looking forward to seeing future patches in
these areas.

LGTM.

[kenrb, 2014-05-29 15:34:57]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2014-05-29 15:35:00]

The CQ bit was unchecked by kenrb@chromium.org

[kenrb, 2014-05-29 15:35:27]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-05-29 15:36:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kenrb@chromium.org/303793003/20001

[kenrb, 2014-05-29 20:51:03]

The CQ bit was unchecked by kenrb@chromium.org

[kenrb, 2014-05-29 20:51:10]

The CQ bit was checked by kenrb@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:53:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kenrb@chromium.org/303793003/20001

[dcheng, 2014-05-29 21:45:27]

For similar changes for HTMLFrameOwnerElement, I've failed open for now (because
otherwise we'd never load any child frames). I guess the impact here might be
much less?

https://codereview.chromium.org/303793003/diff/20001/Source/core/frame/csp/CS...
File Source/core/frame/csp/CSPDirectiveList.cpp (right):

https://codereview.chromium.org/303793003/diff/20001/Source/core/frame/csp/CS...
Source/core/frame/csp/CSPDirectiveList.cpp:97: if (!current->isLocalFrame() ||
!directive->allows(toLocalFrame(current)->document()->url()))
Should we make these security checks fail open for now? RemoteFrame is not going
to be a shipping configuration for some time, and if it fails closed, it might
make development harder for us.

[kenrb, 2014-05-29 22:23:14]

In general I think you are right, but I would prefer to fail closed on security
checks. This only affects a specific CSP directive, and shouldn't interfere with
development.

[commit-bot: I haz the power, 2014-05-30 00:47:40]

Change committed as 175084