363891 - Indirect use of eval can run code with the wrong principal.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Johnny Stenback (:jst)]

Attachment: Patch per initial comment.

Comment 2

[Brendan Eich [:brendan]]

Man, I want to ban indirect eval (except for w.eval(s) for a window object w, or whatever the global class is in a non-DOM embedding).

/be

Comment 5

[chris hofmann]

is the patch any good, or should we be going with brendan's idea in comment 2 ?

Comment 6

[Johnny Stenback (:jst)]

The patch is no good (or not good enough), so we'll need to go with brendan's idea.

IOW, we'll want to limit eval to being callable on global objects, and ideally also to being only callable directly (i.e. window.foo = window.eval; window.foo() should fail, at least from chrome). Brendan says the JS 2 spec already limits eval() similarly to this.

I'll be out of the country for a week starting tomorrow and won't be of much help on this issue right now. mrbkap or brendan, can you guys have a stab at this problem?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I could take a stab at this on Friday.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More like this

So, does this do the trick?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> IOW, we'll want to limit eval to being callable on global objects, and ideally
> also to being only callable directly (i.e. window.foo = window.eval;
> window.foo() should fail, at least from chrome). Brendan says the JS 2 spec
> already limits eval() similarly to this.

So, what about cases like bug 320172, where eval is being used (mildly incorrectly) as an argument to Array.forEach. Granted, this is an edge case, but it is out there in the wild (or, at least, it was). If we do want to enforce this, then I think the easiest way would be to disallow using "eval" in a non-call context.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 261181 [details] [diff] [review]
More like this

>+    uintN flags;

Block local and initialized.

>     fp = cx->fp;
>     caller = JS_GetScriptedCaller(cx, fp);
>     JS_ASSERT(!caller || caller->pc);
>     indirectCall = (caller && *caller->pc != JSOP_EVAL);

We talked about fixing the emitter to choose JSOP_EVAL for o.eval as well as eval (pn_arity == PN_NAME instead of pn_op == JSOP_NAME before the pn_atom == evalAtom test). We must allow o.eval if o is a window object, which the OBJ_GET_PARENT(cx, obj) != NULL below allows -- but that also allows w.evil = eval, which we don't want to support.

So use JSOP_EVAL as the first filter, to throw out any calls to a function or method named anything but eval.

The second filter is to make any obj_eval activation whose |obj| (|this|) parameter is not a global object be an indirectCall.

mrbkap pointed out the good use-case of Array.map(arraylike, eval). To make that work, we must stop using JS_GetScriptedCaller, and add special-case code to recognize good native trampoline frames such as the one for array_extra.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch

So, this fixes brendan's comments, but see the big XXX. We can't allow seemingly benign Array.map(..., eval) because it undoes the advantage of the rest of the fix. brendan says he'll take the issue up with ECMA.

Comment 12

[Igor Bukanov]

This bug reminds about a similar problem with Java and its ClassLoader (Java version of eval). There was bug at least until JDK 1.4 that effectively allowed to gain all permissions as long as code was allowed to use ClassLoader. The reason was that the runtime did not impose dynamic restrictions according to the current stack on the code generated using ClassLoader. So when the generated code was called with only system code on the stack, it run with all permissions.

Comment 13

[Johnny Stenback (:jst)]

Since we're making eval() not callable off of a non-global object, shouldn't we then also make it so that obj.eval is only defined on global objects as well? I mean what's the point of exposing that function (which IE never exposed) if it's never callable? If we'd remove eval from non-global objects then an unqualified eval() call scoped at a non-global object would continue to work, whereas if we don't do that we'd find eval() in the current scope and we'd fail to do the eval, no?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Ah, true. I wonder if we should resolve it in JS_ResolveStandardClass and take it off of Object.prototype.

Comment 15

[Brendan Eich [:brendan]]

Take it out of Object.prototype, make it a global function only.

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Best fix yet

With this patch applied:

js> ["print('a')", "print('b')"].forEach(eval)
a
b
js> this.eval
function eval() {
    [native code]
}
js> ({}).eval
js> eval.call({}, "")
typein:3: EvalError: function eval must be called directly, and not by way of a function of another name

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 266004 [details] [diff] [review]
Best fix yet

>Index: jsapi.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.c,v
>retrieving revision 3.322
>diff -p -U8 -r3.322 jsapi.c
>--- jsapi.c	18 May 2007 20:55:06 -0000	3.322
>+++ jsapi.c	24 May 2007 22:50:35 -0000
>@@ -1395,16 +1395,17 @@ static JSStdName object_prototype_names[
>     {js_InitObjectClass,        EAGER_ATOM(parent), NULL},
>     {js_InitObjectClass,        EAGER_ATOM(count), NULL},
> #if JS_HAS_TOSOURCE
>     {js_InitObjectClass,        EAGER_ATOM(toSource), NULL},
> #endif
>     {js_InitObjectClass,        EAGER_ATOM(toString), NULL},
>     {js_InitObjectClass,        EAGER_ATOM(toLocaleString), NULL},
>     {js_InitObjectClass,        EAGER_ATOM(valueOf), NULL},
>+    {js_InitObjectClass,        EAGER_ATOM(eval), NULL},

Why add this to object_prototype_names if this patch removes Object.prototype.eval? The entry in standard_class_names should suffice.


>+    /*
>+     * Note: This restriction allows for Array.map(array, eval), but does not
>+     * allow the user to pass a this parameter that isn't a global object.
>+     */
>+    if (indirectCall || OBJ_GET_PARENT(c, obj)) {
>+        uintN flags = (OBJ_GET_PARENT(cx, obj) != NULL)

Nit: house style eschews != NULL tests except when nesting assignment in a loop condition. Outer parens in the ?: condition can be avoided too, thereby.

Looks great otherwise. We are close to the promised land... r=me with these changes.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #17)
> Why add this to object_prototype_names if this patch removes
> Object.prototype.eval? The entry in standard_class_names should suffice.

Oops, I didn't see that when I wrote the patch.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

So, should we try this for a6? Or should this wait until after 1.9 has branched (based on bug 380236 comment 14)?

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What's ready to be checked in

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

James, Neil, do you know if any extensions are depending on indirect eval on something that isn't the window object (Brendan and I seem to remember Venkman using the 2-parameter form, but don't remember if it also uses the indirect form)?

Comment 22

[[e-mail]]

(In reply to comment #21)
>James, Neil, do you know if any extensions are depending on indirect eval on
>something that isn't the window object (Brendan and I seem to remember Venkman
>using the 2-parameter form, but don't remember if it also uses the indirect form)?
Are you perhaps thinking of evalInTargetScope in venkman-static.js?

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Argh, this got wacky. Jeff, this bug ended up being about removing indirect eval, we can use bug 382509 as a cover bug. Perhaps the discussion about Venkman should move there.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

This was checked into trunk.

Comment 25

[Daniel Veditz [:dveditz]]

Does this have too much potential for extension-breaking, or could we safely take this on the 1.8 branch?

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

I think I've seen spurious errors caused by this checkin (including one in chrome code!), which makes me wary to check this in on branches.

Comment 27

[Daniel Veditz [:dveditz]]

alright, let's wait at least another cycle to see what the trunk fall-out is and then reconsider taking it on the branch.

Comment 28

[Daniel Veditz [:dveditz]]

Still don't see regressions from this on trunk, can we take it now? 1.8.1.9?

Comment 29

[Daniel Veditz [:dveditz]]

The branch has waited a long time for this, is there any objection to me just checking it in?

Comment 30

[Daniel Veditz [:dveditz]]

After 2.0.0.11 I'm too scared to check this in without explicit approval from the patch author, will have to wait yet again.

Comment 31

[Samuel Sidler (old account; do not CC)]

Fixed on the branch in bug 382509.

Comment 34

[Al Billings [:abillings - ex-MoCo]]

All right. Marking as verified for this release then.