Closed Bug 427196 Opened 12 years ago Closed 11 years ago

"Assertion failure: OBJ_SCOPE(pobj)->object == pobj" with __proto__, __defineSetter__

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: igor)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [sg:critical] fixed-in-tracemonkey)

Attachments

(5 files, 1 obsolete file)

Pasting the following script into a debug js shell triggers
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at jsinterp.c:370


function boom()
{
  var b = {};
  Array.__proto__ = b;
  b.__proto__ = {};
  var c = {};
  c.__proto__ = [];
  try { c.__defineSetter__("t", undefined); } catch(e) { }
  c.__proto__ = Array;
  try { c.__defineSetter__("v", undefined); } catch(e) { }
}

boom();
Worked for me with 'changeset:   24950:86c57e08cfe7' on Tiger.
Still happens for me on Leopard (TM branch, rev c8a855f68fd4+).
This should be a regression of bug 418139.

Seems to work as expected with cvs js shell checkout at 2008-03-01 20:05 PST

Asserts with cvs js shell checkout at 2008-03-01 20:07 PST with:
Assertion failure: obj == pobj, at jsinterp.c:149

and later morphed to: (by around the 2008-03-05 00:00 GMT timeframe)
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at jsinterp.c:365

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-03-01+20%3A05%3A00&maxdate=2008-03-01+20%3A07%3A00&cvsroot=%2Fcvsroot

Bonsai message:

Fix property cache fill to recompute protoIndex to handle XBL and other JS_SetPrototype users (418139, r/a=shaver).
Blocks: 418139
Flags: blocking1.9.1?
Keywords: regression
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
I cannot reproduce the assert on Linux with TN or CVS tips. Is it Mac-only?
(In reply to comment #4)
> I cannot reproduce the assert on Linux with TN or CVS tips. Is it Mac-only?

I definitely see this on TM tip without -j on 32-bit Ubuntu and Mac Leopard.

Debug console output in Ubuntu 32-bit: (Mac console spew is similar)

[Thread debugging using libthread_db enabled]
js> function boom()
{
  var b = {};
  Array.__proto__ = b;
  b.__proto__ = {};
  var c = {};
  c.__proto__ = [];
  try { c.__defineSetter__("t", undefined); } catch(e) { }
  c.__proto__ = Array;
  try { c.__defineSetter__("v", undefined); } catch(e) { }
}
js> 
js> boom();
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at ../jsinterp.cpp:397
[New Thread 0xb7dc86c0 (LWP 12367)]

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 0xb7dc86c0 (LWP 12367)]
JS_Assert (s=0x81e4a30 "OBJ_SCOPE(pobj)->object == pobj", file=0x81e3f61 "../jsinterp.cpp", ln=397) at ../jsutil.cpp:68
68	    abort();
(gdb) bt
#0  JS_Assert (s=0x81e4a30 "OBJ_SCOPE(pobj)->object == pobj", file=0x81e3f61 "../jsinterp.cpp", ln=397) at ../jsutil.cpp:68
#1  0x080b0599 in js_FullTestPropertyCache (cx=0x87b0d88, pc=0x87bc931 "�", objp=0xbf993168, pobjp=0xbf993368, entryp=0xbf9930a0) at ../jsinterp.cpp:397
#2  0x081c18d2 in js_Interpret (cx=0x87b0d88) at ../jsinterp.cpp:4380
#3  0x080ae95e in js_Execute (cx=0x87b0d88, chain=0x87b4000, script=0x87bab58, down=0x0, flags=0, result=0xbf993524) at ../jsinterp.cpp:1589
#4  0x08058547 in JS_ExecuteScript (cx=0x87b0d88, obj=0x87b4000, script=0x87bab58, rval=0xbf993524) at ../jsapi.cpp:5038
#5  0x08051b26 in Process (cx=0x87b0d88, obj=0x87b4000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:485
#6  0x08052330 in ProcessArgs (cx=0x87b0d88, obj=0x87b4000, argv=0xbf993698, argc=0) at ../../shell/js.cpp:786
#7  0x08052632 in main (argc=0, argv=0xbf993698, envp=0xbf99369c) at ../../shell/js.cpp:4638
(gdb)
OS: Mac OS X → All
Hardware: x86 → All
Assignee: general → igor
Here is slightly simpler example that also works when run through a file:

@watson-32~> cat ~/s/x.js
function boom()
{
    var c = {__proto__: []};
    var a = {__proto__: {__proto__: {}}};
    c.hasOwnProperty("t");
    c.__proto__ = a;
    c.hasOwnProperty("v");
}

boom();
@watson-32~> ~/build/js32.tm.dbg/js ~/s/x.js
Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at /home/igor/m/tm/js/src/jsinterp.cpp:397
Trace/breakpoint trap

It seems that in all cases one needs a fast array on the prototype chain to lead to the bug. I am not sure the exact consequences of that so I mark the bug as restricted for now.
Group: core-security
The assert is not harmless as the following modification of the example shows with non-debug build of js shell:

@watson-32~> cat ~/s/x.js
var c = {__proto__: []};
var a = {__proto__: {__proto__: {}}};
c.hasOwnProperty;
c.__proto__ = a;
c.hasOwnProperty;
@watson-32~> ~/build/js32.tm.opt/js ~/s/x.js
Segmentation fault

What is going on here is that the first c.hasOwnProperty finds the property at the prototype depth == 3 in Object.prototype and records that in the cache together with the shape of the scope. Then c.__proto__ = a mutates the prototype chain so its depth becomes 5. Then the second c.hasOwnProperty comes. It finds c.hasOwnProperty in the cache and then access the third element at the prototype chain. That is {__proto__: {}}. But that object shares the scope with Object.protype so its shape matches. Then the code proceeds with accessing the slot of {__proto__: {}} that corresponds to the slot for Object.prototype.hasOwnProperty. In this particular it segfaults as JSObject.dslots is null.

But the bug is not limitted just to null segfaults as one can put, say, 1e6 of properties in Object.prototype and then the code can be used to access the data at the address around 4e6. That can be junk and if that junk would match GC-thing value, this would lead to heap corruption. Another possibility for an exploit is to construct prototype chains with shared scopes with objects that use private slots but still have shares scopes.
This is why the code in ProcessSetSlotRequests in jsgc.cpp has this paragraph:


        /*
         * Regenerate property cache shape ids for all of the scopes along the
         * old prototype chain, in case any property cache entries were filled
         * by looking up starting from obj.
         */
        while (oldproto && OBJ_IS_NATIVE(oldproto)) {
            scope = OBJ_SCOPE(oldproto);
            SCOPE_MAKE_UNIQUE_SHAPE(cx, scope);
            oldproto = STOBJ_GET_PROTO(scope->object);
        }

But of course the dense array [] stops this loop from regenerating the shape of Object.prototype. D'oh!

Easy fix, just remove OBJ_IS_NATIVE(oldproto) from the loop condition and  move it into the body, with an if-else to do what the current body does if native, else follow JSSLOT_PROTO for non-natives.

/be
Alternative would make dense array on prototype chain slow, but the more general fix in ProcessSetSlotRequests is needed anyway. We should review other places where a dense array on the proto chain could combine with bad OBJ_IS_NATIVE tests to make for bugs like this one.

/be
Another alternative would be not to cache properties that comes after a non-native object is discovered on the prototype chain. It would not stop caching of Array.prototype method lookups for fast arrays due to special code that explicitly looks for fast array methods in their prototypes.

AFAICS such caching of properties behind non-native objects on the protype chain is not correct in general as the non-native object can be mutated without changing its shape which includes altering of its prototype.
(In reply to comment #10)
> Another alternative would be not to cache properties that comes after a
> non-native object is discovered on the prototype chain. It would not stop
> caching of Array.prototype method lookups for fast arrays due to special code
> that explicitly looks for fast array methods in their prototypes.

And Array.prototype is slow, and people don't delegate to dense arrays, only fuzz-tests do.

> AFAICS such caching of properties behind non-native objects on the protype
> chain is not correct in general as the non-native object can be mutated without
> changing its shape which includes altering of its prototype.

You're right, and this is the true fix. The bogus code is in js_LookupPropertyWithFlags, here:

This is easy to patch too (jsobj.cpp mozilla-central tip line numbers):

3982	        if (!OBJ_IS_NATIVE(proto)) {
3983	            if (!OBJ_LOOKUP_PROPERTY(cx, proto, id, objp, propp))
3984	                return -1;
3985	            return protoIndex + 1;
3986	        }

The return value should be PCVCAP_PROTOSIZE.

/be
(In reply to comment #11)
> This is easy to patch too (jsobj.cpp mozilla-central tip line numbers):
> 
> 3982            if (!OBJ_IS_NATIVE(proto)) {
> 3983                if (!OBJ_LOOKUP_PROPERTY(cx, proto, id, objp, propp))
> 3984                    return -1;
> 3985                return protoIndex + 1;
> 3986            }
> 
> The return value should be PCVCAP_PROTOSIZE.

That would not work as js_FillPropertyCache recalculates protoIndex on its own. I guess during recalculation js_FillPropertyCache can just check that the object is native.
That's right, I had to recalc in fill to get the true (no scope->object shared scope skipping) index.

/be
Looks exploitable so re-nominating.

/be
Flags: blocking1.9.1- → blocking1.9.1?
Here is a test case with a better test coverage:

var c = {__proto__: []};
var a = {__proto__: {__proto__: {}}};
var tmp1 = c.hasOwnProperty;
c.__proto__ = a;
var tmp2 = c.hasOwnProperty;

if (tmp1 !== Object.prototype.hasOwnProperty)
    throw "Bad";  
if (tmp1 !== tmp2)
    throw "Bad";
Flags: blocking1.9.1?
Reinstating accidentally-cleared blocking1.9.1? re-nomination flag.
Flags: blocking1.9.1?
Attached patch v1Splinter Review
As discussed, the patch makes sure that js_FillPropertyCache does not cache properties coming from natives-behind-non-natives on the prototype chain.
Attachment #368937 - Flags: review?(brendan)
The bug exists on 1.9.0 in the same form so nominating for that branch as well.
Flags: blocking1.9.0.9?
Comment on attachment 368937 [details] [diff] [review]
v1

Great, thanks.

/be
Attachment #368937 - Flags: review?(brendan) → review+
landed to TM - http://hg.mozilla.org/tracemonkey/rev/37ae217b403e
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/37ae217b403e
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Resolution: --- → FIXED
Here's the backported patch for the 1.9.0.x branch.

$ diff ~/Desktop/190patch-v1.diff ~/Desktop/tmpatch.diff
1c1
< Index: js/src/jsinterp.c
---
> Index: tm/js/src/jsinterp.cpp
3,8c3,5
< RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
< retrieving revision 3.505
< diff -u -8 -p -r3.505 jsinterp.c
< --- js/src/jsinterp.c	3 Mar 2009 01:53:11 -0000	3.505
< +++ js/src/jsinterp.c	24 Mar 2009 19:05:56 -0000
< @@ -158,17 +158,23 @@ js_FillPropertyCache(JSContext *cx, JSOb
---
> --- tm.orig/js/src/jsinterp.cpp	2009-03-23 19:50:10.000000000 +0100
> +++ tm/js/src/jsinterp.cpp	2009-03-23 19:56:58.000000000 +0100
> @@ -164,17 +164,23 @@ js_FillPropertyCache(JSContext *cx, JSOb
Attachment #369116 - Flags: review?(igor)
Attachment #369116 - Flags: approval1.9.0.9?
Attachment #369116 - Flags: review?(igor) → review+
Whiteboard: fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey
Flags: blocking1.9.0.10? → blocking1.9.0.10+
Comment on attachment 369116 [details] [diff] [review]
backported 1.9.0.x patch v1

Approved for 1.9.0.10. a=ss
Attachment #369116 - Flags: approval1.9.0.10? → approval1.9.0.10+
Flags: in-testsuite?
landed to 1.9.0:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.506; previous revision: 3.505
Keywords: fixed1.9.0.10
Attached file js1_5/extensions/regress-427196-03.js (obsolete) —
Flags: in-testsuite? → in-testsuite+
fix gTestfile
Attachment #373906 - Attachment is obsolete: true
v 1.9.0, 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
These testcases do not appear to affect the 1.8 branch (and wouldn't be expected to given the regression range).
Flags: wanted1.8.1.x-
Group: core-security
http://hg.mozilla.org/tracemonkey/rev/56cf031fdc6c

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-427196-01.js,v  <--  regress-427196-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-427196-02.js,v  <--  regress-427196-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-427196-03.js,v  <--  regress-427196-03.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.