1541858 - Move AssertEvalNotUsingSystemPrincipal into the ContentSecurityManager and also call it for worker code

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1541858: Move AssertEvalNotUsingSystemPrincipal into the ContentSecurityManager and also call it for worker code. r=baku

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6045657a5621a73a3ade6f4055e284912ac61c4

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 4

[Jonas Allmann [:jallmann]]

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?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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

1. 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.
2. 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).
3. Add the files to the whitelist and land the patch :-)

Thanks!

Comment 6

[Jonas Allmann [:jallmann]]

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 GetLoadingPrincipalreplaced with GetPrincipal?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 8

[Jonas Allmann [:jallmann]]

Attachment: Bug 1541858, AssertEvalNotUsingSystemPrincipal into the ContentSecurityManager and also call it for worker code

Bug 1541858 - Extended eval()-Assertion whitelist, r=ckerschb

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5be51df44c5c
AssertEvalNotUsingSystemPrincipal into the ContentSecurityManager and also call it for worker code r=ckerschb

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/5be51df44c5c