Closed Bug 477380 Opened 11 years ago Closed 10 years ago

Disallow using eval() and setTimeout(String) in chrome

Categories

(Core :: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED DUPLICATE of bug 528950

People

(Reporter: gaubugzilla, Unassigned)

References

()

Details

(Whiteboard: [sg:want])

I have been looking into how extensions use eval(), the results are summed up on http://adblockplus.org/blog/five-wrong-reasons-to-use-eval-in-an-extension. In short, typically an extension using eval() is a sign of trouble - even if no data from untrusted sources is used extensions pretty much never bother escaping the strings they insert. It is very hard to find valid uses for eval() in an extension.

It is similar with setTimeout() where first parameter is a string rather than a closure - most extensions just use it because that's what all the web is doing. I haven't seen any cases where a closure wouldn't do as well.

I created this bug to decide whether all this is worth the trouble. 54% of the Top100 extensions I looked at use eval(), 42% use setTimeout(String) - and very often the result is bugs or even security issues (in many cases it is simply not clear whether the bug is also a security issue). How about disallowing eval() and setTimeout() if code would be compiled with the system principal? That's a scary step which would break almost everybody (even Firefox has one case of using eval() to parse JSON, one to deal with something similar to inline event handlers and another to access a property by its name, all easily fixed). On the other hand, I think it would largely reduce those nasty "execution of remote JavaScript" security issues in extensions. And it would promote better coding practices of course (the very fact that these many popular extensions use eval() is scary).

The change doesn't need to happen from one day to another - a warning visible in Error Console can be an intermediate solution, it will allow AMO reviewers to discover these issues more easily and to push for fixes.
(In reply to comment #0)
> It is similar with setTimeout() where first parameter is a string

Same thing with setInterval().
Sure, setInterval() is the same - only that I didn't see it used with dynamically generated strings anywhere.
This is one of those times I wish we had taint checking...

I really do think that eval() with system principal is a bad idea, since we have native JSON support.  Not sure about setTimeout.
Whiteboard: [sg:want]
(In reply to comment #3)
> This is one of those times I wish we had taint checking...

Only to have people "validate" with /.*/ because "then it works" :-(

(In reply to comment #3)
> I really do think that eval() with system principal is a bad idea, since we
> have native JSON support.  Not sure about setTimeout.

Yes, I only found one extension that introduced a vulnerability via setTimeout() (bug 476816). However, I am afraid that if eval() is forbidden setTimeout() will be used as an obvious "workaround".
What would remain to mirror |uneval| and |.toSource()|?

What about faking namespaces through |eval("code", object)| to prevent variable leaks in XBL components?

And what would prevent authors from replacing |eval("code")| with |new Function("code")()| (except for no longer being able to dynamically create variables and constants and maybe having to insert a |return| at the correct place)?

For further objections see the comments at the URL.
Looks like bug 515443 is implementing the ability to selectively block all of this. Maybe once that is done it can be turned on for all extensions?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 528950
You need to log in before you can comment on or make changes to this bug.