Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

BUG=611232
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Łukasz Anforowicz, 2016-07-28 23:32:34]

Description was changed from

==========
Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

BUG=611232
==========

to

==========
Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

BUG=611232
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-07-29 23:33:03]

lukasza@chromium.org changed reviewers:
+ mkwst@chromium.org

[Łukasz Anforowicz, 2016-07-29 23:33:04]

Mike, can you take a look please?

The browser->renderer path is here for the long-term.  The renderer->browser
path is needed only until we move frame-src validation to the browser process.

I wasn't sure how to cause a difference in behavior between
ContentSecurityPolicy::reportViolation invoked on a local vs remote frame. 
Making this method virtual + overriding it in RemoteContentSecurityPolicy works,
but I am certainly open to feedback and suggestions.

https://codereview.chromium.org/2190183002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/FlagExpectations/site-per-process (right):

https://codereview.chromium.org/2190183002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/FlagExpectations/site-per-process:67: #
https://crbug.com/611232 - CSP checks from remote parent frame do not properly
report violations
The only text diff at this point is line numbers on the console (which are
missing in case of OOPIFs).  This will be fixed by a follow-up CL at
https://codereview.chromium.org/2191143002/

[Mike West, 2016-08-02 07:42:33]

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

[commit-bot: I haz the power, 2016-08-02 07:42:44]

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-08-02 10:52:02]

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

[commit-bot: I haz the power, 2016-08-02 10:52:03]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-08-04 00:05:50]

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

[commit-bot: I haz the power, 2016-08-04 00:06:03]

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-08-04 02:00:51]

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

[commit-bot: I haz the power, 2016-08-04 02:00:52]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-08-05 22:46:05]

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

[Łukasz Anforowicz, 2016-08-05 22:46:06]

Alex, could you also take a look please?

[alexmos, 2016-08-09 18:01:20]

Thanks, this looks fine to me.  A few questions and nits below.

Re: how to initiate violation forwarding for remote frames, I can't think of
anything better than declaring a RemoteContextSecurityPolicy either.  At least
that separates out the remote parts into a separate class that will be easy to
clean up when we move the enforcement into the browser process.

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
What's the reason for this check?

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:565: IPC_STRUCT_TRAITS_MEMBER(header)
Does |report_endpoints| also need to be here?

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:824: // Reports Content Security Policy
violation from within the target frame.
I was a bit confused by what "from within the target frame" means.   Maybe just
unify with next sentence into something like "Reports a CSP violation that was
triggered in another process"?  Also, maybe explicitly mention frame-src, so one
can quickly get an idea of which directives this is for?

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:829: // so that *this* frame can raise
SecurityPolicyViolationEvent event (and do
nit: I'd remove the parens and simplify their contents to just "and finish
reporting the violation".

https://codereview.chromium.org/2190183002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5335:
GetWebFrame()->reportContentSecurityPolicyViolation(
nit: this is fine, but most other code seems to just use frame_ directly.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:54:
setContentSecurityPolicy(RemoteContentSecurityPolicy::create(m_remoteFrameClient));
It's unfortunate that we need to store m_remoteFrameClient here in addition to
remote CSP just for this, but the alternatives I'm thinking about are probably
worse, so this seems fine. :)

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.h (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.h:29:
RemoteSecurityContext(RemoteFrameClient*);
nit: explicit

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:890: if
(!shouldSendViolationReport(stringifiedReport))
I was thinking whether instead of forwarding all the violation params
separately, could we potentially simplify things by just sending the stringified
report?  But I guess if we eventually want to generate violations from the
browser process, your current plumbing is actually better.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h
(right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h:15: class
RemoteContentSecurityPolicy : public ContentSecurityPolicy {
Perhaps add a class-level comment explaining why this class is needed, and that
the plan is for it to eventually go away once the violations are caught in the
browser process.  (Is that right?)

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h:24:
RemoteContentSecurityPolicy(RemoteFrameClient*);
nit: explicit

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2195: nullptr, //
contextFrame
Looking at when reportViolation uses |contextFrame|, it seems it's used for
frame-ancestors.  So would we need to plumb this for the browser->renderer path
when the browser process starts enforcing frame-ancestors?

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2197: 0); // contextLine
nit: maybe a comment explaining why we don't have/want the line number? 
(Similar to the one you have in RemoteContentSecurityPolicy.cpp)

[Łukasz Anforowicz, 2016-08-09 18:36:59]

Description was changed from

==========
Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

BUG=611232
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Forward CSP violation reporting from RenderFrameProxy to RenderFrameImpl.

BUG=611232
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Łukasz Anforowicz, 2016-08-09 22:23:21]

Thanks Alex.  Can you take another look please?

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/09 18:01:19, alexmos wrote:
> What's the reason for this check?

Thanks for asking this question - I blindly copied this check from
RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making sure that
RFPH and RFH are in sync (and drops the message if RFH has navigated away).

Does the above make sense?  Let's chat to figure out what should be done here.

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:565: IPC_STRUCT_TRAITS_MEMBER(header)
On 2016/08/09 18:01:19, alexmos wrote:
> Does |report_endpoints| also need to be here?

Ooops.  Thanks for catching this.  Done.

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:824: // Reports Content Security Policy
violation from within the target frame.
On 2016/08/09 18:01:19, alexmos wrote:
> I was a bit confused by what "from within the target frame" means.   Maybe
just
> unify with next sentence into something like "Reports a CSP violation that was
> triggered in another process"?  Also, maybe explicitly mention frame-src, so
one
> can quickly get an idea of which directives this is for?

Done (I think).

https://codereview.chromium.org/2190183002/diff/80001/content/common/frame_me...
content/common/frame_messages.h:829: // so that *this* frame can raise
SecurityPolicyViolationEvent event (and do
On 2016/08/09 18:01:19, alexmos wrote:
> nit: I'd remove the parens and simplify their contents to just "and finish
> reporting the violation". 

Done.

https://codereview.chromium.org/2190183002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:5335:
GetWebFrame()->reportContentSecurityPolicyViolation(
On 2016/08/09 18:01:19, alexmos wrote:
> nit: this is fine, but most other code seems to just use frame_ directly.

Done.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:54:
setContentSecurityPolicy(RemoteContentSecurityPolicy::create(m_remoteFrameClient));
On 2016/08/09 18:01:19, alexmos wrote:
> It's unfortunate that we need to store m_remoteFrameClient here in addition to
> remote CSP just for this, but the alternatives I'm thinking about are probably
> worse, so this seems fine. :)

Acknowledged.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.h (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.h:29:
RemoteSecurityContext(RemoteFrameClient*);
On 2016/08/09 18:01:20, alexmos wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:890: if
(!shouldSendViolationReport(stringifiedReport))
On 2016/08/09 18:01:20, alexmos wrote:
> I was thinking whether instead of forwarding all the violation params
> separately, could we potentially simplify things by just sending the
stringified
> report?

I don't like duplication+boilerplate that comes with *separately* sending of all
the individual report pieces.  OTOH, I think that a stringified report is
insufficient for making the SecurityPolicyViolationEvent::create call from
ContentSecurityPolicy::reportViolation.  Additionally sending the individual
pieces separately, lets me reuse the existing
ContentSecurityPolicy::reportViolation method.

> But I guess if we eventually want to generate violations from the
> browser process, your current plumbing is actually better.

Yes - I hope that the current shape of the
FrameMsg_ReportContentSecurityPolicyViolation message can be used without any
changes for violations detected in the browser process.

OTOH, I am not sure if this is entirely true.  For example - |report_endpoints|
might be handled from the browser process (and so would not need to be part of
this message).

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h
(right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h:15: class
RemoteContentSecurityPolicy : public ContentSecurityPolicy {
On 2016/08/09 18:01:20, alexmos wrote:
> Perhaps add a class-level comment explaining why this class is needed, and
that
> the plan is for it to eventually go away once the violations are caught in the
> browser process.

Done.

> (Is that right?)

Yes - this class should go away once violations are caught in the browser
process.  I've added the removal as an explicit TODO tied to bug
https://crbug.com/376522 (which I assume is used as the tracking bug for moving
some CSP checks to the browser process).

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h:24:
RemoteContentSecurityPolicy(RemoteFrameClient*);
On 2016/08/09 18:01:20, alexmos wrote:
> nit: explicit

Done.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2195: nullptr, //
contextFrame
On 2016/08/09 18:01:20, alexmos wrote:
> Looking at when reportViolation uses |contextFrame|, it seems it's used for
> frame-ancestors.  So would we need to plumb this for the browser->renderer
path
> when the browser process starts enforcing frame-ancestors?

I am not sure.  I don't think so.  I am not sure how exactly |contextFrame| is
used in ContentSecurityPolicy::reportViolation (e.g. why and how it influences
|url| passed to PingLoader::sendViolationReport).  But... I think that instead
of passing |contextFrame|, we could just invoke
ContentSecurityPolicy::reportViolation associated with the right frame (so that
|this->document()| can always be used instead of having to do |contextFrame ?
contextFrame->document() : this->document()|).

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2197: 0); // contextLine
On 2016/08/09 18:01:20, alexmos wrote:
> nit: maybe a comment explaining why we don't have/want the line number? 
> (Similar to the one you have in RemoteContentSecurityPolicy.cpp)

Done.

[Łukasz Anforowicz, 2016-08-10 19:50:25]

I think I need to rebase + added some extra bits of knowledge to one of the open
issues below.

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> On 2016/08/09 18:01:19, alexmos wrote:
> > What's the reason for this check?
> 
> Thanks for asking this question - I blindly copied this check from
> RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making sure
that
> RFPH and RFH are in sync (and drops the message if RFH has navigated away).
> 
> Does the above make sense?  Let's chat to figure out what should be done here.

FWIW, I checked that the same condition in RenderFrameProxyHost::OnOpenURL has
been introduced in https://codereview.chromium.org/1715993002 (by alexmos@) - it
replaces a TODO(creis) that also points at NavigatorImpl::RequestOpenURL.

[alexmos, 2016-08-10 20:34:32]

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

[alexmos, 2016-08-10 20:34:33]

Adding Charlie for discussion about the BrowsingInstance check below.

Other than that, LGTM.  Thanks for taking care of this!

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/10 19:50:25, Łukasz Anforowicz wrote:
> On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> > On 2016/08/09 18:01:19, alexmos wrote:
> > > What's the reason for this check?
> > 
> > Thanks for asking this question - I blindly copied this check from
> > RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making sure
> that
> > RFPH and RFH are in sync (and drops the message if RFH has navigated away).
> > 
> > Does the above make sense?  Let's chat to figure out what should be done
here.
> 
> FWIW, I checked that the same condition in RenderFrameProxyHost::OnOpenURL has
> been introduced in https://codereview.chromium.org/1715993002 (by alexmos@) -
it
> replaces a TODO(creis) that also points at NavigatorImpl::RequestOpenURL.

Yes, I moved that check according to Charlie's TODO.  The actual check traces
back to Charlie's https://chromiumcodereview.appspot.com/10350013 and
http://crbug.com/126174.  

Two thoughts about this.  Currently, this will only be used to forward
violations to the parent frame (right?).  If the parent commits a navigation to
another BrowsingInstance, that should cause the children to be deleted at that
point, which should currently cause any subsequent messages from them to be
dropped, but that might change once we implement pending delete FTNs
(http://crbug.com/609963).  So I think it's ok to keep this check here, just to
ensure that we won't try to report a violation to a WebUI page in a race, etc.

Second, if this ever gets used to also report the violation in a popup where the
CSP lives in the opener, then it seems we'd also need this check for scenarios
like the one in http://crbug.com/126174.  So, another reason to keep it.

Charlie, does that sound about right?  Any thoughts about whether we need this
check here?

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:890: if
(!shouldSendViolationReport(stringifiedReport))
On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> On 2016/08/09 18:01:20, alexmos wrote:
> > I was thinking whether instead of forwarding all the violation params
> > separately, could we potentially simplify things by just sending the
> stringified
> > report?
> 
> I don't like duplication+boilerplate that comes with *separately* sending of
all
> the individual report pieces.  OTOH, I think that a stringified report is
> insufficient for making the SecurityPolicyViolationEvent::create call from
> ContentSecurityPolicy::reportViolation.  Additionally sending the individual
> pieces separately, lets me reuse the existing
> ContentSecurityPolicy::reportViolation method.
> 
> > But I guess if we eventually want to generate violations from the
> > browser process, your current plumbing is actually better.
> 
> Yes - I hope that the current shape of the
> FrameMsg_ReportContentSecurityPolicyViolation message can be used without any
> changes for violations detected in the browser process.
> 
> OTOH, I am not sure if this is entirely true.  For example -
|report_endpoints|
> might be handled from the browser process (and so would not need to be part of
> this message).

Acknowledged.

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h
(right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/RemoteContentSecurityPolicy.h:15: class
RemoteContentSecurityPolicy : public ContentSecurityPolicy {
On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> On 2016/08/09 18:01:20, alexmos wrote:
> > Perhaps add a class-level comment explaining why this class is needed, and
> that
> > the plan is for it to eventually go away once the violations are caught in
the
> > browser process.
> 
> Done.
> 
> > (Is that right?)
> 
> Yes - this class should go away once violations are caught in the browser
> process.  I've added the removal as an explicit TODO tied to bug
> https://crbug.com/376522 (which I assume is used as the tracking bug for
moving
> some CSP checks to the browser process).
> 

The new comment is awesome.  Thanks for adding!

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2190183002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2195: nullptr, //
contextFrame
On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> On 2016/08/09 18:01:20, alexmos wrote:
> > Looking at when reportViolation uses |contextFrame|, it seems it's used for
> > frame-ancestors.  So would we need to plumb this for the browser->renderer
> path
> > when the browser process starts enforcing frame-ancestors?
> 
> I am not sure.  I don't think so.  I am not sure how exactly |contextFrame| is
> used in ContentSecurityPolicy::reportViolation (e.g. why and how it influences
> |url| passed to PingLoader::sendViolationReport).  But... I think that instead
> of passing |contextFrame|, we could just invoke
> ContentSecurityPolicy::reportViolation associated with the right frame (so
that
> |this->document()| can always be used instead of having to do |contextFrame ?
> contextFrame->document() : this->document()|).

Acknowledged.

[Łukasz Anforowicz, 2016-08-11 17:32:36]

Charlie, I've added some more thoughts on the remaining open issue below.

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/10 20:34:33, alexmos wrote:
> On 2016/08/10 19:50:25, Łukasz Anforowicz wrote:
> > On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> > > On 2016/08/09 18:01:19, alexmos wrote:
> > > > What's the reason for this check?
> > > 
> > > Thanks for asking this question - I blindly copied this check from
> > > RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making sure
> > that
> > > RFPH and RFH are in sync (and drops the message if RFH has navigated
away).
> > > 
> > > Does the above make sense?  Let's chat to figure out what should be done
> here.
> > 
> > FWIW, I checked that the same condition in RenderFrameProxyHost::OnOpenURL
has
> > been introduced in https://codereview.chromium.org/1715993002 (by alexmos@)
-
> it
> > replaces a TODO(creis) that also points at NavigatorImpl::RequestOpenURL.
> 
> Yes, I moved that check according to Charlie's TODO.  The actual check traces
> back to Charlie's https://chromiumcodereview.appspot.com/10350013 and
> http://crbug.com/126174.  
> 
> Two thoughts about this.  Currently, this will only be used to forward
> violations to the parent frame (right?).  If the parent commits a navigation
to
> another BrowsingInstance, that should cause the children to be deleted at that
> point, which should currently cause any subsequent messages from them to be
> dropped, but that might change once we implement pending delete FTNs
> (http://crbug.com/609963).  So I think it's ok to keep this check here, just
to
> ensure that we won't try to report a violation to a WebUI page in a race, etc.
> 
> Second, if this ever gets used to also report the violation in a popup where
the
> CSP lives in the opener, then it seems we'd also need this check for scenarios
> like the one in http://crbug.com/126174.  So, another reason to keep it.
> 
> Charlie, does that sound about right?  Any thoughts about whether we need this
> check here?

Hmmm... actually, I think that the check above might be insufficient. 
Navigation can succeed as long as RFH is in the same site instance -so this kind
of check works okay for RenderFrameProxyHost::OnOpenURL.  OTOH, we probably
should NOT report a CSP violation if a frame has been navigated to another
origin (while racing with the CSP violation message from RFP).  I am not sure
what is the best way for enforcing this - maybe the IPC can include
|csp_self_origin| and here we can verify if it is still the same as RFH's
origin?  WDYT?

FWIW, I looked at other RenderFrameProxyHost::On* methods and most seem not to
care about the current origin of RFH - I think the concern raised here only
applies to the new IPC.

[Łukasz Anforowicz, 2016-08-11 18:04:35]

+estark@ to CC as FYI

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/11 17:32:36, Łukasz Anforowicz wrote:
> On 2016/08/10 20:34:33, alexmos wrote:
> > On 2016/08/10 19:50:25, Łukasz Anforowicz wrote:
> > > On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> > > > On 2016/08/09 18:01:19, alexmos wrote:
> > > > > What's the reason for this check?
> > > > 
> > > > Thanks for asking this question - I blindly copied this check from
> > > > RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making
sure
> > > that
> > > > RFPH and RFH are in sync (and drops the message if RFH has navigated
> away).
> > > > 
> > > > Does the above make sense?  Let's chat to figure out what should be done
> > here.
> > > 
> > > FWIW, I checked that the same condition in RenderFrameProxyHost::OnOpenURL
> has
> > > been introduced in https://codereview.chromium.org/1715993002 (by
alexmos@)
> -
> > it
> > > replaces a TODO(creis) that also points at NavigatorImpl::RequestOpenURL.
> > 
> > Yes, I moved that check according to Charlie's TODO.  The actual check
traces
> > back to Charlie's https://chromiumcodereview.appspot.com/10350013 and
> > http://crbug.com/126174.  
> > 
> > Two thoughts about this.  Currently, this will only be used to forward
> > violations to the parent frame (right?).  If the parent commits a navigation
> to
> > another BrowsingInstance, that should cause the children to be deleted at
that
> > point, which should currently cause any subsequent messages from them to be
> > dropped, but that might change once we implement pending delete FTNs
> > (http://crbug.com/609963).  So I think it's ok to keep this check here, just
> to
> > ensure that we won't try to report a violation to a WebUI page in a race,
etc.
> > 
> > Second, if this ever gets used to also report the violation in a popup where
> the
> > CSP lives in the opener, then it seems we'd also need this check for
scenarios
> > like the one in http://crbug.com/126174.  So, another reason to keep it.
> > 
> > Charlie, does that sound about right?  Any thoughts about whether we need
this
> > check here?
> 
> Hmmm... actually, I think that the check above might be insufficient. 
> Navigation can succeed as long as RFH is in the same site instance -so this
kind
> of check works okay for RenderFrameProxyHost::OnOpenURL.  OTOH, we probably
> should NOT report a CSP violation if a frame has been navigated to another
> origin (while racing with the CSP violation message from RFP).  I am not sure
> what is the best way for enforcing this - maybe the IPC can include
> |csp_self_origin| and here we can verify if it is still the same as RFH's
> origin?  WDYT?
> 
> FWIW, I looked at other RenderFrameProxyHost::On* methods and most seem not to
> care about the current origin of RFH - I think the concern raised here only
> applies to the new IPC.

I talked with Alex about this a little bit more and:

- Navigating a frame should delete all child frames, so the concern about
reporting a different origin's CSP violation in a wrong frame doesn't apply to
this scenario (this scenario = CSP violation detected via remote parent).

- OTOH, the general concern remains in case RemoteContentSecurityPolicy is not
associated with a parent frame, but (for example) with "opener".  I am not sure
if such scenario is possible today, but I have a feeling that estark@ wants to
depend on proper behavior in this scenario when fixing https://crbug.com/630332.

[Charlie Reis, 2016-08-11 18:32:15]

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/11 17:32:36, Łukasz Anforowicz wrote:
> On 2016/08/10 20:34:33, alexmos wrote:
> > On 2016/08/10 19:50:25, Łukasz Anforowicz wrote:
> > > On 2016/08/09 22:23:20, Łukasz Anforowicz wrote:
> > > > On 2016/08/09 18:01:19, alexmos wrote:
> > > > > What's the reason for this check?
> > > > 
> > > > Thanks for asking this question - I blindly copied this check from
> > > > RenderFrameProxyHost::OnOpenURL.  I assumed that this check is making
sure
> > > that
> > > > RFPH and RFH are in sync (and drops the message if RFH has navigated
> away).
> > > > 
> > > > Does the above make sense?  Let's chat to figure out what should be done
> > here.
> > > 
> > > FWIW, I checked that the same condition in RenderFrameProxyHost::OnOpenURL
> has
> > > been introduced in https://codereview.chromium.org/1715993002 (by
alexmos@)
> -
> > it
> > > replaces a TODO(creis) that also points at NavigatorImpl::RequestOpenURL.
> > 
> > Yes, I moved that check according to Charlie's TODO.  The actual check
traces
> > back to Charlie's https://chromiumcodereview.appspot.com/10350013 and
> > http://crbug.com/126174.  
> > 
> > Two thoughts about this.  Currently, this will only be used to forward
> > violations to the parent frame (right?).  If the parent commits a navigation
> to
> > another BrowsingInstance, that should cause the children to be deleted at
that
> > point, which should currently cause any subsequent messages from them to be
> > dropped, but that might change once we implement pending delete FTNs
> > (http://crbug.com/609963).  So I think it's ok to keep this check here, just
> to
> > ensure that we won't try to report a violation to a WebUI page in a race,
etc.
> > 
> > Second, if this ever gets used to also report the violation in a popup where
> the
> > CSP lives in the opener, then it seems we'd also need this check for
scenarios
> > like the one in http://crbug.com/126174.  So, another reason to keep it.
> > 
> > Charlie, does that sound about right?  Any thoughts about whether we need
this
> > check here?


Sorry for chiming in late.  I'll reply to Alex's question first.

The check in RFPH::OnOpenURL is there so that a frame can't initiate a
navigation in a frame from a different BrowsingInstance.  (Note that this is
different from whether it's cross-origin or not.  We only swap BrowsingInstances
when we're doing a complete privilege level change, like the user typing in
chrome://settings after being on a web page.)

I don't think we can ever have frames from different BrowsingInstances in the
same frame tree, but this matters in the case of a popup/opener where one gets
navigated to a WebUI URL, etc.  We basically sever postMessage, navigation, and
other script relationships at that point.

It's not clear to me whether CSP reporting needs to follow this model.  If it
only happens for frames within the same frame tree, it's probably moot since
we'll never see frames from a different BrowsingInstance.  If it's possible that
we could forward a report across tabs, then it becomes a more interesting
question-- can a CSP policy affect a popup tab after it has left the
BrowsingInstance?  (Actually, can CSP policies affect popups at all, or are
there plans for that?)

I would imagine that CSP would stop applying if the user navigated the tab to a
different BrowsingInstance, in which case we wouldn't need to forward the
violation.


> 
> Hmmm... actually, I think that the check above might be insufficient. 
> Navigation can succeed as long as RFH is in the same site instance -so this
kind
> of check works okay for RenderFrameProxyHost::OnOpenURL.

Not sure I follow this part.  Navigation can work across SiteInstances as well,
as long as we're in the same BrowsingInstance.  That's what RFPH::OpenURL is
for.

>  OTOH, we probably
> should NOT report a CSP violation if a frame has been navigated to another
> origin (while racing with the CSP violation message from RFP).  I am not sure
> what is the best way for enforcing this - maybe the IPC can include
> |csp_self_origin| and here we can verify if it is still the same as RFH's
> origin?  WDYT?

I do see the concern here, and it's related to Alex's pending delete FTN
question.  I agree that we don't want to deliver a report to a RFH just after
we've left the page it was intended for.

I *think* it's the case that we don't have this problem today.  For same-site
navigations, the parent RFH will detach its children before committing the new
page, so the FTN will be gone.  For cross-site navigations, we clear the
children FTNs when swapping.  In both cases, no one will be listening on the
browser side when the report IPC arrives.  Correct me if I'm wrong.

If we add pending delete FTNs, we have to worry about this.  Nick and I have
been chatting about an alternative to pending delete FTNs, though, which might
allow us to avoid races like this.  (I won't go into it in depth here, but it
basically involves a Mojo channel to NavigationController for PageState, and
ignoring most other things during unload.)  I'm inclined to say we should post
this concern to bug 609963 and not worry about it here for now.

Just for a sanity check, though, is it the case that this message only goes from
a child to its parent, or are there other cases it could be sent?



> 
> FWIW, I looked at other RenderFrameProxyHost::On* methods and most seem not to
> care about the current origin of RFH - I think the concern raised here only
> applies to the new IPC.

[Łukasz Anforowicz, 2016-08-11 19:51:35]

Charlie, can you take a look at the latest patchset?  (for the question/concern
we are discussing below + for overall //content OWNERS review).

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/11 18:32:14, Charlie Reis wrote:
> Just for a sanity check, though, is it the case that this message only goes
from
> a child to its parent, or are there other cases it could be sent?

Today sending of this message is only triggered when a child violates policy of
a (remote) parent.  But... in the future the message might be sent in other
cases - specifically, https://crbug.com/630332 talks about "form-action" CSP
directive.  In case when form element targets another window by name (*) the CSP
to verify can come from an arbitrary remote frame.

So - in the latest patchset I am trying to compare origins, to make sure they
are still the same.  One thing that worries me is the "LastCommitted" part of
GetLastCommittedOrigin, but hopefully this is still correct.  Can you please
take a look?

(*) LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
shows how form.target attribute can be used to target another frame.  I assume
that this attribute can also be used to target another window.

[Charlie Reis, 2016-08-11 20:40:26]

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:278: if
(!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
On 2016/08/11 19:51:35, Łukasz Anforowicz wrote:
> On 2016/08/11 18:32:14, Charlie Reis wrote:
> > Just for a sanity check, though, is it the case that this message only goes
> from
> > a child to its parent, or are there other cases it could be sent?
> 
> Today sending of this message is only triggered when a child violates policy
of
> a (remote) parent.  But... in the future the message might be sent in other
> cases - specifically, https://crbug.com/630332 talks about "form-action" CSP
> directive.  In case when form element targets another window by name (*) the
CSP
> to verify can come from an arbitrary remote frame.
> 
> So - in the latest patchset I am trying to compare origins, to make sure they
> are still the same.  One thing that worries me is the "LastCommitted" part of
> GetLastCommittedOrigin, but hopefully this is still correct.  Can you please
> take a look?
> 
> (*) LayoutTests/http/tests/navigation/form-targets-cross-site-frame-post.html
> shows how form.target attribute can be used to target another frame.  I assume
> that this attribute can also be used to target another window.

Hmm, I have some concerns about that, and no solutions yet.  I'll leave comments
on that patch set.

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:278: // Verify that the
CSP violation will be reported in the same frame
"same frame": Did you mean to say "same document?"  (And I agree with the
parenthetical that we're falling back to origin, since same document is hard.)

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:284:
current_rfh->GetLastCommittedOrigin()))
I'm wondering if there's something better we can do here, because I'm not very
happy with treating the origin as a proxy for whether we've navigated.  It seems
like there's lots of ways this could go wrong:

1) The origin doesn't change, but we've committed a different page where the CSP
policy doesn't apply anymore.
2) The renderer lies about the origin.  (Not sure if there's a viable attack in
this case, but we have to be careful anytime we hear an origin claim from the
renderer.)
3) We have to be careful that current_rfh's origin is always correct, since
there are cases where we clear some of its navigation state (e.g., after an
ignored navigation).

From one perspective, the document sequence number is a better indication of
whether we've moved to a different page, but the renderer process won't know the
DSN of a remote frame (and I'm pretty sure we don't want to add that).

Another not perfect idea: the renderer could send up the CSP policy it was using
when it noticed the violation, and we can compare it against what the browser
has in the current replication state for the frame?  We would notice if they
differ, which would help with case (1) where the origin doesn't change. 
However, that doesn't help if you go to a different page with the same CSP
policy, which would erroneously get an error.

I'm not sure how else to tell that we've left the page that had the CSP policy.

[Łukasz Anforowicz, 2016-08-12 18:55:04]

Charlie, can you take another look?

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:278: // Verify that the
CSP violation will be reported in the same frame
On 2016/08/11 20:40:26, Charlie Reis (OOO soon) wrote:
> "same frame": Did you mean to say "same document?"  (And I agree with the
> parenthetical that we're falling back to origin, since same document is hard.)

Good point.  Done.

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:284:
current_rfh->GetLastCommittedOrigin()))
On 2016/08/11 20:40:26, Charlie Reis (OOO soon) wrote:
> I'm wondering if there's something better we can do here, because I'm not very
> happy with treating the origin as a proxy for whether we've navigated.  It
seems
> like there's lots of ways this could go wrong:
> 
> 1) The origin doesn't change, but we've committed a different page where the
CSP
> policy doesn't apply anymore.

Ack.  Things are a bit improved by your suggestion to cross-check CSP contents.

> 2) The renderer lies about the origin.  (Not sure if there's a viable attack
in
> this case, but we have to be careful anytime we hear an origin claim from the
> renderer.)

If the renderer lies, then we will ignore the IPC message.  If the renderer
sends a correct origin, then it is indistinguishable from a normal CSP violation
report - OTOH, your remark got me thinking more about potential abuse of the new
renderer->browser message (i.e. thinking about the impact of fake CSP violation
reports a compromised/hostile renderer can send):

1. Since the new message contains |report_endpoints|, a hostile renderer can
start sending CSP reports to endpoints controlled by an attacker.  This can be
seen as a way to bypass CSRF protection (because the reports will be sent with
the Origin header coming from the frame the report gets forwarded to).  If you
are curious to learn more, you can start looking at code around
PingLoader::sendViolationReport.  

Hmmm... I am not sure I understand the threat of CSRF - an attacker can simply
use telnet and send a fake Origin header, instead of coercing the browser to do
it.  I am probably missing something - can you help me learn?

I am comparing against the actual list of report endpoints in patchset #10 -
this at least means that a hostile renderer doesn't control the endpoints
(although it can still spam/forge reports send to the real endpoints).

2. Ability to spam cross-origin console is not a new threat AFAICT - there is
already a pair of FrameHostMsg_AddMessageToConsole and
FrameMsg_AddMessageToConsole.

3. A hostile renderer can raise "securitypolicyviolation" cross-origin.

I am slightly fixing #1 in the patchset #10, but I am not sure if we can protect
against #2 and #3 without moving CSP checks to the browser.

> 3) We have to be careful that current_rfh's origin is always correct, since
> there are cases where we clear some of its navigation state (e.g., after an
> ignored navigation).
> 
Let's follow-up on this in
https://codereview.chromium.org/2190183002/patch/160001/170004

> From one perspective, the document sequence number is a better indication of
> whether we've moved to a different page, but the renderer process won't know
the
> DSN of a remote frame (and I'm pretty sure we don't want to add that).

Ack.

> Another not perfect idea: the renderer could send up the CSP policy it was
using
> when it noticed the violation, and we can compare it against what the browser
> has in the current replication state for the frame?  We would notice if they
> differ, which would help with case (1) where the origin doesn't change. 
> However, that doesn't help if you go to a different page with the same CSP
> policy, which would erroneously get an error.

Okay - I am doing this in patchset #9.  The IPC already contains the CSP header
that is violated - we just need to check if this header value is still present
in FrameReplicationState (this gets reset when navigating the frame - see how
NavigatorImpl::DidNavigate calls FrameTreeNode::ResetContentSecurityPolicy).

Is this better (or maybe good enough), or is the direction of this CL doomed?

> I'm not sure how else to tell that we've left the page that had the CSP
policy.

Yes, it seems to be a difficult problem.


I could possibly get rid of the checks if only I could guarantee that the
forwarding is only happening from child to parent (AFAIK this is true today). 
Unfortunately, when ContentSecurityPolicy::reportViolation is called, it doesn't
know that it was checking something for a child.

https://codereview.chromium.org/2190183002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:278:
current_rfh->GetLastCommittedOrigin())) {
creis@ said in another comment:

> 3) We have to be careful that current_rfh's origin is always correct, since
> there are cases where we clear some of its navigation state (e.g., after an
> ignored navigation).

Can you tell me more please?  Is the usage of GetLastCommittedOrigin wrong
above?

[Charlie Reis, 2016-08-12 20:47:30]

A few responses below.  (Caveat: I haven't reviewed the new patch; just
responded to questions.)

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:284:
current_rfh->GetLastCommittedOrigin()))
On 2016/08/12 18:55:04, Łukasz Anforowicz wrote:
> On 2016/08/11 20:40:26, Charlie Reis (OOO soon) wrote:
> > I'm wondering if there's something better we can do here, because I'm not
very
> > happy with treating the origin as a proxy for whether we've navigated.  It
> seems
> > like there's lots of ways this could go wrong:
> > 
> > 1) The origin doesn't change, but we've committed a different page where the
> CSP
> > policy doesn't apply anymore.
> 
> Ack.  Things are a bit improved by your suggestion to cross-check CSP
contents.
> 
> > 2) The renderer lies about the origin.  (Not sure if there's a viable attack
> in
> > this case, but we have to be careful anytime we hear an origin claim from
the
> > renderer.)
> 
> If the renderer lies, then we will ignore the IPC message.  If the renderer
> sends a correct origin, then it is indistinguishable from a normal CSP
violation
> report - OTOH, your remark got me thinking more about potential abuse of the
new
> renderer->browser message (i.e. thinking about the impact of fake CSP
violation
> reports a compromised/hostile renderer can send):
> 
> 1. Since the new message contains |report_endpoints|, a hostile renderer can
> start sending CSP reports to endpoints controlled by an attacker.  This can be
> seen as a way to bypass CSRF protection (because the reports will be sent with
> the Origin header coming from the frame the report gets forwarded to).  If you
> are curious to learn more, you can start looking at code around
> PingLoader::sendViolationReport.  

I don't see any comments there, so I'll avoid digging deeper at the moment.  :)

> 
> Hmmm... I am not sure I understand the threat of CSRF - an attacker can simply
> use telnet and send a fake Origin header, instead of coercing the browser to
do
> it.  I am probably missing something - can you help me learn?

Sure.  CSRF attacks are a threat when the attacker can get a victim's machine to
make the request.  Attackers can't make the requests themselves (e.g., with
telnet) because they don't have the victim's cookies on their machine.  When the
victim's browser sends the POST submission, it carries the victim's cookies (or
other ambient authority, like intranet access).

I'm not up to speed on CSP reports being used for CSRF, but I suppose the
attacker could make up an endpoint like victim.com/do_state_change?  (I'd be
surprised if the CSP report that the browser sends could be interpreted as a
valid command other than reporting a CSP error, but maybe there are cases where
that's a problem.)

> I am comparing against the actual list of report endpoints in patchset #10 -
> this at least means that a hostile renderer doesn't control the endpoints
> (although it can still spam/forge reports send to the real endpoints).
> 
> 2. Ability to spam cross-origin console is not a new threat AFAICT - there is
> already a pair of FrameHostMsg_AddMessageToConsole and
> FrameMsg_AddMessageToConsole.
> 
> 3. A hostile renderer can raise "securitypolicyviolation" cross-origin.
> 
> I am slightly fixing #1 in the patchset #10, but I am not sure if we can
protect
> against #2 and #3 without moving CSP checks to the browser.
> 
> > 3) We have to be careful that current_rfh's origin is always correct, since
> > there are cases where we clear some of its navigation state (e.g., after an
> > ignored navigation).
> > 
> Let's follow-up on this in
> https://codereview.chromium.org/2190183002/patch/160001/170004
> 
> > From one perspective, the document sequence number is a better indication of
> > whether we've moved to a different page, but the renderer process won't know
> the
> > DSN of a remote frame (and I'm pretty sure we don't want to add that).
> 
> Ack.
> 
> > Another not perfect idea: the renderer could send up the CSP policy it was
> using
> > when it noticed the violation, and we can compare it against what the
browser
> > has in the current replication state for the frame?  We would notice if they
> > differ, which would help with case (1) where the origin doesn't change. 
> > However, that doesn't help if you go to a different page with the same CSP
> > policy, which would erroneously get an error.
> 
> Okay - I am doing this in patchset #9.  The IPC already contains the CSP
header
> that is violated - we just need to check if this header value is still present
> in FrameReplicationState (this gets reset when navigating the frame - see how
> NavigatorImpl::DidNavigate calls FrameTreeNode::ResetContentSecurityPolicy).
> 
> Is this better (or maybe good enough), or is the direction of this CL doomed?

I suppose it's good enough if we have to defend against this.  I'm just worried
about it being interpreted as a pattern for others to follow when they're
concerned about skipping an action when the frame has been navigated away. 
Maybe we can add some clear comments about what policy we'd like to have, and
why this approach is a reasonable (and necessary) tradeoff in this case.

> > I'm not sure how else to tell that we've left the page that had the CSP
> policy.
> 
> Yes, it seems to be a difficult problem.
> 
> 
> I could possibly get rid of the checks if only I could guarantee that the
> forwarding is only happening from child to parent (AFAIK this is true today). 
> Unfortunately, when ContentSecurityPolicy::reportViolation is called, it
doesn't
> know that it was checking something for a child.

I would love to leave it out if we can confirm the reports are limited to the
parent/child case.  I've emailed estark@ to see if we can confirm that.

Otherwise, I think the comments I mention above might be sufficient.

https://codereview.chromium.org/2190183002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/2190183002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:277: if
(!origin_declaring_violated_csp.IsSameOriginWith(
This might be a problem if the origin of the page is unique.  IsSameOriginWith
returns false in that case, even if the page hasn't navigated away.

(I have had a terrible time dealing with this in IsURLInPageNavigation, and I've
got a CL trying to clean that case up further in
https://codereview.chromium.org/2050423002/.)

https://codereview.chromium.org/2190183002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:278:
current_rfh->GetLastCommittedOrigin())) {
On 2016/08/12 18:55:04, Łukasz Anforowicz wrote:
> creis@ said in another comment:
> 
> > 3) We have to be careful that current_rfh's origin is always correct, since
> > there are cases where we clear some of its navigation state (e.g., after an
> > ignored navigation).
> 
> Can you tell me more please?  Is the usage of GetLastCommittedOrigin wrong
> above?
> 
> 

Here's some of the navigation state gotchas I was referring to:

RFHI::last_committed_url_ gets cleared if the process dies, and it is sometimes
set to a cross-site (illegal) URL in the case of a network error, where the page
doesn't actually commit.

RFHI::last_successful_url_ is similar but doesn't get updated for network
errors, allowing us to account for the network error problem above.

RFHI::nav_entry_id_ is set to the NavigationEntry that this RFH is currently
showing, unless its most recent navigation was ignored by NavigationController
(in which case it gets reset).

GetLastCommittedOrigin's biggest wrinkle seems to be that it fails if you don't
call it on the current RFH, but that's not a problem here.  

As far as I can tell, it should be safe to use here, but all this might explain
why I was nervous at first glance.  :)