336601 - Fix for Bug 319263 can be bypassed by calling valueOf() via a local variable

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

js_SafeComputeThis is used in fun_call, fun_apply and array_extra, thus, it is
possible to bypass access checks by calling valueOf() via a local variable.

  function a() {
    var f = fun.valueOf;
    var x = f();
  }
  a();

  var g = fun.valueOf; // global variable
  var y = g();

x is the global object that created fun. y is the global object in the current
context.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - Malicious PAC script

Steps to Reproduce:
See bug 321101 comment #0

I'll attach testcases for Greasemonkey if needed.

Comment 2

[Daniel Veditz [:dveditz]]

The PAC testcase should be fine, thanks.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Testing Tp and Ts locally showed no regressions.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to trunk

Note the shiny new way to handle the v out parameter from OBJ_CHECK_ACCESS.

Brendan, also note the shiny new assertion before the JSVAL_IS_VOID test.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 220898 [details] [diff] [review]
Updated to trunk

>+            for (;;) {
>+                if (!OBJ_CHECK_ACCESS(cx, thisp, id, JSACC_PARENT, &v, &attrs))
>+                    return NULL;
>+                if (JSVAL_IS_NULL(v))
>+                    break;
>+                JS_ASSERT(JSVAL_IS_VOID(v) ||
>+                          JSVAL_TO_OBJECT(v) == OBJ_GET_PARENT(v));
>+                if (JSVAL_IS_VOID(v))
>+                    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);

What if v at this point is JSVAL_NULL?

I know, it shouldn't happen with a Call object, which is the only object we know of that does not delegate to Object.prototype, therefore lacks __parent__, and so will have JSVAL_VOID result from OBJ_CHECK_ACCESS(... JSACC_PARENT...).  But what if?

One fix is to test and break after v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT).

>+                thisp = JSVAL_TO_OBJECT(v);
>+            }

Another is to make this a do-while loop, but that tests redundantly in the common case where we didn't get undefined back from OBJ_CHECK_ACCESS.  So I say change

>+                if (JSVAL_IS_VOID(v))
>+                    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);

to

>+                if (JSVAL_IS_VOID(v)) {
>+                    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);
>+                    if (JSVAL_IS_NULL(v))
>+                        break;
>+                }

r=me with that.

/be

Comment 6

[Daniel Veditz [:dveditz]]

mrbkap: I suspect this is one vendors will want back-ported to 1.7, could you please attach an updated patch reflecting Brendan's comments as you're going to check it in? Thanks! (If there's trunk/1.8.0 differences, a 1.8.0 version would be the one I want).

Comment 7

[Daniel Veditz [:dveditz]]

Just to be clear I'm not asking for a 1.7 backport, just for a patch that reflects what actually gets checked in, e.g. reflecting brendan's review comments and any other nits fixed.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Really updated to trunk and comments

Sorry, I got tired last night, my "updated to trunk" patch was nothing of the sort, even.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #7)
> Just to be clear I'm not asking for a 1.7 backport, just for a patch that
> reflects what actually gets checked in, e.g. reflecting brendan's review
> comments and any other nits fixed.

Dan, the layering of this patch goes through the patch for bug 319263, is that OK?

In other words, in order to backport this patch to the 1.7 branch, it would be much easier and safer to first backport the patch for bug 319263 and then bakckport this one on top of that.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 220981 [details] [diff] [review]
Really updated to trunk and comments

>         if (JSVAL_IS_PRIMITIVE(argv[-2]) ||
>-            !(parent = OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(argv[-2])))) {
>+            !(OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(argv[-2])))) {

Lose the extra parens.

>+            /* walk up to find the top-level object */
>+            thisp = JSVAL_TO_OBJECT(argv[-2]);

Fix the comment to be a sentence, and to say "up from the callee".

>+            id = ATOM_TO_JSID(cx->runtime->atomState.parentAtom);
>+            for (;;) {
>+                if (!OBJ_CHECK_ACCESS(cx, thisp, id, JSACC_PARENT, &v, &attrs))
>+                    return NULL;
>+                if (JSVAL_IS_VOID(v))
>+                    v = OBJ_GET_SLOT(cx, thisp, JSSLOT_PARENT);
>+                JS_ASSERT(JSVAL_TO_OBJECT(v) == OBJ_GET_PARENT(v));

Blake said to overlook this ;-)

r=me with nits picked and assertion fixed.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Final patch for checkin

This has all comments addressed.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 220991 [details] [diff] [review]
Final patch for checkin

approved for 1.8.0 branch, a=dveditz

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fixed everywhere.

Comment 14

[Boris Zbarsky [:bzbarsky]]

This caused bug 336875.

Comment 15

[Tracy Walker [:tracy]]

verified with Fx 1.5.0.4rc2

Comment 16

[Alexander Sack]

Attachment: 1.0.x version mfsa2006-31 for 319263, 336601, 336313

proposed 1.0.x patch for mfsa2006-31: Combined patch for 319263, 336601 and 336313

Comment 17

[Bob Clary [:bc] (inactive)]

According to brendan this caused bug 343713

Comment 18

[Brendan Eich [:brendan]]

This bug is closed, and fixed, but as often happens, it caused a regression.  The "depends on" setting doesn't make sense, given that this bug is not being reopened -- removing that but noting this bug's # in bug 343713.

/be

Comment 19

[Daniel Veditz [:dveditz]]

> The "depends on" setting doesn't make sense, given that this bug is not being
> reopened -- removing that but noting this bug's # in bug 343713.

Bugzilla has no mechanism to track regressions (they get lost in the comments), but they're important for back-porters to know about. Sticking them in the "depends on" field of fixed bugs seems to be a fairly common kluge

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> > The "depends on" setting doesn't make sense, given that this bug is not being
> > reopened -- removing that but noting this bug's # in bug 343713.
> 
> Bugzilla has no mechanism to track regressions (they get lost in the comments),
> but they're important for back-porters to know about. Sticking them in the
> "depends on" field of fixed bugs seems to be a fairly common kluge

Yeah, I realized this on further reflection, and meant to come back here and undo my change.  Thanks for the reminder.

/be

Comment 21

[Bob Clary [:bc] (inactive)]

moz_bug_r_a4 verified this is fixed in 1.8 and 1.9 in Bug 321101 Comment #38