[meta] Assert we do not use eval() when executing with SystemPrincipal
Categories
(Core :: DOM: Security, enhancement, P3)
Tracking
()
People
(Reporter: ckerschb, Unassigned)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
(Keywords: meta, 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•6 years ago
|
Comment 1•6 years ago
|
||
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®exp=false&path=toolkit
Reporter | ||
Comment 2•6 years 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®exp=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•6 years ago
|
Reporter | ||
Comment 3•6 years 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
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Comment 4•6 years ago
|
||
(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®exp=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 years ago
|
Updated•5 years ago
|
Comment 6•5 years 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.
Reporter | ||
Comment 9•5 years ago
|
||
(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?
Comment 10•5 years 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 ).
Reporter | ||
Comment 11•5 years ago
|
||
(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.
Comment 12•5 years ago
|
||
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:
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:
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•5 years 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?
Comment 14•5 years ago
|
||
(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.
Comment 15•5 years ago
|
||
(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.
Updated•5 years ago
|
Reporter | ||
Updated•4 years ago
|
Comment 16•4 years ago
|
||
We landed Bug 1633374 to enforce in Nightly/Early Beta. We're logging telemetry in all channels. I don't see any telemetry in 77 or above. I think we can let enforcement roll out to release (enforcement and logging telemetry.)
SELECT event_object,
event_method,
event_string_value,
app_version,
normalized_channel,
TO_JSON_STRING(event_map_values),
event_process,
count(*) AS count_reports,
count(distinct client_id) as count_distinct_clients
FROM telemetry.events
WHERE event_category = 'security'
AND submission_date >= '2020-03-1'
AND app_version > '75'
and event_method <> 'javascriptLoad'
GROUP BY event_method,
event_object,
event_string_value,
app_version,
normalized_channel,
TO_JSON_STRING(event_map_values),
event_process
ORDER BY app_version desc, normalized_channel, event_string_value
Comment 17•3 years ago
|
||
I looked at Telemetry again. There were around 25k reports in 85 release, 2-3k reports in 85.0.2 and 86. However they were coming from <15 distinct client ids, so I think our having blocked eval usage is pretty safe.
SELECT event_object,
event_method,
event_string_value,
app_version,
normalized_channel,
TO_JSON_STRING(event_map_values),
event_process,
count(*) AS total_reports,
count(distinct client_id) AS distinct_clientids
FROM telemetry.events
WHERE event_category = 'security'
AND event_method = 'evalUsage'
AND submission_date >= '2021-2-1'
and normalized_channel = 'release'
GROUP BY event_method,
event_object,
event_string_value,
app_version,
normalized_channel,
TO_JSON_STRING(event_map_values),
event_process
ORDER BY app_version desc, normalized_channel, event_string_value
I suspect when Bug 1696306 hits release we will be even lower.
Comment 18•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:ckerschb, could you have a look please?
For more information, please visit auto_nag documentation.
Reporter | ||
Comment 19•2 years ago
|
||
I think we have added hardening measurements everywhere needed. This meta bug does not need an assignee anymore at this point.
Updated•1 year ago
|
Description
•