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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

defect
P1
normal
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.8.0.4, verified1.8.1})

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.8.1 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] caused 343713. priv escalation by malicious PAC or greasemonkey user script)

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

13 years ago
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.
Reporter

Comment 1

13 years ago
Steps to Reproduce:
See bug 321101 comment #0

I'll attach testcases for Greasemonkey if needed.
The PAC testcase should be fine, thanks.
Assignee: dveditz → mrbkap
Blocks: 319263
Flags: blocking1.8.1+
Flags: blocking1.8.0.4+
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Posted patch Fix (obsolete) — Splinter Review
Testing Tp and Ts locally showed no regressions.
Attachment #220894 - Flags: review?(brendan)
Posted patch Updated to trunk (obsolete) — Splinter Review
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.
Attachment #220894 - Attachment is obsolete: true
Attachment #220898 - Flags: review?(brendan)
Attachment #220894 - Flags: review?(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
Attachment #220898 - Flags: review?(brendan) → review+
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).
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.
Sorry, I got tired last night, my "updated to trunk" patch was nothing of the sort, even.
Attachment #220898 - Attachment is obsolete: true
Attachment #220981 - Flags: review?(brendan)
(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 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
Attachment #220981 - Flags: review?(brendan) → review+
This has all comments addressed.
Attachment #220981 - Attachment is obsolete: true
Attachment #220991 - Flags: review+
Attachment #220991 - Flags: approval1.8.0.4?
Attachment #220991 - Flags: approval-branch-1.8.1?(brendan)
Attachment #220991 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
Comment on attachment 220991 [details] [diff] [review]
Final patch for checkin

approved for 1.8.0 branch, a=dveditz
Attachment #220991 - Flags: approval1.8.0.4? → approval1.8.0.4+
Fixed everywhere.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
verified with Fx 1.5.0.4rc2
Whiteboard: [sg:moderate] priv escalation by malicious PAC or greasemonkey user script

Comment 16

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

Updated

13 years ago
Attachment #225473 - Flags: review?(caillon)

Comment 17

13 years ago
According to brendan this caused bug 343713
Depends on: 343713
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
No longer depends on: 343713
> 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
Whiteboard: [sg:moderate] priv escalation by malicious PAC or greasemonkey user script → [sg:moderate] caused 343713. priv escalation by malicious PAC or greasemonkey user script
(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
Depends on: 343713

Comment 21

13 years ago
moz_bug_r_a4 verified this is fixed in 1.8 and 1.9 in Bug 321101 Comment #38 
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.