768029 - Apply CSP policy to privileged and certified apps

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

[Lucas Adamski [:ladamski]]

All trusted and certified apps should enforce a default CSP policy to protect against code injection attacks.  This means we need to define what that policy should be, and how to enforce consistently it at runtime for all app assets.

Comment 1

[Lucas Adamski [:ladamski]]

Recommended policy: "script-src: 'self'; object-src: 'none'; style-src: 'self'"

This would effectively prevent code and style injection attacks.  The object-src is hopefully unnecessary but would provide a valuable defense in depth for platforms where plugins may be present (by disallowing plugin content).  

Still allows cross-domain loading of frames, images, media and other assets.  Does not restrict XHR.

Comment 2

[Lucas Adamski [:ladamski]]

Point was made we should be explicit about the default policy, so adding "default-src: *".  

New recommended policy: "default-src: *; script-src: 'self'; object-src: 'none'; style-src: 'self'"

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Sounds good.  This means that apps will be required to host their own script libraries (e.g., jquery) from their own domain.

Comment 4

[Lucas Adamski [:ladamski]]

Since we are looking at the package approach for trusted and certified apps they would just bundle the framework in there, but I agree that it adds a wrinkle to the model.  The alternative approach of using asset hashing could have let us potentially use caching to avoid re-downloading common framework files, but looks like we aren't going down that path.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

So what happens if a given app contains an iframe... do we apply the "global CSP" to that iframe too?  (The behavior on the web is that CSP policies are only applied to the document with which they're served, so if a top level document has a CSP but an iframe does not, that iframe's content is not protected.)

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: first whack

Here's a first whack at a patch.  There's no tests (and I haven't tested manually yet), but I'd like some feedback on the direction.

Basically this modifies nsDocument to look both in HTTP headers and also in a pref for a policy when the principal of the document being loaded says it's an app.

I don't know how to go about testing this... any pointers to similar types of tests would be really really helpful.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 646784 [details] [diff] [review]
first whack

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

I think it'd be also awesome to support a "csp" property in the manifest which would allow specifying a policy which is merged in with existing policies. But that can be left for another bug.

::: content/base/src/nsDocument.cpp
@@ +2432,5 @@
>  
> +    // ----- Figure out if we need to apply an app default CSP
> +    bool applyAppDefaultCSP = false;
> +    nsIPrincipal* principal = GetPrincipal();
> +    if (principal) {

Just call NodePrincipal() and remove the nullcheck.

@@ +2471,5 @@
> +    // ----- process the app default policy, if necessary
> +    if (applyAppDefaultCSP) {
> +      const nsAdoptingString& appCSP = Preferences::GetString("security.apps.CSP.default");
> +      if (appCSP)
> +        mCSP->RefinePolicy(appCSP, chanURI);

Does this "merge" with any existing policies by only allowing things that both policies allow?

@@ +2494,5 @@
> +      // headers present.  If there are, then we ignore the ReportOnly mode and
> +      // toss a warning into the error console, proceeding with enforcing the
> +      // regular-strength CSP.
> +      if (cspHeaderValue.IsEmpty()) {
> +        mCSP->SetReportOnlyMode(true);

Hmm.. we can't really allow this for privileged/certified apps, right? (even though there right now won't be a way to get in here for privileged/certified apps since they don't have headers)

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> I think it'd be also awesome to support a "csp" property in the manifest
> which would allow specifying a policy which is merged in with existing
> policies. But that can be left for another bug.

Bug 773891.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Comment on attachment 646784 [details] [diff] [review]
> first whack
> 
> Review of attachment 646784 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it'd be also awesome to support a "csp" property in the manifest
> which would allow specifying a policy which is merged in with existing
> policies. But that can be left for another bug.
> 
> ::: content/base/src/nsDocument.cpp
> @@ +2432,5 @@
> >  
> > +    // ----- Figure out if we need to apply an app default CSP
> > +    bool applyAppDefaultCSP = false;
> > +    nsIPrincipal* principal = GetPrincipal();
> > +    if (principal) {
> 
> Just call NodePrincipal() and remove the nullcheck.
> 
> @@ +2471,5 @@
> > +    // ----- process the app default policy, if necessary
> > +    if (applyAppDefaultCSP) {
> > +      const nsAdoptingString& appCSP = Preferences::GetString("security.apps.CSP.default");
> > +      if (appCSP)
> > +        mCSP->RefinePolicy(appCSP, chanURI);
> 
> Does this "merge" with any existing policies by only allowing things that
> both policies allow?
> 
> @@ +2494,5 @@
> > +      // headers present.  If there are, then we ignore the ReportOnly mode and
> > +      // toss a warning into the error console, proceeding with enforcing the
> > +      // regular-strength CSP.
> > +      if (cspHeaderValue.IsEmpty()) {
> > +        mCSP->SetReportOnlyMode(true);
> 
> Hmm.. we can't really allow this for privileged/certified apps, right? (even
> though there right now won't be a way to get in here for
> privileged/certified apps since they don't have headers)

Good call.  To be sure, privileged/certified apps are always locally installed?  

I'll disable support for ReportOnlyMode for privileged/certified apps.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> > +        mCSP->RefinePolicy(appCSP, chanURI);
> 
> Does this "merge" with any existing policies by only allowing things that
> both policies allow?

Yep.  RefinePolicy makes things more strict (e.g., can only block more things than it did before.

> > +        mCSP->SetReportOnlyMode(true);
> 
> Hmm.. we can't really allow this for privileged/certified apps, right? (even
> though there right now won't be a way to get in here for
> privileged/certified apps since they don't have headers)

Good call.  To be sure, privileged/certified apps are always locally installed?  

I'll disable support for ReportOnlyMode for privileged/certified apps.

Thanks Jonas.  I'm having trouble testing this right now since it seems like there's some dependencies that aren't implemented yet (marking an app as privileged/certified).  Are there bugs open and patches or tests for this?

Comment 11

[Lucas Adamski [:ladamski]]

Correct, privileged & certified apps (app packaged apps) are always locally installed.

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Attachment: second whack

So here's a patch with tests that simulate apps.  Problem is they're loaded from example.org (not installed locally) and there doesn't seem to be any plumbing that will their status to be APP_STATUS_TRUSTED.  If the if statement in nsDocument.cpp is tweaked to also include APP_STATUS_INSTALLED as valid target for the app default CSP, the tests will pass.  (Thanks, Myk!)

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

While nsIPrincipal is ready to recognize apps, nothing is setting app values as TRUSTED or CERTIFIED.  Blocking on that connection glue.

Comment 14

[Lucas Adamski [:ladamski]]

We should land this with a pref set of off for the moment, so we can test and update Gaia apps w/o rendering the device unusable.

Comment 15

[Lucas Adamski [:ladamski]]

Looks like https://bugzilla.mozilla.org/show_bug.cgi?id=781620 has landed, let me know if you're still blocked.  Most of Gaia is "eagerly" awaiting this patch.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Lucas Adamski from comment #1)
> Recommended policy: "script-src: 'self'; object-src: 'none'; style-src:
> 'self'"

So what about inline stylesheets and inline scripts?  Do we want to allow those?  (I imagine we want to stop inline scripts, but what about stylesheets)?

For context, see bug 763879: For CSP 1.0 (the new w3c proposal), we will be blocking inline styles unless the policy says to allow them.

Comment 17

[Lucas Adamski [:ladamski]]

Per discussions with Ben Francis, there should be little to no inline CSS in Gaia so we should block both.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Attachment: third whack with working tests

Thanks Fabrice for the pointers to automation.py.in where I could add a couple of webapps that are privileged and certified.  Could you take a look at that and the rest of this to let me know what you think?

Also, who should I flag for review on the test automation bits?

This patch is on Try: https://tbpl.mozilla.org/?tree=Try&rev=9cc750ff7a58

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 657397 [details] [diff] [review]
third whack with working tests

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

The changes in automation.py.in look fine to me, but I'm no build peer. That works well in any case, and I will also reuse this in bug 787487.

::: b2g/app/b2g.js
@@ +353,5 @@
>  // from other dirs as webgl textures and more.  Remove me when we have
>  // installable apps or wifi support.
>  pref("security.fileuri.strict_origin_policy", false);
>  
> +// Default Content Security Policy to apply to trusted and certified apps

Nit: s/trusted/privileged

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 650736 [details] [diff] [review]
second whack

Ted, could you review the automation.py.in changes?
Mounir, could you review the rest?

I will fix the nit Fabrice pointed out, but will wait for review feedback first.

Comment 21

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 657397 [details] [diff] [review]
third whack with working tests

arrgh, wrong patch.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If "inline CSS" means both <style>...</style> elements, as well as <div style="..."> attributes, then I think it would be unfortunate to disable those.

While gaia apps might not use them, style attributes, and setting .style.X is very common on the web. I think the cost of disabling those is quite high whereas the value is much lower than for disabling inline script.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #22)
> If "inline CSS" means both <style>...</style> elements, as well as <div
> style="..."> attributes, then I think it would be unfortunate to disable
> those.

The CSP spec calls out attribute-based styles as something to block.  We could possibly build something in that turns that part off for apps, but I'd rather not make the implementation more complex than it needs to be.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I wasn't proposing just enabling style="..", but also enabling <style>..</style>. Is that possible?

I don't think either poses a significantly high threat though it is of course non-zero.

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Oh sure, we can enable all inline styles.

The currently shipping version of CSP doesn't actually disable them, but version 1.0 will (see bug 763879).  We can write a CSP that allows inline stylesheets via the style element and through attribute settings.

Lucas: do you think this is a tolerable risk?

Comment 26

[Lucas Adamski [:ladamski]]

I can tolerate the risk but my concern is that we'll need to go to CSP 1.0 compliant policy sooner than later, at which point we'll break a lot of apps.  I'd rather break them from the get-go.  This only applies to privileged and certified, which currently is only Gaia so breakage at time=0 should be 0.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does CSP1.0 not have a way to enable "inline CSS" the way it has a way to enable "inline script"? (Or does it no longer have a way to enable inline script?)

Comment 28

[Lucas Adamski [:ladamski]]

CSP 1.0 does but we aren't proposing to let apps relax the default restrictions.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. I feel like we're talking past each other.

What I'm proposing is that we use the following as the built-in CSP policy for privileged/certified apps:

script-src: 'self'; object-src: 'none'; style-src: 'self' 'unsafe-inline'


However, as far as I understand Sid's comments, our current CSP implementation doesn't support the 'unsafe-inline' keyword for style-src, but instead always allow inline CSS. Hence we would for now use 

script-src: 'self'; object-src: 'none'; style-src: 'self'

which in our current CSP implementation has exactly the same behavior as the first policy above. Once we do fix our CSP implementation to be 1.0 compliant, we'd also change the policy to be 

script-src: 'self'; object-src: 'none'; style-src: 'self' 'unsafe-inline'

which wouldn't be a behavioral change from the app's point of view.


But I might very well be misunderstanding something.

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 657397 [details] [diff] [review]
third whack with working tests

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

::: b2g/app/b2g.js
@@ +355,5 @@
>  pref("security.fileuri.strict_origin_policy", false);
>  
> +// Default Content Security Policy to apply to trusted and certified apps
> +pref("security.apps.CSP.applyDefaultPolicy", false);
> +pref("security.apps.CSP.default", "default-src *; script-src 'self'; object-src 'none'; style-src 'self'");

Just curious, do we only care about this on B2G? I guess we don't have the concept of "trusted apps" on other platforms yet?

::: build/automation.py.in
@@ +518,5 @@
>          'name': 'http_example_org',
>          'origin': 'http://example.org',
>          'manifestURL': 'http://example.org/manifest.webapp',
> +        'description': 'http://example.org App',
> +        'appStatus': '1'

Might be nice to define constants for appStatus somewhere so it's clear what we're setting.

@@ +552,5 @@
> +      {
> +        'name': 'https_example_com_privileged',
> +        'origin': 'https://example.com',
> +        'manifestURL': 'https://example.com/manifest_priv.webapp',
> +        'description': 'https://example.com Priviliged App',

"Privileged"

::: content/base/src/nsDocument.cpp
@@ +2461,4 @@
>        return NS_OK;
>      }
>  
>  #ifdef PR_LOGGING 

Can you remove the trailing space from this line while you're here?

@@ +2469,5 @@
>      nsCOMPtr<nsIContentSecurityPolicy> mCSP;
>      mCSP = do_CreateInstance("@mozilla.org/contentsecuritypolicy;1", &rv);
>  
>      if (NS_FAILED(rv)) {
>  #ifdef PR_LOGGING 

Trailing space on this line too, if you don't mind.

@@ +2485,5 @@
> +      if (appCSP)
> +        mCSP->RefinePolicy(appCSP, chanURI);
> +#ifdef PR_LOGGING
> +      else
> +        PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("App but no default CSP in 'security.apps.CSP.default'"));

You should probably put both branches of this conditional in braces. With the #ifdef there it could get confusing.

@@ +2500,1 @@
>  #ifdef PR_LOGGING 

Argh trailing space.

@@ +2523,5 @@
> +          }
> +#endif
> +        }
> +      } else {
> +        //XXX(sstamm): maybe we should post a warning when both read only and regular 

Trailing space. Also if you're going to leave XXX comments in you should probably file followup bugs for them.

Comment 31

[Lucas Adamski [:ladamski]]

Sid can better clarify, but since we are also working on CSP 1.0 compliance I thought the idea was we'd ship CSP 1.0.  If its not then I agree its somewhat of a moot point for basecamp.

I suggested not including 'unsafe-inline' in the policy since Gaia doesn't need it (and that's currently 100% of our packaged apps), along with the fact that its generally considered a bad practice and few serious developers do that.  In fact externalizing all CSS is a lot more common than externalizing all JS AFAIK, which is what we require anyway.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Lucas, it's your call what to use for policy. However I believe that inline CSS is *very* common, specifically through the use of a style attribute. Basically any time someone uses element.style.X = "y", they are using inline style.

We can certainly still forbid inline CSS, but it's likely going to require just as big changes as forbidding inline script (which I absolutely agree that we need to forbid).

Comment 33

[Sid Stamm [:geekboy or :sstamm]]

Regardless of the policy decision, I still need review to land this patch.  I am waiting to address ted's comments until I get the other review back. Jonas: do you want to do it?  Mounir is currently flagged, but he's probably buried under his review queue.

Comment 34

[Lucas Adamski [:ladamski]]

I care mostly that whatever policy we pick we are willing to live with for a *long* time, as it will be all but impossible to change this in the future due to the resultant breakage.

Sid, can you provide some background as to what drove the inclusion of default blocking of inline styles in CSP 1.0?  If this is a defense in depth change that is not addressing a specific (and compelling) threat, then I could live this hole... forever.

Comment 35

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Lucas Adamski from comment #34)
> Sid, can you provide some background as to what drove the inclusion of
> default blocking of inline styles in CSP 1.0?

I don't have the whole background, but based on discussing it with Adam Barth, the main reason for this is to block attribute selectors -- apparently they can be used for things like password theft.

> If this is a defense in depth
> change that is not addressing a specific (and compelling) threat, then I
> could live this hole... forever.

I think that's the other half of it (aside from attribute selectors).  Making sure we can block more robust and more-like-a-programming-language features put into CSS.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We can always block the more-like-a-programming-language features if they are added to CSS. I.e. use a policy like the:

script-src: 'self'; object-src: 'none'; style-src: 'self' 'unsafe-inline'; style-caps 'block-xbl2'

once "xbl2" is added.


I'd like to understand more how the password attack would work. Can you get more details?


Another thing to keep in mind is that this is just the default policy that we are applying to privileged/certifed apps in order to protect the elevate permissions that we are granting them. It's not intended to stop all types of attacks, right?

In particular, we really should be allowing apps to opt in to further tightened policies if they want. See bug 773891. Though obviously that won't make v1.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 657397 [details] [diff] [review]
third whack with working tests

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

::: content/base/src/nsDocument.cpp
@@ +2505,5 @@
> +      // headers present.  If there are, then we ignore the ReportOnly mode and
> +      // toss a warning into the error console, proceeding with enforcing the
> +      // regular-strength CSP.
> +      if (cspHeaderValue.IsEmpty()) {
> +        mCSP->SetReportOnlyMode(true);

Doesn't this still have the problem that you can turn on read-only mode and thus effectively disable the built-in CSP?

In general, I can't tell what you are actually changing here. Whitespace?

Comment 38

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #37)
> ::: content/base/src/nsDocument.cpp
> @@ +2505,5 @@
> > +      // headers present.  If there are, then we ignore the ReportOnly mode and
> > +      // toss a warning into the error console, proceeding with enforcing the
> > +      // regular-strength CSP.
> > +      if (cspHeaderValue.IsEmpty()) {
> > +        mCSP->SetReportOnlyMode(true);
> 
> Doesn't this still have the problem that you can turn on read-only mode and
> thus effectively disable the built-in CSP?

Yes, good catch.  Wasn't a problem before when there was only one source for the policy (HTTP header).  I'll fix this by ignoring report-only headers when in an app.  We can tweak this later and allow the report-only policy set via http header when we land bug 552523 (supports multiple policies at once -- report only and enforced).

> In general, I can't tell what you are actually changing here. Whitespace?

Yes, indentation due to an if block.  See the 20 lines prior to it for additions and indentation context.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could you attach another patch just so we can ensure that the report-only code is correct.

Comment 40

[Lucas Adamski [:ladamski]]

After sleeping on this here's what I think:
a) we don't need to enforce inline style blocking for privileged apps; the goal of CSP here is to protect against code injection attacks, not data theft.  As Jonas pointed out, developers will be able to define additional CSP restrictions in the future.
b) we should enforce it for certified apps as we care more about data theft there and (assuming Ben is right) there is no cost for doing so
c) we can find creative ways of blocking future extensions to CSS that would re-introduce the risk of code injection

Comment 41

[Mounir Lamouri (:mounir)]

Comment on attachment 657397 [details] [diff] [review]
third whack with working tests

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

r=me regarding new principal's API for apps. I can't really comment on anything else.

::: content/base/src/nsDocument.cpp
@@ +2440,5 @@
> +    // ----- Figure out if we need to apply an app default CSP
> +    bool applyAppDefaultCSP = false;
> +    nsIPrincipal* principal = GetPrincipal();
> +
> +    if (Preferences::GetBool("security.apps.CSP.applyDefaultPolicy")) {

I think it would be better to be explicit about the default value being false.

::: content/base/test/chrome/test_csp_bug768029.html
@@ +4,5 @@
> +  https://bugzilla.mozilla.org/show_bug.cgi?id=768029
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test for CSP on trusted/certified apps -- bug 768029</title>

nit: s/trusted/privileged/

@@ +181,5 @@
> +var checksTodo = gData.length;
> +
> +// quick check to make sure we can test apps:
> +is('appStatus' in document.nodePrincipal, true,
> +   'appStatus should be present in nsIPrincipal, if not the rest of this test will fail');

I'm not sure if this test is actually useful.

@@ +206,5 @@
> +var gTestRunner = runTest();
> +
> +// load the default CSP and pref it on
> +SpecialPowers.pushPrefEnv({'set': [["dom.mozBrowserFramesEnabled", true],
> +                                   ["dom.mozBrowserFramesWhitelist", "https://example.com"],

That's no longer being used. You have to set a permission.

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Lucas Adamski from comment #40)
> After sleeping on this here's what I think:
> a) we don't need to enforce inline style blocking for privileged apps; the
> goal of CSP here is to protect against code injection attacks, not data
> theft.  As Jonas pointed out, developers will be able to define additional
> CSP restrictions in the future.
> b) we should enforce it for certified apps as we care more about data theft
> there and (assuming Ben is right) there is no cost for doing so
> c) we can find creative ways of blocking future extensions to CSS that would
> re-introduce the risk of code injection

This sounds good to me.

Comment 43

[Ben Francis [:benfrancis]]

(In reply to Lucas Adamski from comment #40)
> b) we should enforce it for certified apps as we care more about data theft
> there and (assuming Ben is right) there is no cost for doing so

If this includes element.style.X = "y" from JavaScript then there is definitely a cost of doing so, and it's a potentially significant one.

Comment 44

[Lucas Adamski [:ladamski]]

(In reply to Ben Francis [:benfrancis] from comment #43)
> 
> If this includes element.style.X = "y" from JavaScript then there is
> definitely a cost of doing so, and it's a potentially significant one.

It would not effect setting styles from JS, only within HTML.  Though inline JS is obviously blocked too, so it would have to be from externalized JS.

Comment 45

[Sid Stamm [:geekboy or :sstamm]]

So sounds like either way we want two policies: one for privileged, and one for certified.  I'll include 'unsafe-inline' for privileged apps (so they can use inline styles) but not for certified apps.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Lucas Adamski from comment #44)
> (In reply to Ben Francis [:benfrancis] from comment #43)
> > 
> > If this includes element.style.X = "y" from JavaScript then there is
> > definitely a cost of doing so, and it's a potentially significant one.
> 
> It would not effect setting styles from JS, only within HTML.

Are you sure this is correct? CSS generally involves a lot of string parsing, so you can't really make the distinction "from JS" vs. "from markup" like we can in script.

For example setting element.style.border = X involves parsing a complex syntax and setting a number of CSS properties.

I would imaging that disabling inline styles will disable any and all interactions with element.style.

Comment 47

[Sid Stamm [:geekboy or :sstamm]]

it's not clear if this CSP setting will be disabling *all* inline styles or just some of them.  The spec says just the style tag and the style attribute should be disabled (I think you're supposed to still be able to set styles from JS).  See bug 763879 comment 22 for more info.

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The CSP 1.0 spec says [1] that if unsafe-inline isn't in style-src then:

"Whenever the user agent would apply style from a style attribute, instead the user agent must ignore the style."

Setting element.style.X sets the style attribute and the style is applied from there. So I would say that the spec definitely currently disables element.style.X

[1] http://www.w3.org/TR/CSP/#style-src

Comment 49

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also note that if we add a more-like-a-programming-language CSS property at some point in the future, you will most likely be able to set it using:

element.style.languageFeature = "somestring";

Comment 50

[Lucas Adamski [:ladamski]]

(In reply to Jonas Sicking (:sicking) from comment #48)
> The CSP 1.0 spec says [1] that if unsafe-inline isn't in style-src then:
> 
> "Whenever the user agent would apply style from a style attribute, instead
> the user agent must ignore the style."
> 
> Setting element.style.X sets the style attribute and the style is applied
> from there. So I would say that the spec definitely currently disables
> element.style.X
> 
> [1] http://www.w3.org/TR/CSP/#style-src

I think we can decide how this really applies, especially if there's ambiguity.  To prohibit that from JS doesn't make any sense.  Controlling CSS is a defense-in-depth measure once you've locked down JS.  If you can inject JS, disabling inline styles is completely meaningless.  So prohibiting setting of style attributes from JS is likewise pointless IMO.

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How is this different from eval()?

The reason we disable eval("...") is because a page could be taking a string from an untrusted source and then turning that into harmful content (in this case javascript).

The reason I'm arguing that style-src should disable element.style.X = "..." is because a page could be taking a string from an untrusted source and then turning that into harmful content (in this case evil CSS).

The question isn't if the action is happening from script or not, but if it results in parsing of strings from unverified sources and turning it into harmful behavior.

I definitely do agree that there's some difference between eval() and element.style.X because in the latter the page limits the set of CSS properties that can be set to a pretty small number. So the page has some sense of what effects untrusted data can cause. But I don't think it's clearcut and I definitely think that there needs to be a way for CSP to disable even element.style.X.

Ultimately though, I think this is a question of what the CSP1.0 spec calls for, which is a question for the W3C WG, no?

Comment 52

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, I think we agree in the important aspects here. I'm totally fine with the policy for certified apps to be:


script-src: 'self'; object-src: 'none'; style-src: 'self' 'some-token';

where 'some-token' causes style attributes in the markup to to be blocked but allows .style.X = "..." to be used.

If that 'some-token' is 'unsafe-inline' or something else I'm happy to leave up to the W3C webapp sec WG.

Comment 53

[Sid Stamm [:geekboy or :sstamm]]

FWIW, that token is the opposite.  'unsafe-inline' in the style-src directive means "I get to use inline styles".  They're disabled by default in csp 1.0, and the token name is already decided upon by the WG.  :)

If you want to discuss the particulars of the inline style blocking, please head on over to bug 763879.  I want to finish this patch with a pair of policies that we can tweak with follow-ups if we need to.

Comment 54

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Mounir Lamouri (:mounir) from comment #41)
> r=me regarding new principal's API for apps. I can't really comment on
> anything else.

Thanks, I'll rely on Jonas for the rest.

> ::: content/base/src/nsDocument.cpp
> @@ +2440,5 @@
> > +    // ----- Figure out if we need to apply an app default CSP
> > +    bool applyAppDefaultCSP = false;
> > +    nsIPrincipal* principal = GetPrincipal();
> > +
> > +    if (Preferences::GetBool("security.apps.CSP.applyDefaultPolicy")) {
> 
> I think it would be better to be explicit about the default value being
> false.

Not sure I understand your ask here: do you want a comment that says "this defaults to false, so if you want this to work, you have to set this pref to true"?

My current working patch (not on this bug yet) no longer has this -- the policy is always applied.  Do we want a way to pref off this application of CSP to apps?

> @@ +206,5 @@
> > +var gTestRunner = runTest();
> > +
> > +// load the default CSP and pref it on
> > +SpecialPowers.pushPrefEnv({'set': [["dom.mozBrowserFramesEnabled", true],
> > +                                   ["dom.mozBrowserFramesWhitelist", "https://example.com"],
> 
> That's no longer being used. You have to set a permission.

I don't understand this... what is no longer used and what permission do I have to set?

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sid: I'd still like to see a new patch for due to the report-only issue. See comment 38 and comment 39.

Comment 56

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Absolutely, Jonas.  I took another pass this afternoon and was gonna wait to see how it did on try before uploading it and flagging it for review.  But here it is anyway, complete with code that ignores and warns about report-only policies being applied via HTTP headers on documents that have a real policy.

https://tbpl.mozilla.org/?tree=Try&rev=29fd62cf5ae3

Looks like the new tests are failing (my local machine was taking too long to build so I didn't test them locally before pushing to try), so I'll have to debug that a bit.  In the meantime take a look, and perhaps you can help me understand what Mounir was saying (see comment 54).

Comment 57

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 660680 [details] [diff] [review]
proposed patch

Carrying over the additional r=mounir,ted

Comment 58

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Sid Stamm [:geekboy] from comment #54)
> (In reply to Mounir Lamouri (:mounir) from comment #41)
> > > +// load the default CSP and pref it on
> > > +SpecialPowers.pushPrefEnv({'set': [["dom.mozBrowserFramesEnabled", true],
> > > +                                   ["dom.mozBrowserFramesWhitelist", "https://example.com"],
> > 
> > That's no longer being used. You have to set a permission.
> 
> I don't understand this... what is no longer used and what permission do I
> have to set?

I think I understand now.  I did some digging around, and I'm guessing I have to set the "browser" permission on the specified origin using SpecialPowers.setPermission.

Like this: https://bugzilla.mozilla.org/attachment.cgi?id=651898&action=diff

The other way still seemed to work (mozilla-central rev pulled yesterday).  Is the plan to phase this out, or is the old way still going to work?

Comment 59

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

Fixed the failing tests -- there was a bug in the pref names being used in the tests.

Back on try for verification: https://tbpl.mozilla.org/?tree=Try&rev=92abaab3da91

Comment 60

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 660888 [details] [diff] [review]
proposed patch

Again, carrying over the additional r=mounir,ted, but waiting on review from Jonas.

Comment 61

[Sid Stamm [:geekboy or :sstamm]]

Looks like this patch causes an assertion in nsPrincipal:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15195499&tree=Try#error0

The assertion has to do with the call to GetAppStatus() in InitCSP().  Is there another way to check whether the document's principal thinks it is an app?  The assertion I'm hitting (nsPrincipal.cpp:1088) requires that all calls to GetAppStatus() happen on apps.

Comment 62

[Jason Smith [:jsmith]]

Sid - How hard would it be to extend this implementation to also work with untrusted packaged apps? See https://bugzilla.mozilla.org/show_bug.cgi?id=768868#c32 for context.

Comment 63

[Sid Stamm [:geekboy or :sstamm]]

Technically it would be pretty easy, as far as I can tell... just adding a third policy (for the installed app category), and changing a few bits of logic in nsDocument.cpp.

This bug is currently blocked on an asserting test (see comment 61), which I have to fix before thinking about adding support for other classes of apps.

Comment 64

[Jason Smith [:jsmith]]

(In reply to Sid Stamm [:geekboy] from comment #63)
> Technically it would be pretty easy, as far as I can tell... just adding a
> third policy (for the installed app category), and changing a few bits of
> logic in nsDocument.cpp.
> 
> This bug is currently blocked on an asserting test (see comment 61), which I
> have to fix before thinking about adding support for other classes of apps.

Okay. I filed a followup bug for this in bug 791396.

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 660888 [details] [diff] [review]
proposed patch

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

::: content/base/src/nsDocument.cpp
@@ +2532,5 @@
> +#endif
> +        }
> +      } else {
> +        // post a warning when both read only and regular CSP policies are present.
> +        if (applyAppDefaultCSP || !cspROHeaderValue.IsEmpty()) {

All the logic conditions here make my head hurt. The contents of this is run if

(!cspHeaderValue.IsEmpty() || !cspROHeaderValue.IsEmpty()) &&
!(!cspHeaderValue.IsEmpty() && !cspROHeaderValue.IsEmpty()) &&
(applyAppDefaultCSP || !cspROHeaderValue.IsEmpty())

Which.. well.. I *think* simplifies to

(cspHeaderValue.IsEmpty() ^^ cspROHeaderValue.IsEmpty()) &&
(applyAppDefaultCSP || !cspROHeaderValue.IsEmpty())



Can you move this if-statement outside of the |else| and make it

if (applyAppDefaultCSP && !cspROHeaderValue.IsEmpty())

Isn't that when you want to put in the warning?

Comment 66

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Lucas Adamski from comment #40)
> After sleeping on this here's what I think:
> a) we don't need to enforce inline style blocking for privileged apps; the
> goal of CSP here is to protect against code injection attacks, not data
> theft.  As Jonas pointed out, developers will be able to define additional
> CSP restrictions in the future.

Sorry for piping in so late. I don't understand this. The goal of CSP is to protect against code injection attacks. And the goal of most of those attacks is, pure and simply, data theft. Because that's where the money is. Making a dancing monkey appear on my screen may be irritating, but there's not ROI in it. Get my Paypal password, authentication token, or similar, though, and now we're talking seriously.

And frankly, I as a user wouldn't care if the data was stolen because some unwanted JS code ran on the security context of Paypal, or because of some freak CSS construct that side-leaked the data out, even with JS disabled.

> b) we should enforce it for certified apps as we care more about data theft
> there and (assuming Ben is right) there is no cost for doing so

I think it should be enforced for both certified and privileged, because of the previous reason. Either that, or enforce Rule #4 [1] on the review process. And I don't think we can do that on server side content anyway.


[1] https://www.owasp.org/index.php/XSS_%28Cross_Site_Scripting%29_Prevention_Cheat_Sheet#RULE_.234_-_CSS_Escape_And_Strictly_Validate_Before_Inserting_Untrusted_Data_into_HTML_Style_Property_Values

Comment 67

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Antonio: I can agree with you in certain respects in the abstract.

Concretely speaking though:
1. Do you think it's unacceptable that the default CSP for privileged apps permits
   using inline CSS?
2. If yes, do you think that it's unacceptable that *normal* apps can use inline CSP
   considering that they too will hold bank passwords and other user sensitive data.
3. Do you feel that way even considering that we will add the ability for apps to
   opt in any more strict CSP policy (though probably not for v1).
4. How exactly are you imagining that an attacker would use *only* CSS (i.e. no
   script) to steal passwords or other sensitive data?

Comment 68

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #67)
> Antonio: I can agree with you in certain respects in the abstract.
> 
> Concretely speaking though:
> 1. Do you think it's unacceptable that the default CSP for privileged apps
> permits using inline CSS?

I don't think it's "unnacceptable". I think it's a risk. And that we can allow either inline CSS or things like:

445 function setStatus(msg) {
446 showBannerStatus();
447 document.getElementById('uploaded').innerHTML = msg;
448 }

Well, we probably shouldn't allow that in any case. But specially no if we're allowing inline-anything. As I told originally, if we're going unsafe-inline for CSS, we should *enforce* Rule #4. And that means filtering external content that's shown. 

Does that mean I think we *must* turn off inline completely? No. But if we don't then we should define and enforce escaping rules for certified and privileged apps. Not doing both of those things would be... sucky.


> 2. If yes, do you think that it's unacceptable that *normal* apps can use
> inline CSP considering that they too will hold bank passwords and other user
> sensitive data.

If I could change it for all the apps I would. But that would mean all browsers changing the behavior at the same time... and that will happen a couple of days after hell gets frozen I guess :). So I take the pragmatic approach: Normal apps, fortunately, are not my problem :) 


> 3. Do you feel that way even considering that we will add the ability for
> apps to opt in any more strict CSP policy (though probably not for v1).

That they can opt-in for more strict CSP policy is great, and will work great for normal apps that take security seriously. The default should be good enough though. 

> 4. How exactly are you imagining that an attacker would use *only* CSS (i.e.
> no script) to steal passwords or other sensitive data?

The examples I've seen take advantage of the original formatting of the page, and they use CSS to automatically fetch something from the attacker web site that includes a secret that was used internally on the page/reserved for other site. 

See [1] for an example. The problem here (for me) is that I know almost nothing about CSS, and thus are not qualified to propose new attacks, specially not generic ones. 

[1] http://scarybeastsecurity.blogspot.com.es/2009/12/generic-cross-browser-cross-domain.html

Comment 69

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That article definitely brings up an interesting attack vector. It's very short on details and I think he is outright wrong on some things (this is clearly a yahoo bug, though browser behavior is making it worse. And cookies doesn't matter one way or another afaict).

It's finding an exploit vector which exists anytime you allow:
* Inline style elements. In markup or created through DOM.
* Inline style attributes. In markup or created through DOM.
* element.style.X = "...", though in a way that's harder to exploit. I.e. probably
  requires more unlikely code in the website.

I'm still not convinced that it makes sense to be more restrictive for privileged apps than normal apps. The attack applies equally much to them. If we think this is a problem we should apply it across the board.

Comment 70

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #65)
> All the logic conditions here make my head hurt.

Yeah, sorry, this evolved over time to include a bunch of new use cases.  I need to untangle the spaghetti.

[snip]

> Can you move this if-statement outside of the |else| and make it
> 
> if (applyAppDefaultCSP && !cspROHeaderValue.IsEmpty())
> 
> Isn't that when you want to put in the warning?

I want to warn in this case, but I also want to warn when there's a regular cspHeaderValue and a cspROHeaderValue (since only one of those works at a time).  This was all to remove an XXX comment that's been in CSP for a long time that Ted pointed out; was an easy fix, figured I'd tag it along.  I suppose I could post the warning under this condition:

  if (!cspROHeaderValue.IsEmpty() && 
      (applyAppDefaultCSP || !cspHeaderValue.IsEmpty()))

then have an else clause to apply the report-only policy.

Basically, the app and cspHeaderValue can co-exist, but the cspROHeaderValue can't co-exist with either.  How about I do some code restructuring here to make it clearer.

pseudocode:

 if (app) 
   apply app policy;

 if (cspHeaderValue)
   apply cspHeaderValue;

 if (cspROHeaderValue)
   if (cspHeaderValue || app)
     warn;
   else
     apply cspROHeaderValue;

I'll restructure the patch to follow this logic and factor out the "apply policy x" into a subroutine since it's used three times.  I'll also re-flag you for review due to this and hold off on landing since it sounds like we're not sure to what app classes we want to apply policies (and which policies to apply) -- though that's the easy part of this patch.

Comment 71

[Lucas Adamski [:ladamski]]

CSP doesn't prevent injection of outright HTML so its never intended to prevent all injection attacks.  You can inject HTML forms till the cows come home (or user hits submit).

Security engineering team has been doing research into this and the consensus is that styles applied from JS are NOT covered in the scope of blocking inline styles.  We can continue debating this (in the other bug Sid referenced) but I want to put a fork in this debate and get this thing done.

Comment 72

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Lucas Adamski from comment #71)
> Security engineering team has been doing research into this and the
> consensus is that styles applied from JS are NOT covered in the scope of
> blocking inline styles.  We can continue debating this (in the other bug Sid
> referenced) but I want to put a fork in this debate and get this thing done.

I don't know if I'm reading this correctly, but if I am, then this means there should be not much problem with using a CSP with style-src: 'self' (withouth the 'unsafe-inline' option). Is that right?

Comment 73

[Antonio Manuel Amaya Calvo (:amac)]

Oh, and in any case, I agree we should land this. Changing the default policy (to be more relaxed or restrictive) is something that can be done at a later time, I think.

Comment 74

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jonas Sicking (:sicking) from comment #69)
> I'm still not convinced that it makes sense to be more restrictive for
> privileged apps than normal apps. The attack applies equally much to them.
> If we think this is a problem we should apply it across the board.

That's correct, but the problem for non-privileged apps is the general web problem. And since I don't see a default policy being enforced on the WWW anytime soon, and normal web pages should work as well (or as badly) on the phone than on the desktop, I don't think that's something we can do realistically. 

Something I would like to get (but that's not an argument for this bug, I think) is an indicator that tells the user how serious the page takes HIS security, and that could include things like 'does the page has a CSP?' 'how strict it is?'

Comment 75

[Mozilla RelEng Bot]

Try run for 7919cd10b22f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7919cd10b22f
Results (out of 15 total builds):
    exception: 1
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7919cd10b22f

Comment 76

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Mozilla RelEng Bot from comment #75

Heh, the one time I get a post from the RelEng bot, it's the run I cancelled.

Comment 77

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch (refactored code)

Jonas, here's an updated patch with the logic rework I mentioned in comment 70 that will hopefully not tie your brain in knots.  Please take a look and let me know how it will work.  It's on try along with a fix to bug 791284:
https://tbpl.mozilla.org/?tree=Try&rev=e39c1221326e

Comment 78

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 661942 [details] [diff] [review]
proposed patch (refactored code)

Again, carrying over the additional r=mounir,ted since the relevant parts to their concerns haven't changed.  mounir, ted, let me know if you want to take another look.

Comment 79

[Sid Stamm [:geekboy or :sstamm]]

> https://tbpl.mozilla.org/?tree=Try&rev=e39c1221326e

The new tests for this bug are failing on try due to bug 790747 landing (InitCSP moved earlier in StartDocumentLoad).  I'm not sure why yet, but digging into it.  Moving InitCSP back to a later part of StartDocumentLoad fixes the problem.  Is the app status set mid-way through StartDocumentLoad?

Comment 80

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch (refactored code)

This patch will undo some of 790747.  That bug was preliminary work for a feature that isn't ready yet, and this takes priority.  When we land this patch, I'll re-open bug 790747 and explain why (fixing it with this patch requires non-trivial additional work/complexity with no pay-off).

This is currently on try with bug 791284 (again), but this time I checked the tests that failed last time before pushing it.
https://tbpl.mozilla.org/?tree=Try&rev=90e71284981c

Jonas: if you can take a look, I'd appreciate it.  Bug 791284 is pretty short and needs your eyes too; I'd like to land these two together.

Comment 81

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 661942 [details] [diff] [review]
proposed patch (refactored code)

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

::: content/base/src/nsDocument.cpp
@@ +2599,5 @@
> +  if (docShell) {
> +    bool safeAncestry = false;
> +
> +    // PermitsAncestry sends violation reports when necessary
> +    rv = csp->PermitsAncestry(docShell, &safeAncestry);

This can't cause read-only flags to get set or anything like that, right?

Comment 82

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jonas Sicking (:sicking) from comment #81)
> > +    rv = csp->PermitsAncestry(docShell, &safeAncestry);
> 
> This can't cause read-only flags to get set or anything like that, right?

Nope, PermitsAncestry just returns whether or not the docShell is okay with the policy.  The only side-effect it can cause is the sending of a violation report (which won't happen for the app default policy).

-Sid

Comment 83

[Sid Stamm [:geekboy or :sstamm]]

So InitCSP calls GetAppId, which in turn ends up in nsPrincipal::GetAppId and causes an assertion in a few places due to creation of nsDocuments with a SimpleCodebasePrincipal.  I filed bug 791284 for the first one of these I found.  Here are some more places:

content/base/src/nsDocShell.cpp:8360
browser/extensions/pdfjs/components/PdfStreamConverter.js:543
layout/tools/reftest/reftest.js:853

There are oranges in the try run
https://tbpl.mozilla.org/?tree=Try&rev=90e71284981c

This means that a document is being created for some things with an appId == UNKNOWN_APP_ID, and when InitCSP asks for the AppStatus from the principal, the principal throws an assertion fit.  These are showing up in browser-chrome mochitests, reftests and crashtests.

My patch either:

1.  needs a safe way to determine if something is an app (without assertions), 
2.  or we need to clear up the uses of GetSimpleCodebasePrincipal that cause nsDocuments to be made.  These can be add-ons to bug 791284, but that'll make this bug blocked until we root them all out.

I could continue pushing try runs and finding new places where there's an UNKNOWN_APP_ID in a document's principal, but it's unclear how deep this rabbit hole goes.  Having a safe "isApp" value on the principal would save some headache, and would allow CSP to skip all the app-related processing if the principal has UNKNOWN_APP_ID.

Comment 84

[Sid Stamm [:geekboy or :sstamm]]

This can probably be pushed, but only after we figure out what to do about the assertions in nsPrincipal (see comment 83) and the patch in bug 791284 lands.  Those two things must be fixed before this lands or we'll get assertions in debug builds for crashtests, reftests and some of the browser chrome tests.

Comment 85

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix assertions

Somehow my private key is no longer accepted after upgrading to OSX 10.8, so I can't push this to try.

Mounir, does this look ok to you? If so, could you push it to try to see if it passes without assertions?

Comment 86

[Mounir Lamouri (:mounir)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=c07a23a378ac

Comment 87

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:mounir) from comment #86)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=c07a23a378ac

This patch passed try.

Comment 88

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 663976 [details] [diff] [review]
Fix assertions

I figured out my push problem, so if you r+ this I'll push it.

Comment 89

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/integration/mozilla-inbound/rev/691976d47b57

Comment 90

[[:fabrice] Fabrice Desré]

I had to back out because that broke gaia (see https://github.com/mozilla-b2g/gaia/issues/5177).
We'll re-land once gaia is ready.

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

Comment 91

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked this in again with a policy which allows inline scripts for certified apps to allow gaia to work.

We still use the correct policy for privileged apps though, so all should be fine for 3rd party app development.

I'll file a followup to fix the policy once gaia can handle it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fcabe5f5ca5

Comment 92

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For what it's worth, the reason we ended up backing this out was that we couldn't figure out how to enable inline scripts. It was easy to find what the specced syntax was, and I was even able to find what the "old, as originally proposed by mozilla" syntax was. But neither worked.

Ian Melven and Dan Veditz helped on irc to put together a policy which eventually actually worked.

Comment 93

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/4fcabe5f5ca5

Comment 94

[Carmen Jimenez Cabezas]

Shouldn't the follow up bug be opened already so this doesn't fall between the lines? Amongst other thing we would like to have something as a reference in Gaia issues to fix inline scripts usage.

(In reply to Jonas Sicking (:sicking) from comment #91)
> Checked this in again with a policy which allows inline scripts for
> certified apps to allow gaia to work.
> 
> We still use the correct policy for privileged apps though, so all should be
> fine for 3rd party app development.
> 
> I'll file a followup to fix the policy once gaia can handle it.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4fcabe5f5ca5

Comment 95

[Lucas Adamski [:ladamski]]

This is why I'd suggesting making this feature opt-in via a pref to enable Gaia devs to test, then once we've updated the Gaia apps we could just flip the pref.

Comment 96

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Lucas Adamski from comment #95)
> This is why I'd suggesting making this feature opt-in via a pref to enable
> Gaia devs to test, then once we've updated the Gaia apps we could just flip
> the pref.

That sounds great too. Still, as this one is marked as FIXED a new bug should be opened to track that, shouldn't it?

Comment 97

[Antonio Manuel Amaya Calvo (:amac)]

Wait, I didn't read you correctly, sorry. It seems that it's already a pref:

https://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#363

So if that pref can be overridden on the profile preferences, then that should allow Gaia devs to test their apps on a controlled manner as you suggested.

Comment 98

[Lucas Adamski [:ladamski]]

I would have preferred that the *.CSP.default prefs are set to the final policy we intend to enforce and have separate boolean prefs to enable them.

Comment 99

[Carmen Jimenez Cabezas]

While looking for a way to specify a selective policy for an app I've found bug 773891, which would allow to do that, but isn't implemented. Besides that being the best solution for testing (since it would allow selective testing) that's something tha should be implemented definitely. 

As I said there, we can take that bug if you wish.

Comment 100

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I filed bug 797657

The advantage of adjusting the policy pref rather than a separate pref is that it was easier to do. It also allows us to add restrictions gradually as we pass them.