Bug 97603 – (Mobile Pwn2Own) ZDI-CAN-1657: : WebKit Shiftcount Vulnerability

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jeffrey Czerniak]

Created attachment 165669 [details]
poc for iOS 5.1.1

ZDI-CAN-1657: (Mobile Pwn2Own) WebKit Shiftcount Remote Code Execution Vulnerability

-- CVSS -----------------------------------------

7.5, AV:N/AC:L/Au:N/C:P/I:P/A:P


-- ABSTRACT -------------------------------------

TippingPoint has identified a vulnerability affecting the following
products:

  WebKit


-- CREDIT ---------------------------------------

This vulnerability was discovered by:

   Joost Pol, Certified Secure
   Daan Keuper, Certified Secure

[Jeffrey Czerniak]

Abstract: Vulnerability in shiftCount/splice 

	ArrayPrototype.cpp::arrayProtoFuncSplice can be tricked into passing a 
	"count" >= array.length to JSArray.cpp::shiftCount. This in turn can
	be used to trick JSArray.cpp::shiftCount into shifting an array beyond
	its length and/or shifting array with sparse values in it.

	This leads to an exploitable scenario in both IOS 5.1.1 and IOS 6.

	It is debatle wether the vulnerability is in arrayProtoFuncSplice (which
	contains a simple toctou) or in shiftCount (which does not validate its
	passed parameters correctly). We would suggest patching both shiftCount
	(validating count) and splice (fixup the toctou).

JavaScriptCore/runtime/ArrayProtoType.cpp::arrayProtoFuncSplice

	The "deleteCount" is checked against the array length, but the array
	length can be changed from the toInteger callback. See the code below
	from the arrayProtoFuncSplice function (IOS 5.1.1 fragment):

	Simple toctou in the splice routine:

		unsigned length = thisObj->get(exec, exec->propertyNames().length).toUInt32(exec);
		...
		double deleteDouble = exec->argument(1).toInteger(exec); // *
		..
		if (deleteDouble > length - begin) deleteCount = length - begin;
		..
		((JSArray *)thisObj)->shiftCount(exec, deleteCount - additionalArgs);

	*) toInteger -> interrupt and shrink thisObj (making deleteCount >= array.length)

JavaScriptCore/runtime/JSArray.cpp::shiftCount

	Both IOS5.1.1 and IOS 6: When a "count" > length is passed both the 
	numValuesInVector and m_length members will wrap-around leading to
	an exploitable scenario.

	Specific to IOS 5.1.1: When passing a count == length *and* there
	are sparse-values in the map, the length can be shifted *without*
	removing/invalidating the sparse-value entries. 

exploitPath: IOS 5.1.1

	Straightforward exploitation on IOS 5.1.1.

	The exploit path we chose for IOS 5.1.1 uses the above situation to 
	setup an array with valid values in the vector but where the array's
	m_length < m_NumValuesInVector. When GC kicks in, the values in the
	vector will not be marked (see visitChildren). Since we can still
	access the values (see getIndex) we have a classic use-after-free.

	This use-after-free can be abused in multiple ways and will most
	certainly lead to code execution. In the exploit demonstrated at
	the pwn2own we chose to use the dangling references to shift the
	"resObj" local-variable in the splice routine. This will trigger
	another memory-overwrite. 

	The use-after-free is illustarted in the POC.

exploitPath: IOS 6

	More complicated exploitation on IOS 6:

	Garbage colletion and allocations work differently. Important differnce
	is also that shiftCount does not allowing shifting when there are sparse
	values in the map:

	    if (oldLength != storage->m_numValuesInVector ...) return;

	First we trigger the vulnerability to setup an (empty) array where the
	m_numValuesInVector wrapped to -1 (0xFFFFFFFF). Next we insert some 
	sparse-value entries at the end of the map (0xFFFFFFFE and down).

	Important: Nos we set the array length to 0xFFFFFFF, this does not
	delete any values from the map (*increment* of length) but does make
	sure that m_length == mNumValuesInVector (both -1).

	Now we can trigger the splice-routine again, but this time *with* some
	values in the sparsemap. This will decrease the length again (*below*
	our sparse-value-indexes) but will leave our values untouched. 

	For clarity we set the length to zero (changes nothing).

	Now we again have an array with sparse-values in it but with a zero
	length. Inserting non-sparse values into this array will trigger a
	sparse-map -> vector move in putBeyondIndex

	The vector will be (re)allocated based on the (faulty) length and the 
	values from the sparse map will be moved into the vector. Since we are 
	using large indexes (X - 0xFFFFFFE) this will actually start *underflowing* 
	the m_vector array:

		vector[it->first].set(globalData, this, it->second.getNonSparseMode());

	This underflow can be abused easiliy and will most certainly lead to code
	execution. The path we confirmed for IOS6 uses the underflow to overwrite
	(with an atrbitrary address) the "allocBase" of another array. Whenever a 
	reallocation of this array is triggered, values will be copied into the
	array from the address we specified. 

	This in turn gives us a nice info-leak but also leads directly to code
	execution since we can just copy abtitrary values/cell into the vector
	and access them.

	The underflow and info-leak vector are illustrated in the POCs.

[Jeffrey Czerniak]

<rdar://problem/12370864>

[Jeffrey Czerniak]

Created attachment 165670 [details]
poc of iOS 6.0 crash

[Jeffrey Czerniak]

Created attachment 165671 [details]
poc of iOS 6.0 leak

[Abhishek Arya]

looks like a JSC bug.

[Filip Pizlo]

Created attachment 165696 [details]
the patch

[Gavin Barraclough]

Comment on attachment 165696 [details]
the patch

You might want to put an ASSERT inside the array shift/unshift methods to check count is sane for current length.  r+

[Filip Pizlo]

(In reply to comment #7)
> (From update of attachment 165696 [details])
> You might want to put an ASSERT inside the array shift/unshift methods to check count is sane for current length.  r+

Yup, I've done that.  It doesn't break things.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/129577