[1.3>master] [1.2>1.3] [MERGE #1982 @suwc] 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

Suwei Chen · Suwei Chen · commit 55358e3c44c0 · 2016-11-22T22:19:13.000-08:00
Merge pull request #1982 from suwc:build/suwc/bugfix 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.
diff --git a/lib/Runtime/Base/CallInfo.h b/lib/Runtime/Base/CallInfo.h
@@ -72,6 +72,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
@@ -493,7 +493,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
@@ -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
@@ -752,6 +752,40 @@ namespace Js
         return IsMissingHeadSegmentItemImpl<double>(index);
     }
 
+    template<typename T>
+    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<uint32>(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)
     {
@@ -3380,6 +3414,9 @@ namespace Js
         pDestObj = ArraySpeciesCreate(args[0], 0, scriptContext);
         if (pDestObj)
         {
+#if ENABLE_COPYONACCESS_ARRAY
+            JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(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.
@@ -5958,13 +5995,19 @@ namespace Js
             }
 
             newArr = CreateNewArrayHelper(static_cast<uint32>(newLenT), isIntArray, isFloatArray, pArr, scriptContext);
+#if ENABLE_COPYONACCESS_ARRAY
+            JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(newArr);
+#endif
             newObj = newArr;
         }
         else
         {
             // 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<Var>(newObj);
+#endif
                 newArr = JavascriptArray::FromVar(newObj);
             }
         }
@@ -6744,6 +6787,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<Var>(newObj);
+#endif
                     newArr = JavascriptArray::FromVar(newObj);
                 }
             }
@@ -6752,10 +6798,13 @@ namespace Js
             {
                 pArr->GetArrayTypeAndConvert(&isIntArray, &isFloatArray);
                 newArr = CreateNewArrayHelper(deleteLen, isIntArray, isFloatArray, pArr, scriptContext);
+#if ENABLE_COPYONACCESS_ARRAY
+                JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(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
@@ -7246,6 +7295,9 @@ namespace Js
 
         if (JavascriptArray::Is(pNewObj))
         {
+#if ENABLE_COPYONACCESS_ARRAY
+            JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(pNewObj);
+#endif
             pnewArr = JavascriptArray::FromVar(pNewObj);
         }
 
@@ -8993,6 +9045,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<Var>(newObj);
+#endif
                 newArr = JavascriptArray::FromVar(newObj);
             }
         }
@@ -9182,7 +9237,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)
@@ -9196,6 +9252,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<Var>(newObj);
+#endif
                 newArr = JavascriptArray::FromVar(newObj);
             }
         }
@@ -9224,7 +9283,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);
                     }
@@ -10071,6 +10130,60 @@ namespace Js
     }
 #endif
 
+    template <typename Fn>
+    void JavascriptArray::ForEachOwnArrayIndexOfObject(RecyclableObject* obj, uint32 startIndex, uint32 limitIndex, Fn fn)
+    {
+        Assert(DynamicObject::IsAnyArray(obj) || JavascriptOperators::IsObject(obj));
+
+        JavascriptArray* arr = nullptr;
+        if (DynamicObject::IsAnyArray(obj))
+        {
+            arr = JavascriptArray::FromAnyArray(obj);
+        }
+        else if (DynamicType::Is(obj->GetTypeId()))
+        {
+            DynamicObject* dynobj = DynamicObject::FromVar(obj);
+            arr = dynobj->GetObjectArray();
+        }
+
+        if (arr != nullptr)
+        {
+            if (JavascriptArray::Is(arr))
+            {
+                arr = EnsureNonNativeArray(arr);
+                ArrayElementEnumerator e(arr, startIndex, limitIndex);
+
+                while(e.MoveNext<Var>())
+                {
+                    fn(e.GetIndex(), e.GetItem<Var>());
+                }
+            }
+            else
+            {
+                ScriptContext* scriptContext = obj->GetScriptContext();
+
+                Assert(ES5Array::Is(arr));
+
+                ES5Array* es5Array = ES5Array::FromVar(arr);
+                ES5ArrayIndexEnumerator<true> e(es5Array);
+
+                while (e.MoveNext())
+                {
+                    uint32 index = e.GetIndex();
+
+                    if (index < startIndex) continue;
+                    else if (index >= limitIndex) break;
+
+                    Var value = nullptr;
+                    if (JavascriptOperators::GetOwnItem(es5Array, index, &value, scriptContext))
+                    {
+                        fn(index, value);
+                    }
+                }
+            }
+        }
+    }
+
     template <typename T, typename Fn>
     void JavascriptArray::ForEachOwnMissingArrayIndexOfObject(JavascriptArray *baseArray, JavascriptArray *destArray, RecyclableObject* obj, uint32 startIndex, uint32 limitIndex, T destIndex, Fn fn)
     {
@@ -10093,6 +10206,7 @@ namespace Js
         {
             if (JavascriptArray::Is(arr))
             {
+                arr = EnsureNonNativeArray(arr);
                 ArrayElementEnumerator e(arr, startIndex, limitIndex);
 
                 while(e.MoveNext<Var>())
@@ -10846,6 +10960,9 @@ namespace Js
 
     JavascriptArray *JavascriptArray::EnsureNonNativeArray(JavascriptArray *arr)
     {
+#if ENABLE_COPYONACCESS_ARRAY
+        JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(arr);
+#endif
         if (JavascriptNativeIntArray::Is(arr))
         {
             arr = JavascriptNativeIntArray::ToVarArray((JavascriptNativeIntArray*)arr);
@@ -10928,23 +11045,6 @@ namespace Js
         }
     }
 
-    template<typename T>
-    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<uint32> *spreadIndices, ScriptContext *scriptContext)
     {
         // At this stage we have an array literal with some arguments to be spread.
@@ -11559,6 +11659,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
@@ -956,7 +956,7 @@ namespace Js
 
         try
         {
-            values->DirectSetItemAt(index, x);
+            values->SetItem(index, x, PropertyOperation_None);
         }
         catch (const JavascriptException& err)
         {
diff --git a/test/Array/Array_TypeConfusion_bugs.js b/test/Array/Array_TypeConfusion_bugs.js
diff --git a/test/es6/ES6NewTarget_bugfixes.js b/test/es6/ES6NewTarget_bugfixes.js

Original file line number	Diff line number	Diff line change
`@@ -493,7 +493,7 @@ namespace Js`
`493`	`493`	`// TODO: Handle call from global scope, strict mode`
`494`	`494`	`BOOL isIndirect = FALSE;`
`495`	`495`
`496`		`- if (args.Info.Flags & CallFlags_ExtraArg)`
	`496`	`+ if (Js::CallInfo::isDirectEvalCall(args.Info.Flags))`
`497`	`497`	`{`
`498`	`498`	`// This was recognized as an eval call at compile time. The last one or two args are internal to us.`
`499`	`499`	`// Argcount will be one of the following when called from global code`
Original file line number	Diff line number	Diff line change
`@@ -956,7 +956,7 @@ namespace Js`
`956`	`956`
`957`	`957`	`try`
`958`	`958`	`{`
`959`		`- values->DirectSetItemAt(index, x);`
	`959`	`+ values->SetItem(index, x, PropertyOperation_None);`
`960`	`960`	`}`
`961`	`961`	`catch (const JavascriptException& err)`
`962`	`962`	`{`