Implement CSP check for manifest fetching

Implement CSP check for manifest fetching

The CSP bits include the 'manifest-src' directive, but _not_ the
forthcoming "manifest that sets a CSP for its documents" feature.

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

[kenneth.r.christiansen, 2014-09-12 13:22:29]

kenneth.r.christiansen@intel.com changed reviewers:
+ mkwst@chromium.org

[mlamouri (slow - plz ping), 2014-09-15 15:48:20]

So, that will use the CSP rules of the WebLocalFrame's document passed to
AssociatedURLLoader, right?

[mlamouri (slow - plz ping), 2014-09-15 15:48:44]

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

[mlamouri (slow - plz ping), 2014-09-15 15:48:45]

https://codereview.chromium.org/570563003/diff/1/Source/web/AssociatedURLLoad...
File Source/web/AssociatedURLLoader.cpp (right):

https://codereview.chromium.org/570563003/diff/1/Source/web/AssociatedURLLoad...
Source/web/AssociatedURLLoader.cpp:325: 
nit: empty line edit.

[Mike West, 2014-09-16 09:52:10]

On 2014/09/15 15:48:45, Mounir Lamouri wrote:
>
https://codereview.chromium.org/570563003/diff/1/Source/web/AssociatedURLLoad...
> File Source/web/AssociatedURLLoader.cpp (right):
> 
>
https://codereview.chromium.org/570563003/diff/1/Source/web/AssociatedURLLoad...
> Source/web/AssociatedURLLoader.cpp:325: 
> nit: empty line edit.

Is this ready for review? The title is still "WIP"...

[kenneth.christiansen, 2014-09-16 11:15:25]

On 2014/09/16 at 09:52:10, mkwst wrote:
> On 2014/09/15 15:48:45, Mounir Lamouri wrote:
> >
https://codereview.chromium.org/570563003/diff/1/Source/web/AssociatedURLLoad...
> > File Source/web/AssociatedURLLoader.cpp (right):
> > 
> >
https://codereview.chromium.org/570563003/diff/1/Source/web/AssociatedURLLoad...
> > Source/web/AssociatedURLLoader.cpp:325: 
> > nit: empty line edit.
> 
> Is this ready for review? The title is still "WIP"...

Yes, I would like to write some tests first. I was waiting for Mounir's initial
tests to land.

[Mike West, 2014-09-16 11:46:20]

This looks pretty reasonable, just two comments.

Once the prerequisite patches land, please do add some tests and ping the review
again. I'll be happy to take another look.

To answer mounir@'s question, it should pull the ResourceFetcher associated with
the document associated with the AssociatedURLLoader. I think that will give you
the policy object you're looking for. Tests will confirm that.

https://codereview.chromium.org/570563003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/570563003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:558: // FIXME(mkwst): Make sure that CSP
uses RequestContext instead of this terrible Resource::Type enum.
Nit: We don't generally do the (<nick>) thing in Blink. Please just link to
http://crbug.com/390497 instead... bugs usually live longer than contributors.
:)

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

https://codereview.chromium.org/570563003/diff/1/Source/core/frame/csp/CSPDir...
Source/core/frame/csp/CSPDirectiveList.cpp:666:
setCSPDirective<SourceListDirective>(name, value, m_manifestSrc);
This needs to be gated on a runtime flag. I think you'll need to add one for
Manifest, as I think it was removed in an earlier commit.

[kenneth.christiansen, 2014-09-16 12:00:38]

On 2014/09/16 at 11:46:20, mkwst wrote:
> This looks pretty reasonable, just two comments.
> 
> Once the prerequisite patches land, please do add some tests and ping the
review again. I'll be happy to take another look.

Will do. That test will probably live in Chromium though and thus be another CL.


> Nit: We don't generally do the (<nick>) thing in Blink. Please just link to
http://crbug.com/390497 instead... bugs usually live longer than contributors.
:)

Sure thing, though we hope to keep you around for the long run :-)

> This needs to be gated on a runtime flag. I think you'll need to add one for
Manifest, as I think it was removed in an earlier commit.

Would that really matter - I mean noone is using the value for anything. Anyway
if I add the runtime flag back, now can I make sure it is turned on for the
chromium tests?

[mlamouri (slow - plz ping), 2014-09-16 12:49:06]

On 2014/09/16 12:00:38, kenneth.christiansen wrote:
> On 2014/09/16 at 11:46:20, mkwst wrote:
> > This looks pretty reasonable, just two comments.
> > 
> > Once the prerequisite patches land, please do add some tests and ping the
> review again. I'll be happy to take another look.
> 
> Will do. That test will probably live in Chromium though and thus be another
CL.

Wouldn't that make more sense to test this in Blink given that it is a Blink
feature?

[kenneth.christiansen, 2014-09-17 13:39:16]

> Wouldn't that make more sense to test this in Blink given that it is a Blink
feature?

It might :-) so I am attempting this right this moment!

[kenneth.christiansen, 2014-09-17 14:05:51]

On 2014/09/17 at 13:39:16, kenneth.christiansen wrote:
> > Wouldn't that make more sense to test this in Blink given that it is a Blink
feature?
> 
> It might :-) so I am attempting this right this moment!

Done.

[Mike West, 2014-09-17 14:48:24]

On 2014/09/17 14:05:51, kenneth.christiansen wrote:
> On 2014/09/17 at 13:39:16, kenneth.christiansen wrote:
> > > Wouldn't that make more sense to test this in Blink given that it is a
Blink
> feature?
> > 
> > It might :-) so I am attempting this right this moment!
> 
> Done.

The code changes look good to me. You'll need someone to stamp the platform/ and
web/ changes, of course.

I also think you need to add a few more tests before this is good to go. Before
landing the patch, please add some parsing tests to ensure that the new
directive is parsed correctly
(LayoutTests/http/tests/security/contentSecurityPolicy/directive-parsing-*) and
an end-to-end layout test which ensures that blocking resources triggers the
reporting mechanisms (similar to
LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-*).

Thanks!

[mlamouri (slow - plz ping), 2014-09-17 20:07:52]

Pretty great to see that happening. We might end up with a quite advanced
manifest support for 39 :)

https://codereview.chromium.org/570563003/diff/20001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/570563003/diff/20001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:79: Manifest status=test
Why did you add that? I was asked to remove the Manifest entry from here, FWIW.

[kenneth.christiansen, 2014-09-18 07:46:21]

On 2014/09/17 at 20:07:52, mlamouri wrote:
> Pretty great to see that happening. We might end up with a quite advanced
manifest support for 39 :)

That would be super awesome!

>
https://codereview.chromium.org/570563003/diff/20001/Source/platform/RuntimeE...
> File Source/platform/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/570563003/diff/20001/Source/platform/RuntimeE...
> Source/platform/RuntimeEnabledFeatures.in:79: Manifest status=test
> Why did you add that? I was asked to remove the Manifest entry from here,
FWIW.

Because Mike West asked me to add it back and have this part guarded.

[Mike West, 2014-09-18 07:49:23]

On 2014/09/18 07:46:21, kenneth.christiansen wrote:
> On 2014/09/17 at 20:07:52, mlamouri wrote:
> > Pretty great to see that happening. We might end up with a quite advanced
> manifest support for 39 :)
> 
> That would be super awesome!
> 
> >
>
https://codereview.chromium.org/570563003/diff/20001/Source/platform/RuntimeE...
> > File Source/platform/RuntimeEnabledFeatures.in (right):
> > 
> >
>
https://codereview.chromium.org/570563003/diff/20001/Source/platform/RuntimeE...
> > Source/platform/RuntimeEnabledFeatures.in:79: Manifest status=test
> > Why did you add that? I was asked to remove the Manifest entry from here,
> FWIW.
> 
> Because Mike West asked me to add it back and have this part guarded.

If there's a flag somewhere else that you're using on the Chromium side, I'm
happy to have you pipe it down.

Alternatively, if this is the only piece of Manifest that's implemented on the
Blink side, then you could guard it behind the experiemental CSP flag (see the
experimental block further down in CSPDirectiveList). Either way, it shouldn't
be exposed to the web before we decide to ship Manifest

[mlamouri (slow - plz ping), 2014-09-18 13:35:21]

On 2014/09/18 07:49:23, Mike West (OOO until 24th) wrote:
> Alternatively, if this is the only piece of Manifest that's implemented on the
> Blink side, then you could guard it behind the experiemental CSP flag (see the
> experimental block further down in CSPDirectiveList). Either way, it shouldn't
> be exposed to the web before we decide to ship Manifest

WebFrameClient is notified when a page has a manifest change. That was guarded
by a Manifest runtime feature and I was asked to remove it. I'm not sure why
manifest-csp shouldn't follow that directive.

FWIW, Manifest being implemented in chromium, it might not really follow Blink
policy but I will send an intent to ship still. It should ship in M39 I believe.

[Mike West, 2014-09-18 13:41:31]

On 2014/09/18 13:35:21, Mounir Lamouri wrote:
> On 2014/09/18 07:49:23, Mike West (OOO until 24th) wrote:
> > Alternatively, if this is the only piece of Manifest that's implemented on
the
> > Blink side, then you could guard it behind the experiemental CSP flag (see
the
> > experimental block further down in CSPDirectiveList). Either way, it
shouldn't
> > be exposed to the web before we decide to ship Manifest
> 
> WebFrameClient is notified when a page has a manifest change. That was guarded
> by a Manifest runtime feature and I was asked to remove it. I'm not sure why
> manifest-csp shouldn't follow that directive.

If we haven't shipped manifest, then 'manifest-src' needs to be treated as an
invalid directive. I don't particularly care how we make that determination,
only that it is made. Hiding behind the experimental CSP flag is fine with me.

> FWIW, Manifest being implemented in chromium, it might not really follow Blink
> policy but I will send an intent to ship still. It should ship in M39 I
believe.

IMO: Manifest is a web-facing feature, and clearly falls under Blink's
guidelines. But I doubt we'll have to argue about that. :)

[kenneth.christiansen, 2014-09-18 13:58:24]

OK added the layout tests as requested. This, though, depends on
https://codereview.chromium.org/584553002 which doesn't pass CL upload hooks.
Any suggestion on implementing the same in an acceptable way would be highly
appreciated.

Mounir, if you want to give a try at fixing
https://codereview.chromium.org/584553002 feel free, as I am going on a business
travel tomorrow and will first be back tuesday afternoon.

[kenneth.christiansen, 2014-09-18 16:06:58]

New patch up for review, depends on https://codereview.chromium.org/584553002

[Mike West, 2014-09-29 11:00:47]

Looks good, but the patch fails to apply, and I'd like you to add a test which
verifies that manifests do load when they're supposed to.

A few other nits inline.

https://codereview.chromium.org/570563003/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/security/contentSecurityPolicy/manifest-src-blocked.html
(right):

https://codereview.chromium.org/570563003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/security/contentSecurityPolicy/manifest-src-blocked.html:2:
<link rel="manifest" href="http://www.evil.com/manifest.json">
Please add a test which verifies that a manifest which matches the policy is
loaded.

https://codereview.chromium.org/570563003/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/570563003/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:560: // FIXME(http://crbug.com/390497):
Make sure that CSP uses RequestContext instead of this terrible Resource::Type
enum.
Nit: Can you change this to something like "FIXME: Once we use RequestContext
for CSP ([bug link]), remove this extra check."?

https://codereview.chromium.org/570563003/diff/80001/Source/core/frame/csp/Co...
File Source/core/frame/csp/ContentSecurityPolicy.h (right):

https://codereview.chromium.org/570563003/diff/80001/Source/core/frame/csp/Co...
Source/core/frame/csp/ContentSecurityPolicy.h:89: // Manifest Directives (to be
merged into CSP 1.1)
Nit: Can you change this and line 80 to refer to "CSP Level 2"? We changed the
versioning.

https://codereview.chromium.org/570563003/diff/80001/Source/web/tests/WebFram...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/570563003/diff/80001/Source/web/tests/WebFram...
Source/web/tests/WebFrameTest.cpp:135: , m_notBaseURL("http://www.nottest.com/")
Nit: These shouldn't be real URLs. Would you mind changing them to
`test.example` and `nottest.example`? You don't need to do that in this CL, of
course, but since you're touching it, maybe you could clean it up in a
subsequent CL?

https://codereview.chromium.org/570563003/diff/80001/Source/web/tests/WebFram...
Source/web/tests/WebFrameTest.cpp:161:
response.addHTTPHeaderField("Content-Security-Policy",
WebString::fromUTF8(csp));
It would be good to add a test which verified that '-Report-Only' didn't block
the fetch.

[kenneth.christiansen, 2014-09-29 13:36:00]

Fixed the nits.

>
https://codereview.chromium.org/570563003/diff/80001/Source/web/tests/WebFram...
> Source/web/tests/WebFrameTest.cpp:135: ,
m_notBaseURL("http://www.nottest.com/")
> Nit: These shouldn't be real URLs. Would you mind changing them to
`test.example` and `nottest.example`? You don't need to do that in this CL, of
course, but since you're touching it, maybe you could clean it up in a
subsequent CL?

Yes, but maybe you want them called example.test and notexample.test ? I will do
it in a separate CL

[Mike West, 2014-09-29 18:17:20]

win_blink_rel is crashing on the new tests you added. I've thrown the patch to
win_blink_dbg in the hopes of getting you a stack trace.

[mlamouri (slow - plz ping), 2014-09-29 23:37:40]

On 2014/09/29 18:17:20, Mike West wrote:
> win_blink_rel is crashing on the new tests you added. I've thrown the patch to
> win_blink_dbg in the hopes of getting you a stack trace.

Maybe related: http://crbug.com/418814

[kenneth.christiansen, 2014-09-30 06:09:10]

On 2014/09/29 at 23:37:40, mlamouri wrote:
> On 2014/09/29 18:17:20, Mike West wrote:
> > win_blink_rel is crashing on the new tests you added. I've thrown the patch
to
> > win_blink_dbg in the hopes of getting you a stack trace.
> 
> Maybe related: http://crbug.com/418814

I don't have access to that one

[kenneth.christiansen, 2014-09-30 06:13:41]

On 2014/09/30 at 06:09:10, kenneth.christiansen wrote:
> On 2014/09/29 at 23:37:40, mlamouri wrote:
> > On 2014/09/29 18:17:20, Mike West wrote:
> > > win_blink_rel is crashing on the new tests you added. I've thrown the
patch to
> > > win_blink_dbg in the hopes of getting you a stack trace.
> > 
> > Maybe related: http://crbug.com/418814
> 
> I don't have access to that one

I actually do not see a crash in the log, but at least the -allowed test could
time out if the manifest is never fetched.

[kenneth.christiansen, 2014-09-30 09:01:23]

FYI: I got access to the link using my other account and VPN, looking at it now.

[Mike West, 2014-10-06 10:03:59]

https://codereview.chromium.org/570563003/diff/180001/LayoutTests/http/tests/...
File
LayoutTests/http/tests/security/contentSecurityPolicy/manifest-src-blocked.html
(right):

https://codereview.chromium.org/570563003/diff/180001/LayoutTests/http/tests/...
LayoutTests/http/tests/security/contentSecurityPolicy/manifest-src-blocked.html:1:
<meta http-equiv="Content-Security-Policy" content="manifest-src *">
This should probably be "manifest-src 'none'".

[Mike West, 2014-10-06 10:04:39]

LGTM otherwise, thanks for going a few rounds on this.

[kenneth.r.christiansen, 2014-10-06 10:12:51]

kenneth.r.christiansen@intel.com changed reviewers:
+ jochen@chromium.org

[kenneth.r.christiansen, 2014-10-06 14:40:32]

This latest patch changes now the failure (CSP blocking) is propagated to the
ManifestFetcher so that it knows that the load finished (with success or
failure)

The earlier patch following the sync path in DocumentThreadableLoader and called
m_client->didFail in case the resource was null (blocked by CSP). It seems
though that the client as implemented by the InspectorResourceAgent crashes as
the DocumentThreadableLoader gets freed in case the resource is null
(DocumentThreadableLoader::create).

The new patch follows the existing patch for not-allowed access in
AssociatedURLLoader and reports the error to the m_clientAdapter.

[Mike West, 2014-10-06 14:49:11]

On 2014/10/06 14:40:32, kenneth.r.christiansen wrote:
> The earlier patch following the sync path in DocumentThreadableLoader and
called
> m_client->didFail in case the resource was null (blocked by CSP). It seems
> though that the client as implemented by the InspectorResourceAgent crashes as
> the DocumentThreadableLoader gets freed in case the resource is null
> (DocumentThreadableLoader::create).

I'm not sure I understand how synchronous loading plays into this. The manifest
should be loaded asynchronously, right? Is this just a test problem in the
testrunner implementation that kicks off manifest loading?
 
> The new patch follows the existing patch for not-allowed access in
> AssociatedURLLoader and reports the error to the m_clientAdapter.

This seems like a reasonable hardening of the AssociatedURLLoader, but it's not
clear to me that it's the right place to fix the error.

[kenneth.r.christiansen, 2014-10-06 14:53:00]

On 2014/10/06 at 14:49:11, mkwst wrote:
> On 2014/10/06 14:40:32, kenneth.r.christiansen wrote:
> > The earlier patch following the sync path in DocumentThreadableLoader and
called
> > m_client->didFail in case the resource was null (blocked by CSP). It seems
> > though that the client as implemented by the InspectorResourceAgent crashes
as
> > the DocumentThreadableLoader gets freed in case the resource is null
> > (DocumentThreadableLoader::create).
> 
> I'm not sure I understand how synchronous loading plays into this. The
manifest should be loaded asynchronously, right? Is this just a test problem in
the testrunner implementation that kicks off manifest loading?

Yes, it should be async. The test which crashes has nothing to do with the
manifest loading.

>  
> > The new patch follows the existing patch for not-allowed access in
> > AssociatedURLLoader and reports the error to the m_clientAdapter.
> 
> This seems like a reasonable hardening of the AssociatedURLLoader, but it's
not clear to me that it's the right place to fix the error.

It already handles other - not allow - the same way. basically if (!allowLoad)
m_clientAdapter->setDelayedError(ResourceError());, so now I just do the same
for CSP.

[Mike West, 2014-10-06 15:06:12]

On 2014/10/06 14:53:00, kenneth.r.christiansen wrote:
> > This seems like a reasonable hardening of the AssociatedURLLoader, but it's
> not clear to me that it's the right place to fix the error.
> 
> It already handles other - not allow - the same way. basically if (!allowLoad)
> m_clientAdapter->setDelayedError(ResourceError());, so now I just do the same
> for CSP.

Ok, that makes more sense to me. I don't understand why it fails in this test,
however. This test has nothing to do with CSP blocking Manifest loads. What
changed in this CL that made it crash?

[kenneth.r.christiansen, 2014-10-06 15:18:02]

On 2014/10/06 at 15:06:12, mkwst wrote:
> On 2014/10/06 14:53:00, kenneth.r.christiansen wrote:
> > > This seems like a reasonable hardening of the AssociatedURLLoader, but
it's
> > not clear to me that it's the right place to fix the error.
> > 
> > It already handles other - not allow - the same way. basically if
(!allowLoad)
> > m_clientAdapter->setDelayedError(ResourceError());, so now I just do the
same
> > for CSP.
> 
> Ok, that makes more sense to me. I don't understand why it fails in this test,
however. This test has nothing to do with CSP blocking Manifest loads. What
changed in this CL that made it crash?

I can explain that. Because when the requestResource returns 0, I called
m_client->didFail in the previous patch (which was done in the sync code in
DocumentThreadableClient). That though means that the web inspector test gets a
failure twice - one from my didFail call (removed in latest patch and handled in
AssociatedLoader instead) and one from the !loader in which case it calls
didFailLoaderCreating. Each failure deletes the InspectorThreadableLoaderClient
as it calls dispose() which does delete this.

[Mike West, 2014-10-06 15:38:49]

On 2014/10/06 15:18:02, kenneth.r.christiansen wrote:
> On 2014/10/06 at 15:06:12, mkwst wrote:
> > On 2014/10/06 14:53:00, kenneth.r.christiansen wrote:
> > > > This seems like a reasonable hardening of the AssociatedURLLoader, but
> it's
> > > not clear to me that it's the right place to fix the error.
> > > 
> > > It already handles other - not allow - the same way. basically if
> (!allowLoad)
> > > m_clientAdapter->setDelayedError(ResourceError());, so now I just do the
> same
> > > for CSP.
> > 
> > Ok, that makes more sense to me. I don't understand why it fails in this
test,
> however. This test has nothing to do with CSP blocking Manifest loads. What
> changed in this CL that made it crash?
> 
> I can explain that. Because when the requestResource returns 0, I called
> m_client->didFail in the previous patch (which was done in the sync code in
> DocumentThreadableClient). That though means that the web inspector test gets
a
> failure twice - one from my didFail call (removed in latest patch and handled
in
> AssociatedLoader instead) and one from the !loader in which case it calls
> didFailLoaderCreating. Each failure deletes the
InspectorThreadableLoaderClient
> as it calls dispose() which does delete this.

OK. That makes sense. Can you combine the allowLoad check with the m_loader
check? That is, rather than an 'else' on the 'allowLoad 'if', would putting the
'if (!m_loader)' check after the whole 'if' have the same effect? That seems
clearer than repeating the error.

(Sorry. On a train. Typing is hard.)

[Mike West, 2014-10-06 15:39:30]

On 2014/10/06 15:38:49, Mike West wrote:
> On 2014/10/06 15:18:02, kenneth.r.christiansen wrote:
> > On 2014/10/06 at 15:06:12, mkwst wrote:
> > > On 2014/10/06 14:53:00, kenneth.r.christiansen wrote:
> > > > > This seems like a reasonable hardening of the AssociatedURLLoader, but
> > it's
> > > > not clear to me that it's the right place to fix the error.
> > > > 
> > > > It already handles other - not allow - the same way. basically if
> > (!allowLoad)
> > > > m_clientAdapter->setDelayedError(ResourceError());, so now I just do the
> > same
> > > > for CSP.
> > > 
> > > Ok, that makes more sense to me. I don't understand why it fails in this
> test,
> > however. This test has nothing to do with CSP blocking Manifest loads. What
> > changed in this CL that made it crash?
> > 
> > I can explain that. Because when the requestResource returns 0, I called
> > m_client->didFail in the previous patch (which was done in the sync code in
> > DocumentThreadableClient). That though means that the web inspector test
gets
> a
> > failure twice - one from my didFail call (removed in latest patch and
handled
> in
> > AssociatedLoader instead) and one from the !loader in which case it calls
> > didFailLoaderCreating. Each failure deletes the
> InspectorThreadableLoaderClient
> > as it calls dispose() which does delete this.
> 
> OK. That makes sense. Can you combine the allowLoad check with the m_loader
> check? That is, rather than an 'else' on the 'allowLoad 'if', would putting
the
> 'if (!m_loader)' check after the whole 'if' have the same effect? That seems
> clearer than repeating the error.
> 
> (Sorry. On a train. Typing is hard.)

Other than that, patchset 12 LGTM again.

[kenneth.r.christiansen, 2014-10-07 08:36:51]

The CQ bit was checked by kenneth.r.christiansen@intel.com

[commit-bot: I haz the power, 2014-10-07 08:38:03]

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

[commit-bot: I haz the power, 2014-10-07 08:42:57]

Committed patchset #13 (id:220001) as 183317