Last Comment Bug 336601 - Fix for Bug 319263 can be bypassed by calling valueOf() via a local variable
: Fix for Bug 319263 can be bypassed by calling valueOf() via a local variable
Status: VERIFIED FIXED
[sg:moderate] caused 343713. priv esc...
: verified1.8.0.4, verified1.8.1
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on: 336875 343713
Blocks: 319263
  Show dependency treegraph
 
Reported: 2006-05-04 06:56 PDT by moz_bug_r_a4
Modified: 2008-10-10 15:19 PDT (History)
6 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase 1 - Malicious PAC script (762 bytes, text/plain)
2006-05-04 07:04 PDT, moz_bug_r_a4
no flags Details
Fix (6.94 KB, patch)
2006-05-05 00:35 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
no flags Details | Diff | Review
Updated to trunk (7.03 KB, patch)
2006-05-05 01:15 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
brendan: review+
Details | Diff | Review
Really updated to trunk and comments (8.04 KB, patch)
2006-05-05 14:07 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
brendan: review+
Details | Diff | Review
Final patch for checkin (8.09 KB, patch)
2006-05-05 15:09 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
mrbkap: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review
1.0.x version mfsa2006-31 for 319263, 336601, 336313 (3.37 KB, patch)
2006-06-13 14:37 PDT, Alexander Sack
asac: review? (caillon)
Details | Diff | Review

Description moz_bug_r_a4 2006-05-04 06:56:38 PDT
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 2006-05-04 07:04:53 PDT
Created attachment 220777 [details]
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] 2006-05-04 15:54:52 PDT
The PAC testcase should be fine, thanks.
Comment 3 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-05-05 00:35:34 PDT
Created attachment 220894 [details] [diff] [review]
Fix

Testing Tp and Ts locally showed no regressions.
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-05-05 01:15:12 PDT
Created attachment 220898 [details] [diff] [review]
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] 2006-05-05 11:26:11 PDT
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] 2006-05-05 11:39:57 PDT
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] 2006-05-05 12:35:47 PDT
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) (please use needinfo!) 2006-05-05 14:07:03 PDT
Created attachment 220981 [details] [diff] [review]
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) (please use needinfo!) 2006-05-05 14:10:53 PDT
(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] 2006-05-05 14:50:43 PDT
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) (please use needinfo!) 2006-05-05 15:09:02 PDT
Created attachment 220991 [details] [diff] [review]
Final patch for checkin

This has all comments addressed.
Comment 12 Daniel Veditz [:dveditz] 2006-05-05 15:27:28 PDT
Comment on attachment 220991 [details] [diff] [review]
Final patch for checkin

approved for 1.8.0 branch, a=dveditz
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-05-05 16:35:03 PDT
Fixed everywhere.
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-05-07 15:51:55 PDT
This caused bug 336875.
Comment 15 Tracy Walker [:tracy] 2006-05-09 14:59:33 PDT
verified with Fx 1.5.0.4rc2
Comment 16 Alexander Sack 2006-06-13 14:37:17 PDT
Created attachment 225473 [details] [diff] [review]
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:] 2006-07-05 22:21:39 PDT
According to brendan this caused bug 343713
Comment 18 Brendan Eich [:brendan] 2006-07-05 22:47:57 PDT
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] 2006-07-06 21:53:16 PDT
> 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] 2006-07-06 22:31:55 PDT
(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:] 2006-08-23 07:15:06 PDT
moz_bug_r_a4 verified this is fixed in 1.8 and 1.9 in Bug 321101 Comment #38 

Note You need to log in before you can comment on or make changes to this bug.