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

RESOLVED FIXED in mozilla33

Status

()

Core
DOM: Security
P2
major
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug, {feature, perf})

unspecified
mozilla33
feature, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 33+)

Details

(Whiteboard: [c= p= s=2014.07.04.t u=])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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
Created attachment 815664 [details] [diff] [review]
fast path for certified apps

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.
Blocks: 924337
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)

Updated

4 years ago
Whiteboard: [c=progress p= s= u=]

Updated

4 years ago
No longer blocks: 927493
Depends on: 927493
Blocks: 493857
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
(Assignee)

Updated

4 years ago
Depends on: 988616
(Assignee)

Updated

4 years ago
Blocks: 991466
(Assignee)

Updated

4 years ago
Blocks: 991468
(Assignee)

Updated

4 years ago
Blocks: 991474
(Assignee)

Updated

4 years ago
Blocks: 992382
(Assignee)

Updated

4 years ago
Blocks: 993477
Depends on: 949533
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
Blocks: 994782
No longer depends on: 949533
Depends on: 991466
Depends on: 952737
(Assignee)

Updated

4 years ago
Depends on: 994872
No longer depends on: 994872
(Assignee)

Updated

4 years ago
Depends on: 1006612
Priority: -- → P2
Whiteboard: [c=progress p= s= u=] → [c= p= s= u=]
(Assignee)

Updated

4 years ago
Depends on: 1011058
Depends on: 1013559
(Assignee)

Updated

4 years ago
Depends on: 1020407
(Assignee)

Updated

4 years ago
Depends on: 1020477
(Assignee)

Updated

4 years ago
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.
Depends on: 949533
(Assignee)

Comment 8

3 years ago
Created attachment 8446590 [details] [diff] [review]
bug_925004.patch

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)
(Assignee)

Comment 9

3 years ago
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+
(Assignee)

Comment 11

3 years ago
Created attachment 8446613 [details] [diff] [review]
bug_925004_v2.patch

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/integration/mozilla-inbound/rev/a8f96ce0c95e
Target Milestone: --- → mozilla33
Blocks: 1030936
https://hg.mozilla.org/mozilla-central/rev/a8f96ce0c95e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 1031259

Comment 14

3 years ago
Relnote mostly due to Bug 949533 but this and that bug should both be covered by a single relnote.
relnote-firefox: --- → ?
Keywords: feature

Comment 15

3 years ago
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).
(Assignee)

Comment 16

3 years ago
(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

Updated

3 years ago
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)
(Assignee)

Comment 18

3 years ago
(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)
Depends on: 1031658
QA Whiteboard: [qa-]
Depends on: 1033001
Depends on: 1034157
Blocks: 1033423
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.
relnote-firefox: ? → 33+
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.
(Assignee)

Updated

3 years ago
Depends on: 1049289
Depends on: 1069762
Depends on: 1086612
Depends on: 1094067
You need to log in before you can comment on or make changes to this bug.