1496663 - Usage of Eval() in content-task.js

Status: RESOLVED FIXED

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

Description

[Vinothkumar Nagasayanan [:vinoth]]

Usage of eval function along with system principal is being removed from everywhere as part of Bug 1473549. Here eval is used in content-task.js (https://dxr.mozilla.org/mozilla-central/rev/f87eeba88f1cf3f4d41095f7a58cb518a59f844c/testing/mochitest/BrowserTestUtils/content/content-task.js#59), which was added as part of Bug 1107609. 

Looks like around 800 test files using ContentTask.spawn (https://dxr.mozilla.org/mozilla-central/search?q=contenttask.spawn+ext%3Ajs&redirect=false)

Comment 1

[Vinothkumar Nagasayanan [:vinoth]]

Hi Christoph,

We can potentially remove the eval() and replace it with something like loadSubScript, but that doesn't provide any security and it does mostly the same thing as eval().

Since this script is part of the test environment and ContentTask.spawn was specially added to run the user added functions in the content process, Shall I proceed with adding prefs to these test files for disabling the assertion?
Please let me know your comments.

Thanks.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
> Since this script is part of the test environment and ContentTask.spawn was
> specially added to run the user added functions in the content process,
> Shall I proceed with adding prefs to these test files for disabling the
> assertion?
> Please let me know your comments.

Since this test is only used for testing purposes anyway, I guess we should just rewrite it to use something like loadSubScript instead. That seems more feasible (actually reasonable) than annotating more than 800 tests. I assume it's not too much work to replace eval() with something like loadSubscript, or is it?

Comment 3

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> (In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)
> > Since this script is part of the test environment and ContentTask.spawn was
> > specially added to run the user added functions in the content process,
> > Shall I proceed with adding prefs to these test files for disabling the
> > assertion?
> > Please let me know your comments.
> 
> Since this test is only used for testing purposes anyway, I guess we should
> just rewrite it to use something like loadSubScript instead. That seems more
> feasible (actually reasonable) than annotating more than 800 tests. I assume
> it's not too much work to replace eval() with something like loadSubscript,
> or is it?

Sure I will replace the eval() with something similar to loadSubscript or try to execute this script in sandbox and check if the assertion is not triggering.

Comment 4

[Vinothkumar Nagasayanan [:vinoth]]

Hello Gijs,

I am working on a patch to replace eval with loadFrameScript instead[1]. I am kind of stuck during the process. I am getting timeout errors when running this patch in TRY server[2]. I am not sure if the async messages I am sending is correct. 
Please kindly take a look at it and let me know in case you can figure out the problem.

[1] - https://phabricator.services.mozilla.com/D12098
[2] - https://treeherder.mozilla.org/#/jobs?repo=try&revision=36b52cee4cb6051ed0b0e26c7e82402d6bd7f702

Thanks.

Comment 5

[:Gijs (he/him)]

I responded over IRC. :-)

Comment 6

[Jonas Allmann [:jallmann]]

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&regexp=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

[:Gijs (he/him)]

(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&regexp=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

[:Gijs (he/him)]

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

Comment 9

[Jonas Allmann [:jallmann]]

Discussing this exceeds my current expertise on the topic. What would you suggest, Christoph?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(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

[:Gijs (he/him)]

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

Comment 12

[Jonas Allmann [:jallmann]]

This file will stay permanently whitelisted, see Bug 1560915.