968906 - Certified apps can inline css

Status: RESOLVED WONTFIX

(Core :: DOM: Security, defect)

Description

[Alexandre Poirot [:ochameau]]

The only CSP difference between privileged and certified is about inlining CSS and it looks like we can still inline CSS attributes in certified apps.
http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#376

The following code, introduced for devtools layers is injecting html strings with inlined css attributes within the system app:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/devtools_layers.js#L65-L66

And it works fine on device...
I identified this bug while working on firefox mulet (bug 943878) where the csp works correctly and then break the devtools layers.

As I don't know exactly how much we could get from this security leak,
I set the bug private, but do not hesitate to make it public if that makes sense.

Is it related to bug 927493?

Comment 1

[[:fabrice] Fabrice Desré]

Sid, can you help on this? I'm not sure which codepath we take for these csp checks.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

I'm not sure this is super risky especially since CSP works on device, but it is probably a bug.  Can we get some debug info (like prlogging for csp:5 or something)?  We're confident the app is being treated as certified, and there is a CSP being applied, right?  (Quick check would be to try and source an external script and see if CSP blocks it.)

Fabrice: the CSP check is done in nsStyledElement::ParseStyleAttribute():
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsStyledElement.cpp#233

This could be related to bug 858836, so ccing grobinson. Garrett, what do you think?

Comment 3

[Garrett Robinson [:grobinson]]

I'm pretty sure the problem here is because we are still using the pre-1.0 X-CSP parser for Firefox OS (including the default app policies). X-CSP doesn't have inline style blocking, which is why you're not seeing styles blocked. You could try running with "security.csp.speccompliant" set to true and see if you get the behavior you're expecting.

We're working on getting 1.0 to be the default ASAP, but there are a lot of problems with the test harness that have been making it difficult.

Since this problem only affects styles, this is not ideal, and is confusing to developers who are trying to work with the default policies, but it is a low security risk.

Comment 4

[Daniel Veditz [:dveditz]]

Why do we care if privileged apps use inline styles?

But in any case this appears to be known and expected given the use of the old CSP parser in b2g.

Comment 5

[Garrett Robinson [:grobinson]]

Alex, can you confirm if this is still a problem in the latest Nightly? Should be fixed by the patch for bug 858787.

Comment 6

[Garrett Robinson [:grobinson]]

Actually, don't confirm just yet, as that patch was just backed out. I'll comment here when we get it landed again :(

Comment 7

[Alexandre Poirot [:ochameau]]

Wait, have you removed all inline css before enable inline css rejection?
I opened bug 968907 to do that.

The example I gave in first comment is still here:
  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/devtools_view.js#L106
The system app also have inlined css:
  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L380
Here is a command that can help finding them:
  egrep -r \<style --include=*.html --exclude=_sandbox.html --exclude=_proxy.html apps/ dev_apps/

I'm also wondering if style="" attribute is also rejected from this CSP?
  egrep -r style=\" --include=*.html apps/ dev_apps/

Comment 8

[Garrett Robinson [:grobinson]]

Goodness, I didn't realize that certified apps were using inline styles... are B2G developers aware of the default CSP policies, and taking them into account?

I'm changing 968907 from a blocker to a dependency here. When we land bug 858787, all certified apps will have the default CSP correctly applied (which will block inline styles). Therefore, we should remove the use of inline styles from all certified apps before landing that, to avoid breakage.

I also think this bug does not require security confidentiality, and would benefit from being opened it. sstamm, what do you think?

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

I don't see a reason to keep this hidden.  Having it opened will help get attention to fix the apps and even if someone can inject styles into a running app, the implications are not earth shattering.  But yeah, we should fix this (and the apps).

Who can make sure app developers know about this coming change to block inline styles for certified apps so they don't introduce more inline styles?

Comment 10

[Garrett Robinson [:grobinson]]

> Who can make sure app developers know about this coming change to block inline styles for certified apps so they don't introduce more inline styles?

I will be emailing gaia-dev about this shortly, I just want to make sure bug 1006781 isn't a CSP bug first (to avoid confusion).

Comment 11

[Alexandre Poirot [:ochameau]]

Yep, you should post on dev-gaia ML. If the csp is going to reject any usage of domElement.style attribute from JS, it is going to break critical performances tweaks using transition/animation/whatever-css-trick relying on modifying css attribute values dynamically via style object on DOM elements.

Comment 12

[Julien Wajsberg [:julienw]]

AFAIK this is not breaking setting domElement.style from JS, but only the use of "style=" attributes or <style> blocks in markup.

Comment 13

[Julien Wajsberg [:julienw]]

Forget comment 12, I haven't read all my mails yet, Garett made it clear in bug 1006781 comment 18.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Paul, we are triaging DOM:Security bugs at the moment. Since B2G is basically in Tier 3 support now, I was wondering what we should do with this bug? Can we close it as wontfix? Should we reclassify it so it shows up under a different Component? I don't know :-)

Comment 15

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Certified apps are going away. I am trying to get a CSP applied, but that's not this bug, and will be difficult since Gaia is likely moving to be chrome:// hosted instead of a app://.

In any case this can be closed I think.