Issue 1957783002: Replicate Content-Security-Policy into remote frame proxies.

Łukasz Anforowicz

Description was changed from ========== Replicate Content-Security-Policy into remote frame proxies. After this CL, when ...

4 years, 7 months ago (2016-05-06 23:18:45 UTC) #1

Description was changed from

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RemoteFrameProxies.
RemoteFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.

BUG=585501
==========

to

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RemoteFrameProxies.
RemoteFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

Łukasz Anforowicz

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

4 years, 7 months ago (2016-05-09 04:25:39 UTC) #2

Łukasz Anforowicz

Mike, could you take a look please? The goal of this CL is to make ...

4 years, 7 months ago (2016-05-09 04:25:40 UTC) #3

Mike, could you take a look please?

The goal of this CL is to make --isolate-extensions mode of site isolation
shippable to beta and stable channels.  Until now, frame-src and child-src CSP
checks were effectively disabled when they were coming from a remote parent
frame.  After this CL, we replicate the CSP headers into RemoteSecurityContext,
so the child frames get blocked if they navigate to a location forbidden by
remote parent's CSP directives.

I think that this CL is pretty compatible with the long-term goal of moving CSP
checks to the browser.  In particular - I assume that even in the long-term, we
would parse html in the renderer and would have to therefore report CSP policies
from <meta> elements back to the browser process - this could be accomplished
with some of the IPCs and plumbing introduced by the current CL.

OTOH, parts of this CL would need to be thrown away in the long-term.  I think
that (unlike, for example, data:-scheme based subresources) child-src and
frame-src locations always go through and can be vetted by the browser and
therefore replicating these CSP headers to the renderer might not make sense in
the long-term. 

This CL plugs an important hole in CSP <-> OOPIFs interaction, but please note
that even after this CL some things are not working - even though a CSP
violation blocks a navigation, in case of a child-src or frame-src enforced from
a remote frame there will be:
- no console message
- no event fired
- no JSON report sent to the report endpoint
Let's focus on making the current CL landable to unblock further
--isolate-extensions trials, but we need to also discuss how to fix these
remaining issues - by continuing down the ad-hoc replication path VS by focusing
on the long-term goal of moving CSP checks to the browser process (I vote for
the latter).

PS. Hmmm... some trybots turned purple... let me try to kick them off again and
hopefully they'll turn green.  The one red bot (mac_blink_dbg) seems unrelated
to this CL - it'll also hopefully turn green when retrying...

Mike West

This LGTM % a few nits. I think the ductwork is pretty reasonable, and as ...

4 years, 7 months ago (2016-05-11 06:21:05 UTC) #4

alexmos

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

4 years, 7 months ago (2016-05-11 19:46:40 UTC) #5

alexmos

(I'm also taking a look at this after discussing some of it with Lukasz offline.) ...

4 years, 7 months ago (2016-05-11 19:46:41 UTC) #6

alexmos

Also, nit in CL description: RemoteFrameProxies -> RenderFrameProxies or RemoteFrames

4 years, 7 months ago (2016-05-11 19:49:01 UTC) #7

dcheng

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

4 years, 7 months ago (2016-05-11 20:15:06 UTC) #8

dcheng

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right): https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp#newcode40 third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:40: if (getSecurityOrigin()) Is this ever actually null? it seems ...

4 years, 7 months ago (2016-05-11 20:15:07 UTC) #9

Łukasz Anforowicz

Description was changed from ========== Replicate Content-Security-Policy into remote frame proxies. After this CL, when ...

4 years, 7 months ago (2016-05-11 21:59:36 UTC) #10

Description was changed from

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RemoteFrameProxies.
RemoteFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RenderFrameProxies.
RenderFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

Łukasz Anforowicz

Thank you for reviewing. I've tried to address most of the feedback, but I still ...

4 years, 7 months ago (2016-05-11 23:14:49 UTC) #11

Thank you for reviewing.  I've tried to address most of the feedback, but I
still need to

1) work on the browser test suggested by Alex
2) grok and reply to Alex's question [1] about handling of http headers when
dealing with a pending, but not yet committed navigation.  I hope that this can
be to a large extent addressed by authoring the browser tests.

[1]
https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:942: for (const auto&
pair : proxy_hosts_) {
On 2016/05/11 19:46:40, alexmos wrote:
> Should we return early if AreCrossProcessFramesPossible() is false, similarly
to
> functions above? (It would only be sending updates to popups/openers in other
> FrameTrees in that case, which don't need CSP replication.)  

Done.  I've also added this to RenderFrameImpl::didAddContentSecurityPolicy to
avoid unnecessary IPC when there are no RenderFrameProxies possible.
> 
> This is minor though, as AreCrossProcessFramesPossible will be true once we
ship
> --isolate-extensions.  Also, perhaps this should really replicate only to
> proxies in the same FrameTree, 

I don't understand - I thought that RenderFrameHostManager manages
RenderFrameHost + RenderFrameProxyHosts for a single frame (and doesn't know
about other frames, or frames from another frame tree).

> but probably no need to do it in this CL, as this
> also applies for a few other functions here.

Ack.

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:783: // Updates ContentSecurityPolicy in a frame
proxy / in RemoteSecurityContext.
On 2016/05/11 19:46:40, alexmos wrote:
> nit: Updates replicated ContentSecurityPolicy in a frame proxy.
> 
Done.

> Might also mention when it's sent (when the frame adds a CSP header via <meta>
> in another process).

I'd rather avoid this - this is tricky (there are more triggers for sending CSP
to the browser) and I'd rather keep the explanation in a single place (i.e. in
comment for FrameLoaderClient::didAddContentSecurityPolicy).

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.cc (right):

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_replic...
content/common/frame_replication_state.cc:7: #include
"content/common/frame_messages.h"
On 2016/05/11 19:46:40, alexmos wrote:
> Is this necessary?

Ooops - this is a left-over from an earlier draft, where there was no separate
content_security_policy_header.h file, but instead there was a struct defined
via IPC macros within frame_messages.h (yes - yuck :-).

Done.

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_replic...
content/common/frame_replication_state.h:87: // parent frames (in case of
about:blank frames).
On 2016/05/11 19:46:41, alexmos wrote:
> Sanity check: if a page dynamically removes a <meta> element with a CSP
header,
> does that affect the current CSP at all?  I'd expect not, but wanted to
> double-check.

It doesn't.  I am basing this on the fact that ContentSecurityPolicy::m_policies
is only modified in ContentSecurityPolicy::addPolicyFromHeaderValue (and we only
append to the list of policies).

Also - interestingly, changing the value of <meta> element's content attribute
should also have no effect.  I am basing this on
https://www.w3.org/TR/CSP2/#delivery-html-meta-element.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:40: if
(getSecurityOrigin())
On 2016/05/11 20:15:07, dcheng wrote:
> Is this ever actually null? it seems broken if it is. Can we just assert that
> origin is always non-null?

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:162: void
ContentSecurityPolicy::bindToSecurityOrigin(const SecurityOrigin&
securityOrigin)
On 2016/05/11 06:21:04, Mike West wrote:
> Nit: Now about naming this something like `setupSelf`? "Bind to security
origin"
> sounds like something different than what this method is actually doing.

Good point - this name wasn't quite right.  Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:270:
ASSERT(!m_executionContext);
On 2016/05/11 06:21:04, Mike West wrote:
> We create CSP without an execution context for local frames as well (in
> `DocumentLoader::responseReceived`). Eventually we'll be able to get rid of
that
> mechanism by moving everything to the browser, but until then it's not clear
to
> me what the assertion needs to be.

I've just removed the assert.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:286: // 2)
not-yet / long-term: CSP can be enforced by the browser.
On 2016/05/11 19:46:41, alexmos wrote:
> Might be good to add a link to the bug for moving CSP to the browser (I think
> it's 376522?).

Done.

I am having second thoughts about this comment... maybe I should just remove
stuff from "This is needed in order to..." forward?

> For 2), do you know how this would reconcile with parsing CSP
> HTTP headers in the browser?

This is an interesting question.  nasko@ or mkwst@ might probably have a better
idea on how to accomplish this, but I thought about:
1. moving parsing, storing, checking code into //content/common layer.
2. rewriting blink::ContentSecurityPolicy to use some kind of a delegate to do
parsing, storing, checking via //content layer (yucky?)
3. reuse //content/common code to do parsing, storing, checking in the browser
as well.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:822: //
frame-src checks to the browser).
On 2016/05/11 06:21:04, Mike West wrote:
> Nit: Prefer `TODO(...)` to FIXME.
> 
> Nit: Can you point to a bug here so that we don't lose track of the work?

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:279: void
addPolicyFromHeaderValue(const String&, ContentSecurityPolicyHeaderType,
ContentSecurityPolicyHeaderSource, bool notifyFrameLoaderClient = true);
On 2016/05/11 06:21:04, Mike West wrote:
> Nit: Prefer a meaningfully named enum to a bool.

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1367: 
On 2016/05/11 19:46:41, alexmos wrote:
> nit: unnecessary blank line.

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/FrameLoaderClient.h:269: virtual void
didAddContentSecurityPolicy(const String& headerValue,
ContentSecurityPolicyHeaderType, ContentSecurityPolicyHeaderSource) { }
On 2016/05/11 19:46:41, alexmos wrote:
> nit: maybe move this up to where sandbox flags and origin are updated.

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1045:
ContentSecurityPolicyHeaderSource source) {
On 2016/05/11 20:15:07, dcheng wrote:
> I'm surprised check-webkit-style didn't complain, but { should be on its own
> line.

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp:1046:
m_webFrame->client()->didAddContentSecurityPolicy(
On 2016/05/11 19:46:41, alexmos wrote:
> Do you need to check for !m_webFrame->client(), similar to other replication
> functions?  IIRC, it might be null in tests.

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebContentSecurityPolicy.h (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebContentSecurityPolicy.h:44:
WebContentSecurityPolicySourceMeta,
On 2016/05/11 19:46:41, alexmos wrote:
> I think these also need entries in Source/web/AssertMatchingEnums.cpp,
similarly
> to WebContentSecurityPolicyType

Great - thanks for pointing this out.  I was wondering about having a
static_assert for this, but incorrectly concluded that it wouldn't work with
current layering constraints (I haven't considered doing this from a separate
compilation unit).

Done.

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebFrameClient.h:725: // document.  This can be
trigerred by handling of HTTP headers, handling
On 2016/05/11 06:21:04, Mike West wrote:
> Nit: s/trigerred/triggered/

Done.

alexmos

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/render_frame_host_manager.cc File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/render_frame_host_manager.cc#newcode942 content/browser/frame_host/render_frame_host_manager.cc:942: for (const auto& pair : proxy_hosts_) { > > ...

4 years, 7 months ago (2016-05-12 22:37:25 UTC) #12

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:942: for (const auto&
pair : proxy_hosts_) {
> > Also, perhaps this should really replicate only to
> > proxies in the same FrameTree, 
> 
> I don't understand - I thought that RenderFrameHostManager manages
> RenderFrameHost + RenderFrameProxyHosts for a single frame (and doesn't know
> about other frames, or frames from another frame tree).

The proxy_hosts_ for this node (the node with updated CSP), will have proxies
for all SiteInstances that can reach this frame.  This includes all
SiteInstances for cross-site frames in the same FrameTree, but might also
include SiteInstances of frames from other FrameTrees (e.g., those that might
reach this frame via window.opener, or from a window.open reference, or via
window.open("","foo")).  So if we have A-embed-B-embed-C, and C opens a popup D,
C's RFHM will have proxy_hosts_ for {A,B,D}.  What I meant was that there's no
need to replicate CSP to D, since things like frame-src or child-src don't apply
across popups.  (We already do something like this in
FrameTree::SetFocusedFrame, with CollectSiteInstances.)

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_messag...
content/common/frame_messages.h:783: // Updates ContentSecurityPolicy in a frame
proxy / in RemoteSecurityContext.
On 2016/05/11 23:14:48, Łukasz Anforowicz wrote:
> On 2016/05/11 19:46:40, alexmos wrote:
> > nit: Updates replicated ContentSecurityPolicy in a frame proxy.
> > 
> Done.
> 
> > Might also mention when it's sent (when the frame adds a CSP header via
<meta>
> > in another process).
> 
> I'd rather avoid this - this is tricky (there are more triggers for sending
CSP
> to the browser) and I'd rather keep the explanation in a single place (i.e. in
> comment for FrameLoaderClient::didAddContentSecurityPolicy).
> 

Acknowledged.

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_replic...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/1957783002/diff/1/content/common/frame_replic...
content/common/frame_replication_state.h:87: // parent frames (in case of
about:blank frames).
On 2016/05/11 23:14:48, Łukasz Anforowicz wrote:
> On 2016/05/11 19:46:41, alexmos wrote:
> > Sanity check: if a page dynamically removes a <meta> element with a CSP
> header,
> > does that affect the current CSP at all?  I'd expect not, but wanted to
> > double-check.
> 
> It doesn't.  I am basing this on the fact that
ContentSecurityPolicy::m_policies
> is only modified in ContentSecurityPolicy::addPolicyFromHeaderValue (and we
only
> append to the list of policies).
> 
> Also - interestingly, changing the value of <meta> element's content attribute
> should also have no effect.  I am basing this on
> https://www.w3.org/TR/CSP2/#delivery-html-meta-element.

Acknowledged.

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

https://codereview.chromium.org/1957783002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:6022: if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
I think this is ok.  One thought is that this might be needed when we start
enforcing CSP from the browser process even when site isolation is off, but I
guess we can revisit this then and not incur unnecessary overhead in the
meantime.

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

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:272: void
ContentSecurityPolicy::addPolicyFromHeaderValue(const String& header,
ContentSecurityPolicyHeaderType type, ContentSecurityPolicyHeaderSource source,
FrameLoaderClientNotificationStatus notificationStatus)
nit: maybe "action" instead of "status"?

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:283: // 2)
enforce CSP in the browser (not yet / long-term - see https://crbug.com/376522).
nit: s/browser/browser process/

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:819: // (or
move CSP child-src and frame-src checks to the browser -
https://crbug.com/376522).
nit: s/browser/browser process/

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:284: void
addPolicyFromHeaderValue(const String&, ContentSecurityPolicyHeaderType,
ContentSecurityPolicyHeaderSource, FrameLoaderClientNotificationStatus =
NotifyFrameLoaderClient);
Note that this enum might be useful more generally (e.g., see
FocusController::focusDocumentView).  Fine not to do this here though.

(speaking of which, NotifyEmbedder might be a more succinct name to use :) )

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:725: // document.  This can be
trigered by handling of HTTP headers, handling
nit: triggered still needs a "g" :)

Łukasz Anforowicz

Thanks for reviewing Alex - can you take another look please? Open issues we still ...

4 years, 7 months ago (2016-05-13 17:29:15 UTC) #13

Thanks for reviewing Alex - can you take another look please?

Open issues we still need to track and resolve before landing this:
- I need to add 1 more browser test (about inheriting csp into srcdoc)
- We need to figure out what to do about resetting CSP when navigating to a
non-http location

On 2016/05/11 19:46:41, alexmos wrote:
> It is a bit sad that we end up not having an enabled test for this though. 
> Could we perhaps add a rudimentary browser test that doesn't depend on console
> messages?  Maybe something similar to
> SitePerProcessBrowserTest.CrossSiteIframeBlockedByXFrameOptionsOrCSP that I
> added a while back?  That would also allow us to verify the browser-side
> replicated state, and perhaps it could also cover the scenario of starting
with
> a CSP HTTP header, then dynamically adding a <meta> CSP and verifying that it
> takes effect.

Great call - I am very happy that you raised this.  I was planning on adding a
test for getting CSP via HTTP headers, but I let it slide after seeing that
making layout tests green would also require plumbing the console message.  Well
- it turned out that my original CL was not handling CSP from HTTP headers at
all :-/.  I should have added more tests - thanks for asking for them.

Right now I have:
- CrossSiteIframeBlockedByParentCSPFromHeaders
- CrossSiteIframeBlockedByParentCSPFromMeta

Before landing this CL, I plan to at least also add:
- CrossSiteIframeBlockedByCSPFromSrcDocParent which would go something like
this:
  - Setup: a.com(srcDoc(b.com)) + a.com HTTP headers allow only frame-src from
a.com and b.com
  - Test: navigate b.com frame to c.com (should be blocked)

I also think it would be good to add other tests in a follow-up CL (I think
tests above cover the most important scenarios and hopefully let us land the
current CL before the branchpoint):
- Navigating the CSP-declaring-frame from cross-site-http to non-http location.
    - Setup: a.com(b.com) + a.com HTTP headers allow only frame-src from a.com
and b.com
    - Setup: navigate a.com to about:blank or data: (this should reset/clear CSP
right?)
    - Test: navigate b.com frame to c.com (should be blocked I think, but I am
not sure)

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1957783002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:942: for (const auto&
pair : proxy_hosts_) {
On 2016/05/12 22:37:24, alexmos wrote:
> > > Also, perhaps this should really replicate only to
> > > proxies in the same FrameTree, 
> > 
> > I don't understand - I thought that RenderFrameHostManager manages
> > RenderFrameHost + RenderFrameProxyHosts for a single frame (and doesn't know
> > about other frames, or frames from another frame tree).
> 
> The proxy_hosts_ for this node (the node with updated CSP), will have proxies
> for all SiteInstances that can reach this frame.  This includes all
> SiteInstances for cross-site frames in the same FrameTree, but might also
> include SiteInstances of frames from other FrameTrees (e.g., those that might
> reach this frame via window.opener, or from a window.open reference, or via
> window.open("","foo")).  So if we have A-embed-B-embed-C, and C opens a popup
D,
> C's RFHM will have proxy_hosts_ for {A,B,D}.  What I meant was that there's no
> need to replicate CSP to D, since things like frame-src or child-src don't
apply
> across popups.  (We already do something like this in
> FrameTree::SetFocusedFrame, with CollectSiteInstances.)

Thank you for the explanation - I think I got it now.

As you say - we might want to do this in a separate CL (because it applies to
other things here + because hopefully this code will be short-lived and replaced
with browser-side checks).

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:289:
->didAddContentSecurityPolicy(header, type, source);
On 2016/05/11 19:46:41, alexmos wrote:
> Does this also work with pending navigations that are canceled?  E.g., would
it
> be at all possible to have a pending navigation that sends a CSP http header
> before commit but then never commits due to an error?  Can 
> DocumentLoader::responseReceived lead to a DidFailProvisionalLoad after
applying
> the CSP header?  I'm basically curious whether we need to delay replicating
CSP
> headers until a successful commit.

This is a great question - thanks for raising this.

In patchset 7, we are resetting the old CSP headers (and processing new HTTP
headers in NavigationHandle::WillProcessResponse) right when we know it is okay
to say ReadyToCommitNavigation().  Before that point we should have processed
any CSP from the not-yet-commited page (because HTTP headers are dropped on the
floor on Blink side + because Blink couldn't yet process CSP from <meta>
elements because we haven't yet sent to Blink the body of the HTTP response). 
After that point we are commited to the navigation, so it is okay to forget the
old CSP headers.

OTOH, there are still some questions left:

1. Can a non-http navigation clobber old headers too early - i.e. via Blink
notifications that happen too early / before commit.  In particular about:blank
navigation is a bit special because Blink copies CSP headers from the parent in
this case [1].

2. In patchset 7 I've removed the call to
frame_tree_node->ResetContentSecurityPolicy() that in earlier patchsets was done
from NavigatorImpl::DidNavigate.  This means that this call is now only
happening for HTTP responses, because otherwise
NavigationHandle::WillProcessResponse won't get involved (right?).  So - there
are 2 subquestions here:

2a. Should navigating from http to non-http location reset CSP for a frame?

2b. What would be a good place to call
frame_tree_node->ResetContentSecurityPolicy() in this case (place that would
happen earlier than NavigationHandle::WillProcessResponse because otherwise we
would clobber CSP the browser found in the headers).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

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

https://codereview.chromium.org/1957783002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:6022: if
(!SiteIsolationPolicy::AreCrossProcessFramesPossible())
On 2016/05/12 22:37:24, alexmos wrote:
> I think this is ok.  One thought is that this might be needed when we start
> enforcing CSP from the browser process even when site isolation is off, but I
> guess we can revisit this then and not incur unnecessary overhead in the
> meantime.

This reminds me - there are other places where we avoid sending extra IPC
messages when OOPIFs are disabled (most notably FrameHostMsg_DidChangeName). 
I'll mention bug https://crbug.com/169110 on chat.

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

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:272: void
ContentSecurityPolicy::addPolicyFromHeaderValue(const String& header,
ContentSecurityPolicyHeaderType type, ContentSecurityPolicyHeaderSource source,
FrameLoaderClientNotificationStatus notificationStatus)
On 2016/05/12 22:37:25, alexmos wrote:
> nit: maybe "action" instead of "status"?  

Done.

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:283: // 2)
enforce CSP in the browser (not yet / long-term - see https://crbug.com/376522).
On 2016/05/12 22:37:25, alexmos wrote:
> nit: s/browser/browser process/

Done.

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:819: // (or
move CSP child-src and frame-src checks to the browser -
https://crbug.com/376522).
On 2016/05/12 22:37:24, alexmos wrote:
> nit: s/browser/browser process/

Done.

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:284: void
addPolicyFromHeaderValue(const String&, ContentSecurityPolicyHeaderType,
ContentSecurityPolicyHeaderSource, FrameLoaderClientNotificationStatus =
NotifyFrameLoaderClient);
On 2016/05/12 22:37:25, alexmos wrote:
> Note that this enum might be useful more generally (e.g., see
> FocusController::focusDocumentView).  Fine not to do this here though.
> 
> (speaking of which, NotifyEmbedder might be a more succinct name to use :) )

Acknowledged.

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1957783002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:725: // document.  This can be
trigered by handling of HTTP headers, handling
On 2016/05/12 22:37:25, alexmos wrote:
> nit: triggered still needs a "g" :)

Done.

Łukasz Anforowicz

Description was changed from ========== Replicate Content-Security-Policy into remote frame proxies. After this CL, when ...

4 years, 7 months ago (2016-05-13 20:40:36 UTC) #14

Description was changed from

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RenderFrameProxies.
RenderFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RenderFrameProxies.
RenderFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.  To work around
this, a few regression tests are being added by this CL as browser
tests.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

Łukasz Anforowicz

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

4 years, 7 months ago (2016-05-13 23:31:41 UTC) #15

Łukasz Anforowicz

Nick, could you please take a look? (to review as a //content owner, but I ...

4 years, 7 months ago (2016-05-13 23:31:44 UTC) #16

Nick, could you please take a look?  (to review as a //content owner, but I also
had a specific question below).

Could you advise on one important piece that is still missing from this CL?  We
need to reset content security policy between navigations, but I am not sure
where would be a good place to do this.  NavigatorImpl::DidNavigate [1] is too
late - at this time Blink already sent an IPC with CSP from HTTP headers and/or
CSP inherited (i.e. by srcdoc frame) from the parent frame. 
NavigationHandle::WillProcessResponse [2] sounds like a good place timing-wise
(right after deciding to commit the navigation), but also sounds like something
that is specific to http (and therefore wouldn't reset CSP after navigations to
other schemes).

Please also take a look at a question [3] that Alex raised about making sure
that 1) we don't reset old CSP and 2) we don't get IPC for new doc, unless we
are sure the new navigation will commit.

[1] old patchset 6:
https://codereview.chromium.org/1957783002/patch/100001/110003
Note that this is wrong, because it resets too late, cloberring CSP from http
and/or srcdoc

[2] old patchset 8:
https://codereview.chromium.org/1957783002/patch/140001/150003
Note that after patchset 9 we no longer process http headers on the browser side
- we rely on them getting forwarded from the renderer side instead (to fix
inheritance for srcdoc)

[3]
https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1957783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:289:
->didAddContentSecurityPolicy(header, type, source);
On 2016/05/13 17:29:15, Łukasz Anforowicz wrote:
> On 2016/05/11 19:46:41, alexmos wrote:
> > Does this also work with pending navigations that are canceled?  E.g., would
> it
> > be at all possible to have a pending navigation that sends a CSP http header
> > before commit but then never commits due to an error?  Can 
> > DocumentLoader::responseReceived lead to a DidFailProvisionalLoad after
> applying
> > the CSP header?  I'm basically curious whether we need to delay replicating
> CSP
> > headers until a successful commit.
> 
> This is a great question - thanks for raising this.
> 
> In patchset 7, we are resetting the old CSP headers (and processing new HTTP
> headers in NavigationHandle::WillProcessResponse) right when we know it is
okay
> to say ReadyToCommitNavigation().  Before that point we should have processed
> any CSP from the not-yet-commited page (because HTTP headers are dropped on
the
> floor on Blink side + because Blink couldn't yet process CSP from <meta>
> elements because we haven't yet sent to Blink the body of the HTTP response). 
> After that point we are commited to the navigation, so it is okay to forget
the
> old CSP headers.
> 
> OTOH, there are still some questions left:
> 
> 1. Can a non-http navigation clobber old headers too early - i.e. via Blink
> notifications that happen too early / before commit.  In particular
about:blank
> navigation is a bit special because Blink copies CSP headers from the parent
in
> this case [1].
> 
> 2. In patchset 7 I've removed the call to
> frame_tree_node->ResetContentSecurityPolicy() that in earlier patchsets was
done
> from NavigatorImpl::DidNavigate.  This means that this call is now only
> happening for HTTP responses, because otherwise
> NavigationHandle::WillProcessResponse won't get involved (right?).  So - there
> are 2 subquestions here:
> 
> 2a. Should navigating from http to non-http location reset CSP for a frame?
> 
> 2b. What would be a good place to call
> frame_tree_node->ResetContentSecurityPolicy() in this case (place that would
> happen earlier than NavigationHandle::WillProcessResponse because otherwise we
> would clobber CSP the browser found in the headers).
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Nick / Charlie - could you please also look at Alex's question?

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

https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:251: // header multiple times, as
ContentSecurityPolicy object is copied around).
I tried to look into why Blink processes the same header multiple times - it
seems that there are multiple copies of ContentSecurityPolicy object floating
around (multiple copies per html document).  I am not sure exactly why there are
multiple copies.

For example - when running
SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders (with the
second iframe commented out, so there is only one frame with CSP), I see that
CSP::bindToExecutionContext is called 3 times (each time on a different instance
of ContentSecurityPolicy).  Having 3 copies of ContentSecurityPolicy seems a bit
wasteful - each time ContentSecurityPolicy::addPolicyFromHeaderValue gets called
it copies a String (twice: once into Vector<UChar> and once into |policy|) and
parses the CSP.

Callstacks:

#2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
#3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
#4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
#5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
#6 0x7f8b1e755331 blink::DocumentLoader::commitData()
#7 0x7f8b1e758cfc blink::DocumentLoader::processData()
#8 0x7f8b1e758860 blink::DocumentLoader::dataReceived()
#9 0x7f8b1e2f30ef blink::RawResource::appendData()
#10 0x7f8b2b179e8a content::WebURLLoaderImpl::Context::OnReceivedData()
#11 0x7f8b2b17bfc2 content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
#12 0x7f8b2b10d56d content::ResourceDispatcher::OnReceivedData()

#2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
#3 0x7f8b1d1abc8d blink::Document::initSecurityContext()
#4 0x7f8b1d1a8e6e blink::Document::Document()
#5 0x7f8b1d616aac blink::HTMLDocument::HTMLDocument()
#6 0x7f8b1d16910a blink::DOMImplementation::createDocument()
#7 0x7f8b1e41d7dd blink::LocalDOMWindow::createDocument()
#8 0x7f8b1e41db16 blink::LocalDOMWindow::installNewDocument()
#9 0x7f8b1e758544 blink::DocumentLoader::createWriterFor()
#10 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
#11 0x7f8b1e755331 blink::DocumentLoader::commitData()
#12 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
#13 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
#14 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
#15 0x7f8b1e794dba blink::FrameLoader::init()
#16 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
#17 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
#18 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
#19 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
#20 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
#21 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
#22 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
#23 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
#24 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
#25 0x7f8b1d8c76b9
blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
#26 0x7f8b1d8c5c7c
blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
#27 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()

#2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
#3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
#4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
#5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
#6 0x7f8b1e755331 blink::DocumentLoader::commitData()
#7 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
#8 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
#9 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
#10 0x7f8b1e794dba blink::FrameLoader::init()
#11 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
#12 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
#13 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
#14 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
#15 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
#16 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
#17 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
#18 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
#19 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
#20 0x7f8b1d8c76b9
blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
#21 0x7f8b1d8c5c7c
blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
#22 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()

alexmos

Thanks! One thought about your CSP reset question below, and a couple of nits. https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_host/frame_tree_node.cc ...

4 years, 7 months ago (2016-05-16 16:17:03 UTC) #17

Thanks!  One thought about your CSP reset question below, and a couple of nits.

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

https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:251: // header multiple times, as
ContentSecurityPolicy object is copied around).
On 2016/05/13 23:31:44, Łukasz Anforowicz wrote:
> I tried to look into why Blink processes the same header multiple times - it
> seems that there are multiple copies of ContentSecurityPolicy object floating
> around (multiple copies per html document).  I am not sure exactly why there
are
> multiple copies.
> 
> For example - when running
> SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders (with
the
> second iframe commented out, so there is only one frame with CSP), I see that
> CSP::bindToExecutionContext is called 3 times (each time on a different
instance
> of ContentSecurityPolicy).  Having 3 copies of ContentSecurityPolicy seems a
bit
> wasteful - each time ContentSecurityPolicy::addPolicyFromHeaderValue gets
called
> it copies a String (twice: once into Vector<UChar> and once into |policy|) and
> parses the CSP.
> 
> Callstacks:
> 
> #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> #3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
> #4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
> #5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> #6 0x7f8b1e755331 blink::DocumentLoader::commitData()
> #7 0x7f8b1e758cfc blink::DocumentLoader::processData()
> #8 0x7f8b1e758860 blink::DocumentLoader::dataReceived()
> #9 0x7f8b1e2f30ef blink::RawResource::appendData()
> #10 0x7f8b2b179e8a content::WebURLLoaderImpl::Context::OnReceivedData()
> #11 0x7f8b2b17bfc2
content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
> #12 0x7f8b2b10d56d content::ResourceDispatcher::OnReceivedData()
> 
> #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> #3 0x7f8b1d1abc8d blink::Document::initSecurityContext()
> #4 0x7f8b1d1a8e6e blink::Document::Document()
> #5 0x7f8b1d616aac blink::HTMLDocument::HTMLDocument()
> #6 0x7f8b1d16910a blink::DOMImplementation::createDocument()
> #7 0x7f8b1e41d7dd blink::LocalDOMWindow::createDocument()
> #8 0x7f8b1e41db16 blink::LocalDOMWindow::installNewDocument()
> #9 0x7f8b1e758544 blink::DocumentLoader::createWriterFor()
> #10 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> #11 0x7f8b1e755331 blink::DocumentLoader::commitData()
> #12 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
> #13 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
> #14 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
> #15 0x7f8b1e794dba blink::FrameLoader::init()
> #16 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
> #17 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
> #18 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
> #19 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
> #20 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
> #21 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
> #22 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
> #23 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
> #24 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
> #25 0x7f8b1d8c76b9
> blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
> #26 0x7f8b1d8c5c7c
> blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
> #27 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()
> 
> #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> #3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
> #4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
> #5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> #6 0x7f8b1e755331 blink::DocumentLoader::commitData()
> #7 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
> #8 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
> #9 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
> #10 0x7f8b1e794dba blink::FrameLoader::init()
> #11 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
> #12 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
> #13 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
> #14 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
> #15 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
> #16 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
> #17 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
> #18 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
> #19 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
> #20 0x7f8b1d8c76b9
> blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
> #21 0x7f8b1d8c5c7c
> blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
> #22 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()
> 

Seems like at least two of the copies are plausible -- one is for the initial
about:blank document in the child frame, another is for the actual document
that's loaded afterward.  But it looks like the initial blank Document calls
bindToExecutionContext twice (the stacks with createChildFrame and
maybeLoadEmpty), and that seems redundant.

Thinking some more about your question about when to clear CSP, I'm actually
curious if sending the http CSP headers at commit time (i.e., DidNavigate) and
letting that clear the old headers is really too late?  This is because commit
time is really when these CSP headers can start to have effect -- a doc can't
create a child frame before commit.  That would also take care of this
duplication problem, as you won't see commits from the initial blank document. 
It should work for srcdoc too.  And resetting old headers at commit time sounds
like the right thing also, as that's when they really stop taking effect, and
you won't need to deal with potentially temporary CSP headers from pending RFHs.
 Maybe I'm forgetting something that makes this not work -- thoughts?

I'm not sure if you could apply more than one http CSP header before commit -
but if so, seems like you could buffer them up and send them all as part of
DidCommitProvisionalLoad, similar to the origin.  <meta> CSP would still be sent
at the time the header is added, for but <meta> that would be guaranteed to
happen after commit.

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6192: NavigateToURL(shell(),
main_url);
nit: EXPECT_TRUE (also below)

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6199:
MatchesRegex("http://b.com.*/title2.html"));
Why not just check against the exact URL,
embedded_test_server()->GetURL("b.com", "/title1.html")? (also below)

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6256: load_observer.Wait();
Is renderer-initiated navigation required here?  Using NavigateFrameToURL is a
bit shorter and doesn't require an observer. :)

https://codereview.chromium.org/1957783002/diff/160001/content/test/data/fram...
File content/test/data/frame-src-self-and-b.html (right):

https://codereview.chromium.org/1957783002/diff/160001/content/test/data/fram...
content/test/data/frame-src-self-and-b.html:11: &lt;html&gt;
Looking at some other test files with srcdoc iframes, some of them get away
without the escaping - is it really required here?

https://codereview.chromium.org/1957783002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/1957783002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:40: ASSERT(origin);
Blink recently switched to prefer DCHECK, see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Łukasz Anforowicz

Alex, can you take another look please? https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_host/frame_tree_node.cc File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_host/frame_tree_node.cc#newcode251 content/browser/frame_host/frame_tree_node.cc:251: // header ...

4 years, 7 months ago (2016-05-16 19:44:46 UTC) #18

Alex, can you take another look please?

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

https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:251: // header multiple times, as
ContentSecurityPolicy object is copied around).
On 2016/05/16 16:17:03, alexmos wrote:
> On 2016/05/13 23:31:44, Łukasz Anforowicz wrote:
> > I tried to look into why Blink processes the same header multiple times - it
> > seems that there are multiple copies of ContentSecurityPolicy object
floating
> > around (multiple copies per html document).  I am not sure exactly why there
> are
> > multiple copies.
> > 
> > For example - when running
> > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders (with
> the
> > second iframe commented out, so there is only one frame with CSP), I see
that
> > CSP::bindToExecutionContext is called 3 times (each time on a different
> instance
> > of ContentSecurityPolicy).  Having 3 copies of ContentSecurityPolicy seems a
> bit
> > wasteful - each time ContentSecurityPolicy::addPolicyFromHeaderValue gets
> called
> > it copies a String (twice: once into Vector<UChar> and once into |policy|)
and
> > parses the CSP.
> > 
> > Callstacks:
> > 
> > #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> > #3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
> > #4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
> > #5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> > #6 0x7f8b1e755331 blink::DocumentLoader::commitData()
> > #7 0x7f8b1e758cfc blink::DocumentLoader::processData()
> > #8 0x7f8b1e758860 blink::DocumentLoader::dataReceived()
> > #9 0x7f8b1e2f30ef blink::RawResource::appendData()
> > #10 0x7f8b2b179e8a content::WebURLLoaderImpl::Context::OnReceivedData()
> > #11 0x7f8b2b17bfc2
> content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
> > #12 0x7f8b2b10d56d content::ResourceDispatcher::OnReceivedData()
> > 
> > #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> > #3 0x7f8b1d1abc8d blink::Document::initSecurityContext()
> > #4 0x7f8b1d1a8e6e blink::Document::Document()
> > #5 0x7f8b1d616aac blink::HTMLDocument::HTMLDocument()
> > #6 0x7f8b1d16910a blink::DOMImplementation::createDocument()
> > #7 0x7f8b1e41d7dd blink::LocalDOMWindow::createDocument()
> > #8 0x7f8b1e41db16 blink::LocalDOMWindow::installNewDocument()
> > #9 0x7f8b1e758544 blink::DocumentLoader::createWriterFor()
> > #10 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> > #11 0x7f8b1e755331 blink::DocumentLoader::commitData()
> > #12 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
> > #13 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
> > #14 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
> > #15 0x7f8b1e794dba blink::FrameLoader::init()
> > #16 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
> > #17 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
> > #18 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
> > #19 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
> > #20 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
> > #21 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
> > #22 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
> > #23 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
> > #24 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
> > #25 0x7f8b1d8c76b9
> > blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
> > #26 0x7f8b1d8c5c7c
> > blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
> > #27 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()
> > 
> > #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> > #3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
> > #4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
> > #5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> > #6 0x7f8b1e755331 blink::DocumentLoader::commitData()
> > #7 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
> > #8 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
> > #9 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
> > #10 0x7f8b1e794dba blink::FrameLoader::init()
> > #11 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
> > #12 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
> > #13 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
> > #14 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
> > #15 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
> > #16 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
> > #17 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
> > #18 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
> > #19 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
> > #20 0x7f8b1d8c76b9
> > blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
> > #21 0x7f8b1d8c5c7c
> > blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
> > #22 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()
> > 
> 
> Seems like at least two of the copies are plausible -- one is for the initial
> about:blank document in the child frame, another is for the actual document
> that's loaded afterward.  But it looks like the initial blank Document calls
> bindToExecutionContext twice (the stacks with createChildFrame and
> maybeLoadEmpty), and that seems redundant.
> 
> Thinking some more about your question about when to clear CSP, I'm actually
> curious if sending the http CSP headers at commit time (i.e., DidNavigate) and
> letting that clear the old headers is really too late?  This is because commit
> time is really when these CSP headers can start to have effect -- a doc can't
> create a child frame before commit.  That would also take care of this
> duplication problem, as you won't see commits from the initial blank document.

> It should work for srcdoc too.  And resetting old headers at commit time
sounds
> like the right thing also, as that's when they really stop taking effect, and
> you won't need to deal with potentially temporary CSP headers from pending
RFHs.
>  Maybe I'm forgetting something that makes this not work -- thoughts?
> 
> I'm not sure if you could apply more than one http CSP header before commit -
> but if so, seems like you could buffer them up and send them all as part of
> DidCommitProvisionalLoad, similar to the origin.  <meta> CSP would still be
sent
> at the time the header is added, for but <meta> that would be guaranteed to
> happen after commit.

Thanks for the suggestion - it worked out really well in patchset 11, where I:

- Went back to resetting CSP in NavigatorImpl::DidNavigate
- Removed the code to notify FrameLoaderClient upon
ContentSecurityPolicy::bindToExecutionContext.  This goes back to the behavior
of early patchsets where CSP from http headers (or inherited from parent frame)
won't be reported (mulitple times) via FrameLoaderClient - this allows to also
remove deduplication code in frame_tree_node.cc.
- Added ContentSecurityPolicy::reportAccumulatedHeaders(FrameLoaderClient*) that
gets called right after FrameLoader reports that the navigation has committed.

I've thought about reporting the CSP headers within DidCommitProvisionalLoad
message, but went with the approach above because in the long-term CSP from http
headers (and inherited from parent frame) should be handled within the browser
process - I wanted to avoid adding more temporary code to handle this case
(handling of new DidCommitProvisionalLoad payload + exposing
ContentSecurityPolicy through web layer to enable populating the message
payload).  I think sending CSP from http headers via the same path as used for
CSP from <meta> is working out okay.

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6192: NavigateToURL(shell(),
main_url);
On 2016/05/16 16:17:03, alexmos wrote:
> nit: EXPECT_TRUE (also below)

Done (but only for the 3 new tests... :-/ ).

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6199:
MatchesRegex("http://b.com.*/title2.html"));
On 2016/05/16 16:17:03, alexmos wrote:
> Why not just check against the exact URL,
> embedded_test_server()->GetURL("b.com", "/title1.html")? (also below)

Good idea.  Done.

https://codereview.chromium.org/1957783002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6256: load_observer.Wait();
On 2016/05/16 16:17:03, alexmos wrote:
> Is renderer-initiated navigation required here?  Using NavigateFrameToURL is a
> bit shorter and doesn't require an observer. :)

Done.

https://codereview.chromium.org/1957783002/diff/160001/content/test/data/fram...
File content/test/data/frame-src-self-and-b.html (right):

https://codereview.chromium.org/1957783002/diff/160001/content/test/data/fram...
content/test/data/frame-src-self-and-b.html:11: &lt;html&gt;
On 2016/05/16 16:17:03, alexmos wrote:
> Looking at some other test files with srcdoc iframes, some of them get away
> without the escaping - is it really required here?

Oh, indeed - I assumed that I need to escape, but HTML5 spec seems to say that I
only need to escape the quote character.  OTOH it also says that "'text' must
not contain control characters other than space characters" and I remember
reading elsewhere that newlines are okay.  Some people think that indeed only
quote needs to be escaped (http://stackoverflow.com/a/8047332).

Done.

https://codereview.chromium.org/1957783002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/1957783002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:40: ASSERT(origin);
On 2016/05/16 16:17:03, alexmos wrote:
> Blink recently switched to prefer DCHECK, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Done.  (It seems that this check has also been recently added to
check-webkit-style).

alexmos

Thanks, LGTM! Just a couple of non-blocking nits on the test. https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_host/frame_tree_node.cc File content/browser/frame_host/frame_tree_node.cc (right): ...

4 years, 7 months ago (2016-05-16 22:31:56 UTC) #20

Thanks, LGTM!  Just a couple of non-blocking nits on the test.

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

https://codereview.chromium.org/1957783002/diff/160001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:251: // header multiple times, as
ContentSecurityPolicy object is copied around).
On 2016/05/16 19:44:45, Łukasz Anforowicz wrote:
> On 2016/05/16 16:17:03, alexmos wrote:
> > On 2016/05/13 23:31:44, Łukasz Anforowicz wrote:
> > > I tried to look into why Blink processes the same header multiple times -
it
> > > seems that there are multiple copies of ContentSecurityPolicy object
> floating
> > > around (multiple copies per html document).  I am not sure exactly why
there
> > are
> > > multiple copies.
> > > 
> > > For example - when running
> > > SitePerProcessBrowserTest.CrossSiteIframeBlockedByParentCSPFromHeaders
(with
> > the
> > > second iframe commented out, so there is only one frame with CSP), I see
> that
> > > CSP::bindToExecutionContext is called 3 times (each time on a different
> > instance
> > > of ContentSecurityPolicy).  Having 3 copies of ContentSecurityPolicy seems
a
> > bit
> > > wasteful - each time ContentSecurityPolicy::addPolicyFromHeaderValue gets
> > called
> > > it copies a String (twice: once into Vector<UChar> and once into |policy|)
> and
> > > parses the CSP.
> > > 
> > > Callstacks:
> > > 
> > > #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> > > #3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
> > > #4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
> > > #5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> > > #6 0x7f8b1e755331 blink::DocumentLoader::commitData()
> > > #7 0x7f8b1e758cfc blink::DocumentLoader::processData()
> > > #8 0x7f8b1e758860 blink::DocumentLoader::dataReceived()
> > > #9 0x7f8b1e2f30ef blink::RawResource::appendData()
> > > #10 0x7f8b2b179e8a content::WebURLLoaderImpl::Context::OnReceivedData()
> > > #11 0x7f8b2b17bfc2
> > content::WebURLLoaderImpl::RequestPeerImpl::OnReceivedData()
> > > #12 0x7f8b2b10d56d content::ResourceDispatcher::OnReceivedData()
> > > 
> > > #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> > > #3 0x7f8b1d1abc8d blink::Document::initSecurityContext()
> > > #4 0x7f8b1d1a8e6e blink::Document::Document()
> > > #5 0x7f8b1d616aac blink::HTMLDocument::HTMLDocument()
> > > #6 0x7f8b1d16910a blink::DOMImplementation::createDocument()
> > > #7 0x7f8b1e41d7dd blink::LocalDOMWindow::createDocument()
> > > #8 0x7f8b1e41db16 blink::LocalDOMWindow::installNewDocument()
> > > #9 0x7f8b1e758544 blink::DocumentLoader::createWriterFor()
> > > #10 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> > > #11 0x7f8b1e755331 blink::DocumentLoader::commitData()
> > > #12 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
> > > #13 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
> > > #14 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
> > > #15 0x7f8b1e794dba blink::FrameLoader::init()
> > > #16 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
> > > #17 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
> > > #18 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
> > > #19 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
> > > #20 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
> > > #21 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
> > > #22 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
> > > #23 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
> > > #24 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
> > > #25 0x7f8b1d8c76b9
> > > blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
> > > #26 0x7f8b1d8c5c7c
> > > blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
> > > #27 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()
> > > 
> > > #2 0x7f8b1e4ea5ab blink::ContentSecurityPolicy::bindToExecutionContext()
> > > #3 0x7f8b1e79a927 blink::FrameLoader::didInstallNewDocument()
> > > #4 0x7f8b1e75857d blink::DocumentLoader::createWriterFor()
> > > #5 0x7f8b1e7581c1 blink::DocumentLoader::ensureWriter()
> > > #6 0x7f8b1e755331 blink::DocumentLoader::commitData()
> > > #7 0x7f8b1e754dbd blink::DocumentLoader::finishedLoading()
> > > #8 0x7f8b1e7593ae blink::DocumentLoader::maybeLoadEmpty()
> > > #9 0x7f8b1e7596c4 blink::DocumentLoader::startLoadingMainResource()
> > > #10 0x7f8b1e794dba blink::FrameLoader::init()
> > > #11 0x7f8b227ae5b3 blink::WebLocalFrameImpl::initializeCoreFrame()
> > > #12 0x7f8b227aef1e blink::WebLocalFrameImpl::createChildFrame()
> > > #13 0x7f8b1d6611c3 blink::HTMLFrameOwnerElement::loadOrRedirectSubframe()
> > > #14 0x7f8b1d65ac6e blink::HTMLFrameElementBase::openURL()
> > > #15 0x7f8b1d65c0fb blink::HTMLFrameElementBase::setNameAndOpenURL()
> > > #16 0x7f8b1d141953 blink::ContainerNode::notifyNodeInserted()
> > > #17 0x7f8b1d13b690 blink::ContainerNode::parserAppendChild()
> > > #18 0x7f8b1d8a7e6a blink::HTMLConstructionSite::executeTask()
> > > #19 0x7f8b1d8aad10 blink::HTMLConstructionSite::executeQueuedTasks()
> > > #20 0x7f8b1d8c76b9
> > > blink::HTMLDocumentParser::constructTreeFromCompactHTMLToken()
> > > #21 0x7f8b1d8c5c7c
> > > blink::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
> > > #22 0x7f8b1d8be08d blink::HTMLDocumentParser::pumpPendingSpeculations()
> > > 
> > 
> > Seems like at least two of the copies are plausible -- one is for the
initial
> > about:blank document in the child frame, another is for the actual document
> > that's loaded afterward.  But it looks like the initial blank Document calls
> > bindToExecutionContext twice (the stacks with createChildFrame and
> > maybeLoadEmpty), and that seems redundant.
> > 
> > Thinking some more about your question about when to clear CSP, I'm actually
> > curious if sending the http CSP headers at commit time (i.e., DidNavigate)
and
> > letting that clear the old headers is really too late?  This is because
commit
> > time is really when these CSP headers can start to have effect -- a doc
can't
> > create a child frame before commit.  That would also take care of this
> > duplication problem, as you won't see commits from the initial blank
document.
> 
> > It should work for srcdoc too.  And resetting old headers at commit time
> sounds
> > like the right thing also, as that's when they really stop taking effect,
and
> > you won't need to deal with potentially temporary CSP headers from pending
> RFHs.
> >  Maybe I'm forgetting something that makes this not work -- thoughts?
> > 
> > I'm not sure if you could apply more than one http CSP header before commit
-
> > but if so, seems like you could buffer them up and send them all as part of
> > DidCommitProvisionalLoad, similar to the origin.  <meta> CSP would still be
> sent
> > at the time the header is added, for but <meta> that would be guaranteed to
> > happen after commit.
> 
> Thanks for the suggestion - it worked out really well in patchset 11, where I:
> 
> - Went back to resetting CSP in NavigatorImpl::DidNavigate
> - Removed the code to notify FrameLoaderClient upon
> ContentSecurityPolicy::bindToExecutionContext.  This goes back to the behavior
> of early patchsets where CSP from http headers (or inherited from parent
frame)
> won't be reported (mulitple times) via FrameLoaderClient - this allows to also
> remove deduplication code in frame_tree_node.cc.
> - Added ContentSecurityPolicy::reportAccumulatedHeaders(FrameLoaderClient*)
that
> gets called right after FrameLoader reports that the navigation has committed.
> 
> I've thought about reporting the CSP headers within DidCommitProvisionalLoad
> message, but went with the approach above because in the long-term CSP from
http
> headers (and inherited from parent frame) should be handled within the browser
> process - I wanted to avoid adding more temporary code to handle this case
> (handling of new DidCommitProvisionalLoad payload + exposing
> ContentSecurityPolicy through web layer to enable populating the message
> payload).  I think sending CSP from http headers via the same path as used for
> CSP from <meta> is working out okay.

Thanks -- yes, sending them right after dispatching the commit message looks
fine to me.

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6349:
root->child_at(0)->current_url());
Perhaps we could also check
root->current_replication_state().accumulated_csp_headers somewhere here?  I.e.,
that the vector has one item and that it's "frame-src 'self' http://b.com:*"? 
This would verify that the browser-side representation is correct, and it might
simplify debugging the test if something screws up later.  Same idea for other
tests.

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6401:
embedded_test_server()->GetURL("b.com", "/title2.html"));
nit: consider defining the b.com URL in a var and then just reusing that (since
it's used three times).

Łukasz Anforowicz

Thanks for reviewing Alex (and for overall help with this CL :-). https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_per_process_browsertest.cc File content/browser/site_per_process_browsertest.cc ...

4 years, 7 months ago (2016-05-16 23:12:51 UTC) #21

Łukasz Anforowicz

Daniel, could you please do a quick sanity check of the Blink changes? They've been ...

4 years, 7 months ago (2016-05-16 23:16:16 UTC) #22

alexmos

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_per_process_browsertest.cc File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_per_process_browsertest.cc#newcode6349 content/browser/site_per_process_browsertest.cc:6349: root->child_at(0)->current_url()); On 2016/05/16 23:12:51, Łukasz Anforowicz wrote: > On ...

4 years, 7 months ago (2016-05-16 23:38:56 UTC) #23

Łukasz Anforowicz

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_per_process_browsertest.cc File content/browser/site_per_process_browsertest.cc (right): https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_per_process_browsertest.cc#newcode6349 content/browser/site_per_process_browsertest.cc:6349: root->child_at(0)->current_url()); On 2016/05/16 23:38:56, alexmos wrote: > On 2016/05/16 ...

4 years, 7 months ago (2016-05-17 00:19:43 UTC) #24

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1957783002/diff/220001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:6349:
root->child_at(0)->current_url());
On 2016/05/16 23:38:56, alexmos wrote:
> On 2016/05/16 23:12:51, Łukasz Anforowicz wrote:
> > On 2016/05/16 22:31:56, alexmos wrote:
> > > Perhaps we could also check
> > > root->current_replication_state().accumulated_csp_headers somewhere here? 
> > I.e.,
> > > that the vector has one item and that it's "frame-src 'self'
> http://b.com:*%22
> > 
> > > This would verify that the browser-side representation is correct, and it
> > might
> > > simplify debugging the test if something screws up later.  Same idea for
> other
> > > tests.
> > 
> > Done.  This sounds like a good idea.
> > 
> > I guess one could also argue that this makes the test too coupled with
> > implementation details - in the future we will probably move where CSP is
> stored
> > inside the browser process [i.e. once enforcing CSP in the browser, we won't
> > need to store it and replicate it via FrameReplicationState].  Still -
having
> > this extra sanity check in the test seems useful.
> > 
> > BTW: one more thing that this extra test assertion can potentially catch is
> > duplication of CSP headers (i.e. right now deduplication is kind of implicit
> in
> > the way that notifications from ContentSecurityPolicy don't reach
> > RenderFrameImpl until navigation commits - it will be good to have some test
> > coverage to make sure duplication doesn't creep back in).
> 
> Thanks!  One final thought I just had is that it might be useful to also check
> that the FTN's CSP is cleared if the frame navigates away to a page without
CSP.
>  Perhaps that can be added just in this test at the end - WDYT? :)

Done (here and for the srcdoc test).

dcheng

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right): https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp#newcode48 third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:48: if (getSecurityOrigin()) Is it possible to call this when ...

4 years, 7 months ago (2016-05-17 05:57:15 UTC) #26

Łukasz Anforowicz

Daniel, please take another look. https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right): https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp#newcode48 third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:48: if (getSecurityOrigin()) On 2016/05/17 ...

4 years, 7 months ago (2016-05-17 17:01:23 UTC) #27

Daniel, please take another look.

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp (right):

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/RemoteSecurityContext.cpp:48: if
(getSecurityOrigin())
On 2016/05/17 05:57:15, dcheng wrote:
> Is it possible to call this when the security origin is null? I would expect
> not, but I wonder if I'm missing something.

When this is called from the constructor, then the security origin is null.  I
guess it might be clearer if the constructor didn't call
resetReplicatedContentSecurityPolicy but kept calling setContentSecurityPolicy
as before.

Done?

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right):

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:290: return
static_cast<FrameLoaderClient*>(document()->frame()->client());
On 2016/05/17 05:57:15, dcheng wrote:
> Just make LocalFrame::client() return a FrameLoaderClient, I think. Something
> like
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So....

Good idea.  Done.

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/1957783002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.h:283: enum
FrameLoaderClientNotificationAction {
On 2016/05/17 05:57:15, dcheng wrote:
> This enum is quite a mouthful. Since the replication logic looks quite simple,
> how about:
> 
> - The original logic for addPolicyFromheaderValue() goes into
replicateHeader()
> (or whatever you'd like to call it)
> - addPolicyFromHeaderValue() just delegates to replicateHeader(), and always
> calls FrameLoaderClient::didAddContentSecurityPolicy(...)

Done.

After the changes the check "if this is a report-only header inside a <meta>
element, bail out" happens *after* notifying FrameLoaderClient, but this should
be okay (the check will happen again when replicating so the incorrect header
will still get filtered out + the extra header in FrameReplicationState should
be rare).  I guess I could report success-vs-failure from
addPolicyFromHeaderValue and only notify FrameLoaderClient upon success (but
then this would require introducing a success-or-failure enum?  couldn't find a
generic one in Blink).

dcheng

lgtm minor nitpick about inconsistency in terminology: some places we use "forward" (input events, which ...

4 years, 7 months ago (2016-05-17 18:32:23 UTC) #28

Łukasz Anforowicz

On 2016/05/17 18:32:23, dcheng wrote: > lgtm > > minor nitpick about inconsistency in terminology: ...

4 years, 7 months ago (2016-05-17 18:36:29 UTC) #29

Łukasz Anforowicz

The CQ bit was checked by lukasza@chromium.org

4 years, 7 months ago (2016-05-17 18:36:52 UTC) #30

Łukasz Anforowicz

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, nick@chromium.org, alexmos@chromium.org Link ...

4 years, 7 months ago (2016-05-17 18:36:53 UTC) #31

commit-bot: I haz the power

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

4 years, 7 months ago (2016-05-17 18:37:16 UTC) #32

commit-bot: I haz the power

Description was changed from ========== Replicate Content-Security-Policy into remote frame proxies. After this CL, when ...

4 years, 7 months ago (2016-05-17 20:06:46 UTC) #34

Message was sent while issue was closed.

Description was changed from

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RenderFrameProxies.
RenderFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.  To work around
this, a few regression tests are being added by this CL as browser
tests.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Replicate Content-Security-Policy into remote frame proxies.

After this CL, when a local frame parses a new CSP header (from http
headers, from <meta> element, or when copying CSP from the parent frame
in case of about:blank children), a notification will be sent all the
way to the browser.  The browser will store the CSP accumulated headers
in FrameReplicationState and notify RenderFrameProxies.
RenderFrameProxy will take care of using CSP headers received from the
browser to fill out RemoteSecurityContext's ContentSecurityPolicy.

After this CL, frame-src is properly enforced when navigating a frame
when its parent is a remote frame proxy.  For example, when running
http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html
the subframe navigation is blocked after this CL.  OTOH, we cannot yet
enable this test, because the console message about CSP violation is
currently dropped when CSP comes from a remote frame.  To work around
this, a few regression tests are being added by this CL as browser
tests.

BUG=585501
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/8e1c02e4ee2f50b204c234c033d09a4a866bf932
Cr-Commit-Position: refs/heads/master@{#394200}
==========

commit-bot: I haz the power

4 years, 7 months ago (2016-05-17 20:06:48 UTC) #35

Message was sent while issue was closed.

Patchset 16 (id:??) landed as
https://crrev.com/8e1c02e4ee2f50b204c234c033d09a4a866bf932
Cr-Commit-Position: refs/heads/master@{#394200}

Issue 1957783002: Replicate Content-Security-Policy into remote frame proxies. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Addressed most CR feedback from mkwst@, alexmos@ and dcheng@. #

Patch Set 3 : Rebasing... #

Patch Set 4 : More reordering of methods. #

Patch Set 5 : Added browser tests. #

Patch Set 6 : Addressed second round of CR feedback from alexmos@. #

Patch Set 7 : Fixing handling of CSP coming from HTTP headers. #

Patch Set 8 : Test for inheriting CSP via srcdoc frame. #

Patch Set 9 : Fixing CSP inheritance for srcdoc. #

Patch Set 10 : Addressed feedback from alexmos@ related to tests and asserts. #

Patch Set 11 : Fixing resetting of old CSP after a new navigation commits. #

Patch Set 12 : Rebasing + small self-review tweaks (comments + assert->dcheck). #

Patch Set 13 : Test tweaks. #

Patch Set 14 : Added test verification for resetting the CSP after navigation. #

Patch Set 15 : s/title.html/title1.html/ #

Patch Set 16 : Addressed CR feedback from dcheng@. #

Messages