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

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P2
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: ckerschb, Assigned: jallmann)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

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.

Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Type: defect → enhancement
Priority: -- → P2
Whiteboard: [domsecurity-active]
Blocks: 1473549

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: ckerschb → jallmann
Flags: needinfo?(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?

Flags: needinfo?(jallmann) → needinfo?(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!

Flags: needinfo?(ckerschb)

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?

Flags: needinfo?(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!

Flags: needinfo?(ckerschb)
Attachment #9055794 - Attachment is obsolete: true
Keywords: checkin-needed

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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.