247606 - Review Firefox JS for dangerous eval() use

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jesse Ruderman]

Comment 1

[Jesse Ruderman]

Uses of these functions should be removed if they are not necessary:

* eval(name, formalParams, code)
    parseInt
    new RegExp
    call an event handler directly (might require a fix for bug 246720)
* new Function(code)
* setTimeout(code, time).  
    setTimeout(function, time, param1, ...)

I think these only become security holes when untrusted strings are passed in,
like in bug 87980 and bug 191817.  But removing as many uses as possible
improves performance slightly and makes future security audits easier.

Comment 2

[Jesse Ruderman]

Attachment: patch for trunk

There were two "security holes", but both were only exploitable by users, and
one was in about:config which kiosk users wouldn't have access to anyway.

* The "Open Web Location" dialog, which only appears when the address bar is
disabled.  When "New window" is selected, it calls delayedOpenWindow, which
used setTimeout in a buggy way.

* about:config.  The "Enter boolean value" and "Enter integer value" dialogs
used eval() in a buggy way.

I fixed a bunch of other setTimeouts to use the function form rather than the
string form to make the code more readable, faster to run, and easier to audit.


I left a few evals and string setTimeouts alone:

* I left "eval(e.objectSource)" in extension manager.  It seems to be used for
passing a JavaScript data structure through XPCOM as a string, and it's safe.

* I left "this.mDialog.setTimeout('dialog.postShowCallback()', 0);" in
mozilla/toolkit/mozapps/downloads/ because I didn't know what the correct
replacement was.  This doesn't use untrusted strings, so it's safe.

Comment 3

[Jesse Ruderman]

cc [e-mail], since my patch changes his code in about:config.

Comment 4

[[e-mail]]

Your delayedOpenWindow issue is due to the fact that Mozilla calls the
setTimeout function with a "magic" extra argument which I understand equates to
the lateness of the timer, i.e. the delay between the expected and actual time.

I don't see how my use of eval in about:config is any less secure than the
JavaScript console, except that you're expecting the eval in that case.

You might want to consider auto-detecting the base when parsing integers.

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 151649 [details] [diff] [review]
patch for trunk

My only concern is whether you used closures everywhere you needed to for the
timeout callbacks to have the correct global object.  Assuming you've tested
all this, r=me.

Comment 6

[Jesse Ruderman]

I'll test the ones that reference global variables.  Bryner, are you worried
that the global object itself will change due to scoping rules?  (I don't see
why it would change.)  Or are you worried that the values of global variables
will change before the timer fires?

Comment 7

[Jesse Ruderman]

Checked in, trunk and aviary, with a comment change in browser.js based on what
Neil said.  The only conflict in moving to the branch was that credits.html moved.