Closed Bug 247606 Opened 16 years ago Closed 16 years ago

Review Firefox JS for dangerous eval() use

Categories

(Firefox :: General, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox1.0beta

People

(Reporter: jruderman, Assigned: jruderman)

References

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(1 file)

 
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.
Summary: Review Firefox JS for dangerous eval() and setTimeout() use → Review Firefox JS for dangerous eval() use
Attached patch patch for trunkSplinter Review
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.
Attachment #151649 - Flags: review?(bryner)
cc neil@parkwaycc.co.uk, since my patch changes his code in about:config.
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 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.
Attachment #151649 - Flags: review?(bryner) → review+
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?
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: fixed-aviary1.0
Target Milestone: --- → Firefox1.0beta
You need to log in before you can comment on or make changes to this bug.