[CVE-2018-0931] CallInfo.Count converted to wrong type while passed as function parameter - Individual

tcare · akroshg · commit 46c98f0645df · 2018-03-12T11:59:33.000-07:00
There are a few places where we are silently truncating the 24 bit CallInfo to 16 bits. It is possible in spread scenarios that we could use &gt;16bits.

I changed these shorts to uints and added the non-default warnings for truncation which would have found these issues.
diff --git a/Build/Common.Build.props b/Build/Common.Build.props
@@ -78,6 +78,9 @@
       <!-- Separate global variable for linker -->
       <AdditionalOptions>%(AdditionalOptions) /Gw</AdditionalOptions>
 
+      <!-- Enable warnings not included in W4 by default -->
+      <AdditionalOptions>%(AdditionalOptions) /w44242 /w44254</AdditionalOptions>
+
       <ProgramDataBaseFileName Condition="'$(ConfigurationType)'=='StaticLibrary'">$(IntDir)$(TargetName).pdb</ProgramDataBaseFileName>
       <ProgramDataBaseFileName Condition="'$(ConfigurationType)'!='StaticLibrary'">$(IntDir)</ProgramDataBaseFileName>
 
diff --git a/lib/Runtime/Base/CallInfo.h b/lib/Runtime/Base/CallInfo.h
@@ -50,7 +50,7 @@ namespace Js
             AssertOrFailFastMsg(count < CallInfo::kMaxCountArgs, "Argument list too large");
         }
 
-        CallInfo(CallFlags flags, ArgSlot count)
+        CallInfo(CallFlags flags, uint count)
             : Flags(flags)
             , Count(count)
 #ifdef TARGET_64
@@ -60,16 +60,6 @@ namespace Js
             // Keeping this version to avoid the assert
         }
 
-        // The bool is used to avoid the signature confusion between the ArgSlot and uint version of the constructor
-        CallInfo(CallFlags flags, uint count, bool unusedBool)
-            : Flags(flags)
-            , Count(count)
-#ifdef TARGET_64
-            , unused(0)
-#endif
-        {
-            AssertOrFailFastMsg(count < CallInfo::kMaxCountArgs, "Argument list too large");
-        }
 
         CallInfo(VirtualTableInfoCtorEnum v)
         {
@@ -107,7 +97,7 @@ namespace Js
         }
 
         // New target value is passed as an extra argument which is nto included in the Count
-        static Var GetNewTarget(CallFlags flag, Var* values, ArgSlot count)
+        static Var GetNewTarget(CallFlags flag, Var* values, uint count)
         {
             if (HasNewTarget(flag))
             {
diff --git a/lib/Runtime/Language/DynamicProfileInfo.cpp b/lib/Runtime/Language/DynamicProfileInfo.cpp
@@ -505,7 +505,7 @@ namespace Js
     }
 #endif
 
-    void DynamicProfileInfo::RecordCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId, FunctionInfo* calleeFunctionInfo, JavascriptFunction* calleeFunction, ArgSlot actualArgCount, bool isConstructorCall, InlineCacheIndex ldFldInlineCacheId)
+    void DynamicProfileInfo::RecordCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId, FunctionInfo* calleeFunctionInfo, JavascriptFunction* calleeFunction, uint actualArgCount, bool isConstructorCall, InlineCacheIndex ldFldInlineCacheId)
     {
 #if DBG_DUMP || defined(DYNAMIC_PROFILE_STORAGE) || defined(RUNTIME_DATA_COLLECTION)
         // If we persistsAcrossScriptContext, the dynamic profile info may be referred to by multiple function body from
diff --git a/lib/Runtime/Language/DynamicProfileInfo.h b/lib/Runtime/Language/DynamicProfileInfo.h
@@ -389,7 +389,7 @@ namespace Js
 #ifdef ASMJS_PLAT
         void RecordAsmJsCallSiteInfo(FunctionBody* callerBody, ProfileId callSiteId, FunctionBody* calleeBody);
 #endif
-        void RecordCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId, FunctionInfo * calleeFunctionInfo, JavascriptFunction* calleeFunction, ArgSlot actualArgCount, bool isConstructorCall, InlineCacheIndex ldFldInlineCacheId = Js::Constants::NoInlineCacheIndex);
+        void RecordCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId, FunctionInfo * calleeFunctionInfo, JavascriptFunction* calleeFunction, uint actualArgCount, bool isConstructorCall, InlineCacheIndex ldFldInlineCacheId = Js::Constants::NoInlineCacheIndex);
         void RecordConstParameterAtCallSite(ProfileId callSiteId, int argNum);
         static bool HasCallSiteInfo(FunctionBody* functionBody);
         bool HasCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId); // Does a particular callsite have ProfileInfo?
diff --git a/lib/Runtime/Language/JavascriptStackWalker.cpp b/lib/Runtime/Language/JavascriptStackWalker.cpp
@@ -1443,15 +1443,20 @@ namespace Js
     {
         Assert(!IsTopMostFrame());
         Assert(currentIndex);
-
+#pragma warning(push)
+#pragma warning(disable: 4254)
         return GetFrameAtIndex(currentIndex - 1)->callInfo.InlineeStartOffset;
+#pragma warning(pop)
     }
 
     uint32 InlinedFrameWalker::GetBottomMostInlineeOffset() const
     {
         Assert(frameCount);
 
+#pragma warning(push)
+#pragma warning(disable: 4254)
         return GetFrameAtIndex(frameCount - 1)->callInfo.InlineeStartOffset;
+#pragma warning(pop)
     }
 
     Js::JavascriptFunction *InlinedFrameWalker::GetBottomMostFunctionObject() const
diff --git a/lib/Runtime/Library/JavascriptExternalFunction.cpp b/lib/Runtime/Library/JavascriptExternalFunction.cpp
@@ -273,6 +273,14 @@ namespace Js
 
         ScriptContext * scriptContext = externalFunction->type->GetScriptContext();
         AnalysisAssert(scriptContext);
+
+        if (args.Info.Count > USHORT_MAX)
+        {
+            // Due to compat reasons, stdcall external functions expect a ushort count of args.
+            // To support more than this we will need a new API.
+            Js::JavascriptError::ThrowTypeError(scriptContext, JSERR_ArgListTooLarge);
+        }
+
         Var result = NULL;
 
 #if ENABLE_TTD
@@ -284,14 +292,15 @@ namespace Js
         {
             BEGIN_LEAVE_SCRIPT(scriptContext)
             {
-                result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, args.Info.Count, externalFunction->callbackState);
+                // ArgCount truncation has been verified above.
+                result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, static_cast<USHORT>(args.Info.Count), externalFunction->callbackState);
             }
             END_LEAVE_SCRIPT(scriptContext);
         }
 #else
         BEGIN_LEAVE_SCRIPT(scriptContext)
         {
-            result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, args.Info.Count, externalFunction->callbackState);
+            result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, static_cast<USHORT>(args.Info.Count), externalFunction->callbackState);
         }
         END_LEAVE_SCRIPT(scriptContext);
 #endif
@@ -417,6 +426,13 @@ namespace Js
         }
         else
         {
+            if (args.Info.Count > USHORT_MAX)
+            {
+                // Due to compat reasons, stdcall external functions expect a ushort count of args.
+                // To support more than this we will need a new API.
+                Js::JavascriptError::ThrowTypeError(scriptContext, JSERR_ArgListTooLarge);
+            }
+
             TTDAssert(scriptContext->ShouldPerformRecordAction(), "Check either record/replay before calling!!!");
 
             TTD::EventLog* elog = scriptContext->GetThreadContext()->TTDLog;
@@ -426,7 +442,8 @@ namespace Js
 
             BEGIN_LEAVE_SCRIPT(scriptContext)
             {
-                result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, args.Info.Count, externalFunction->callbackState);
+                // ArgCount truncation has been verified above.
+                result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, static_cast<ushort>(args.Info.Count), externalFunction->callbackState);
             }
             END_LEAVE_SCRIPT(scriptContext);
 
diff --git a/lib/Runtime/Library/JavascriptFunction.cpp b/lib/Runtime/Library/JavascriptFunction.cpp
@@ -878,7 +878,7 @@ namespace Js
         if (overridingNewTarget != nullptr)
         {
             ScriptFunction * scriptFunctionObj = JavascriptOperators::TryFromVar<ScriptFunction>(functionObj);
-            ushort newCount = args.Info.Count;
+            uint newCount = args.Info.Count;
             if (scriptFunctionObj && scriptFunctionObj->GetFunctionInfo()->IsClassConstructor())
             {
                 thisAlreadySpecified = true;
@@ -1097,7 +1097,7 @@ namespace Js
         uint32 actualLength = CallInfo::GetLargeArgCountWithExtraArgs(args.Info.Flags, spreadSize);
 
         // Allocate (if needed) space for the expanded arguments.
-        Arguments outArgs(CallInfo(args.Info.Flags, spreadSize, /* unUsedBool */ false), nullptr);
+        Arguments outArgs(CallInfo(args.Info.Flags, spreadSize), nullptr);
         Var stackArgs[STACK_ARGS_ALLOCA_THRESHOLD];
         size_t outArgsSize = 0;
         if (actualLength > STACK_ARGS_ALLOCA_THRESHOLD)

Original file line number	Diff line number	Diff line change
`@@ -50,7 +50,7 @@ namespace Js`
`50`	`50`	`AssertOrFailFastMsg(count < CallInfo::kMaxCountArgs, "Argument list too large");`
`51`	`51`	`}`
`52`	`52`
`53`		`- CallInfo(CallFlags flags, ArgSlot count)`
	`53`	`+ CallInfo(CallFlags flags, uint count)`
`54`	`54`	`: Flags(flags)`
`55`	`55`	`, Count(count)`
`56`	`56`	`#ifdef TARGET_64`
`@@ -60,16 +60,6 @@ namespace Js`
`60`	`60`	`// Keeping this version to avoid the assert`
`61`	`61`	`}`
`62`	`62`
`63`		`- // The bool is used to avoid the signature confusion between the ArgSlot and uint version of the constructor`
`64`		`- CallInfo(CallFlags flags, uint count, bool unusedBool)`
`65`		`- : Flags(flags)`
`66`		`- , Count(count)`
`67`		`-#ifdef TARGET_64`
`68`		`- , unused(0)`
`69`		`-#endif`
`70`		`- {`
`71`		`- AssertOrFailFastMsg(count < CallInfo::kMaxCountArgs, "Argument list too large");`
`72`		`- }`
`73`	`63`
`74`	`64`	`CallInfo(VirtualTableInfoCtorEnum v)`
`75`	`65`	`{`
`@@ -107,7 +97,7 @@ namespace Js`
`107`	`97`	`}`
`108`	`98`
`109`	`99`	`// New target value is passed as an extra argument which is nto included in the Count`
`110`		`- static Var GetNewTarget(CallFlags flag, Var* values, ArgSlot count)`
	`100`	`+ static Var GetNewTarget(CallFlags flag, Var* values, uint count)`
`111`	`101`	`{`
`112`	`102`	`if (HasNewTarget(flag))`
`113`	`103`	`{`
Original file line number	Diff line number	Diff line change
`@@ -505,7 +505,7 @@ namespace Js`
`505`	`505`	`}`
`506`	`506`	`#endif`
`507`	`507`
`508`		`- void DynamicProfileInfo::RecordCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId, FunctionInfo* calleeFunctionInfo, JavascriptFunction* calleeFunction, ArgSlot actualArgCount, bool isConstructorCall, InlineCacheIndex ldFldInlineCacheId)`
	`508`	`+ void DynamicProfileInfo::RecordCallSiteInfo(FunctionBody* functionBody, ProfileId callSiteId, FunctionInfo* calleeFunctionInfo, JavascriptFunction* calleeFunction, uint actualArgCount, bool isConstructorCall, InlineCacheIndex ldFldInlineCacheId)`
`509`	`509`	`{`
`510`	`510`	`#if DBG_DUMP \|\| defined(DYNAMIC_PROFILE_STORAGE) \|\| defined(RUNTIME_DATA_COLLECTION)`
`511`	`511`	`// If we persistsAcrossScriptContext, the dynamic profile info may be referred to by multiple function body from`
Original file line number	Diff line number	Diff line change
`@@ -1443,15 +1443,20 @@ namespace Js`
`1443`	`1443`	`{`
`1444`	`1444`	`Assert(!IsTopMostFrame());`
`1445`	`1445`	`Assert(currentIndex);`
`1446`		`-`
	`1446`	`+#pragma warning(push)`
	`1447`	`+#pragma warning(disable: 4254)`
`1447`	`1448`	`return GetFrameAtIndex(currentIndex - 1)->callInfo.InlineeStartOffset;`
	`1449`	`+#pragma warning(pop)`
`1448`	`1450`	`}`
`1449`	`1451`
`1450`	`1452`	`uint32 InlinedFrameWalker::GetBottomMostInlineeOffset() const`
`1451`	`1453`	`{`
`1452`	`1454`	`Assert(frameCount);`
`1453`	`1455`
	`1456`	`+#pragma warning(push)`
	`1457`	`+#pragma warning(disable: 4254)`
`1454`	`1458`	`return GetFrameAtIndex(frameCount - 1)->callInfo.InlineeStartOffset;`
	`1459`	`+#pragma warning(pop)`
`1455`	`1460`	`}`
`1456`	`1461`
`1457`	`1462`	`Js::JavascriptFunction *InlinedFrameWalker::GetBottomMostFunctionObject() const`
Original file line number	Diff line number	Diff line change
`@@ -273,6 +273,14 @@ namespace Js`
`273`	`273`
`274`	`274`	`ScriptContext * scriptContext = externalFunction->type->GetScriptContext();`
`275`	`275`	`AnalysisAssert(scriptContext);`
	`276`	`+`
	`277`	`+ if (args.Info.Count > USHORT_MAX)`
	`278`	`+ {`
	`279`	`+ // Due to compat reasons, stdcall external functions expect a ushort count of args.`
	`280`	`+ // To support more than this we will need a new API.`
	`281`	`+ Js::JavascriptError::ThrowTypeError(scriptContext, JSERR_ArgListTooLarge);`
	`282`	`+ }`
	`283`	`+`
`276`	`284`	`Var result = NULL;`
`277`	`285`
`278`	`286`	`#if ENABLE_TTD`
`@@ -284,14 +292,15 @@ namespace Js`
`284`	`292`	`{`
`285`	`293`	`BEGIN_LEAVE_SCRIPT(scriptContext)`
`286`	`294`	`{`
`287`		`- result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, args.Info.Count, externalFunction->callbackState);`
	`295`	`+ // ArgCount truncation has been verified above.`
	`296`	`+ result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, static_cast<USHORT>(args.Info.Count), externalFunction->callbackState);`
`288`	`297`	`}`
`289`	`298`	`END_LEAVE_SCRIPT(scriptContext);`
`290`	`299`	`}`
`291`	`300`	`#else`
`292`	`301`	`BEGIN_LEAVE_SCRIPT(scriptContext)`
`293`	`302`	`{`
`294`		`- result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, args.Info.Count, externalFunction->callbackState);`
	`303`	`+ result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, static_cast<USHORT>(args.Info.Count), externalFunction->callbackState);`
`295`	`304`	`}`
`296`	`305`	`END_LEAVE_SCRIPT(scriptContext);`
`297`	`306`	`#endif`
`@@ -417,6 +426,13 @@ namespace Js`
`417`	`426`	`}`
`418`	`427`	`else`
`419`	`428`	`{`
	`429`	`+ if (args.Info.Count > USHORT_MAX)`
	`430`	`+ {`
	`431`	`+ // Due to compat reasons, stdcall external functions expect a ushort count of args.`
	`432`	`+ // To support more than this we will need a new API.`
	`433`	`+ Js::JavascriptError::ThrowTypeError(scriptContext, JSERR_ArgListTooLarge);`
	`434`	`+ }`
	`435`	`+`
`420`	`436`	`TTDAssert(scriptContext->ShouldPerformRecordAction(), "Check either record/replay before calling!!!");`
`421`	`437`
`422`	`438`	`TTD::EventLog* elog = scriptContext->GetThreadContext()->TTDLog;`
`@@ -426,7 +442,8 @@ namespace Js`
`426`	`442`
`427`	`443`	`BEGIN_LEAVE_SCRIPT(scriptContext)`
`428`	`444`	`{`
`429`		`- result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, args.Info.Count, externalFunction->callbackState);`
	`445`	`+ // ArgCount truncation has been verified above.`
	`446`	`+ result = externalFunction->stdCallNativeMethod(function, ((callInfo.Flags & CallFlags_New) != 0), args.Values, static_cast<ushort>(args.Info.Count), externalFunction->callbackState);`
`430`	`447`	`}`
`431`	`448`	`END_LEAVE_SCRIPT(scriptContext);`
`432`	`449`