[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 3 open bugs, Blocks 2 open bugs)
Details
(Keywords: meta, Whiteboard: [domsecurity-meta], [domsecurity-backlog1])
Reporter | ||
Updated•6 years ago
|
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 3•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Comment 4•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 6•6 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 8•6 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 9•6 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 10•6 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 11•6 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 12•6 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 13•6 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 14•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•5 years ago
|
Comment 15•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 16•4 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 17•3 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 18•3 years ago
|
||
I think we have added hardening measurements everywhere needed. This meta bug does not need an assignee anymore at this point.
Updated•2 years ago
|
Description
•