Created attachment 330250 [details] [diff] [review] return if scopeobj in obj_eval, where we report BAD_INDIRECT_CALL, we don't return JS_FALSE in the case where there is a scopeobj. Worse, this whole business can be circumvented entirely by using the eval(o,s) form being removed in bug 442333. If we restore that functionality, we must remain conscious of the indirect eval issue. mrbkap: Think I should go ahead and move this paragraph below the eval(o,s) handling stuff, for future-proofing? Or move -that- paragraph up?
Comment on attachment 330250 [details] [diff] [review] return if scopeobj Nevermind, this is bogus. I hadn't realized how tricksy JS_ReportError* and friends were. This apparently returns FALSE in the JSREPORT_ERROR case! You learn something new every day. :) The eval(o,s) issue should still be addressed, I'll morph the bug.
Summary: indirect eval prevention code has a bug → indirect eval prevention code can be circumvented by eval(o,s)
What is this bug about now? /be
Summary: indirect eval prevention code can be circumvented by eval(o,s) → indirect eval prevention code can be circumvented by eval(s,o)
Brendan: The indirect eval check in jsobj.cpp happens before the scopeobj replacement in the eval(s, o) case.
This needs a better approach, and we need to be cautious of breaking extensions with the rephrasing I propose. I'll try to talk with mrbkap asap about the right thing here.
moz_bug_r_a4: Can you beat on the patch in bug 446026 to see if it still presents the same issue?
(In reply to comment #3) > Brendan: The indirect eval check in jsobj.cpp happens before the scopeobj > replacement in the eval(s, o) case. I'm not sure what the problem is. It seems to me that the indirect eval check is not affected by the second argument. Is there something I am missing?
This is currently fixed by the removal of eval(s,o) in bug 442333. I'll make sure I deal with the security issues revealed here in bug 446026.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
AIUI, this bug is fixed, as long as we don't have to restore support for the second argument to eval(). If this bug isn't actually fixed, or we must support the second argument to eval, please reopen.
The other bug has one, for eval(s, o) removal. I don't have a test for the specific security issue here, though eyeballing the code still leaves me concerned. Based on comment #6, perhaps I was overreacting.
You need to log in before you can comment on or make changes to this bug.