[meta] Assert we do not use eval() when executing with SystemPrincipal

ASSIGNED
Assigned to

Status

()

P3
normal
ASSIGNED
9 months ago
20 days ago

People

(Reporter: ckerschb, Assigned: jallmann)

Tracking

(Depends on: 6 bugs, Blocks: 1 bug, {meta})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-meta], [domsecurity-backlog1])

Ideally we should never execute with system privileges and use eval() at the same time. Probably it's worth adding an assertion to make sure this never happens. I guess we can add such an assertion wherever we call:
> csp->GetAllowsEval()

Let's see what TRY returns for an initial test run of such an assertion:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7acd68dc7e12b136f298187c0c9c27d1ea36e293
(Reporter)

Updated

9 months ago
Blocks: 1430748
(Reporter)

Updated

9 months ago
Assignee: nobody → ckerschb
Priority: -- → P2
Whiteboard: [domsecurity-active]
We need to make sure to consider the Function constructor as well, it suffers from pretty much the same security issues. Unfortunately that one seems pretty widely used:

https://searchfox.org/mozilla-central/search?q=new+Function&path=browser
https://searchfox.org/mozilla-central/search?q=new+Function&case=false&regexp=false&path=toolkit
(Reporter)

Comment 2

9 months ago
(In reply to Johann Hofmann [:johannh] from comment #1)
> We need to make sure to consider the Function constructor as well, it
> suffers from pretty much the same security issues. Unfortunately that one
> seems pretty widely used:
> 
> https://searchfox.org/mozilla-central/search?q=new+Function&path=browser
> https://searchfox.org/mozilla-central/
> search?q=new+Function&case=false&regexp=false&path=toolkit

Whenever CSP checks if eval is allowed, then I am pretty sure that check covers the Function constructor as well. At least strongly hope so, but we should definitely verify that.
(Reporter)

Updated

9 months ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

9 months ago
For all the tests where we want to bypass that assertion for whatever reason, we could add a pref which we flip in the test that the assertion is bypassed, something similar to what we did in the loadinfo, see:
  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#98
Status: ASSIGNED → NEW
(Reporter)

Updated

9 months ago
Status: NEW → ASSIGNED
(Reporter)

Updated

9 months ago
Assignee: ckerschb → cegvinoth
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> (In reply to Johann Hofmann [:johannh] from comment #1)
> > We need to make sure to consider the Function constructor as well, it
> > suffers from pretty much the same security issues. Unfortunately that one
> > seems pretty widely used:
> > 
> > https://searchfox.org/mozilla-central/search?q=new+Function&path=browser
> > https://searchfox.org/mozilla-central/
> > search?q=new+Function&case=false&regexp=false&path=toolkit
> 
> Whenever CSP checks if eval is allowed, then I am pretty sure that check
> covers the Function constructor as well. At least strongly hope so, but we
> should definitely verify that.

Yes this assertion also covers Function constructor. 

After initial analysis,
Files with eval/new Function/setTimeout executing with system principal are,

1) specialpowersAPI.js (https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#30)
2) Redux.jsm (https://dxr.mozilla.org/mozilla-central/source/browser/extensions/activity-stream/vendor/Redux.jsm#695)
3) autocomplete.xml (https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#421)
4) nsHelperAppDlg.js (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#521)

I will add new files as I find those.
(Reporter)

Updated

6 months ago
Priority: P2 → P3
Summary: Assert we do not use eval() when executing with SystemPrincipal → [meta] Assert we do not use eval() when executing with SystemPrincipal
Whiteboard: [domsecurity-active] → [domsecurity-meta]
Keywords: meta
Assignee: cegvinoth → jallmann
Whiteboard: [domsecurity-meta] → [domsecurity-meta], [domsecurity-backlog1]
(Assignee)

Comment 5

2 months ago

Bug 88314 looks like an old duplicate of this bug to me.

(Assignee)

Updated

2 months ago
Depends on: 1525636
(Assignee)

Updated

a month ago
Depends on: 1529231
You need to log in before you can comment on or make changes to this bug.