Usage of Eval() in content-task.js
Categories
(Core :: DOM: Security, enhancement, P3)
Tracking
()
People
(Reporter: vinoth, Assigned: jallmann)
References
(Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-backlog1])
Reporter | ||
Comment 1•6 years ago
|
||
Comment 2•6 years ago
|
||
Reporter | ||
Comment 3•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Reporter | ||
Comment 4•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 6•5 years ago
|
||
Hi Gijs,
I'm picking up on this bug, and I'm wondering what Vinoth and you ended up discussing regarding a solution.
If I understand correctly, ContentTask is exclusively meant for use in tests, and it would be easiest to just allow the use of eval() in all tests that use it, instead of rewriting the code itself. That would mean flipping the "security.allow_eval_with_system_principal"-pref in many testcases, or maybe centrally in ContentTask.jsm, to avoid amending hundreds of testfiles.
On the other hand, that would require making sure that ContentTask is never used outside tests, which looks like it's already not the case:
https://searchfox.org/mozilla-central/search?q=ContentTask.spawn&case=false®exp=false&path=
Are these uses of ContentTask in the mozscreenshots tool even intended?
And do you have a suggestion on how to proceed?
It would be great if you could have a quick look at this, thanks!
Comment 7•5 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #6)
I'll try to reply in detail tomorrow, will keep ni for that.
On the other hand, that would require making sure that ContentTask is never used outside tests, which looks like it's already not the case:
No, it is, those are all test code, searchfox is just wrong.
https://searchfox.org/mozilla-central/search?q=ContentTask.spawn&case=false®exp=false&path=
Are these uses of ContentTask in the mozscreenshots tool even intended?
Yes, note that (confusingly!) mozscreenshot(s) is a test job on infra that produces automated full-window screenshots of UI so you can compare changes to the UI and ensure there aren't unexpected regressions. It's not related to the "Firefox screenshots" code, which lives elsewhere.
The other FooTestUtils jsms are also test code, of course.
Comment 8•5 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #6)
Hi Gijs,
I'm picking up on this bug, and I'm wondering what Vinoth and you ended up discussing regarding a solution.
If I understand correctly, ContentTask is exclusively meant for use in tests, and it would be easiest to just allow the use of eval() in all tests that use it, instead of rewriting the code itself. That would mean flipping the "security.allow_eval_with_system_principal"-pref in many testcases, or maybe centrally in ContentTask.jsm, to avoid amending hundreds of testfiles.
Maybe...
In theory, it shouldn't be too difficult to make ContentTask.spawn() load the passed function as a new frame script using a data: URI, into just the browser that we care about, wrapped in a way where it passes the return value back to the parent using message passing.
Of course, the question is whether we want to keep allowing that at all: as long as we support loading frame scripts with a data: URI that could potentially run in the parent process (if the browser is not remote, ie its contents run in the parent process) then that's effectively still eval()'ing things... just using a different method. I don't know how that fits into our long-term goals for this project of using CSP and disallowing eval. I think code run through the frame script mechanism isn't subject to CSP, but it obviously has access to the content loaded in the tab, and runs with system privileges.
Assignee | ||
Comment 9•5 years ago
|
||
Discussing this exceeds my current expertise on the topic. What would you suggest, Christoph?
Comment 10•5 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #9)
Discussing this exceeds my current expertise on the topic. What would you suggest, Christoph?
I think for now we should try to keep things simple. Obviously there are a variety of possibilities, but each involves engineering effort which I am not sure is worth pursuing given the benefit.
As of now, I guess my biggest question is: how many tests would we need to update? Put differently, if you remove content-task.js from the whitelist - how many tests break?
Comment 11•5 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
(In reply to Jonas Allmann [:jallmann] from comment #9)
Discussing this exceeds my current expertise on the topic. What would you suggest, Christoph?
I think for now we should try to keep things simple. Obviously there are a variety of possibilities, but each involves engineering effort which I am not sure is worth pursuing given the benefit.
As of now, I guess my biggest question is: how many tests would we need to update? Put differently, if you remove content-task.js from the whitelist - how many tests break?
I expect the majority of browser mochitests use ContentTask.spawn
, as it's the main way you can test things relating to the content document in an e10s world, so we need to keep that working one way or another.
Assignee | ||
Comment 12•5 years ago
|
||
This file will stay permanently whitelisted, see Bug 1560915.
Description
•