446030 - indirect eval prevention code can be circumvented by eval(s,o)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

Attachment: 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 1

[Brian Crowder]

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.

Comment 2

[Brendan Eich [:brendan]]

What is this bug about now?

/be

Comment 3

[Brian Crowder]

Brendan:  The indirect eval check in jsobj.cpp happens before the scopeobj replacement in the eval(s, o) case.

Comment 4

[Brian Crowder]

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.

Comment 5

[Brian Crowder]

moz_bug_r_a4:  Can you beat on the patch in bug 446026 to see if it still presents the same issue?

Comment 6

[moz_bug_r_a4]

(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?

Comment 7

[Brian Crowder]

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.

Comment 8

[Robert Sayre]

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.

Comment 9

[Bob Clary [:bc] (inactive)]

crowder, gotatest?

Comment 10

[Brian Crowder]

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.