Closed Bug 1550465 Opened 7 months ago Closed 5 months ago

Usage of `new Function` in third-party library sinon-7.2.7.js

Categories

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

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: jallmann, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-backlog1])

All eval()-like functions (eval(), new Function(), setTimeout("")) are being removed from code running with system privileges, see Bug 1473549. An assertion is active to enforce this.

There are two occurences of new Function in sinon-7.2.7.js that require the file to be whitelisted for this assertion. In order to clear sinon-7.2.7.js from the whitelist, new Function needs to be removed or refactored.

new Function to get the global object:

https://searchfox.org/mozilla-central/source/testing/modules/sinon-7.2.7.js#11047

new Function in template() function:

https://searchfox.org/mozilla-central/source/testing/modules/sinon-7.2.7.js#25491

A simple try run with the file removed from the whitelist to see which tests fail.

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

I identified 83 tests that fail due to the eval()-assertion after removing sinon-7.2.7.js from the whitelist.

Should I just flip the pref to allow eval() in all of them?

Flags: needinfo?(ckerschb)

(In reply to Jonas Allmann [:jallmann] from comment #2)

I identified 83 tests that fail due to the eval()-assertion after removing sinon-7.2.7.js from the whitelist.

Should I just flip the pref to allow eval() in all of them?

Gijs, what's you take on this. Obviously we could flip the pref in all 83 cases but we might expose ourselves to the risk that we accidentally greenlight system privileged evals() that just happen to be triggered in the same tests. Maybe it's wise to just keep file's like that in the whitelist.

Personally, I think we are getting to a point with that project where I think it's better to just keep certain files like e.g. sinon-7.2.7.js in the whitelist. What do you think?

Flags: needinfo?(ckerschb) → needinfo?(gijskruitbosch+bugs)

I don't know, off-hand.

(In reply to Jonas Allmann [:jallmann] from comment #0)

new Function to get the global object:

https://searchfox.org/mozilla-central/source/testing/modules/sinon-7.2.7.js#11047

I would expect us not to hit this as the freeGlobal should get the test its 'global'. If not, we can at least fix the Sinon.jsm consumers by defining a global property on the loading context that points to the loading context itself.

new Function in template() function:

https://searchfox.org/mozilla-central/source/testing/modules/sinon-7.2.7.js#25491

This isn't used inside the file, as far as I can tell from "find in page", and I would be surprised if any tests explicitly consumed it.

There are a few other eval() uses, as well as some setTimeout uses where the argument isn't clear, that I hope we don't hit in practice, but I'm not sure given that the trypush only has C++ stacks for assertions and no JS stack... You can probably use xpc_DumpJSStack(true, false, false) with the assertion to get a JS stack.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)

Personally, I think we are getting to a point with that project where I think it's better to just keep certain files like e.g. sinon-7.2.7.js in the whitelist. What do you think?

I don't know, it depends what we think the goal is. I think we should definitely get production (non-testing) code out of the whitelist. I think for the test-only code we could potentially only allow the eval() code if we're running in automation, ie we could use MOZ_DISABLE_NONLOCAL_CONNECTIONS (xpc::AreNonLocalConnectionsDisabled()) to only allow this in automation and not elsewhere.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ckerschb)

(In reply to :Gijs (he/him) from comment #4)

I don't know, it depends what we think the goal is. I think we should definitely get production (non-testing) code out of the whitelist.
That for sure - we definitely want to remove all eval() in system land for production code. We are only discussing tests here, where I personally think it's fine to keep the whitelist, but maybe baking it into the code itself instead of having a pref whitelist so devs do not simply update the whitelist.

I think for the test-only code we could potentially only allow the eval() code if we're running in automation, ie we could use MOZ_DISABLE_NONLOCAL_CONNECTIONS (xpc::AreNonLocalConnectionsDisabled()) to only allow this in automation and not elsewhere.

Right, but we would still need the whitelist, right? Because otherwise all tests would be whitelisted which is also not what we want or am I missing something?

Flags: needinfo?(ckerschb)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)

(In reply to :Gijs (he/him) from comment #4)

I don't know, it depends what we think the goal is. I think we should definitely get production (non-testing) code out of the whitelist.
That for sure - we definitely want to remove all eval() in system land for production code. We are only discussing tests here, where I personally think it's fine to keep the whitelist, but maybe baking it into the code itself instead of having a pref whitelist so devs do not simply update the whitelist.

I think for the test-only code we could potentially only allow the eval() code if we're running in automation, ie we could use MOZ_DISABLE_NONLOCAL_CONNECTIONS (xpc::AreNonLocalConnectionsDisabled()) to only allow this in automation and not elsewhere.

Right, but we would still need the whitelist, right? Because otherwise all tests would be whitelisted which is also not what we want or am I missing something?

Yes, we would. We'd also want to make sure the whitelist entries are as specific as possible in terms of URLs, so it's not possible for people to introduce identically named files elsewhere in the tree that we ship to production and which will pass tests but fail in practice.

(In reply to :Gijs (he/him) from comment #6)

Yes, we would. We'd also want to make sure the whitelist entries are as specific as possible in terms of URLs, so it's not possible for people to introduce identically named files elsewhere in the tree that we ship to production and which will pass tests but fail in practice.
Agreed, that sounds like a good path forward:

Here is a summary for the next steps:

  • Remove all production eval() in system land
  • update assertion to always assert in debug builds
    ** unless we are running in automation && the file that uses eval is whitelisted
    ** additionally try to use the full blown path of the file that needs to be whitelisted
    ** remove the whitelist from all.js and hardcode it within the assertion code.

Just to unify the discussion which is also happening in bug 1550473:

Ideally, I'd like a whitelist of test-only files that are only allowed when running tests. I think that probably matches up with what's being suggested above (i.e., checking xpc::IsInAutomation() or xpc::AreNonLocalConnectionsDisabled()), though my initial thought was to just have tests which need those scripts manually add them to the whitelist.

This file will stay permanently whitelisted, see Bug 1560915.

Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.