All users were logged out of Bugzilla on October 13th, 2018

Indirect eval should be allowed under ES5

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: emk, Assigned: jorendorff)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
Although ES3 allowed prohibiting indirect eval, ES5 doesn't anymore (even in strict mode).
[eval][0]("this") should not throw EvalError.
Busted! I was talking about this in other fora recently.

This should block. Thanks for reporting,

/be
blocking2.0: --- → ?

Updated

8 years ago
blocking2.0: ? → final+
(Assignee)

Comment 2

8 years ago
Likewise (from bug 600193 comment 3), this should be allowed:

   var x = 1;
   var obj = {x: 2};
   obj.eval = eval;
   assertEq(obj.eval("x"), 1);

Currently the call to eval throws an EvalError.
(Assignee)

Comment 3

8 years ago
Created attachment 480439 [details] [diff] [review]
v1

What the heck, I'm in this code anyway.

There's a lot of not-particularly-relevant tidying up in the patch. I think it's still possible to see what's going on, but if not, I can split that out.
Assignee: general → jorendorff
Attachment #480439 - Flags: review?(brendan)
(Assignee)

Comment 4

8 years ago
Created attachment 480440 [details] [diff] [review]
v1 jsobj.cpp only, alternative diff

v1 was produced by hg qdiff. It has the tests in it, and it shows exactly what changed in the eval cache code (nothing significant I hope), so it's worth looking at.

Here is what GNU diff has to say about the exact same set of changes to jsobj.cpp. It shows what changed about the actual eval part of obj_eval, which is more interesting.
Comment on attachment 480439 [details] [diff] [review]
v1

Thanks for the better alterna-diff.
>+    /*
>+     * CSP check: Is eval() allowed at all?
>+     * Report errors via CSP is done in the script security mgr.
>+     */
>+    if (!js_CheckContentSecurityPolicy(cx)) {
>+        JS_ReportError(cx, "call to eval() blocked by CSP");
>+        return false;
>+    }

The comment (you touched it ;-) seems wrong -- the JS_ReportError call (no js.msg l10n support) is here, not in some callback impl'ed in the script secmgr. Fix or followup (or a bit of both -- fix the comment now and cite the followup on js.msg usage).

Great tests.

/be
Attachment #480439 - Flags: review?(brendan) → review+
(Assignee)

Comment 6

8 years ago
Created attachment 481241 [details] [diff] [review]
v2

v1 wouldn't even compile; I must have built and run tests in the wrong tree.

The fix is pretty straightforward though.

I noticed that we used to cache even eval scripts that could never be used, such as indirect eval scripts. With this patch we don't do that anymore.

I also fixed the CSP error message and comment.
Attachment #480439 - Attachment is obsolete: true
Attachment #480440 - Attachment is obsolete: true
Attachment #481241 - Flags: review?(brendan)
(Assignee)

Comment 7

8 years ago
Created attachment 481243 [details] [diff] [review]
v2

How about we try that one with Unix newlines.
Attachment #481241 - Attachment is obsolete: true
Attachment #481243 - Flags: review?(brendan)
Attachment #481241 - Flags: review?(brendan)
(Assignee)

Updated

8 years ago
Duplicate of this bug: 602212
(Assignee)

Comment 9

8 years ago
Sorry, undercaffeinated this morning. The CSP error message patch is in bug 602212 after all.
Comment on attachment 481243 [details] [diff] [review]
v2

Does JS_ALWAYS_INLINE matter for EvalCacheLookup? r=me in any event, just thought this might be better for perf and codesize (both).

/be
Attachment #481243 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

8 years ago
Sadly, it *does* matter, in MSVC anyway -- even though this is a static function only called from one place. I wonder why MSVC doesn't inline that unless we insist.

Anyway, I made it JS_ALWAYS_INLINE for checkin.

http://hg.mozilla.org/tracemonkey/rev/89006937466d
Whiteboard: [fixed-in-tracemonkey]
(Assignee)

Comment 12

8 years ago
Backed out. Crashing tests.
Whiteboard: [fixed-in-tracemonkey]
(Assignee)

Comment 13

8 years ago
Created attachment 481363 [details] [diff] [review]
v3 - part 1, the actual change

I deleted a little too eagerly. This version of the patch restores the code that bails out early if there's no calling script.
Attachment #481243 - Attachment is obsolete: true
Attachment #481363 - Flags: review?(brendan)
(Assignee)

Comment 14

8 years ago
Created attachment 481364 [details] [diff] [review]
v3 - part 2, misc. cleanup
Attachment #481364 - Flags: review?(brendan)
(Assignee)

Comment 15

8 years ago
Created attachment 481365 [details] [diff] [review]
Interdiff from v2 to v3

The diff between v2 and (the two v3 patches combined).
(Assignee)

Comment 16

8 years ago
Now, v2 was crashing because I had deleted the "if (!caller)" check quoted below, and then soon after that we do caller->script(). My first inclination was to go through and fix up all the places where obj_eval uses caller.

The comment I'm adding here is meant to explain why I didn't do that:

>     if (!caller) {
>+        /* Eval code needs to inherit principals from the caller. */
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>                              JSMSG_BAD_INDIRECT_CALL, js_eval_str);
>         return JS_FALSE;
>     }

To be a little more explicit, one of the places obj_eval uses caller is here:

>    JSPrincipals *principals = js_EvalFramePrincipals(cx, callee, caller);
js_EvalFramePrincipals does have some code to cope with NULL caller, but I'm not sure it does exactly what we want. So to be on the safe side I'm going to leave that fix for another day.
Attachment #481363 - Flags: review?(brendan) → review+
Attachment #481364 - Flags: review?(brendan) → review+

Updated

8 years ago
Blocks: 514568
(Assignee)

Comment 17

8 years ago
Landed part 1:     http://hg.mozilla.org/tracemonkey/rev/8402d56a7777
and part 2:        http://hg.mozilla.org/tracemonkey/rev/7598b7ab2e76
and backed it out: http://hg.mozilla.org/tracemonkey/rev/a4383c16a9b5
and relanded it:   http://hg.mozilla.org/tracemonkey/rev/2e20ecdd1194

To top it off I put the wrong bug number in all four commits. Awesome. :-(
(Assignee)

Updated

8 years ago
Whiteboard: [fixed-in-tracemonkey]

Comment 18

8 years ago
http://hg.mozilla.org/mozilla-central/rev/ab87d9da6e2f
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.