Move AssertEvalNotUsingSystemPrincipal into the ContentSecurityManager and also call it for worker code
Categories
(Core :: DOM: Security, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: ckerschb, Assigned: jallmann)
References
(Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-active])
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-phabricator-request
|
Details |
I just realized that we only assert that we do not use eval() in system privileged code within the ScriptSecuritManager, but we should also call it from within our worker code - now I hope we simply do not call eval() in worker code and we can just land the assertion.
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
Reporter | ||
Comment 3•6 years ago
|
||
Jonas, it seems my plan didn't quite work out - the code should be fine, but I guess you have to update the whistelist - I would have thought that workers do not use eval in system land, but apparently we do.
One more note: I am not entirely sure if we should use mWorkerPrivate->GetLoadingPrincipal() or mWorkerPrivate->GetPrincipal(). I would have thought that GetLoadingPricnipal() is correct, but not 100% sure.
Can you please drive that one over the finishing line?
Assignee | ||
Comment 4•6 years ago
|
||
I'll take at look and see if I have any questions.
It's mostly about checking which files are affected and adding them to the whitelist, right?
One more note: I am not entirely sure if we should use mWorkerPrivate->GetLoadingPrincipal() or mWorkerPrivate->GetPrincipal(). I would have thought that GetLoadingPricnipal() is correct, but not 100% sure.
Is this something I should try to find out? What exactly would I be looking for?
Reporter | ||
Comment 5•6 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #4)
I'll take at look and see if I have any questions.
It's mostly about checking which files are affected and adding them to the whitelist, right?One more note: I am not entirely sure if we should use mWorkerPrivate->GetLoadingPrincipal() or mWorkerPrivate->GetPrincipal(). I would have thought that GetLoadingPricnipal() is correct, but not 100% sure.
Is this something I should try to find out? What exactly would I be looking for?
I think what we should do is the following:
- Locally run the failing tests from the TRY run from comment 2 and generate a list of all the files that would need to go into the whitelist.
- Update the code to actually use mWorkerPrivate->GetPrincipal and do (1) again to make sure the list of files is identical (which I hope it is).
- Add the files to the whitelist and land the patch :-)
Thanks!
Assignee | ||
Comment 6•6 years ago
|
||
I identified 5 new files for the whitelist, try is currently running to verify that fixes all test failures.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00da81659cf1a167a5de55dcab02250e42355f85
Most of them will be easy to fix, it is mostly setTimeout()
without any need to pass string literals, afaict. Am I right that setTimeout()
is benign if it gets passed a proper function?
Anyways, I assume it makes sense to add thefiles to the whitelist first and apply the easy fixes afterwards?
The files are:
simpletest/testrunner.js, simpletest/simpletest.js, file_bug1018265.xul, helperappdlg.jsm, test_execute_async_script.py
Update the code to actually use mWorkerPrivate->GetPrincipal and do (1) again to make sure the list of files is identical (which I hope it is).
I haven't done that with all the tests, since it's a little tedious to run them all locally. It's around 130 individual tests that are affected, and even letting a script run them takes quite a while, with browser windows popping up all the time. So far, I haven't found any deviation from mWorkerPrivate->GetLoadingPrincipal()
. Should I keep checking this or maybe just do another try run with GetLoadingPrincipal
replaced with GetPrincipal
?
Reporter | ||
Comment 7•6 years ago
|
||
(In reply to Jonas Allmann [:jallmann] from comment #6)
I identified 5 new files for the whitelist, try is currently running to verify that fixes all test failures.
The files are:
simpletest/testrunner.js, simpletest/simpletest.js, file_bug1018265.xul, helperappdlg.jsm, test_execute_async_script.py
Good, that doesn't sound too bad. For now, let's just add those 5 files to the whitelist and then we should evaluate on a case by case basis how to fix.
Update the code to actually use mWorkerPrivate->GetPrincipal and do (1) again to make sure the list of files is identical (which I hope it is).
I haven't done that with all the tests, since it's a little tedious to run them all locally.
Sounds good, just keep the code with ->GetLoadingPricnipal() and get that ready for review - thanks!
Updated•6 years ago
|
Assignee | ||
Comment 8•6 years ago
|
||
Bug 1541858 - Extended eval()-Assertion whitelist, r=ckerschb
Assignee | ||
Updated•6 years ago
|
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5be51df44c5c
AssertEvalNotUsingSystemPrincipal into the ContentSecurityManager and also call it for worker code r=ckerschb
Comment 10•6 years ago
|
||
bugherder |
Description
•