Usage of Function('return this') in Debugger

NEW
Unassigned

Status

defect
P3
normal
7 months ago
5 months ago

People

(Reporter: vinoth, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 months ago
Usage of new Function() along with system principal is being removed from everywhere as part of Bug 1473549.

Files redux.js, react-redux.js, lodash.js, vendors.js, search-worker.js and parser-worker.js use new Function() to get the 'this' object. This should be replaced with secure alternatives.
(Reporter)

Comment 1

7 months ago
Hi Mike,

These files use this new Function(), 
https://dxr.mozilla.org/mozilla-central/search?q=var+root+%3D+freeGlobal+%7C%7C+freeSelf+%7C%7C+Function(%27return+this%27)()%3B+ext%3Ajs&redirect=false

We have to replace it with secure alternatives. Seems like you made changes to these files in Bug 1441147. Please let us know the possibilities of changing these vendor codes and is it possible to directly make changes to all these files redux.js, react-redux.js, lodash.js, vendors.js, search-worker.js and parser-worker.js?

Please let me know if you have any idea on these or please direct us to the right person in the Devtools team.

Thanks.
Flags: needinfo?(mratcliffe)
Sounds like you need to speak with Jason Laster.
Flags: needinfo?(mratcliffe) → needinfo?(jlaster)
Jason is out on PTO for the next 3 weeks FYI. I'm still new to this codebase, so I can't 100% confidently say much, but I can try to keep this conversation moving.

That code is pulled in from Lodash: https://github.com/lodash/lodash/blob/6018350ac10d5ce6a5b7db625140b82aeab804df/.internal/root.js#L7. That's a very common way to get a reference to a global, since there is no standardized way to do that yet, though hopefully one will exist soon.

Is this logic actually triggering runtime errors when restrictions on `Function()` are put in place? I'm surprised because I would assume that the `freeSelf` check would shortcircuit any calls to `Function()` in that context.
(Reporter)

Comment 4

5 months ago
(In reply to Logan Smyth [:loganfsmyth] from comment #3)
> 
> Is this logic actually triggering runtime errors when restrictions on
> `Function()` are put in place? I'm surprised because I would assume that the
> `freeSelf` check would shortcircuit any calls to `Function()` in that
> context.

Yes this logic is triggering assertion errors for the following files,

[1] - lodash.js
https://dxr.mozilla.org/mozilla-central/rev/b0b856065d5b7ad2996f707e6e797d0d72afd803/devtools/client/shared/vendor/lodash.js#423

[2] - react-redux.js
https://dxr.mozilla.org/mozilla-central/rev/b0b856065d5b7ad2996f707e6e797d0d72afd803/devtools/client/shared/vendor/react-redux.js#1536

[3] - redux.js
https://dxr.mozilla.org/mozilla-central/rev/b0b856065d5b7ad2996f707e6e797d0d72afd803/devtools/client/shared/vendor/redux.js#18

Few test files that doesnt use 'freeSelf' check but use Function('return this')() are,

1) toolkit/components/extensions/test/mochitest/test_ext_background_page.html
2) dom/security/test/general/browser_test_FTP_console_warning.js
3) browser/components/extensions/test/browser/browser_ext_addon_debugging_netmonitor.js

A similar Bug 1486375 was using eval to get the 'this', but it has been replaced now.
Please kindly let me know if you have alternatives to fix these files.

Thanks.
So it looks like react-redux and redux pull in Lodash's root module [1], which is why the three examples are equivalent.

As a general rule of thumb, I would prefer not to modify 3rd-party code. Would it be possible to follow up with Lodash folks and see what they think?


[1] https://github.com/lodash/lodash/blob/4ea8c2ec249be046a0f4ae32539d652194caf74f/.internal/root.js#L7
Flags: needinfo?(jlaster)
I filed an issue in lodash to see what others think (https://github.com/lodash/lodash/issues/4066). Lets see if we can find some good alternatives!
Vinothkumar, I understand that eval's in general could be dangerous, but is there anything particularly dangerous about `Function("return this")`? 

I'm asking because if this case is safe, perhaps we can whitelist it given it is used by Lodash, which is a top 5 used library on npm.
Can we create a `self` into the global manually in the contexts where it isn't already available? It seems like, as long as we create it before `lodash` loads, it would short-circuit before calling `Function('return this')` and I assume avoid the issue. e.g.

> if (typeof self === "undefined") {
>   self = function(){ return this; }();
> }

In the long run we'll standardize on 'globalThis' and this will resolve itself, so hopefully this would only be needed until then.
Routing @jdalton's question from github

> How do you import scripts into the privileged environment and what can be used as the global object reference?
You need to log in before you can comment on or make changes to this bug.