Closed Bug 1323630 Opened 8 years ago Closed 5 years ago

Apply CSP to content scripts for Manifest V3


(WebExtensions :: General, defect, P3)



(Not tracked)



(Reporter: pauljt, Unassigned)


(Blocks 1 open bug)


(Keywords: addon-compat, sec-want, Whiteboard: [CSP] triaged)

      No description provided.
Oops. SO what I meant to say was, can we block eval in the content script context? I'm thinking about web extensions, but I'd like this to fix SDK add-ons too. My concern was raised by the recent Angular issues. TLDR is that if angular is run in a content script, it evals content from the page DOM. Since the eval happens in the content script context, that means malicious DOM content can get privilege escalation (could be malicious website or one vulnerable to HTML injection). 

We ban this usage now in the review process for add-ons. But this feels like a brittle control. I would like this behavior reversed, similar to how CSP applies to the rest of the Web Extension context. IE I think we should apply a CSP (or just prohibit eval and friends) by default, and then provide a mechanism to override. 

CSP is a bit weird for this, since technically the effective script origin is actually that of the page where the content script is injected. But page CSP does not apply to content scripts. 

The default CSP for Web Extensions is "script-src 'self'; object-src 'self';". Can we just use that? Or is that too strict (self would be the page origin, I think? And maybe content scripts need moz-extension or resource ?

What do you think?
I think this is a good idea. It likely wouldn't fix the Angular problem, since does its own symbolic evaluation if eval isn't available, but I do think that it would prevent a lot of other footguns.

The easiest thing to do would be to apply the eval provisions of the extension CSP to the content scripts. None of the other provisions make sense in the context of a Sandbox, and we can't currently apply a separate policy for content injected into the document by the extension, in any case.

The other option is to just ban eval in the context of the Sandbox entirely, and possibly add another manifest entry to allow it for short term compatibility. I think that banning eval in the context of content_scripts, regardless of the CSP applied elsewhere, makes sense, given the hostility of the environment, so I think this may be the better option.

In either case, though, I'm not sure how this would affect existing extensions. We may need to do some outreach first.
Yes, this will need some investigation. Add-ons rely so much on library and framework code that I fear this would break lots of them for code they don't really need (but still runs).
Priority: -- → P2
Whiteboard: triaged
Whiteboard: triaged → [CSP] triaged
webextensions: --- → ?
webextensions: ? → ---
Flags: needinfo?(amckay)
Just a cheeky putting this back in :pauljt's queue after Kris's other CSP changes and all the other things that have happened in the last year. We've been reviewing P2's and I'm wondering if P2 is the right priority for this.
Flags: needinfo?(ptheriault)
Ok I guess first step is to find out how many web extensions we would break. What I was ultimately thinking here was that at least initially we could encourage/reward developers for opting into a strict CSP (rather than forcing a secure default).  Somewhat tangential and naive suggestion, but maybe we could display some kind of 'seal of approval' for add-ons which follow best security practices.

Note that eval is _not_ the only important flag here. The source list is also important right? (e.g. worker-src). There is also WASM to consider (currently our implementation doesn't follow CSP but it probably will). Another thing to consider is service workers - these can generate synthetic responses which is effectively eval. Which isn't an issue in terms of developer footguns (hard *accidentally* implement a service worker to do this), but if we are hoping that CSP can enforce package integrity, then this is something to consider.

cr, any chance you could adapt web-extaware to give us some stats on how many Web Extensions have content scripts which will break if we forbid eval?
Flags: needinfo?(ptheriault) → needinfo?(cr)
It would be very hard to conclusively answer the question, because there is no easy way to follow which other eval-using scripts a content script includes. Going that route would be a real technical challenge, even with webextaware modification.

However, without modifications, a bit of shell and python scripting around webextaware is enough to arrive at the following metric:

There are 83 out of 6952 Web Extensions (counted by unique AMO IDs) that have the pattern '\beval *(' in one of the content scripts directly linked in their manifests[1].

The real number of potential eval() users will be quite a bit higher thanks to inclusion of jquery, angular.js and friends, but in those cases it would be extremely hard to determine upfront whether those scripts would actually break.

[1] 309621, 512340, 887896, 870704, 880582, 888968, 873182, 873029, 708610, 413520, 266850, 858029, 854057, 715902, 687728, 838624, 676942, 836412, 683157, 869387, 464096, 8381, 690595, 860066, 831292, 885455, 842611, 856582, 874938, 769492, 799508, 798414, 745469, 687221, 502608, 790140, 8542, 722, 2792, 850389, 803317, 724993, 741791, 662434, 494518, 797686, 888192, 856348, 813092, 869140, 712366, 1865, 881749, 520576, 501472, 713112, 476537, 871083, 566314, 885251, 453070, 782720, 581614, 793294, 844827, 736754, 729537, 858649, 684006, 855221, 887824, 415920, 886844, 52369, 615374, 686646, 718598, 562120, 709519, 860500, 850440, 690669, 851519
Flags: needinfo?(cr)
Can you also check for use of new Function() ? I think there are also other patterns that lead to an eval.
Flags: needinfo?(amckay)
Product: Toolkit → WebExtensions

Bug 1581608 for manifest v3 is adding a content script csp, where currently the default wouldn't allow eval.

Priority: P2 → P3
Summary: Apply CSP (or similar) to content scripts → Apply CSP to content scripts for Manifest V3
Blocks: manifest-v3
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.