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

ASSIGNED
Assigned to

Status

()

enhancement
P3
normal
ASSIGNED
10 months ago
6 days ago

People

(Reporter: ckerschb, Assigned: jallmann)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

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

(Reporter)

Description

10 months ago
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

10 months ago
Blocks: 1430748
(Reporter)

Updated

10 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

10 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

10 months ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

10 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

10 months ago
Status: NEW → ASSIGNED
(Reporter)

Updated

10 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

7 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]
Assignee: cegvinoth → jallmann
Whiteboard: [domsecurity-meta] → [domsecurity-meta], [domsecurity-backlog1]
(Assignee)

Comment 5

3 months ago

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

(Assignee)

Updated

3 months ago
Depends on: 1525636
(Assignee)

Updated

2 months ago
Depends on: 1529231
(Assignee)

Updated

29 days ago
Depends on: 1539183
(Assignee)

Updated

22 days ago
Depends on: 1541062

Comment 6

20 days ago

What's the plan for inline event handlers in chrome privileged documents? Because we have... quite a lot of those.

In particular, I'm wondering if as a stopgap measure, we could somehow allow these for chrome: URI documents during the initial parse, but make setting them at runtime (ie outside of the initial parse) a runtime error.

Flags: needinfo?(ckerschb)

Updated

20 days ago
Duplicate of this bug: 88314

(In reply to :Gijs (he/him) from comment #6)

What's the plan for inline event handlers in chrome privileged documents? Because we have... quite a lot of those.

Indeed, and ultimately we would like to enforce CSP on more/all chrome privileged documents - those inline event handlers are definitely blockers for enforcing a strong CSP, but one step at a time ...

In particular, I'm wondering if as a stopgap measure, we could somehow allow these for chrome: URI documents during the initial parse, but make setting them at runtime (ie outside of the initial parse) a runtime error.

Yes, I would like that too and I thought about that, but the question I have is: how can we dinstinguish between static strings and dynamic strings at runtime. I suppose we have done something similar when linting for innerHTML assignments, right?

What would be ideal for this particular problem if we could use the entry points for 'csp->allowsInline()' and annotate that enforcement.

Is that what you had in mind? Or was I misinterpreting your idea?

Flags: needinfo?(ckerschb) → needinfo?(gijskruitbosch+bugs)

Comment 10

20 days ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)

(In reply to :Gijs (he/him) from comment #6)

In particular, I'm wondering if as a stopgap measure, we could somehow allow these for chrome: URI documents during the initial parse, but make setting them at runtime (ie outside of the initial parse) a runtime error.

Yes, I would like that too and I thought about that, but the question I have is: how can we dinstinguish between static strings and dynamic strings at runtime. I suppose we have done something similar when linting for innerHTML assignments, right?

Yes, but I wouldn't really be interested in that. At runtime we should really use event listeners, not inline things, even for 'static' strings.

I think we could make setAttribute and any .on<foo> setters in a XUL doc throw/fail, unless we are setting the attribute as part of the initial parse of a document with an "allowed" URL (probably chrome://, maybe some specific about: pages).

Not sure what the runtime/perf impact of the checks would be... but hopefully not too bad.

Alternatively we could just bite the bullet and try to do some automatic rewriting, though there's some old annoying bugs that will bite us (cf. https://bugzilla.mozilla.org/show_bug.cgi?id=371900 ).

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to :Gijs (he/him) from comment #10)

I think we could make setAttribute and any .on<foo> setters in a XUL doc throw/fail, unless we are setting the attribute as part of the initial parse of a document with an "allowed" URL (probably chrome://, maybe some specific about: pages).

Right, so ultimately (in the long run) we should extend the attribute setter with a new security struct and putting the current triggeringPrincipal inside of it:

nsresult SetAttr(int32_t aNameSpaceID, nsAtom* aName, const nsAString& aValue,
nsIPrincipal* aTriggeringPrincipal, bool aNotify) {

Doing so would allow us to have more context and we could enforce more restrictions on attributes setter and getters.

Not sure what the runtime/perf impact of the checks would be... but hopefully not too bad.

So, I tried doing something like that in the DOM bindings, but those are highly optimized and so every perf hit there is basically a no go. Using my approach from above would not add a big perf hit because the DOM Bindings would just provide an additional pointer. E.g one for the triggeringPricnial and one for the LoadingPrincipal. We could then enforce restrictions in debug builds and only for those elements that are criticial. E.g. making sure we never assign a javascript: URI to a script element in system land, etc.

Alternatively we could just bite the bullet and try to do some automatic rewriting, though there's some old annoying bugs that will bite us (cf. https://bugzilla.mozilla.org/show_bug.cgi?id=371900 ).

An option, but I am not sure how much pain that is going to cause.

To sum it up, I think we definitely need to do something in that space, maybe there is something less labour intensive than changing the attribute setters to provide more information as a short term solution.

(Reporter)

Updated

20 days ago
Depends on: 1541858

We can definitely prevent adding new event listener attributes after the initial parse, without a major performance hit. It would require a bit of work to thread through whether the SetAttr call came from a non-script-created parser, but it's doable.

And it's something we may need to do anyway for the sake of allowing extension content injected by means of things like innerHTML to ignore CSP.

Essentially, we need to do the check in AfterSetAttr:

https://searchfox.org/mozilla-central/rev/14dc5b7d8a6da1854b2f9f33f1da77a97368cd54/dom/html/nsGenericHTMLElement.cpp#635

The security checks here are already pretty extensive/expensive, so an extra document principal check against the system principal (which is a simple pointer compare) won't matter:

https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/dom/events/EventListenerManager.cpp#778-817

It's mostly just a matter of figuring out how to tell if the attribute is parser-created. In the worst case, we could probably just do that with activation records.

Comment 13

19 days ago

(In reply to Kris Maglione [:kmag] from comment #12)

It's mostly just a matter of figuring out how to tell if the attribute is parser-created. In the worst case, we could probably just do that with activation records.

I don't think whether the attribute is parser-created should matter. The only exception I think we could make is for the initial document parse. That would mean migrating e.g. inline handlers in parseXULToFragment or setAttribute("oncommand", ...), and I'm OK with that. That should be significantly less work than migrating all the builtin ones. We could just set a flag after the initial XUL document "walk"/parse is done, right?

(In reply to :Gijs (he/him) from comment #13)

(In reply to Kris Maglione [:kmag] from comment #12)

It's mostly just a matter of figuring out how to tell if the attribute is parser-created. In the worst case, we could probably just do that with activation records.

I don't think whether the attribute is parser-created should matter. The only exception I think we could make is for the initial document parse. That would mean migrating e.g. inline handlers in parseXULToFragment or setAttribute("oncommand", ...), and I'm OK with that. That should be significantly less work than migrating all the builtin ones. We could just set a flag after the initial XUL document "walk"/parse is done, right?

Well, if we only want to allow these attributes when set by the initial parser, whether or not it's parser-created clearly matters, just with the extra bit that it must be created by a non-script-created parser.

(Assignee)

Updated

6 days ago
Depends on: 1545415
You need to log in before you can comment on or make changes to this bug.