Remove eval from Redux.jsm

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: vinoth, Assigned: vinoth)

Tracking

(Blocks 1 bug)

unspecified
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 obsolete attachment)

Usage of eval function along with system principal is being removed from everywhere as part of Bug 1473549. Here in redux.jsm, eval should be removed and new way should be used to access the global 'this' object.
Considering that the library has a try..catch around their use of Function and eval to check for CSP restrictions, do we even need to make changes to it?

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/browser/components/newtab/vendor/Redux.jsm#695

Note that I think Redux.jsm is a vendored form of the Redux library (for React stuff) used by Activity Stream, so there's no point in landing fixes in m-c, since they will be overwritten by

a) new versions of the library
b) the fact that AS people do chunked multi-patch imports into m-c https://bugzilla.mozilla.org/show_bug.cgi?id=1446053

It's probably best if you reach out by creating an issue here: https://github.com/mozilla/activity-stream

(In general I think it's preferable to open these bugs in the relevant components that utilize the code we want to remove rather than in Core::DOM:Security)
Assignee

Comment 3

10 months ago
(In reply to Johann Hofmann [:johannh] from comment #2)
> Considering that the library has a try..catch around their use of Function
> and eval to check for CSP restrictions, do we even need to make changes to
> it?
> 
> https://searchfox.org/mozilla-central/rev/
> e126996d9b0a3d7653de205898517f4f5b632e7f/browser/components/newtab/vendor/
> Redux.jsm#695
> 
> Note that I think Redux.jsm is a vendored form of the Redux library (for
> React stuff) used by Activity Stream, so there's no point in landing fixes
> in m-c, since they will be overwritten by
> 
> a) new versions of the library
> b) the fact that AS people do chunked multi-patch imports into m-c
> https://bugzilla.mozilla.org/show_bug.cgi?id=1446053
> 
> It's probably best if you reach out by creating an issue here:
> https://github.com/mozilla/activity-stream
> 
> (In general I think it's preferable to open these bugs in the relevant
> components that utilize the code we want to remove rather than in
> Core::DOM:Security)

Thanks for the info. I got it. Will open an issue in https://github.com/mozilla/activity-stream.
Assignee

Updated

10 months ago
Component: DOM: Security → Activity Streams: Newtab
Product: Core → Firefox
Assignee

Comment 4

10 months ago
Hey Ed lee,

In https://github.com/mozilla/activity-stream/blob/master/vendor/Redux.jsm#L695 , Eval and new Function is used to get the global this object and it is executed with system principal. Hence this should be removed as part of Bug 1473549.

Since redux.jsm is vendor specific and used by activity stream please let me know, if you know someone from the activity-stream team to make these changes.

Thanks.
Flags: needinfo?(edilee)
Assignee

Comment 5

10 months ago
(In reply to Johann Hofmann [:johannh] from comment #2)
> Considering that the library has a try..catch around their use of Function
> and eval to check for CSP restrictions, do we even need to make changes to
> it?

CSP used in about:newtab restricts eval functions but still assertion is triggered. I am investigating on why this is triggered.
Hey Chris, we would like to forbid eval() in system privileged contexts and land an assertion that we can not introduce it. We identified that there is one usage within ActivityStream, in particular in the File Redux.jsm (for details see comment 2).

Can you route this ni? request to the right people so that we can potentially remove the eval() from that file and replace with something else? Thanks!
Flags: needinfo?(ckarlof)

Comment 7

9 months ago
k88hudson originally added Redux.jsm and can probably answer the eval question. We've had custom modifications to Redux.jsm with bug 1399997, which seems to want to do the same thing as this bug ? Although the fix there was:

```diff
 this.EXPORTED_SYMBOLS = ["redux"];
+
+// Defining these prevents redux from using indirect eval or `new
+// Function()` to get its global object.
+const self = this;
+this.Object = Object;
+
 this.redux =
```

Alternatively to getting thisObject in attachment 9004150 [details], why not just

```
/* 6 */
/***/ (function(module, exports) {

module.exports = this;
```
?
Flags: needinfo?(khudson)
Flags: needinfo?(edilee)
Flags: needinfo?(ckarlof)
See Also: → 1399997
I imported this code directly from Redux (except for the EXPORTED_SYMBOLS bit), it seems to be the result of a combination of Redux and webpack. They've since upgraded their version of webpack so many it's not a problem anymore? 

I would rather try to upgrade this / repack it to fix the issue so that we don't run into it again when upgrading, rather than edit the output.
Flags: needinfo?(khudson)
(In reply to Kate Hudson :k88hudson from comment #8)
> I imported this code directly from Redux (except for the EXPORTED_SYMBOLS
> bit), it seems to be the result of a combination of Redux and webpack.
> They've since upgraded their version of webpack so many it's not a problem
> anymore? 
> 
> I would rather try to upgrade this / repack it to fix the issue so that we
> don't run into it again when upgrading, rather than edit the output.

Hey Kate, thanks for your response. I am not entirely sure what this means in terms of moving forward. Are you saying we should try to upgrade redux and hope that the eval() code disappeared? If so, where and how would we do that?
Flags: needinfo?(khudson)

Comment 10

8 months ago
The latest version of Redux 4.0.1 seems to still have `  root = Function('return this')();`

https://unpkg.com/redux@4.0.1/dist/redux.js

Although 3.6.0 which is what's in Redux.jsm doesn't seem to have the function wrapper for module/global detection perhaps added by webpack?

https://unpkg.com/redux@3.6.0/dist/redux.js

k88hudson, were you suggesting with newer Redux and newer webpack, there could be an optimization step that removes the Function/eval?
Indeed, looks like their distribution still has the reference to Function. I was hoping maybe that would be removed, but oh well.

Let's do the upgrade and just remove it manually.
Flags: needinfo?(khudson)

Comment 12

8 months ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/509c8be33765b867061a8fded7561541897a276e
Fix Bug 1486375 - Upgrade redux and react redux manually removing Function (#4530)

Updated

8 months ago
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED

Updated

8 months ago
Blocks: 1502461
(In reply to Kate Hudson :k88hudson from comment #11)
> Let's do the upgrade and just remove it manually.

Thanks Kate!

Updated

8 months ago
Iteration: --- → 65.1 (Nov 2)
Comment on attachment 9004150 [details]
Bug 1486375 - usage of eval removed from redux.jsm

Changes are made in redux.jsm. Hence marking this as obsolete.
Attachment #9004150 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.