Comments: Is eval() in chrome:// really evil()?

try user_pref("capability.policy.default.Window.eval","noAccess");
:-)

Posted by Daniel Wang at August 2, 2004 9:53 PM

If you're running eval() on a string that includes untrusted input, even if it's escaped or run through a regexp, you're probably doing something wrong. In your Abacus scenario, I think I would split

mEdit:execute="mEdit:loopVariable('x') += 2;"

into three parameters: loopVariable, operation, and amount. (Either in the XML or by parsing mEdit:execute.) Then I'd use a switch statement on operation.

Posted by Jesse Ruderman at August 2, 2004 10:08 PM

Yes, eval should be disabled. Its really hard to do some regular expression testing on some string of syntax - and in almost all relatively complex examples there will be something a user can do to get round the problem.

btw, with regard to your problem, the malicious code is now able to put "x /= 0" as the expression, which raises a runtime exception - so you need to consider how this effects the code. Another possible problem might be putting "x += 0" if you then have some termination condition for x, you may loop forever.

Also, instead of the second match checking you didn't just match a substring, if you made hte regular expression /^x\s*[\+\-\*\/\%]?=\s*\d*$/ (^ at the front, $ at the end) this second cehck would not be needed.

As for Jesse's suggestion, I agree, but you still have to be careful you don't just take parameters 1, 2 and 3 and concat them with some syntax. Maybe a generalised framework for building up expressions would be handy, say a library of routines:

function number(string x) string
return (x ~= /\d+(.\d+)/ ? x : "0")

function eqop(string p1, string p2, string p3) string
return (p2 ~= /[\-\+\*]/ ? p1 + p2 + "=" + p3 : "")


where at each stage, if the input to the function was not of hte appropriate type, a cardinal value would be used. For example, in this instance you would use:

param1 = the loop variable
param2 = the operator
param3 = the value

eval(eqop(variable(param1), param2, number(param3))

where each of eqop, variable and number validated all their inputs fully. This means that any bugs with your suggested "test" function would be at a higher level, where they could all be fixed simultaneously.

But personally, if someone uses eval, its likely to be exploitable...

Posted by Neil Mitchell at August 3, 2004 3:42 AM

I'd strongly vote *against* disabling eval(). After all, it's the only way to make extensions "extensible". Think of Macro Editor or Mouse Gestures: Both let you "plug-in" user-defined JS functions...

Posted by jens.b at August 3, 2004 6:11 AM

Great comments so far! :-) I'd like to hear more.

Mr. Wang, that probably would not solve the problem on my end.

Mr. Ruderman, Mr. Mitchell: full-blown code examples with a test script would be *very* much appreciated!

I would disagree as to eval() being the only way to extend extensions. <?xul-overlay?> comes to mind... but being able to plug in user-def'd functions is still evil().

Posted by Alex Vincent (WeirdAl) at August 3, 2004 6:33 PM

p1 = " " + new String(p1);
// add space to be sure this isn't a "special" property name
p2 = new String(p2);
p3 = parseFloat(p3);

switch (p2) {
case "=": vars[p1] = p3;
case "+=": vars[p1] += p3;
...
}

Posted by Jesse Ruderman at August 5, 2004 10:41 AM

I used

p2 = new String(p2)

instead of

p2 = p2.toString().

because an object could override toString() to return another object. new String() can only create a string, throw, or hang.

Posted by Jesse Ruderman at August 8, 2004 11:06 PM