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

ASSIGNED
Assigned to

Status

()

enhancement
P3
normal
ASSIGNED
Last year
6 days ago

People

(Reporter: ckerschb, Assigned: jallmann)

Tracking

(Depends on 12 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
Blocks: 1430748
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
(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.
Status: NEW → ASSIGNED
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
Status: NEW → ASSIGNED
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.
Depends on: 1482347
Depends on: 1486375
Depends on: 1489455
Depends on: 1491759
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]

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

Depends on: 1525636
Depends on: 1529231
Depends on: 1539183
Depends on: 1541062

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

(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.

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.

(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.

Depends on: 1545415
Depends on: 1546990
Depends on: 1547713
Depends on: 1547718
Depends on: 1549326
Depends on: 1549356
Depends on: 1550463
Depends on: 1550465
Depends on: 1550471
Depends on: 1550473
Depends on: 1550476
Depends on: 1550485
Depends on: 1550489

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

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?

Since I apparently missed this part last time... that might be enough if we can trust our code to be sane. But it is possible for scripts to modify the DOM before the parse is done. We could just create activation records that show when the parser is actively inserting DOM nodes, I guess. That's always a bit dicey, but for this use case, it would at least put us in a much better position than we're in without it, so it's probably not too bad of a solution.

Depends on: 1560915
See Also: → 1560178, 1513445
Depends on: 1564527
You need to log in before you can comment on or make changes to this bug.