[CVE-2018-0931] CallInfo.Count converted to wrong type while passed a…

…s function parameter - Individual 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 >16bits. I changed these shorts to uints and added the non-default warnings for truncation which would have found these issues.
chakra-core · Mar 12, 2018 · 46c98f0 · 46c98f0
1 parent 4bd01df
commit 46c98f0
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 20 deletions.
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)