[CVE-2017-11861] [ChakraCore] Chakra JIT - Incorrect integer overflow check in Lowerer::LowerBoundCheck - Google, Inc.

Math on IntConstType should be bounded by IRType of the Opnd. In case of Lowerer::LowerBoundCheck, it ended up that the IntConstOpnd is a TyInt32 and the overflow leads to bad bound check being emitted.
For this I added a new IntConstMath class which takes an IRType as a parameter and validates that the result can be represented by that IRType.

lib/Backend/Backend.h

@@ -139,6 +139,7 @@ enum IRDumpFlags
 #include "SymTable.h"
 #include "IR.h"
 #include "Opnd.h"
+#include "IntConstMath.h"
 #include "IntOverflowDoesNotMatterRange.h"
 #include "IntConstantBounds.h"
 #include "ValueRelativeOffset.h"

lib/Backend/CMakeLists.txt

@@ -39,6 +39,7 @@ add_library (Chakra.Backend OBJECT
     InliningDecider.cpp
     InliningHeuristics.cpp
     IntBounds.cpp
+    IntConstMath.cpp
     InterpreterThunkEmitter.cpp
     JITThunkEmitter.cpp
     JITOutput.cpp

lib/Backend/Chakra.Backend.vcxproj

@@ -218,6 +218,7 @@
     <ClCompile Include="$(MSBuildThisFileDirectory)GlobOptBlockData.cpp" />
     <ClCompile Include="$(MSBuildThisFileDirectory)ValueInfo.cpp" />
     <ClCompile Include="$(MSBuildThisFileDirectory)JITThunkEmitter.cpp" />
+    <ClCompile Include="$(MSBuildThisFileDirectory)IntConstMath.cpp" />
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="AgenPeeps.h" />
@@ -259,6 +260,7 @@
     <ClInclude Include="FunctionJITRuntimeInfo.h" />
     <ClInclude Include="FunctionJITTimeInfo.h" />
     <ClInclude Include="GlobOptBlockData.h" />
+    <ClInclude Include="IntConstMath.h" />
     <ClInclude Include="IRBaseTypeList.h" />
     <ClInclude Include="IRBuilderAsmJs.h" />
     <ClInclude Include="BackendOpCodeAttrAsmJs.h" />

lib/Backend/Chakra.Backend.vcxproj.filters

@@ -130,6 +130,7 @@
     <ClCompile Include="$(MSBuildThisFileDirectory)GlobOptBlockData.cpp" />
     <ClCompile Include="$(MSBuildThisFileDirectory)ValueInfo.cpp" />
     <ClCompile Include="$(MSBuildThisFileDirectory)JITThunkEmitter.cpp" />
+    <ClCompile Include="$(MSBuildThisFileDirectory)IntConstMath.cpp" />
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="AgenPeeps.h" />
@@ -345,6 +346,7 @@
     <ClInclude Include="GlobOptBlockData.h" />
     <ClInclude Include="ValueInfo.h" />
     <ClInclude Include="JITThunkEmitter.h" />
+    <ClInclude Include="IntConstMath.h" />
   </ItemGroup>
   <ItemGroup>
     <MASM Include="$(MSBuildThisFileDirectory)amd64\LinearScanMdA.asm">

lib/Backend/GlobOpt.cpp

@@ -12351,7 +12351,7 @@ GlobOpt::OptConstFoldBinary(
     }
 
     IntConstType tmpValueOut;
-    if (!instr->BinaryCalculator(src1IntConstantValue, src2IntConstantValue, &tmpValueOut)
+    if (!instr->BinaryCalculator(src1IntConstantValue, src2IntConstantValue, &tmpValueOut, TyInt32)
         || !Math::FitsInDWord(tmpValueOut))
     {
         return false;

lib/Backend/IR.cpp

@@ -3807,28 +3807,28 @@ bool Instr::BinaryCalculatorT(T src1Const, T src2Const, int64 *pResult, bool che
 template bool Instr::BinaryCalculatorT<int>(int src1Const64, int src2Const64, int64 *pResult, bool checkWouldTrap);
 template bool Instr::BinaryCalculatorT<int64>(int64 src1Const64, int64 src2Const64, int64 *pResult, bool checkWouldTrap);
 
-bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, IntConstType *pResult)
+bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, IntConstType *pResult, IRType type)
 {
     IntConstType value = 0;
 
     switch (this->m_opcode)
     {
     case Js::OpCode::Add_A:
-        if (IntConstMath::Add(src1Const, src2Const, &value))
+        if (IntConstMath::Add(src1Const, src2Const, type, &value))
         {
             return false;
         }
         break;
 
     case Js::OpCode::Sub_A:
-        if (IntConstMath::Sub(src1Const, src2Const, &value))
+        if (IntConstMath::Sub(src1Const, src2Const, type, &value))
         {
             return false;
         }
         break;
 
     case Js::OpCode::Mul_A:
-        if (IntConstMath::Mul(src1Const, src2Const, &value))
+        if (IntConstMath::Mul(src1Const, src2Const, type, &value))
         {
             return false;
         }
@@ -3852,7 +3852,7 @@ bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, Int
             // folds to -0. Bail for now...
             return false;
         }
-        if (IntConstMath::Div(src1Const, src2Const, &value))
+        if (IntConstMath::Div(src1Const, src2Const, type, &value))
         {
             return false;
         }
@@ -3870,7 +3870,7 @@ bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, Int
             // Bail for now...
             return false;
         }
-        if (IntConstMath::Mod(src1Const, src2Const, &value))
+        if (IntConstMath::Mod(src1Const, src2Const, type, &value))
         {
             return false;
         }
@@ -3884,17 +3884,15 @@ bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, Int
 
     case Js::OpCode::Shl_A:
         // We don't care about overflow here
-        IntConstMath::Shl(src1Const, src2Const & 0x1F, &value);
+        value = src1Const << (src2Const & 0x1F);
         break;
 
     case Js::OpCode::Shr_A:
-        // We don't care about overflow here, and there shouldn't be any
-        IntConstMath::Shr(src1Const, src2Const & 0x1F, &value);
+        value = src1Const >> (src2Const & 0x1F);
         break;
 
     case Js::OpCode::ShrU_A:
-        // We don't care about overflow here, and there shouldn't be any
-        IntConstMath::ShrU(src1Const, src2Const & 0x1F, &value);
+        value = ((UIntConstType)src1Const) >> (src2Const & 0x1F);
         if (value < 0)
         {
             // ShrU produces a UInt32.  If it doesn't fit in an Int32, bail as we don't
@@ -3904,18 +3902,15 @@ bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, Int
         break;
 
     case Js::OpCode::And_A:
-        // We don't care about overflow here, and there shouldn't be any
-        IntConstMath::And(src1Const, src2Const, &value);
+        value = src1Const & src2Const;
         break;
 
     case Js::OpCode::Or_A:
-        // We don't care about overflow here, and there shouldn't be any
-        IntConstMath::Or(src1Const, src2Const, &value);
+        value = src1Const | src2Const;
         break;
 
     case Js::OpCode::Xor_A:
-        // We don't care about overflow here, and there shouldn't be any
-        IntConstMath::Xor(src1Const, src2Const, &value);
+        value = src1Const ^ src2Const;
         break;
 
     case Js::OpCode::InlineMathMin:
@@ -3935,7 +3930,7 @@ bool Instr::BinaryCalculator(IntConstType src1Const, IntConstType src2Const, Int
     return true;
 }
 
-bool Instr::UnaryCalculator(IntConstType src1Const, IntConstType *pResult)
+bool Instr::UnaryCalculator(IntConstType src1Const, IntConstType *pResult, IRType type)
 {
     IntConstType value = 0;
 
@@ -3948,14 +3943,14 @@ bool Instr::UnaryCalculator(IntConstType src1Const, IntConstType *pResult)
             return false;
         }
 
-        if (IntConstMath::Neg(src1Const, &value))
+        if (IntConstMath::Neg(src1Const, type, &value))
         {
             return false;
         }
         break;
 
     case Js::OpCode::Not_A:
-        IntConstMath::Not(src1Const, &value);
+        value = ~src1Const;
         break;
 
     case Js::OpCode::Ld_A:
@@ -3973,14 +3968,14 @@ bool Instr::UnaryCalculator(IntConstType src1Const, IntConstType *pResult)
         break;
 
     case Js::OpCode::Incr_A:
-        if (IntConstMath::Inc(src1Const, &value))
+        if (IntConstMath::Inc(src1Const, type, &value))
         {
             return false;
         }
         break;
 
     case Js::OpCode::Decr_A:
-        if (IntConstMath::Dec(src1Const, &value))
+        if (IntConstMath::Dec(src1Const, type, &value))
         {
             return false;
         }

lib/Backend/IR.h

@@ -334,10 +334,10 @@ public:
     bool            IsCmCC_R8();
     bool            IsCmCC_I4();
     bool            IsNeq();
-    bool            BinaryCalculator(IntConstType src1Const, IntConstType src2Const, IntConstType *pResult);
+    bool            BinaryCalculator(IntConstType src1Const, IntConstType src2Const, IntConstType *pResult, IRType type);
     template <typename T>     
     bool            BinaryCalculatorT(T src1Const, T src2Const, int64 *pResult, bool checkWouldTrap);
-    bool            UnaryCalculator(IntConstType src1Const, IntConstType *pResult);
+    bool            UnaryCalculator(IntConstType src1Const, IntConstType *pResult, IRType type);
     IR::Instr*      GetNextArg();
 
     // Iterates argument chain

lib/Backend/Inline.cpp

@@ -4419,7 +4419,7 @@ bool Inline::InlConstFold(IR::Instr *instr, IntConstType *pValue, __in_ecount_op
     {
         IntConstType src2Constant = *pValue;
 
-        if (!instr->BinaryCalculator(src1Constant, src2Constant, pValue)
+        if (!instr->BinaryCalculator(src1Constant, src2Constant, pValue, TyInt32)
             || !Math::FitsInDWord(*pValue))
         {
             return false;
@@ -4457,7 +4457,7 @@ bool Inline::InlConstFold(IR::Instr *instr, IntConstType *pValue, __in_ecount_op
     }
     else
     {
-        if (!instr->UnaryCalculator(src1Constant, pValue)
+        if (!instr->UnaryCalculator(src1Constant, pValue, TyInt32)
             || !Math::FitsInDWord(*pValue))
         {
             // Skip over BytecodeArgOutCapture

lib/Backend/IntBounds.cpp

@@ -743,22 +743,25 @@ bool IntBoundCheck::SetBoundOffset(const int offset, const bool isLoopCountBased
     // Determine the previous offset from the instruction (src1 <= src2 + dst)
     IR::IntConstOpnd *dstOpnd = nullptr;
     IntConstType previousOffset = 0;
+    IRType offsetType = TyMachReg;
     if (instr->GetDst())
     {
         dstOpnd = instr->GetDst()->AsIntConstOpnd();
         previousOffset = dstOpnd->GetValue();
+        offsetType = dstOpnd->GetType();
     }
 
     IR::IntConstOpnd *src1Opnd = nullptr;
     if (instr->GetSrc1()->IsIntConstOpnd())
     {
         src1Opnd = instr->GetSrc1()->AsIntConstOpnd();
-        if (IntConstMath::Sub(previousOffset, src1Opnd->GetValue(), &previousOffset))
+        if (IntConstMath::Sub(previousOffset, src1Opnd->GetValue(), src1Opnd->GetType(), &previousOffset))
             return false;
+        offsetType = src1Opnd->GetType();
     }
 
     IR::IntConstOpnd *src2Opnd = (instr->GetSrc2()->IsIntConstOpnd() ? instr->GetSrc2()->AsIntConstOpnd() : nullptr);
-    if(src2Opnd && IntConstMath::Add(previousOffset, src2Opnd->GetValue(), &previousOffset))
+    if(src2Opnd && IntConstMath::Add(previousOffset, src2Opnd->GetValue(), src2Opnd->GetType(), &previousOffset))
         return false;
 
     // Given a bounds check (a <= b + offset), the offset may only be decreased such that it does not invalidate the invariant
@@ -768,7 +771,7 @@ bool IntBoundCheck::SetBoundOffset(const int offset, const bool isLoopCountBased
         return true;
 
     IntConstType offsetDecrease;
-    if(IntConstMath::Sub(previousOffset, offset, &offsetDecrease))
+    if(IntConstMath::Sub(previousOffset, offset, offsetType, &offsetDecrease))
         return false;
 
     Assert(offsetDecrease > 0);
@@ -776,14 +779,14 @@ bool IntBoundCheck::SetBoundOffset(const int offset, const bool isLoopCountBased
     {
         // Prefer to increase src1, as this is an upper bound check and src1 corresponds to the index
         IntConstType newSrc1Value;
-        if(IntConstMath::Add(src1Opnd->GetValue(), offsetDecrease, &newSrc1Value))
+        if(IntConstMath::Add(src1Opnd->GetValue(), offsetDecrease, src1Opnd->GetType(), &newSrc1Value))
             return false;
         src1Opnd->SetValue(newSrc1Value);
     }
     else if(dstOpnd)
     {
         IntConstType newDstValue;
-        if(IntConstMath::Sub(dstOpnd->GetValue(), offsetDecrease, &newDstValue))
+        if(IntConstMath::Sub(dstOpnd->GetValue(), offsetDecrease, dstOpnd->GetType(), &newDstValue))
             return false;
         if (newDstValue == 0)
             instr->FreeDst();

lib/Backend/IntConstMath.cpp

@@ -0,0 +1,84 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+#include "Backend.h"
+
+bool IntConstMath::IsValid(IntConstType val, IRType type)
+{
+    switch (type)
+    {
+#if TARGET_32
+    case TyInt32:
+    case TyUint32:
+        CompileAssert(sizeof(IntConstType) == sizeof(int32));
+        return true;
+#elif TARGET_64
+    case TyInt32:
+    case TyUint32:
+        return Math::FitsInDWord(val);
+    case TyInt64:
+    case TyUint64:
+        CompileAssert(sizeof(IntConstType) == sizeof(int64));
+        return true;
+#endif
+    default:
+        Assert(UNREACHED);
+        return false;
+    }
+}
+
+bool IntConstMath::Add(IntConstType left, IntConstType right, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Add(left, right, result);
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Sub(IntConstType left, IntConstType right, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Sub(left, right, result);
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Mul(IntConstType left, IntConstType right, IRType type, IntConstType * result)
+{
+#if TARGET_32
+    bool overflowed = Int32Math::Mul(left, right, result);
+    CompileAssert(sizeof(IntConstType) == sizeof(int32));
+#elif TARGET_64
+    bool overflowed = Int64Math::Mul(left, right, result);
+    CompileAssert(sizeof(IntConstType) == sizeof(int64));
+#endif
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Div(IntConstType left, IntConstType right, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Div(left, right, result);
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Mod(IntConstType left, IntConstType right, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Mod(left, right, result);
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Dec(IntConstType val, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Dec(val, result);
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Inc(IntConstType val, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Inc(val, result);
+    return overflowed || !IsValid(*result, type);
+}
+
+bool IntConstMath::Neg(IntConstType val, IRType type, IntConstType * result)
+{
+    bool overflowed = IntMathCommon<IntConstType>::Neg(val, result);
+    return overflowed || !IsValid(*result, type);
+}

lib/Backend/IntConstMath.h

@@ -0,0 +1,23 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+#pragma once
+
+class IntConstMath
+{
+public:
+    static bool Add(IntConstType left, IntConstType right, IRType type, IntConstType * result);
+    static bool Sub(IntConstType left, IntConstType right, IRType type, IntConstType * result);
+    static bool Mul(IntConstType left, IntConstType right, IRType type, IntConstType * result);
+    static bool Div(IntConstType left, IntConstType right, IRType type, IntConstType * result);
+    static bool Mod(IntConstType left, IntConstType right, IRType type, IntConstType * result);
+
+    static bool Dec(IntConstType val, IRType type, IntConstType * result);
+    static bool Inc(IntConstType val, IRType type, IntConstType * result);
+    static bool Neg(IntConstType val, IRType type, IntConstType * result);
+
+private:
+    static bool IsValid(IntConstType val, IRType type);
+};

lib/Backend/Lower.cpp

@@ -12751,7 +12751,7 @@ void Lowerer::LowerBoundCheck(IR::Instr *const instr)
     {
         // Try to aggregate right + offset into a constant offset
         IntConstType newOffset;
-        if(!IntConstMath::Add(offset, rightOpnd->AsIntConstOpnd()->GetValue(), &newOffset))
+        if(!IntConstMath::Add(offset, rightOpnd->AsIntConstOpnd()->GetValue(), TyInt32, &newOffset))
         {
             offset = newOffset;
             rightOpnd = nullptr;

lib/Common/BackendApi.h

@@ -37,7 +37,6 @@ struct InlinedFrameLayout;
 
 typedef intptr_t IntConstType;
 typedef uintptr_t  UIntConstType;
-typedef IntMath<IntConstType>::Type IntConstMath;
 typedef double  FloatConstType;
 
 #include "EmitBuffer.h"