1180637 - Packaged Apps do not apply CSP

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Frederik Braun [:freddy]]

My test sends a strict Content-Security-Policy header that only allows scripts from the same origin, but no inline scripts.

In my testing an index.html within the package may do all those things the CSP disallows. 

Valentin, is this a good bug for you or should we defer this to someone else?

Comment 1

[Frederik Braun [:freddy]]

Attachment: example package

Attaching the package I used for testing

Comment 2

[Frederik Braun [:freddy]]

(In reply to Frederik Braun [:freddyb] from comment #0)
> Valentin, is this a good bug for you or should we defer this to someone else?

Heh, sorry. I forgot to CC you.
Can you take a look?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I already have a patch incoming. Thanks for filing the bug.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Packaged Apps do not apply CSP

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8630339 [details] [diff] [review]
Packaged Apps do not apply CSP

Review of attachment 8630339 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +74,5 @@
> +  }
> +
> +  nsAutoCString value;
> +  httpChan->GetResponseHeader(NS_LITERAL_CSTRING("Content-Security-Policy"), value);
> +  aHead->SetHeader(nsHttp::ResolveAtom("Content-Security-Policy"), value);

since you copy just the CSP header, should this be called CopyCSPHeader?

::: netwerk/protocol/http/PackagedAppService.h
@@ +88,5 @@
>      // so packaged resources can be accessed over https.
>      nsresult CopySecurityInfo(nsIChannel *aChannel);
>  
> +    // Copy headers that apply to all resources in the package
> +    static nsresult CopyHeaders(nsIChannel *aChannel, nsHttpResponseHead *aHead);

express in the comment the copy direction or even better name the method better like CopyHeadersFromChannel or so..

Comment 6

[Frederik Braun [:freddy]]

Considering that this patch "just" copies the header… are there any way we we could do it so that packages loop into the "normal" content loading flow?

I'm asking because I suppose that we may have bugs similar to this one but for X-Frame-Options, Strict-Transport-Security, etc.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #6)
> Considering that this patch "just" copies the header… are there any way we
> we could do it so that packages loop into the "normal" content loading flow?
> 
> I'm asking because I suppose that we may have bugs similar to this one but
> for X-Frame-Options, Strict-Transport-Security, etc.

I suppose we need to copy all of the applicable headers from the package channel to the cache entries.
I don't think there is a way to do this more easily.
Could you file a new bug for the rest of the headers?

Thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ade76b9d40ef

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ade76b9d40ef

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

dveditz suggested this needs a release note for 42. Valentin, can you suggest wording for a release note and something we can link to? Thanks.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> dveditz suggested this needs a release note for 42. Valentin, can you
> suggest wording for a release note and something we can link to? Thanks.

This bug makes all the resources in a package [1] inherit the same "Content-Security-Policy" header that the package response had. Bug 1181137 (landed in fx43) improves upon this by copying all the allowed headers from the package to each individual resource.

[1] The new package format is defined at https://wiki.mozilla.org/FirefoxOS/New_security_model
    It is based on https://github.com/w3ctag/packaging-on-the-web with the exception that subresources are identified by a unique URL such as protocol://domain/path/to/package.pak!//path/to/subresource.html
The !// separator is essential to identifying a package.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Liz, this is for b2g mostly if I understand correctly. Why do you think we should relnote it?

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Because al mentioned that dveditz thought we should relnote it and he was just about to get on an airplane. Can you check with Daniel some time this week?

Comment 14

[Daniel Veditz [:dveditz]]

I may have misunderstood this bug when I got concerned about it. Since this type of package is a new feature, not the existing web app package types, we don't need to warn users of it.