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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
Security
P1
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.8.0.4, verified1.8.1})

Trunk
mozilla1.9alpha1
verified1.8.0.4, verified1.8.1
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

11 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

11 years ago
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.
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?
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 3

11 years ago
Created attachment 220894 [details] [diff] [review]
Fix

Testing Tp and Ts locally showed no regressions.
Attachment #220894 - Flags: review?(brendan)
(Assignee)

Comment 4

11 years ago
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.
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.
(Assignee)

Comment 8

11 years ago
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.
Attachment #220898 - Attachment is obsolete: true
Attachment #220981 - Flags: review?(brendan)
(Assignee)

Comment 9

11 years ago
(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+
(Assignee)

Comment 11

11 years ago
Created attachment 220991 [details] [diff] [review]
Final patch for checkin

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)

Updated

11 years ago
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+
(Assignee)

Comment 13

11 years ago
Fixed everywhere.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.4, fixed1.8.1
Resolution: --- → FIXED
Depends on: 336875
This caused bug 336875.
verified with Fx 1.5.0.4rc2
Keywords: fixed1.8.0.4 → verified1.8.0.4
Whiteboard: [sg:moderate] priv escalation by malicious PAC or greasemonkey user script

Comment 16

11 years ago
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

Updated

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

Comment 17

11 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

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