A few days ago I wrote about a possible security hole I’d discovered in my own work, and the process I went about in closing it safely.
However, it now occurs to me that, although my particular use-case was protected, the eval() function itself is still a very dangerous function to call from JavaScript chrome.
This is particularly true if the application has loaded the JSLib input/output module, and thus has enabled the application to directly access the filesystem of the client. Chrome files operate in a scope outside the security sandbox model web developers tote (with good reason) as gospel. However, even without JSLib, it would not take a hostile input source long to duplicate the include() function from jslib.js…
Bug 88314 @ b.m.o is an ancient bug for reviewing dangerous eval() calls from chrome. I’m beginning to think that quite frankly, this is not enough! I’m beginning to think that Mozilla would be very wise to completely disable the eval() function when running from chrome, and throw an exception instead.
Here, it becomes a matter of opinion: who’s responsible for security in the context of a Mozilla extension? Taking my own project as an example, I very nearly allowed a potential security hole into the 0.1 release.
The hole isn’t immediately obvious; the exploit would require loading a template with evil code to execute as an mEdit:execute attribute. Unfortunately, in an effort to be friendly, Abacus would allow an application to specify at runtime where to get its data files from. So a web-served page calling on Abacus could insert its own malicious XML data files, and you get the picture from there.
I closed that one, and hopefully I have no others, but it still begs the question: if I hadn’t stopped it, could it have been stopped? The eval() function is part of the ECMA-262 standard, which defines ECMAScript, 3rd Edition. So blocking that one from running inside chrome:// scope would be a violation of ECMAScript.
Who would be to blame if someone exploited eval() for evil purposes from a chrome application’s scope? Certainly the chrome application could be blamed for letting it happen… but Mozilla could also be to blame for making it possible.
I’m undecided on whether I should file a bug at Bugzilla asking for Mozilla to specifically disable eval() in chrome. I’d like your opinions on the subject (particularly those of Mozilla developers and the security group).
I would offer a third alternative, though: requiring a second argument to eval() within chrome apps. Say, eval(expr, testFunc), where testFunc(expr) would return true only if it was safe for eval() to execute the expression. Thus, the implementation of eval() could force an application developer to consider security when writing the eval() call. If the developer simply wrote:
eval(x, function() { return true; });
then there’s no way Mozilla could be blamed when x says:
“var y = new File(‘/usr/bin/mozilla’);y.open(‘w’);y.write(‘you are an idiot’);y.close();”
3. A possible role for Vitamin d deficiency cheap cialis.
ED Management vardenafil dysfunction in severe ED patients who respond poorly to.
constitute the Central Nervous System. viagra for sale by the Food and Drug Administration.
growth factor), and the amount of smooth muscle and endote – DE. 67 subjects were randomized to receive 12 sessio- viagra no prescription emphasized..
relationship problems can impair erectile functioning by canadian pharmacy generic viagra possibility of take Viagra..
CVA (less than 6 weeks) viagra no prescription • Fasting blood glucose.
.
Of course, we could just as easily advise developers to say:
if (testFunc(expr)) {
eval(expr);
}
Comments are open!
try user_pref(“capability.policy.default.Window.eval”,”noAccess”);
🙂
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.
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…
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…
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().
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;
…
}
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.