858836 - CSP inline style blocking doesn't work in the Firefox OS emulator

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Ian Melven :imelven]

I found this while running the patch for bug 763879 through try

I finally got the B2G emulator to run the mochitest for bug 763879 and verified that the try run was accurate - that is, that the inline styles were not blocked and had been incorrectly applied. 

I tried to add logging to gather more details about what's going on here, but have been unsuccessful so far. We don't want to hold up turning the CSP 1.0 parser on for desktop Firefox and we want to be more deliberate about turning it on for apps and documents on Firefox OS so I'm filing this as a followup, blocking bug 858787, which is the bug to turn on the 1.0 CSP parser for Firefox OS after discussing this with :sstamm and :pauljt.

Comment 1

[Ian Melven :imelven]

Note that the patch I'm about to attach to bug 763879 disables content/base/test/test_CSP_inlinestyle.html which should be re-enabled when this bug is fixed. The other CSP tests are also disabled on B2G, due to using observer notifications which don't work in B2G's multiprocess work, AIUI.

Comment 2

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #1)
> Note that the patch I'm about to attach to bug 763879 disables
> content/base/test/test_CSP_inlinestyle.html 

to be clear, disables it on B2G only.

 in B2G's multiprocess work,

sigh - s/work/world

Comment 3

[Ian Melven :imelven]

See https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Testing/Mochitests section "Technical Implementation Details" for the details of how mochitests are run in the emulator - I suspect what's going on is the default policy for the certified tests is causing some inline styles to be blocked, causing some mochitests to fail. I also suspect that the inline style mochitests themselves are failing due to looking at the wrong principals, due to being in an app (maybe one with unsafe-inline specified?)

Comment 4

[Garrett Robinson [:grobinson]]

Attachment: Patch 1

After some digging, the source of these test failures was in the code added to load per-app CSP's from manifests in Bug 773891. This code would first check if the principal in InitCSP() was that of an app, and if so it loaded the CSP from the manifest into one of the header strings. This replaced any CSP provided with a header (even if there was no CSP in the manifest, it still replaced the header CSP with an empty string). This appears to have been done to minimize the changes to the code required to implement the functionality in Bug 773891, but it makes the incorrect assumption that a Firefox OS app is never served with HTTP headers.

The test failure showed up here because of the way Mochitests are implemented on B2G (see https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Platform/Testing/Mochitests#Technical_Implementation_Details). Basically, a special app replaces the homescreen, and has an iframe with mozapp mozbrowser set which loads the mochitest as its src. Therefore, the mochitest is an app - but it is also served with HTTP headers from the test server. The previous code's logic caused it to silently erase the CSP provided in the headers, which caused the test failures.

Furthermore, since it is possible for a hosted Firefox OS app to be served with HTTP headers, we should fix this problem in the more general case. This patch resolves the issue by removing the old buggy logic, and applying any CSP from the manifest via refinePolicy after any app default CSP has been applied but before the csp from the headers is applied.

I assume that there is no need to handle the possibility of multiple CSP's combined and separated by commas for the manifest CSP, as that appears to be specific to HTTP headers. I also assume that the CSP from the manifest is 1.0 spec-compliant. If this cannot be assumed we will have to find a way to differentiate between CSP's in the manifest.

To test this patch, first apply alternate patch v4 from Bug 763879 and then apply this patch. I have confirmed that the test (content/base/test/test_CSP_inlinestyle.html) now works on both desktop Firefox and B2G builds (in the emulator).

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 750069 [details] [diff] [review]
Patch 1

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

Cool.  So this depends on the inline styles patch (this should land after bug 763879, but around the same time).  Is the intent to land the two at the same time, or land bug 763879 with b2g tests off, then land this one?

bz: can you take this review or are you too swamped?

Comment 6

[Garrett Robinson [:grobinson]]

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

Comment 7

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #5)
> Comment on attachment 750069 [details] [diff] [review]
> Patch 1
> 
> Review of attachment 750069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool.  So this depends on the inline styles patch (this should land after
> bug 763879, but around the same time).  Is the intent to land the two at the
> same time, or land bug 763879 with b2g tests off, then land this one?

probably the latter, hoping to land 763879 very, very soon.

Comment 8

[Ian Melven :imelven]

Comment on attachment 750069 [details] [diff] [review]
Patch 1

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

::: content/base/src/nsDocument.cpp
@@ +2550,5 @@
>        csp->RefinePolicy(appCSP, chanURI, specCompliantEnabled);
>    }
>  
> +  // Allow a per-app policy from the manifest
> +  if (appStatus != nsIPrincipal::APP_STATUS_NOT_INSTALLED) {

this will be true for certified/privileged apps, which we already handled in the applyAppDefaultCSP code above. i guess that's what we want though, since a manifest CSP should be able to further refine ie. tighten the default CSP. maybe add a comment to that effect ? or perhaps a comment saying we start with whatever default CSP if we're in a privileged/certified app, then refine with the manifest CSP, if there is one, and then refine with the document's CSP from its headers (if there are any). 

i think the main point to convey is that the order is important here.

Also: we shouldn't apply the manifest CSP if we're inside a mozbrowser (since GetAppStatus should return APP_STATUS_NOT_INSTALLED) - likewise with not applying the default CSP in a mozbrowser

i still don't understand why we DON'T get APP_STATUS_NOT_INSTALLED here actually, given that the mochitests run inside a mozbrowser. i'm concerned about CSP working correctly in documents inside a mozbrowser and wondering if it's that the document needs to be inside another <iframe> inside an <iframe mozbrowser> to have mInMozBrowser be true. Adding Justin to see if he can shed light on this.

@@ +2559,5 @@
> +      if (NS_SUCCEEDED(principal->GetAppId(&appId))) {
> +        appsService->GetCSPByLocalId(appId, manifestCSP);
> +        if (!manifestCSP.IsEmpty()) {
> +          // Assumes the CSP from the manifest is 1.0 spec compliant
> +          csp->RefinePolicy(manifestCSP, chanURI, true);

IMO, the 3rd arg should be specCompliantEnabled - all the CSP stuff should use the same pref to be consistent. as soon as this bug is fixed (and any other mochitests that fail with the 1.0 parser, this will always be true anyways - and like I said earlier, it was always true until very recently, so b2g18 will expect spec compliant CSP, which I think is good).

so yeah, we should assume the policy coming out of the manifest will be spec compliant, but still use the pref here (and endeavor to get the pref flipped ASAP).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750069 [details] [diff] [review]
Patch 1

Seems reasonable.

Comment 10

[Garrett Robinson [:grobinson]]

(In reply to Ian Melven :imelven from comment #8)
> Comment on attachment 750069 [details] [diff] [review]
> Patch 1
> 
> Review of attachment 750069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsDocument.cpp
> @@ +2550,5 @@
> >        csp->RefinePolicy(appCSP, chanURI, specCompliantEnabled);
> >    }
> >  
> > +  // Allow a per-app policy from the manifest
> > +  if (appStatus != nsIPrincipal::APP_STATUS_NOT_INSTALLED) {
> 
> this will be true for certified/privileged apps, which we already handled in
> the applyAppDefaultCSP code above. i guess that's what we want though, since
> a manifest CSP should be able to further refine ie. tighten the default CSP.
> maybe add a comment to that effect ? or perhaps a comment saying we start
> with whatever default CSP if we're in a privileged/certified app, then
> refine with the manifest CSP, if there is one, and then refine with the
> document's CSP from its headers (if there are any). 
> 

Is that not clear from the code?

> i think the main point to convey is that the order is important here.

Is it? refinePolicy performs a set intersection (commutative), so the order in which we refinePolicy based on multiple CSP's doesn't matter.

> Also: we shouldn't apply the manifest CSP if we're inside a mozbrowser
> (since GetAppStatus should return APP_STATUS_NOT_INSTALLED) - likewise with
> not applying the default CSP in a mozbrowser
> 
> i still don't understand why we DON'T get APP_STATUS_NOT_INSTALLED here
> actually, given that the mochitests run inside a mozbrowser. i'm concerned
> about CSP working correctly in documents inside a mozbrowser and wondering
> if it's that the document needs to be inside another <iframe> inside an
> <iframe mozbrowser> to have mInMozBrowser be true. Adding Justin to see if
> he can shed light on this.
> 

The mochitests run inside of an <iframe mozbrowser mozapp> - see http://mxr.mozilla.org/gaia/source/test_apps/test-container/index.html. Therefore, it makes sense to me that we get APP_STATUS_INSTALLED. mozbrowser AFAICT just resets the documents inheritance on the window object so it forms the logical root of a new document tree for anything inside the iframe (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe). I would hope that an <iframe mozbrowser> would not be an app - it might be worth testing this just to be sure going forward.

Justin, if you have any insight it would be greatly appreciated.

> @@ +2559,5 @@
> > +      if (NS_SUCCEEDED(principal->GetAppId(&appId))) {
> > +        appsService->GetCSPByLocalId(appId, manifestCSP);
> > +        if (!manifestCSP.IsEmpty()) {
> > +          // Assumes the CSP from the manifest is 1.0 spec compliant
> > +          csp->RefinePolicy(manifestCSP, chanURI, true);
> 
> IMO, the 3rd arg should be specCompliantEnabled - all the CSP stuff should
> use the same pref to be consistent. as soon as this bug is fixed (and any
> other mochitests that fail with the 1.0 parser, this will always be true
> anyways - and like I said earlier, it was always true until very recently,
> so b2g18 will expect spec compliant CSP, which I think is good).
> 
> so yeah, we should assume the policy coming out of the manifest will be spec
> compliant, but still use the pref here (and endeavor to get the pref flipped
> ASAP).

I agree. Will post an updated patch with this change, plus any others necessitated by your above comments once I've heard back from you.

Comment 11

[Ian Melven :imelven]

(In reply to Garrett Robinson [:grobinson] from comment #10)
> > this will be true for certified/privileged apps, which we already handled in
> > the applyAppDefaultCSP code above. i guess that's what we want though, since
> > a manifest CSP should be able to further refine ie. tighten the default CSP.
> > maybe add a comment to that effect ? or perhaps a comment saying we start
> > with whatever default CSP if we're in a privileged/certified app, then
> > refine with the manifest CSP, if there is one, and then refine with the
> > document's CSP from its headers (if there are any). 
> > 
> 
> Is that not clear from the code?

well, not as clear as an explicit comment ;)
 
> > i think the main point to convey is that the order is important here.
> 
> Is it? refinePolicy performs a set intersection (commutative), so the order
> in which we refinePolicy based on multiple CSP's doesn't matter.

My instinct was that order was important since you can only ever 'tighten' the policies, but yeah, after looking at intersectWith and thinking about it a bit, I think you're right (note that 'none' is a special case...). Just as an FYI, the CSP 1.1 spec defines how multiple policies should work (the 1.0 spec leaves it up to the implementer) : https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#enforcing-multiple-policies.
 
> > Also: we shouldn't apply the manifest CSP if we're inside a mozbrowser
> > (since GetAppStatus should return APP_STATUS_NOT_INSTALLED) - likewise with
> > not applying the default CSP in a mozbrowser
> > 
> > i still don't understand why we DON'T get APP_STATUS_NOT_INSTALLED here
> > actually, given that the mochitests run inside a mozbrowser. i'm concerned
> > about CSP working correctly in documents inside a mozbrowser and wondering
> > if it's that the document needs to be inside another <iframe> inside an
> > <iframe mozbrowser> to have mInMozBrowser be true. Adding Justin to see if
> > he can shed light on this.
> > 
> 
> The mochitests run inside of an <iframe mozbrowser mozapp> - see
> http://mxr.mozilla.org/gaia/source/test_apps/test-container/index.html.
> Therefore, it makes sense to me that we get APP_STATUS_INSTALLED. mozbrowser
> AFAICT just resets the documents inheritance on the window object so it
> forms the logical root of a new document tree for anything inside the iframe
> (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe). I
> would hope that an <iframe mozbrowser> would not be an app - it might be
> worth testing this just to be sure going forward.

The case I'm thinking of here is a browser app using a mozbrowser that loads documents with a CSP on them. In that case, we want the CSP of the document to be used (based on the headers) and NOT the CSP of the browser app, which may break sites/documents the browser is loading. Since the mochitest documents which have CSP headers are loaded into an <iframe> which is a child of an <iframe> mozbrowser, I'd expect mInMozBrowser to be true there and hence to get 
APP_STATUS_NOT_INSTALLED. Then we could check for this, and ignore any app default/manifest CSP and just use the headers in that case.

> Justin, if you have any insight it would be greatly appreciated.

setting needsinfo for jlebar. Justin, if you could help us understand the above, that would be much appreciated :)

Comment 12

[Garrett Robinson [:grobinson]]

(In reply to Garrett Robinson [:grobinson] from comment #10)
> > Also: we shouldn't apply the manifest CSP if we're inside a mozbrowser
> > (since GetAppStatus should return APP_STATUS_NOT_INSTALLED) - likewise with
> > not applying the default CSP in a mozbrowser
> > 
> > i still don't understand why we DON'T get APP_STATUS_NOT_INSTALLED here
> > actually, given that the mochitests run inside a mozbrowser. i'm concerned
> > about CSP working correctly in documents inside a mozbrowser and wondering
> > if it's that the document needs to be inside another <iframe> inside an
> > <iframe mozbrowser> to have mInMozBrowser be true. Adding Justin to see if
> > he can shed light on this.
> > 
> 
> The mochitests run inside of an <iframe mozbrowser mozapp> - see
> http://mxr.mozilla.org/gaia/source/test_apps/test-container/index.html.
> Therefore, it makes sense to me that we get APP_STATUS_INSTALLED. mozbrowser
> AFAICT just resets the documents inheritance on the window object so it
> forms the logical root of a new document tree for anything inside the iframe
> (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe). I
> would hope that an <iframe mozbrowser> would not be an app - it might be
> worth testing this just to be sure going forward.
> 

Actually, you raise a valid point. Sorry for the confusion, there's a lot of nested iframes to keep track of here! This is added clarification of the situation for jlebar et al.

So the document ultimately looks like this (based on [1] and [2]).

test-container/index.html
  <iframe id="test-container" mozbrowser mozapp="...">
    --> .src is set to the mochitest [3]
    <iframe id="cspframe*"></iframe>
      --> .src is file_CSP_inlinestyle_*.html

We are surprised to see an appId and an appStatus of APP_STATUS_INSTALLED on the innermost iframes. Getting an appId could possibly be understood in the context of the comment at nsIPrincipal.idl:215 - the appId returned is that of the containing iframe, which is an app. However, this comment and the comment for appStatus suggest that the app status of such a contained iframe should be APP_STATUS_NOT_INSTALLED. Is this reading correct, and we are witnessing a bug?

[1] gaia/source/test_apps/test-container/index.html
[2] content/base/test/test_CSP_inlinestyle.html
[3] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtestsb2g.py#78

Comment 13

[Garrett Robinson [:grobinson]]

Attachment: Patch 2

Incorporates imelven's feedback to

* comment on the order of refinePolicy
* use specCompliantEnabled to determine which parser (pre-1.0 or 1.0) to use for the app manifest

I also realized that the code that checks for the manifest CSP needs to be run earlier in order to avoid erroneously returning early if there is only a manifest CSP to apply. The new code does the right thing and passes all of the CSP tests.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 752793 [details] [diff] [review]
Patch 2

r=me, I think, based on interdiff

Comment 15

[Ian Melven :imelven]

Comment on attachment 752793 [details] [diff] [review]
Patch 2

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

::: content/base/src/nsDocument.cpp
@@ +2490,3 @@
>    bool unknownAppId;
> +  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> +  uint32_t appId = 0;

why did you move the declaration of appId out of the block in which it's used ?

@@ +2580,5 @@
> +  if (applyAppManifestCSP) {
> +    // Use the 1.0 CSP parser for apps if the pref to do so is set.
> +    csp->RefinePolicy(appManifestCSP, chanURI, specCompliantEnabled);
> +  }
> +

i thought at first we could condense these, but then the comment above reminded me
there could be a case where we have a default app CSP *and* a CSP in the app manifest :)

Comment 16

[Ian Melven :imelven]

Comment on attachment 752793 [details] [diff] [review]
Patch 2

I midaired bz ;(

Comment 17

[Ian Melven :imelven]

Removing the needinfo for jlebar. Discussed with David Chan and learned that if content has the same origin as the app itself it's treated as part of the app content. Since the mochitest app's src is set to be http://mochi.test:8888 and this is where the CSP test documents (which have CSP headers served with them), they are considered to be 'vouched for' by the app, and hence have APP_STATUS_INSTALLED. We verified this by changing the tests to load the CSP test documents cross origin and they then had APP_STATUS_NOT_INSTALLED as expected. Garrett's patch fixes the issue in both cases - in the APP_STATUS_INSTALLED case, we no longer stomp any CSP received in headers with the manifest CSP as we were doing.

Comment 18

[Garrett Robinson [:grobinson]]

(In reply to Ian Melven :imelven from comment #15)
> Comment on attachment 752793 [details] [diff] [review]
> Patch 2
> 
> Review of attachment 752793 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsDocument.cpp
> @@ +2490,3 @@
> >    bool unknownAppId;
> > +  uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> > +  uint32_t appId = 0;
> 
> why did you move the declaration of appId out of the block in which it's
> used ?

Oops - that was leftover from some code I was using for debugging. Will post a final patch to fix.

Comment 19

[Garrett Robinson [:grobinson]]

Attachment: Patch 3

Fixes nit from imelven's last feedback. All CSP tests pass. Carrying over r+ from bz since change was trivial.

Comment 20

[Garrett Robinson [:grobinson]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/f063968fe9ad

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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