Closed Bug 858787 Opened 7 years ago Closed 5 years ago

Flip the pref to turn on the CSP 1.0 parser for Firefox OS

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S4 (20june)
blocking-b2g -
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: imelven, Assigned: geekboy)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 3 obsolete files)

We want to get CSP 1.0 enabled for desktop Firefox as soon as possible. In the Firefox OS world, the inline style blocking in bug 763879 appears to not work correctly in the emulator and we also want to co-ordinate switching to the new parser for Firefox OS, since it will change behavior (the old parser did not block inline styles and uses different syntax than the 1.0 spec compliant default CSP policies used by B2G). This bug is for that purpose.
Blocks: csp-w3c-1.0
Blocks: CSP
Depends on: 858836
There's no patch on this bug, do we need a patch to flip the pref?
Flags: needinfo?(imelven)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #1)
> There's no patch on this bug, do we need a patch to flip the pref?

yes - same as bug 842657 but in wherever the B2G prefs live.
Flags: needinfo?(imelven)
Attached patch Patch 1 (obsolete) — Splinter Review
Try run: https://tbpl.mozilla.org/?tree=Try&rev=0eaf8e33b94c
Assignee: nobody → grobinson
Status: NEW → ASSIGNED
Attachment #765619 - Flags: review?(imelven)
Comment on attachment 765619 [details] [diff] [review]
Patch 1

The patch looks right ;)

As we just discussed on irc, it looks like we need to do a try run with opt builds to get the tests to run for B2G.
Attachment #765619 - Flags: review?(imelven) → feedback+
I suggest "try: -b do -p ics_armv7a_gecko,emulator,panda,unagi -u all -t none" fwiw.
That didn't seem to work. Trying with imeleven's suggestion: "try: -b do -p ics_armv7a_gecko,emulator,panda,unagi -u all -t none". https://tbpl.mozilla.org/?tree=Try&rev=823f13bf1f47
Try run looked ok, but there were a few suspicious failures. :imelven commented off the bug:

mochitest 1 2 and 9 on B2G Arm (VM) opt look a little suspect to me.

the m-1 failures don't look that much like https://bugzilla.mozilla.org/show_bug.cgi?id=775551 and there's other failing/timed out tests than the one mentioned in that bug. there's no unfixed orange candidates for m-2 and m-9. i starred the other ones.
Quick update: the test failures are on multiprocess (B2G/e10s) and are due to the intentional lack of an http-on-modify-request observer in the child process (bug 806753, bug 827269). I am working on a solution to create a proxy in SpecialPowers specifically for tests that will allow them to observer (but not modify) requests, which is all our tests need to do anyway.

If this doesn't work, I will work on re-enabling the availabilty of this observer in the child. I want to avoid this if possible, as it increases attack surface for sandboxing and is a lot of work for a small subset of tests.
Depends on: 945268
Attached patch 858787-b2g-csp-1.0-pref.patch (obsolete) — Splinter Review
Un-bitrotted.
Attachment #765619 - Attachment is obsolete: true
Attachment #8359103 - Flags: feedback+
Now that 945268 has landed, we should be able to land this trivially. Try: https://tbpl.mozilla.org/?tree=Try&rev=3c18db6f7817

Assuming that's green, is there any other work we need to do to encourage or advise people to use 1.0? The default headers are cross-compliant, so at least there are no problems there.
(In reply to Garrett Robinson [:grobinson] from comment #10)
>
> Assuming that's green, is there any other work we need to do to encourage or
> advise people to use 1.0? The default headers are cross-compliant, so at
> least there are no problems there.

All the docs I could find around B2G/Apps and CSP use the 1.0 syntax.

The default policies use 1.0 syntax as well : http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js

But note that since the 1.0 parser hasn't been used on FirefoxOS yet, there's some risk of breakage there when things like inline styles being blocked are actually enforced. Hopefully the try run will shake this out to some degree. I'd definitely suggest giving the B2G folks and Gaia folks a big heads up via whatever their respective mailing lists are :)
(In reply to Ian Melven :imelven from comment #11)
> But note that since the 1.0 parser hasn't been used on FirefoxOS yet,
> there's some risk of breakage there when things like inline styles being
> blocked are actually enforced. Hopefully the try run will shake this out to
> some degree. I'd definitely suggest giving the B2G folks and Gaia folks a
> big heads up via whatever their respective mailing lists are :)

Bingo - on try, looks like there are some test failures being caused by enforcing inline style blocking.
Actually, I'm not sure that inline style blocking is causing those test failures. I'm trying to get PR_LOGGING output so I can better understand which policies are being applied.

Now that inline style blocking is enabled, we are seeing the following error due to the use of inline style in the test-container app (which uses the default certified app CSP):

GeckoConsole: [JavaScript Warning: "Content Security Policy: The page's settings blocked the loading of a resource: An attempt to apply inline style sheets has been blocked" {file: "app://test-container.gaiamobile.org/index.html" line: 0 column: 0 source: "position: absolute; left: 0; top: 0px; b..."}]

This doesn't affect the tests, but would be good to fix since it generates misleading CSP violations.
Spent some time investigating the treatment of CSP in the FxOS Mochitest test harness. I was confused because the Mochitest sources are loaded into an iframe mozapp with a manifest that describes it as a "certified" app. This was confusing because the default "certified app" CSP blocks inline scripts and styles, and Mochitests use those heavily. I would expect almost no Mochitest to successfully run if the test (test_*.html) were executed with the certified app CSP.

As it turns out, the certified app CSP was not being applied to the Mochitest iframe [0]. This is because the iframe also has the "mozbrowser" attribute. This causes nsPrincipal::GetAppStatus to bail out early [1] and return APP_STATUS_NOT_INSTALLED instead of APP_STATUS_CERTIFIED (which is what I initially expected due to the manifest).

This is a little counter-intuitive, but I believe it is what we want. It does not apply a default policy to the Mochitest iframe, which would break almost all mochitests. Since the check is in GetAppStatus (therefore general, not specific to CSP), all of Gecko should treat this frame consistently.

Alternatively, we could make this more intuitive by not making the test-container a certified app, although that might break things.

[0] http://mxr.mozilla.org/gaia/source/test_apps/test-container/index.html?force=1#16
[1] http://dxr.mozilla.org/mozilla-central/source/caps/src/nsPrincipal.cpp#536
Depends on: 967252
Depends on: 969126
I've been working on this consistently, but struggled a lot with setting up a local B2G test environment that would give me consistent results. Now that I finally have one, here's an update.

New try run (with attached patch): https://tbpl.mozilla.org/?tree=Try&rev=c9514dd54b72

Numerous tests are broken, with no obvious pattern. Since none of these tests set a CSP, it suggests that the default certified app CSP is causing breakage. Furthermore, wrt the certified app default CSP, the only change between X- and 1.0 is that inline style blocking is now enforced. As a test, I created a patch that modifies the default certified app CSP to allow inline styles (so, making the 1.0 policy mimic the behavior of the pre-1.0 enforcement of that same policy).

With that simple change, the tree is green: https://tbpl.mozilla.org/?tree=Try&rev=3a91192ed9c4

This is still surprising in the context of Comment 14, where I showed that the certified app CSP is not being applied to the Mochitest iframe (even though it has a manifest that says it's certified) because the iframe has the mozbrowser attribute. Now, if the certified app CSP were being correctly applied, I would expect every Mochitest to break - they all use inline scripts and styles. The fact that only some of them do suggests there are problems with enforcement coverage of CSP in the context of B2G apps.
Depends on: 984716
Since it also took me a while to get the b2g desktop environment set up to test/verify CSP behavior on Linux Desktop, I wanted to list the steps to get it set up here. Generally instructions on this page [1] are pretty awesome, but unfortunately slighlty outdated, maybe someone of the b2g folks can update that.

Anyway, two things you have to do differently in order to get it set up correclty for testing:

1) in your /gaia folger you should do: make
2)a) run b2g-desktop by using the following command:
     .../b2g-bin -profile gaia/profile (note, it's not profile-debug)
  b) The best way to run mochitests is by using the following commands [2]:
     export GAIA_PROFILE=path/to/non/debug/gaia/profile
     ./mach mochitest-b2g-desktop test_path 

Thanks gwagner for your help!

[1] https://developer.mozilla.org/en-US/Firefox_OS/Using_the_B2G_desktop_client
[2] https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Mochitests
Flipping the pref to spec.compliant caused tests on TBPL to fail running the 'B2G ICS Emulator'.
While working on Bug 951457, the new CSP (C++) implementation, I took a closer look why those tests are failing.
It seems that adding 'unsafe-inline' to the style-src attribute [1] caused tests to run normally, which indicates that some inlined style/css code causes tests to fail.
Unfortunately, it was really hard to reproduce the errors I got on TBPL locally, running the B2G Emulator (arm). Under [3] I pasted all the tests that fail on TBPL, where pass/fail indicates whether tests are passing locally or not.

So, now the good news, even though I am not completely sure why this is not breaking all of the tests, only some, (seems like various race conditions), testcase dom/events/test/test_bug432698.html allowed me to reason what's going on.
When running tests on the emulator, we load all the tests into a test-container [2] which uses an inline style, which gets blocked by CSP. Why this blocking is causing issues only on some of the tests, that I don't know, that's why I list all the tests that are failing under [3], so that a B2G peer can take a look and enlighten us.

Anyway, not inlining the style/css in [2], but rather using:

> <head>
>  <meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1">
>  <link rel="stylesheet" type="text/css" href="hotfix.css" ></style>
> </head>

and putting the css information in the file hotfix.css in the same directory as the index.html causes tests to run normally without the need of adding 'unsafe-inline' to [1].

I am going to go ahead and do a pull request on b2g/gaia and see if b2g folks are willing to accept that change. Once the change in gaia was accepted I will add a notice to this bug so we can flip the pref on permanently.

[1] http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#376
[2] https://github.com/mozilla-b2g/gaia/blob/master/dev_apps/test-container/index.html
[3] Failing tests, on TBPL, where pass/fail indicates local reprdoucability:

[fail] /tests/content/html/content/test/test_bug406596.html
[fail] /tests/layout/base/tests/test_bug423523.html 
[fail] /tests/layout/tables/test/test_bug541668_table_event_delivery.html
[fail] /tests/layout/base/tests/test_bug968148.html
[fail] /tests/layout/base/tests/test_event_target_radius.html

[pass] /tests/content/base/test/test_bug331959.html
[pass] /tests/content/base/test/test_bug333198.html
[pass] /tests/content/base/test/test_bug560780.html
[pass] /tests/content/base/test/test_bug622117.html
[pass] /tests/content/base/test/test_bug622246.html
[pass] /tests/content/html/content/test/forms/test_input_color_picker_initial.html
[pass] /tests/content/html/content/test/forms/test_input_color_picker_popup.html
[pass] /tests/content/html/content/test/forms/test_input_color_picker_update.html
[pass] /tests/content/html/content/test/test_bug109445.html
[pass] /tests/content/html/content/test/test_bug421640.html
[pass] /tests/content/html/content/test/test_bug500885.html
[pass] /tests/content/html/content/test/test_bug514856.html
[pass] /tests/content/html/content/test/test_bug564001.html
[pass] /tests/content/html/content/test/test_bug659743.xml
[pass] /tests/content/html/content/test/test_bug694503.html
[pass] /tests/content/html/content/test/test_formSubmission2.html
[pass] /tests/content/svg/content/test/test_non-scaling-stroke.html
[pass] /tests/dom/base/test/test_domwindowutils.html
[pass] /tests/dom/events/test/test_bug402089.html
[pass] /tests/dom/events/test/test_focus_disabled.html
[pass] /tests/dom/events/test/test_moz_mouse_pixel_scroll_event.html
[pass] /tests/dom/tests/mochitest/general/test_domWindowUtils.html 
[pass] /tests/layout/base/tests/test_bug558663.html
[pass] /tests/layout/base/tests/test_bug582771.html
[pass] /tests/layout/base/tests/test_bug677878.html
[pass] /tests/layout/base/tests/test_bug761572.html
Fabrice, any chance you could review that? Otherwise, do you know someone who might review that?
Attachment #8416702 - Flags: review?(fabrice)
Comment on attachment 8416702 [details] [review]
Before flipping the pref we need to land this pull request which loads the css in question as an external resource which is then not blocked by CSP.

r=me
I restarted the failing Travis job to check if it's intermittent.
Attachment #8416702 - Flags: review?(fabrice) → review+
Having the changes accepted in Gaia, I guess it's time to find a reviewer for your patch Garrett.
Try looks good:
https://tbpl.mozilla.org/?tree=Try&rev=f7fb7ad0f5a3
Comment on attachment 8359103 [details] [diff] [review]
858787-b2g-csp-1.0-pref.patch

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

r=fabrice (irc)
Attachment #8359103 - Flags: review+
Attachment #8359103 - Flags: review?(sstamm)
Comment on attachment 8359103 [details] [diff] [review]
858787-b2g-csp-1.0-pref.patch

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

What, no unit tests for this change?  (ha-ha)  

r=me.  Land it.
Attachment #8359103 - Flags: review?(sstamm) → review+
Does this have any performance implication? Should we let the performance team know that this landed?
(In reply to Gregor Wagner [:gwagner] from comment #25)
> Does this have any performance implication? Should we let the performance
> team know that this landed?

I don't think it's necessary. The non-spec-compliant version of CSP was not able to handle inline-styles, but the spec-compliant version is. So, to sum it up, no performance difference, but more security. Please remember that the fast path [1] that Patrice implemented shortcuts all CSP security checks for certified apps anyway.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#109
> Does this have any performance implication?

Additionally, this was a blocker for us to land the new, rewritten CSP parser (because it only supports CSP 1.0+). That parser was rewritten with the primary objective of improving performance, especially on B2G.
Attachment #8417598 - Flags: review?(anygregor) → review+
Lets wait for a green travis before merging.
(In reply to Gregor Wagner [:gwagner] from comment #30)
> Followup:
> https://github.com/mozilla-b2g/gaia/commit/
> 566706795719c269c35fc09767e4cf7dfd7e4223

Thanks Gregor for the quick turnaround time - should be fixed now!
https://hg.mozilla.org/mozilla-central/rev/7b05ebf0a2d5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 2.0 S1 (9may) → ---
Depends on: 1006781
Depends on: 968907
No longer depends on: 970728
Depends on: 1006890
Depends on: 1006614
We can't wait any longer for this, it's holding back our refactoring and C++ implementation on desktop.  It's causing significant overhead for us when we're implementing new tests on desktop and they break on b2g without obvious error because the default on b2g ignores the Content-Security-Policy header.

I want to enable the spec compliant parser (1.0) and Content-Security-Policy header parsing on b2g, but change the default certified app policy to whitelist inline styles.  I'll file a followup bug to change the default CSP back after the apps are fixed and update bug dependencies accordingly.

Try with these changes:
https://tbpl.mozilla.org/?tree=Try&rev=1a47845aff47

I'll upload the patch to this bug shortly.
Attachment #8359103 - Attachment is obsolete: true
Attachment #8436097 - Flags: review?(grobinson)
Blocks: 1021970
No longer depends on: 968907
No longer depends on: 1006781
Comment on attachment 8436097 [details] [diff] [review]
enable csp 1.0 and relax certified app CSP

Gregor: does this sound like a reasonable approach?  Relax the default CSP (defers the inline style fixing to later), enable new backend and then move on?
Attachment #8436097 - Flags: review?(anygregor)
No longer depends on: 1006890
Jason: since you're aware of the issues this change originally brought about, what kind of checks should we do before trying to land this new approach (one that won't disable inline styles)?  I've got try checking it out, but thought I'd ping you too.
Flags: needinfo?(jsmith)
No longer depends on: 969126
No longer depends on: 967252
No longer depends on: 970728
So just a note to say that I have discussed this with Sid and I'm ok with relaxing the CSP policy for now for b2g. Bottom line is that we aren't blocking inline styles with the old parser. But we do need a follow-up bug to revert the certified CSP policy to block inline styles, once 968907 is resolved.
Attachment #8436097 - Flags: review?(anygregor) → review+
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #37)
> Jason: since you're aware of the issues this change originally brought
> about, what kind of checks should we do before trying to land this new
> approach (one that won't disable inline styles)?  I've got try checking it
> out, but thought I'd ping you too.

With a build with the patch here, I'd run the Gaia UI Tests smoketest automation to see if there are any test failures seen with this patch. If you'd like to know how to do that, then I'd suggest talking to Zac Campbell to help get you up and running on this.
Flags: needinfo?(jsmith)
Comment on attachment 8436097 [details] [diff] [review]
enable csp 1.0 and relax certified app CSP

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

A comment would be nice (to explain the discrepancy between this and all of the documentation online and elsewhere), but lgtm either way.
Attachment #8436097 - Flags: review?(grobinson) → review+
Ran this through try on friday:
https://tbpl.mozilla.org/?tree=Try&rev=1a47845aff47

Looks good on B2G platforms.  Jason suggested running Gaia UI Tests smoketests, not sure how much work that is to set up.  Zac, is this something that can be done easily?  I'd like to test the two pref changes in attachment 8436097 [details] [diff] [review].
Flags: needinfo?(zcampbell)
Hi Sid, I guess Jason means running on a real device?  (because the tests were already run against desktopb2g on Try there as Gu and were green).

We've put instructions up here:
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/gaia-ui-tests/Gaia_UI_Tests_Run_Tests#Testing_on_Firefox_OS_Devices

The hardest part is configuring the device and environment but the flip side of that is the coverage is broad. See the readme.md file aswell for how to configure testvars file.
Flags: needinfo?(zcampbell)
This is the same patch as before updated with updated commit message.  Carrying over r=gwagner,grobinson (and thanks for your support, Paul).

I will land this on inbound since I think this is low risk for breakage: it is not much of a logic change (the pre-spec-compliant and spec-compliant parsers are nearly the same code in JS).  The gaia unit tests pass on try.  The low risk level isn't conducive to the time it would take for me to set up a build environment and test this on device, but if someone else wants to give it a try, go for it.
Assignee: grobinson → sstamm
Attachment #8436097 - Attachment is obsolete: true
Attachment #8438013 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/789f447b4cb4

The work to fix inline style use and change the certified app csp to be more strict again will be in follow up bugs.
https://hg.mozilla.org/mozilla-central/rev/789f447b4cb4
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Requesting uplift to 2.0. Note to triage: this patch finally enforces blocking inline styles for Gaia apps. However since we have apps that failed to observe the ban on inline styles (bug 968907) this patch also relaxes the certified CSP policy so that we don't causes issues with blocking inline styles in certified apps. 

It is my hope that we can resolve all the inline style usage ASAP, and then re-enable inline style blocking for certified apps in 2.0.
blocking-b2g: --- → 2.0?
(In reply to Paul Theriault [:pauljt] from comment #46)
> Requesting uplift to 2.0. Note to triage: this patch finally enforces
> blocking inline styles for Gaia apps. However since we have apps that failed
> to observe the ban on inline styles (bug 968907) this patch also relaxes the
> certified CSP policy so that we don't causes issues with blocking inline
> styles in certified apps. 
> 
> It is my hope that we can resolve all the inline style usage ASAP, and then
> re-enable inline style blocking for certified apps in 2.0.

IMO this is too risky to consider taking into 2.0 at this point of the release. We're already behind schedule with some critical 2.0 features quality-wise, so anything non-critical to the release that poses risk should not be taken into 2.0 at this point. In terms of our product roadmap, this was not deemed as a critical 2.0 feature, so I don't think there's a compelling reason to break FL for this patch. We should take this in 2.1.
> IMO this is too risky to consider taking into 2.0 at this point of the release

The change to the default CSP for certified apps makes the default behavior with this change identical to the current default behavior. Otherwise the code path that is enabled by this patch is well-tested and has been used to enforce the spec-compliant headers on desktop for over a year now. As a result, I think the risk is very low.

CSP 1.0 has been standardized since November 2012, but 18 months later we still only support the non-standard X-Content-Security-Policy header on B2G. This confuses developers (and leads to erroneous bug reports), limits the security controls available to them, and unnecessarily increases the maintenance burden of the Security Engineering team.
Spoke offline with Sid on this and got a deeper understanding of how this helps user security and that the recent changes relaxing the policy should not result in too many fallouts. We agreed upon taking this on 2.0 and if QA found many fallouts/regressions we will pref this off.

I am not going to set blocking flag here since this is technically not a release blocker, so lets go the approval route. Sid, can you please see aurora approval to land this landed on 32 which is the current gecko for FxOS.
blocking-b2g: 2.0? → -
(In reply to bhavana bajaj [:bajaj] from comment #49)
> Spoke offline with Sid on this and got a deeper understanding of how this
> helps user security and that the recent changes relaxing the policy should
> not result in too many fallouts. We agreed upon taking this on 2.0 and if QA
> found many fallouts/regressions we will pref this off.
> 
> I am not going to set blocking flag here since this is technically not a
> release blocker, so lets go the approval route. Sid, can you please see
> aurora approval to land this landed on 32 which is the current gecko for
> FxOS.

In addition, I also think you've laid out the approval request information(mainly testing completed) already in the comments above, so we can relax filling the form ;) just set the approval flag when you are ready.
Comment on attachment 8438013 [details] [diff] [review]
enable csp 1.0 and relax certified app CSP

[Approval Request Comment]
Bug caused by (feature/regressing bug #): B2G doesn't support standardized CSP syntax and semantics
User impact if declined: B2G users will not have content security policy protection on web sites they browse (and less-refined CSP support for apps). 
Testing completed (on m-c, etc.): on m-c 
Risk to taking this patch (and alternatives if risky): tiny patch.  we landed it before, but regressed a bunch of certified apps; this newer patch fixes the regressions by relaxing the CSP blocking on apps. 
String or IDL/UUID changes made by this patch: none, just two prefs.
Attachment #8438013 - Flags: approval-mozilla-aurora?
Attachment #8438013 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1012663
See Also: → 1049876
You need to log in before you can comment on or make changes to this bug.