From c2787ef8fdb7401922e9ec6540e4e5895d11c631 Mon Sep 17 00:00:00 2001 From: Suwei Chen Date: Tue, 27 Sep 2016 10:07:12 -0700 Subject: [PATCH] Change to address CVE-2016-7200,CVE-2016-7201,CVE-2016-7202,CVE-2016-7203,CVE-2016-7208,CVE-2016-7240,CVE-2016-7241,CVE-2016-7242,CVE-2016-7243 Type confusion in Array.prototype.filter Type confusion due to reentrancy can cause a Var to be written into a native int array. Fix by making sure type-specialized code path is used only when ArraySpeciesCreate() invokes built-in Array constructor. Heap overflow in Array.prototype.splice In Array.prototype.splice, array length is cached before ArraySpeciesCreate() is invoked. Side-effect from ArraySpeciesCreate() can change array length and result in inconsistent states and possibly heap overflow. Fix by adding length check to keep cases with side effects out of fast path with pre-calculated length. Also tweak logic in ArraySpeciesCreate() to flag a non-built-in constructor with missing [@species] property. Type confusion in FillFromPrototypes In ForEachOwnMissingArrayIndexOfObject(), existing array enumeration logic assumes Var array. A native array from caller can cause type confusion and leak. Fix by converting incoming native arrays to Var arrays. Parameter type confusion in eval Extra argument signified by CallFlags_ExtraArg shall be cast to FrameDisplay unless the extra argument is used for new.target, in which case CallFlags_NewTarget is be set. Type confusion and AV occur because existing logic in eval() does not check if CallFlags_NewTarget is cleared before using extra argument as FrameDisplay. Fix by adding CallFlags_NewTarget check to eval() before cast to FrameDisplay. Type confusion in JSON.parse Non-native array is expected in JSONParser::Walk(). A native array from caller can cause type confusion and heap overflow Fix by converting native arrays to Var arrays. Type confusion in Array.prototype.concat and .splice Array newly created by ArraySpeciesCreate is not being checked if it is a JavascriptCopyOnAccessNativeIntArray, causing near-nullptr AVs. Fix by adding check-and-convert against JavascriptCopyOnAccessNativeIntArray in affected built-ins. --- lib/Runtime/Base/CallInfo.h | 10 ++ lib/Runtime/Library/GlobalObject.cpp | 2 +- lib/Runtime/Library/JSONParser.cpp | 3 +- lib/Runtime/Library/JavascriptArray.cpp | 92 ++++++++--- lib/Runtime/Library/JavascriptPromise.cpp | 2 +- test/Array/Array_TypeConfusion_bugs.js | 184 +++++++++++++++++++++- test/es6/ES6NewTarget_bugfixes.js | 8 + 7 files changed, 274 insertions(+), 27 deletions(-) diff --git a/lib/Runtime/Base/CallInfo.h b/lib/Runtime/Base/CallInfo.h index 12e2d78f9aa..bf7be60d72f 100644 --- a/lib/Runtime/Base/CallInfo.h +++ b/lib/Runtime/Base/CallInfo.h @@ -65,6 +65,16 @@ namespace Js static const ushort ksizeofCount; static const ushort ksizeofCallFlags; static const uint kMaxCountArgs; + + static bool isDirectEvalCall(CallFlags flags) + { + // This was recognized as an eval call at compile time. The last one or two args are internal to us. + // Argcount will be one of the following when called from global code + // - eval("...") : argcount 3 : this, evalString, frameDisplay + // - eval.call("..."): argcount 2 : this(which is string) , frameDisplay + + return (flags & (CallFlags_ExtraArg | CallFlags_NewTarget)) == CallFlags_ExtraArg; // ExtraArg == 1 && NewTarget == 0 + } }; struct InlineeCallInfo diff --git a/lib/Runtime/Library/GlobalObject.cpp b/lib/Runtime/Library/GlobalObject.cpp index bfe4e171a66..02c23d6587d 100644 --- a/lib/Runtime/Library/GlobalObject.cpp +++ b/lib/Runtime/Library/GlobalObject.cpp @@ -491,7 +491,7 @@ namespace Js // TODO: Handle call from global scope, strict mode BOOL isIndirect = FALSE; - if (args.Info.Flags & CallFlags_ExtraArg) + if (Js::CallInfo::isDirectEvalCall(args.Info.Flags)) { // This was recognized as an eval call at compile time. The last one or two args are internal to us. // Argcount will be one of the following when called from global code diff --git a/lib/Runtime/Library/JSONParser.cpp b/lib/Runtime/Library/JSONParser.cpp index ce1dceb2a37..0d629da6e98 100644 --- a/lib/Runtime/Library/JSONParser.cpp +++ b/lib/Runtime/Library/JSONParser.cpp @@ -83,8 +83,7 @@ namespace JSON // this is a post order walk. Visit the children before calling walk on this object if (Js::DynamicObject::IsAnyArray(value)) { - Js::JavascriptArray* arrayVal = Js::JavascriptArray::FromAnyArray(value); - // REVIEW: How do we guarantee that JSON objects are not native arrays? + Js::JavascriptArray* arrayVal = JavascriptArray::EnsureNonNativeArray(Js::JavascriptArray::FromAnyArray(value)); Assert(!Js::JavascriptNativeIntArray::Is(arrayVal) && !Js::JavascriptNativeFloatArray::Is(arrayVal)); uint length = arrayVal->GetLength(); if (!arrayVal->IsCrossSiteObject()) diff --git a/lib/Runtime/Library/JavascriptArray.cpp b/lib/Runtime/Library/JavascriptArray.cpp index 3fec3d4360f..4ec60bef4d5 100644 --- a/lib/Runtime/Library/JavascriptArray.cpp +++ b/lib/Runtime/Library/JavascriptArray.cpp @@ -714,6 +714,40 @@ namespace Js return IsMissingHeadSegmentItemImpl(index); } + template + void JavascriptArray::InternalFillFromPrototype(JavascriptArray *dstArray, const T& dstIndex, JavascriptArray *srcArray, uint32 start, uint32 end, uint32 count) + { + RecyclableObject* prototype = srcArray->GetPrototype(); + while (start + count != end && JavascriptOperators::GetTypeId(prototype) != TypeIds_Null) + { + ForEachOwnMissingArrayIndexOfObject(srcArray, dstArray, prototype, start, end, dstIndex, [&](uint32 index, Var value) { + T n = dstIndex + (index - start); + dstArray->DirectSetItemAt(n, value); + + count++; + }); + + prototype = prototype->GetPrototype(); + } + } + + template<> + void JavascriptArray::InternalFillFromPrototype(JavascriptArray *dstArray, const uint32& dstIndex, JavascriptArray *srcArray, uint32 start, uint32 end, uint32 count) + { + RecyclableObject* prototype = srcArray->GetPrototype(); + while (start + count != end && JavascriptOperators::GetTypeId(prototype) != TypeIds_Null) + { + ForEachOwnMissingArrayIndexOfObject(srcArray, dstArray, prototype, start, end, dstIndex, [&](uint32 index, Var value) { + uint32 n = dstIndex + (index - start); + dstArray->SetItem(n, value, PropertyOperation_None); + + count++; + }); + + prototype = prototype->GetPrototype(); + } + } + /* static */ bool JavascriptArray::HasInlineHeadSegment(uint32 length) { @@ -3308,6 +3342,9 @@ namespace Js pDestObj = ArraySpeciesCreate(args[0], 0, scriptContext); if (pDestObj) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(pDestObj); +#endif // Check the thing that species create made. If it's a native array that can't handle the source // data, convert it. If it's a more conservative kind of array than the source data, indicate that // so that the data will be converted on copy. @@ -5863,6 +5900,9 @@ namespace Js } newArr = CreateNewArrayHelper(static_cast(newLenT), isIntArray, isFloatArray, pArr, scriptContext); +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(newArr); +#endif newObj = newArr; } else @@ -5870,6 +5910,9 @@ namespace Js // If the new object we created is an array, remember that as it will save us time setting properties in the object below if (JavascriptArray::Is(newObj)) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(newObj); +#endif newArr = JavascriptArray::FromVar(newObj); } } @@ -6649,6 +6692,9 @@ namespace Js // If the new object we created is an array, remember that as it will save us time setting properties in the object below if (JavascriptArray::Is(newObj)) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(newObj); +#endif newArr = JavascriptArray::FromVar(newObj); } } @@ -6657,10 +6703,13 @@ namespace Js { pArr->GetArrayTypeAndConvert(&isIntArray, &isFloatArray); newArr = CreateNewArrayHelper(deleteLen, isIntArray, isFloatArray, pArr, scriptContext); +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(newArr); +#endif } // If return object is a JavascriptArray, we can use all the array splice helpers - if (newArr && isBuiltinArrayCtor) + if (newArr && isBuiltinArrayCtor && len == pArr->length) { // Array has a single segment (need not start at 0) and splice start lies in the range @@ -7151,6 +7200,9 @@ namespace Js if (JavascriptArray::Is(pNewObj)) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(pNewObj); +#endif pnewArr = JavascriptArray::FromVar(pNewObj); } @@ -8924,6 +8976,9 @@ namespace Js // If the new object we created is an array, remember that as it will save us time setting properties in the object below if (JavascriptArray::Is(newObj)) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(newObj); +#endif newArr = JavascriptArray::FromVar(newObj); } } @@ -9123,7 +9178,8 @@ namespace Js } // If the source object is an Array exotic object we should try to load the constructor property and use it to construct the return object. - RecyclableObject* newObj = ArraySpeciesCreate(obj, 0, scriptContext); + bool isBuiltinArrayCtor = true; + RecyclableObject* newObj = ArraySpeciesCreate(obj, 0, scriptContext, nullptr, nullptr, &isBuiltinArrayCtor); JavascriptArray* newArr = nullptr; if (newObj == nullptr) @@ -9137,6 +9193,9 @@ namespace Js // If the new object we created is an array, remember that as it will save us time setting properties in the object below if (JavascriptArray::Is(newObj)) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(newObj); +#endif newArr = JavascriptArray::FromVar(newObj); } } @@ -9164,7 +9223,7 @@ namespace Js if (JavascriptConversion::ToBoolean(selected, scriptContext)) { // Try to fast path if the return object is an array - if (newArr) + if (newArr && isBuiltinArrayCtor) { newArr->DirectSetItemAt(i, element); } @@ -10041,6 +10100,7 @@ namespace Js { if (JavascriptArray::Is(arr)) { + arr = EnsureNonNativeArray(arr); ArrayElementEnumerator e(arr, startIndex, limitIndex); while(e.MoveNext()) @@ -10096,6 +10156,7 @@ namespace Js { if (JavascriptArray::Is(arr)) { + arr = EnsureNonNativeArray(arr); ArrayElementEnumerator e(arr, startIndex, limitIndex); while(e.MoveNext()) @@ -10849,6 +10910,9 @@ namespace Js JavascriptArray *JavascriptArray::EnsureNonNativeArray(JavascriptArray *arr) { +#if ENABLE_COPYONACCESS_ARRAY + JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray(arr); +#endif if (JavascriptNativeIntArray::Is(arr)) { arr = JavascriptNativeIntArray::ToVarArray((JavascriptNativeIntArray*)arr); @@ -10931,23 +10995,6 @@ namespace Js } } - template - void JavascriptArray::InternalFillFromPrototype(JavascriptArray *dstArray, const T& dstIndex, JavascriptArray *srcArray, uint32 start, uint32 end, uint32 count) - { - RecyclableObject* prototype = srcArray->GetPrototype(); - while (start + count != end && JavascriptOperators::GetTypeId(prototype) != TypeIds_Null) - { - ForEachOwnMissingArrayIndexOfObject(srcArray, dstArray, prototype, start, end, dstIndex, [&](uint32 index, Var value) { - T n = dstIndex + (index - start); - dstArray->DirectSetItemAt(n, value); - - count++; - }); - - prototype = prototype->GetPrototype(); - } - } - Var JavascriptArray::SpreadArrayArgs(Var arrayToSpread, const Js::AuxArray *spreadIndices, ScriptContext *scriptContext) { // At this stage we have an array literal with some arguments to be spread. @@ -11458,6 +11505,11 @@ namespace Js { if (!JavascriptOperators::GetProperty((RecyclableObject*)constructor, PropertyIds::_symbolSpecies, &constructor, scriptContext)) { + if (pIsBuiltinArrayCtor != nullptr) + { + *pIsBuiltinArrayCtor = false; + } + return nullptr; } if (constructor == scriptContext->GetLibrary()->GetNull()) diff --git a/lib/Runtime/Library/JavascriptPromise.cpp b/lib/Runtime/Library/JavascriptPromise.cpp index b15eb469e32..7a6d4dafaef 100644 --- a/lib/Runtime/Library/JavascriptPromise.cpp +++ b/lib/Runtime/Library/JavascriptPromise.cpp @@ -910,7 +910,7 @@ namespace Js try { - values->DirectSetItemAt(index, x); + values->SetItem(index, x, PropertyOperation_None); } catch (JavascriptExceptionObject* e) { diff --git a/test/Array/Array_TypeConfusion_bugs.js b/test/Array/Array_TypeConfusion_bugs.js index c3661c8bf2a..d432607f1aa 100644 --- a/test/Array/Array_TypeConfusion_bugs.js +++ b/test/Array/Array_TypeConfusion_bugs.js @@ -9,11 +9,19 @@ if (this.WScript && this.WScript.LoadScriptFile) { // Check for running in ch this.WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js"); } +var restorePropertyFromDescriptor = function (obj, prop, desc) { + if (typeof desc == 'undefined') { + delete obj[prop]; + } else { + Object.defineProperty(obj, prop, desc); + } +} + var tests = [ { - name: "OS7342663:OOB writes using type confusion in InternalCopyArrayElements", - body: function () - { + name: "OS7342663:OOB writes using type confusion in InternalCopyArrayElements", + body: function () + { function test() { var arr1 = [0xdead, 0xbabe, 0xdead, 0xbabe]; @@ -85,6 +93,7 @@ var tests = [ { function test() { var arr = [1, 2] + var desc = Object.getOwnPropertyDescriptor(arr.constructor, Symbol.species); //Our species function will get called during chakra!Js::JavascriptArray::SliceHelper Object.defineProperty( @@ -108,6 +117,8 @@ var tests = [ assert.areEqual(2, brr.length, "brr.length == 2"); assert.areEqual(WScript, brr[0], "brr[0] == WScript"); assert.areEqual(2, brr[1], "brr[0] == WScript"); + + restorePropertyFromDescriptor(arr.constructor, Symbol.species, desc); } test(); test(); @@ -169,6 +180,7 @@ var tests = [ { function test() { var arr = [ 1, 2 ]; + var desc = Object.getOwnPropertyDescriptor(arr.constructor, Symbol.species); Object.defineProperty( arr.constructor, @@ -192,6 +204,8 @@ var tests = [ assert.areEqual(2, brr.length, "brr.length == 2"); assert.areEqual(WScript, brr[0], "brr[0] == WScript"); assert.areEqual(undefined, brr[1], "brr[1] == undefined"); + + restorePropertyFromDescriptor(arr.constructor, Symbol.species, desc); } test(); test(); @@ -246,6 +260,7 @@ var tests = [ function test() { //create a TypeIds_Array holding two 64 bit values (The same amount of space for four 32 bit values). var arr = [ WScript, WScript ]; + var desc = Object.getOwnPropertyDescriptor(arr.constructor, Symbol.species); //Our species function will get called during chakra!Js::JavascriptArray::EntrySplice Object.defineProperty( @@ -270,6 +285,8 @@ var tests = [ assert.areEqual(WScript, brr[1], "brr[1] == WScript"); assert.areEqual(undefined, brr[2], "brr[2] == undefined"); assert.areEqual(undefined, brr[3], "brr[3] == undefined"); + + restorePropertyFromDescriptor(arr.constructor, Symbol.species, desc); } test(); test(); @@ -396,5 +413,166 @@ var tests = [ test(); } }, + { + name: "[MSRC34910] type confusion in Array.prototype.filter", + body: function () + { + function mappingFn(elem, index, arr) { + arr[1] = 'hello'; + return true; + } + + var arr = [1, 2, 3]; + + var desc = Object.getOwnPropertyDescriptor(arr.constructor, Symbol.species); + Object.defineProperty(arr.constructor, Symbol.species, { get : function () { return function() { return [22, 33]; } } } ); + var b = Array.prototype.filter.call(arr, mappingFn); + assert.areEqual('hello', b[1]); + + restorePropertyFromDescriptor(arr.constructor, Symbol.species, desc); + } + }, + { + name: "[MSRC35046] heap overflow in Array.prototype.splice", + body: function () + { + var a = []; + var o = {}; + + Object.defineProperty(o, 'constructor', { + get: function() { + a.length = 0xfffffffe; + [].fill.call(a, 0, 0xfffff000, 0xfffffffe); + return Array; + }}); + + a.__proto__ = o; + var q = new Array(50).fill(1.1); + + var b = [].splice.call(a, 0, 0, ...q); + assert.areEqual(50, a.length); + assert.areEqual(q, a); + } + }, + { + name: "[MSRC35086] type confusion in FillFromPrototypes", + body: function () + { + var a = new Array(0x11111111, 0x22222222, 0x33333333, 0x44444444, 0x12121212); + + var handler = { + getPrototypeOf: function(target, name) { + return a; + } + }; + + var p = new Proxy([], handler); + var b = [{}, [], "abc"]; + + b.__proto__ = p; + b.length = 4; + var c = [[],"abc",1145324612]; + + a.shift.call(b); + assert.areEqual(3, b.length); + assert.areEqual([], b[0]); + assert.areEqual("abc", b[1]); + assert.areEqual(1145324612, b[2]); + } + }, + { + name: "[MSRC35272] type confusion in JSON.parse", + body: function () + { + var a = 1; + var once = false; + function f(){ + if(!once){ + a = new Array(2) + this[2] = a; + } + once = true; + return 0x41414141; + } + + var r = JSON.parse("[1111, 22222, 333333]", f); + assert.areEqual(0x41414141, r); + } + }, + { + name: "[MSRC35383] type confusion in Array.prototype.concat", + body: function () + { + var n = []; + for (var i = 0; i < 0x10; i++) + n.push([0x11111111, 0x11111111, 0, 0x11111111,0x11111111, 0x11111111, 0, 0x11111111,0x11111111, 0x11111111, 0, 0x11111111,0x11111111,0x11111111, 0, 0x11111111,0x11111111 ,1 ,2 ,3 ,4]); + + class fake extends Object { + static get [Symbol.species]() { return function() { return n[3]; }; }; + } + + var f = function(a){ return a; } + + var x = ["dabao", 0, 0, 0x41414141]; + var y = new Proxy(x, { + get: function(t, p, r) { + return (p == "constructor") ? fake : x[p]; + } + }); + + assert.areEqual(x, Array.prototype.concat.apply(y)); + } + }, + { + name: "[MSRC35389] type confusion in Array.prototype.splice", + body: function () + { + var arr = [0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344] + class fake extends Object { + static get [Symbol.species]() { return function() { + return arr; + }; }; + } + + var x = [0, 2, 0, 0x41414141]; + var y = new Proxy(x, { + get: function(t, p, r) { + return (p == "constructor") ? fake : x[p]; + } + }); + + Array.prototype.splice.apply(y); + assert.areEqual(x, y); + } + }, + { + name: "[MSRC35389 variation] type confusion in Array.prototype.slice", + body: function () + { + var arr = [0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344,0x41424344]; + + class fake extends Object { + static get [Symbol.species]() { return function() { + return arr; + }; }; + } + + var x = [0, 2, 0, 0x41414141]; + var y = new Proxy(x, { + get: function(t, p, r) { + if (p == "constructor") + return fake + if (p == 'length'); + return 1; + return x[p]; + }, + has: function() { + return false; + } + }); + + assert.areEqual([0x41424344], Array.prototype.slice.call(y)); + } + }, ]; testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" }); diff --git a/test/es6/ES6NewTarget_bugfixes.js b/test/es6/ES6NewTarget_bugfixes.js index 607734b29d0..c5aecfc89a8 100644 --- a/test/es6/ES6NewTarget_bugfixes.js +++ b/test/es6/ES6NewTarget_bugfixes.js @@ -25,6 +25,14 @@ var tests = [ new.target; // bug repro: SyntaxError: Invalid use of the 'new.target' keyword } }, + { + name: "[MSRC35208] parameter type confusion in eval", + body: function () + { + var proxy = new Proxy(eval, {}); + assert.areEqual(0, proxy("Math.sin(0)")); + } + }, ]; testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });