Make SecurityContext available in RemoteFrames.

Make SecurityContext available in RemoteFrames.

With OOPIF, many security checks need to access SecurityContexts of RemoteFrames.  This CL is the first step to make this possible.  It introduces a SecurityContext accessor for Frames.  LocalFrames redirect it to document(), and RemoteFrames return a new RemoteSecurityContext.  A RemoteSecurityContext's origin is initialized using data replicated from the browser process, and its CSP is a default/empty policy (CSP is not replicated because it is moving to the browser process).

More information: https://docs.google.com/a/chromium.org/document/d/1Y0s76YK0ziiL8hddiFlNUyAF4hqRAGpZM8cfnnLsZPg/edit#heading=h.lrzgurbjttfm

The Chromium side of this CL is: https://codereview.chromium.org/692973005/


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

[alexmos, 2014-11-11 23:44:25]

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

[alexmos, 2014-11-11 23:44:26]

Hey Daniel, could you please review this CL, which is the first step towards
enabling origin replication?

https://codereview.chromium.org/520213002/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:852: void
WebRemoteFrameImpl::setReplicatedOrigin(const WebURL& origin, bool isUnique)
const
I don't like that this is taking the isUnique flag separately, and this can't
take a FrameReplicationState.  I'm thinking about changing this to take a
WebSecurityOrigin instead, and the work done here to synthesize
WebSecurityOrigin from a FrameReplicationState should happen in some other
helper function in content.  Does that sounds plausible, and if so, where is a
good place to add such a function?

[dcheng, 2014-11-12 21:55:50]

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.h (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.h:12: class RemoteSecurityContext: public
SecurityContext, public RefCounted<RemoteSecurityContext> {
Nit: space before :.

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.h:88: virtual SecurityContext* securityContext()
const override;
Nit: group this into the Frame overrides: section without a newline separating
them.

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:24: frame->m_securityContext =
RemoteSecurityContext::create();
Should we move this into the constructor?

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.h:34: virtual RemoteSecurityContext*
securityContext() const override;
Nit: move this to the Frame overrides: section.

https://codereview.chromium.org/520213002/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:852: void
WebRemoteFrameImpl::setReplicatedOrigin(const WebURL& origin, bool isUnique)
const
On 2014/11/11 23:44:25, alexmos wrote:
> I don't like that this is taking the isUnique flag separately, and this can't
> take a FrameReplicationState.  I'm thinking about changing this to take a
> WebSecurityOrigin instead, and the work done here to synthesize
> WebSecurityOrigin from a FrameReplicationState should happen in some other
> helper function in content.  Does that sounds plausible, and if so, where is a
> good place to add such a function?

We would probably do the conversion in content. There are a number of different
ways to do this. content::DropData/blink::WebDragData chose to expose a
DropDataBuilder to construct a DropData from WebDragData, and then a static
function in render_view_impl.cc to convert it back to the blink form.

I think it's probably fine to just embed these conversion helpers in
FrameReplicationState directly.

[dcheng, 2014-11-12 21:57:40]

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:31:
securityContext->setContentSecurityPolicy(csp);
Nit: I would just combine these two lines, since you don't need the intermediate
csp anyway.

[alexmos, 2014-11-18 18:35:18]

Daniel, PTAL.  In addition to addressing your comments, I've also added similar
plumbing to expose securityOrigin() from WebFrames, which I needed in my render
view test and which should also come in handy to refactor things like
frame()->document()->securityOrigin() to frame()->securityOrigin() at the Web
layer.

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:31:
securityContext->setContentSecurityPolicy(csp);
On 2014/11/12 21:57:40, dcheng wrote:
> Nit: I would just combine these two lines, since you don't need the
intermediate
> csp anyway.

Done.

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.h (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.h:12: class RemoteSecurityContext: public
SecurityContext, public RefCounted<RemoteSecurityContext> {
On 2014/11/12 21:55:50, dcheng wrote:
> Nit: space before :.

Done.

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.h:88: virtual SecurityContext* securityContext()
const override;
On 2014/11/12 21:55:50, dcheng wrote:
> Nit: group this into the Frame overrides: section without a newline separating
> them.

Done.

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:24: frame->m_securityContext =
RemoteSecurityContext::create();
On 2014/11/12 21:55:50, dcheng wrote:
> Should we move this into the constructor?

Yes, this will be cleaner.  Not sure if the the inline keyword should stay on
the constructor; I left it in there for now.

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/520213002/diff/100001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.h:34: virtual RemoteSecurityContext*
securityContext() const override;
On 2014/11/12 21:55:50, dcheng wrote:
> Nit: move this to the Frame overrides: section.

Done.

https://codereview.chromium.org/520213002/diff/100001/Source/web/WebRemoteFra...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/520213002/diff/100001/Source/web/WebRemoteFra...
Source/web/WebRemoteFrameImpl.cpp:852: void
WebRemoteFrameImpl::setReplicatedOrigin(const WebURL& origin, bool isUnique)
const
On 2014/11/12 21:55:50, dcheng wrote:
> On 2014/11/11 23:44:25, alexmos wrote:
> > I don't like that this is taking the isUnique flag separately, and this
can't
> > take a FrameReplicationState.  I'm thinking about changing this to take a
> > WebSecurityOrigin instead, and the work done here to synthesize
> > WebSecurityOrigin from a FrameReplicationState should happen in some other
> > helper function in content.  Does that sounds plausible, and if so, where is
a
> > good place to add such a function?
> 
> We would probably do the conversion in content. There are a number of
different
> ways to do this. content::DropData/blink::WebDragData chose to expose a
> DropDataBuilder to construct a DropData from WebDragData, and then a static
> function in render_view_impl.cc to convert it back to the blink form.
> 
> I think it's probably fine to just embed these conversion helpers in
> FrameReplicationState directly.

Thanks, I talked to Charlie also and ended up doing the conversion in a static
RenderFrameImpl::SetReplicatedState.

[dcheng, 2014-11-18 23:31:40]

https://codereview.chromium.org/520213002/diff/120001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/520213002/diff/120001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:30:
securityContext->setContentSecurityPolicy(ContentSecurityPolicy::create());
I'd probably move lines 23-30 to the constructor as well. The general pattern is
for create() to just be a very thin wrapper around the actual class constructor.

https://codereview.chromium.org/520213002/diff/120001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/520213002/diff/120001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:19: m_securityContext =
RemoteSecurityContext::create();
Nit: move this into the initializer list.

https://codereview.chromium.org/520213002/diff/120001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/520213002/diff/120001/public/web/WebFrame.h#n...
public/web/WebFrame.h:159: virtual WebSecurityOrigin securityOrigin() const = 0;
Do we need to call this on remote frames? If not, it might be better to start
with just making this available from WebLocalFrame.

[alexmos, 2014-11-19 00:33:04]

Thanks for the review!

https://codereview.chromium.org/520213002/diff/120001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/520213002/diff/120001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:30:
securityContext->setContentSecurityPolicy(ContentSecurityPolicy::create());
On 2014/11/18 23:31:40, dcheng wrote:
> I'd probably move lines 23-30 to the constructor as well. The general pattern
is
> for create() to just be a very thin wrapper around the actual class
constructor.

Done.

https://codereview.chromium.org/520213002/diff/120001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/520213002/diff/120001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:19: m_securityContext =
RemoteSecurityContext::create();
On 2014/11/18 23:31:40, dcheng wrote:
> Nit: move this into the initializer list.

Done.

https://codereview.chromium.org/520213002/diff/120001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/520213002/diff/120001/public/web/WebFrame.h#n...
public/web/WebFrame.h:159: virtual WebSecurityOrigin securityOrigin() const = 0;
On 2014/11/18 23:31:40, dcheng wrote:
> Do we need to call this on remote frames? If not, it might be better to start
> with just making this available from WebLocalFrame.

My RenderView test (see *.OriginReplicationForSwapOut in my other CL) needs
WebRemoteFrame::securityOrigin() to check that the replicated origin has been
set properly.

[dcheng, 2014-11-19 00:53:55]

https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.h:85: virtual WebSecurityOrigin securityOrigin()
const override;
Just curious: what are your thoughts on implementing this directly in
WebFrame.cpp? We have two different ways of doing it right now--add a virtual
method and an implementation for Local/Remote like this patch does, or use the
toCoreFrame() helper in WebFrame.cpp like WebFrame::detach() does.

[nasko, 2014-11-19 01:16:08]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2014-11-19 01:16:09]

Just a few drive-by nits.

https://codereview.chromium.org/520213002/diff/140001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/520213002/diff/140001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:25: // TODO(alexmos):
Document::initSecurityContext has a few other things we
nit: Blink uses FIXME without the username.

https://codereview.chromium.org/520213002/diff/140001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:37: // FIXME(alexmos): currently,
replicated security origins are passed only
nit: No need for "(alexmos)". Also s/currently/Currently/.

https://codereview.chromium.org/520213002/diff/140001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/520213002/diff/140001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.h:36: 
nit: no need for new empty line

[alexmos, 2014-11-19 18:19:19]

https://codereview.chromium.org/520213002/diff/140001/Source/core/dom/RemoteS...
File Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/520213002/diff/140001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:25: // TODO(alexmos):
Document::initSecurityContext has a few other things we
On 2014/11/19 01:16:09, nasko wrote:
> nit: Blink uses FIXME without the username.

Done.

https://codereview.chromium.org/520213002/diff/140001/Source/core/dom/RemoteS...
Source/core/dom/RemoteSecurityContext.cpp:37: // FIXME(alexmos): currently,
replicated security origins are passed only
On 2014/11/19 01:16:09, nasko wrote:
> nit: No need for "(alexmos)". Also s/currently/Currently/.

Done.

https://codereview.chromium.org/520213002/diff/140001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/520213002/diff/140001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.h:36: 
On 2014/11/19 01:16:09, nasko wrote:
> nit: no need for new empty line

Done.  Sneaked through as I was moving securityContext() around.

https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.h:85: virtual WebSecurityOrigin securityOrigin()
const override;
On 2014/11/19 00:53:55, dcheng wrote:
> Just curious: what are your thoughts on implementing this directly in
> WebFrame.cpp? We have two different ways of doing it right now--add a virtual
> method and an implementation for Local/Remote like this patch does, or use the
> toCoreFrame() helper in WebFrame.cpp like WebFrame::detach() does.

That would work too.  In that case, WebFrame::securityOrigin would just be
"return
WebSecurityOrigin(toCoreFrame(frame)->securityContext()->securityOrigin())".  I
actually like this more - makes the patch smaller and doesn't duplicate the
LocalFrame's redirect-to-document behavior at the Web layer.  Any other reasons
why one would prefer one approach over the other?

[dcheng, 2014-11-19 18:28:38]

https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.h (right):

https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.h:85: virtual WebSecurityOrigin securityOrigin()
const override;
On 2014/11/19 18:19:19, alexmos wrote:
> On 2014/11/19 00:53:55, dcheng wrote:
> > Just curious: what are your thoughts on implementing this directly in
> > WebFrame.cpp? We have two different ways of doing it right now--add a
virtual
> > method and an implementation for Local/Remote like this patch does, or use
the
> > toCoreFrame() helper in WebFrame.cpp like WebFrame::detach() does.
> 
> That would work too.  In that case, WebFrame::securityOrigin would just be
> "return
> WebSecurityOrigin(toCoreFrame(frame)->securityContext()->securityOrigin())". 
I
> actually like this more - makes the patch smaller and doesn't duplicate the
> LocalFrame's redirect-to-document behavior at the Web layer.  Any other
reasons
> why one would prefer one approach over the other?

toCoreFrame:
Advantages: less code duplication, smaller WebFrame vtable.
Disadvantages: we pay for two virtual dispatches, unless the compiler is really
clever with toCoreFrame inlining.

WebFrame virtual:
Advantages: only pay for one virtual dispatch (theoretically; we forgot to make
LocalFrame/RemoteFrame final, but if they were...)
Disadvantages: code duplication, bigger WebFrame vtable.

I don't see an overwhelming advantage for one over the other. I personally lean
towards toCoreFrame() because of less code duplication, but more important is
consistently picking one way or another.

[alexmos, 2014-11-19 19:50:12]

On 2014/11/19 18:28:38, dcheng wrote:
>
https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
> File Source/web/WebLocalFrameImpl.h (right):
> 
>
https://codereview.chromium.org/520213002/diff/140001/Source/web/WebLocalFram...
> Source/web/WebLocalFrameImpl.h:85: virtual WebSecurityOrigin securityOrigin()
> const override;
> On 2014/11/19 18:19:19, alexmos wrote:
> > On 2014/11/19 00:53:55, dcheng wrote:
> > > Just curious: what are your thoughts on implementing this directly in
> > > WebFrame.cpp? We have two different ways of doing it right now--add a
> virtual
> > > method and an implementation for Local/Remote like this patch does, or use
> the
> > > toCoreFrame() helper in WebFrame.cpp like WebFrame::detach() does.
> > 
> > That would work too.  In that case, WebFrame::securityOrigin would just be
> > "return
> > WebSecurityOrigin(toCoreFrame(frame)->securityContext()->securityOrigin())".

> I
> > actually like this more - makes the patch smaller and doesn't duplicate the
> > LocalFrame's redirect-to-document behavior at the Web layer.  Any other
> reasons
> > why one would prefer one approach over the other?
> 
> toCoreFrame:
> Advantages: less code duplication, smaller WebFrame vtable.
> Disadvantages: we pay for two virtual dispatches, unless the compiler is
really
> clever with toCoreFrame inlining.
> 
> WebFrame virtual:
> Advantages: only pay for one virtual dispatch (theoretically; we forgot to
make
> LocalFrame/RemoteFrame final, but if they were...)
> Disadvantages: code duplication, bigger WebFrame vtable.
> 
> I don't see an overwhelming advantage for one over the other. I personally
lean
> towards toCoreFrame() because of less code duplication, but more important is
> consistently picking one way or another.

OK, thanks for the explanation.  I switched to the toCoreFrame approach - please
see PS10.  Also, realized that I no longer need WebSecurityOrigin::createUnique
- removed in PS11.

[dcheng, 2014-11-19 22:13:40]

lgtm

[alexmos, 2014-11-20 00:45:19]

alexmos@chromium.org changed reviewers:
+ japhet@chromium.org

[alexmos, 2014-11-20 00:45:19]

Nate, can you please review this CL for Source/web and public/web OWNERS
approval?

[Nate Chapin, 2014-11-20 17:08:02]

lgtm

[alexmos, 2014-11-20 17:59:06]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2014-11-20 17:59:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/520213002/220001

[commit-bot: I haz the power, 2014-11-20 18:08:50]

Committed patchset #12 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185684