Skip to content

Commit

Permalink
[CVE-2017-11861] [ChakraCore] Chakra JIT - Incorrect integer overflow…
Browse files Browse the repository at this point in the history
… 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.
  • Loading branch information
MikeHolman authored and leirocks committed Nov 14, 2017
1 parent c1bdfff commit 85d42e7
Show file tree
Hide file tree
Showing 13 changed files with 144 additions and 34 deletions.
1 change: 1 addition & 0 deletions lib/Backend/Backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions lib/Backend/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ add_library (Chakra.Backend OBJECT
InliningDecider.cpp
InliningHeuristics.cpp
IntBounds.cpp
IntConstMath.cpp
InterpreterThunkEmitter.cpp
JITThunkEmitter.cpp
JITOutput.cpp
Expand Down
2 changes: 2 additions & 0 deletions lib/Backend/Chakra.Backend.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down Expand Up @@ -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" />
Expand Down
2 changes: 2 additions & 0 deletions lib/Backend/Chakra.Backend.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down Expand Up @@ -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">
Expand Down
2 changes: 1 addition & 1 deletion lib/Backend/GlobOpt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 17 additions & 22 deletions lib/Backend/IR.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -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;

Expand All @@ -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:
Expand All @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/Backend/IR.h
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,10 @@ class Instr
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
Expand Down
4 changes: 2 additions & 2 deletions lib/Backend/Inline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
13 changes: 8 additions & 5 deletions lib/Backend/IntBounds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -768,22 +771,22 @@ 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);
if(src1Opnd)
{
// 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();
Expand Down
84 changes: 84 additions & 0 deletions lib/Backend/IntConstMath.cpp
Original file line number Diff line number Diff line change
@@ -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);
}
23 changes: 23 additions & 0 deletions lib/Backend/IntConstMath.h
Original file line number Diff line number Diff line change
@@ -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);
};
2 changes: 1 addition & 1 deletion lib/Backend/Lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 85d42e7

Please sign in to comment.