Closed Bug 925004 Opened 6 years ago Closed 5 years ago

Install C++ Content Security Policy (CSP) parser and backend to replace the old JS code

Categories

(Core :: DOM: Security, defect, P2, major)

defect

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
relnote-firefox --- 33+

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Keywords: feature, perf, Whiteboard: [c= p= s=2014.07.04.t u=])

Attachments

(1 file, 2 obsolete files)

It seems that the JS implementation of CSP causes a major performance drawback for Firefox OS [1].
Also, I think we can leverage the URI parser and eliminate duplicate code necessary for parsing wildcards like '*' for CSP.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=924337
Keywords: perf
Attached patch fast path for certified apps (obsolete) — Splinter Review
Here's a small patch that just implements the certified apps csp on the c++ side. Overall it's killing csp time to less than 10ms for all gaia apps.
Comment on attachment 815664 [details] [diff] [review]
fast path for certified apps

Sid, would you be horrified if we did that while we wait for the full c++ rewrite?
Attachment #815664 - Flags: feedback?(sstamm)
Fabrice: this hardcoding makes my skin crawl, but if we limit it to certified apps I think it would be tolerable given the perf win.  Does the fast-path logic cause any perf hit for non-app loads?
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #3)
> Fabrice: this hardcoding makes my skin crawl, but if we limit it to
> certified apps I think it would be tolerable given the perf win.  Does the
> fast-path logic cause any perf hit for non-app loads?

There's no perf hit for non-app loads because getting the status for these pages is fast (we hit the early return at https://mxr.mozilla.org/mozilla-central/source/caps/src/nsPrincipal.cpp#567)
Blocks: 927493
Attachment #815664 - Attachment is obsolete: true
Attachment #815664 - Flags: feedback?(sstamm)
Whiteboard: [c=progress p= s= u=]
No longer blocks: 927493
Depends on: 927493
Depends on: 951457
There's two parts to this work:

1. Replace the parser and policy classes with C++ counterparts.  Convert unit tests to compiled code tests (CSPUtils.jsm and tests).
2. Replace tie-in to the sprawling CSP enforcement mechanism to use the new parser and policy object reps (contentSecurityPolicy.js, nsCSPService.h/cpp, and all other xpcshell tests).

So I'm turning this bug into part 2 (replace old JS CSP with C++ CSP), and filed bug 951457 for writing a new parser/enforcement/test backend.  Once that's done, we can complete this work.
Assignee: nobody → mozilla
Summary: Rewrite Content Security Policy (CSP) in C++ → Install C++ Content Security Policy (CSP) parser and backend to replace the old JS code
In case anyone cares: as ckerschb, grobinson and I work on this, we'll be using this shared patch queue:
http://hg.mozilla.org/users/sstamm_mozilla.com/csp-rewrite-patches/
Component: Security → DOM: Security
Depends on: 988616
Blocks: 991466
Blocks: 991468
Blocks: 991474
Blocks: 992382
Blocks: 993477
No longer blocks: 992382
No longer depends on: 988616
Depends on: 994466
No longer depends on: 951457
Depends on: 994318
No longer blocks: 991466, 991468, 991474, 993477
Status: NEW → ASSIGNED
Depends on: 994320, 993477
No longer depends on: csp-legacy-removal
Depends on: 991466
Depends on: 952737
Depends on: 994872
No longer depends on: 994872
Depends on: 1006612
Priority: -- → P2
Whiteboard: [c=progress p= s= u=] → [c= p= s= u=]
Depends on: 1011058
Depends on: 1020407
Depends on: 1020477
No longer depends on: 993477
Looks like when we flip the pref to enable this on desktop, it goes green on try:
https://tbpl.mozilla.org/?tree=Try&rev=379b2c0bb3ac

We'll need to enable it in the b2g/app/b2g.js prefs too.

I'm planning to land bug 949533 as soon as the tree opens, then this can follow shortly thereafter.
Attached patch bug_925004.patch (obsolete) — Splinter Review
The attached patch flips the pref to use the new backend on desktop but also explicitly flips the pref for B2G. Probably B2G inherits the pref from Desktop, but to be on the safe side we should include it in b2g.js as well.
Attachment #8446590 - Flags: review?(sstamm)
Additional note: Pushed the pref flip to try as well: https://tbpl.mozilla.org/?tree=Try&rev=5ce5d5013d38
Comment on attachment 8446590 [details] [diff] [review]
bug_925004.patch

>Bug 925004 - CSP in CPP: Flipping pref to use the new backend (r=sstamm)

Nit: Needs a clearer commit message: "flip pref to enable new CSP backend"

I know I was the one who asked for this, but please take out the changes to b2g.js.  I don't think it's necessary: b2g will pick up the prefs from all.js (just like it picks up security.csp.enable).

The rest looks good. r=me with a new patch with a better commit message and no changes to b2g/app/b2g.js... and given this green try (https://tbpl.mozilla.org/?tree=Try&rev=379b2c0bb3ac).
Attachment #8446590 - Flags: review?(sstamm) → review+
Here we go, updated commit message and removed changes from b2g.js. This is ready to land!
Attachment #8446590 - Attachment is obsolete: true
Attachment #8446613 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a8f96ce0c95e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1031259
Relnote mostly due to Bug 949533 but this and that bug should both be covered by a single relnote.
relnote-firefox: --- → ?
Keywords: feature
The new backend appears to have broken our site:

URL: https://micro.cupcake.io/
Content-Security-Policy: default-src https://admin.cupcake.io https://micro.cupcake.io; connect-src *; frame-ancestors 'none'; frame-src https://appsbar.cupcake.io; img-src *; object-src 'none';

> Content Security Policy: The page's settings blocked the loading of a resource at https://user.cupcake.io/config/status ("default-src https://admin.cupcake.io https://micro.cupcake.io").

The XHR to https://user.cupcake.io/config/status should be allowed by the "connect-src *" directive (and is by the previous backend as well as Webkit/Blink).
(In reply to Jonathan Rudenberg from comment #15)
> The new backend appears to have broken our site:
> 
> URL: https://micro.cupcake.io/
> Content-Security-Policy: default-src https://admin.cupcake.io
> https://micro.cupcake.io; connect-src *; frame-ancestors 'none'; frame-src
> https://appsbar.cupcake.io; img-src *; object-src 'none';
> 
> > Content Security Policy: The page's settings blocked the loading of a resource at https://user.cupcake.io/config/status ("default-src https://admin.cupcake.io https://micro.cupcake.io").
> 
> The XHR to https://user.cupcake.io/config/status should be allowed by the
> "connect-src *" directive (and is by the previous backend as well as
> Webkit/Blink).

Working on it: https://bugzilla.mozilla.org/show_bug.cgi?id=1031530
Whiteboard: [c= p= s= u=] → [c= p= s=2014.07.04.t u=]
Is there a need for manual testing on this feature? If so, can you please specify how the Manual QA team can be of assistance?
Flags: needinfo?(mozilla)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #17)
> Is there a need for manual testing on this feature? If so, can you please
> specify how the Manual QA team can be of assistance?

At the moment, I don't think there is need for manual testing - if something comes up, I will definitely let you know. Thanks for reaching out!
Flags: needinfo?(mozilla)
QA Whiteboard: [qa-]
Depends on: 1033001
Added in the release notes with "New CSP (Content Security Policy) backend" as wording.

If you have a doc or a blog post about this, I would be happy to take it.
Oh, by the way, does it impact also Android?
Yes, this new CSP backend affects all Gecko-based platforms.  Still working on a blog post, Sylvestre.
Depends on: 1049289
Depends on: CVE-2014-1591
Depends on: 1086612
Depends on: 1094067
You need to log in before you can comment on or make changes to this bug.