Skip to content

Commit

Permalink
[CVE-2018-0776] JIT: stack-to-heap copy bug - Google, Inc.
Browse files Browse the repository at this point in the history 
This change fixes a type-confusion bug that can occur with Native arrays allocated on the stack. Once JIT'd code expects a Native array to be used on the stack, the POC converts it to a Var array. This is combined with current behavior of the Arguments property, which moves the array from the stack to the heap. The result of these two assumptions is natively setting a Float value where a Var value is expected, letting any arbitrary floating-point number be written to memory and subsequently accessed as a Var.

This fix forces a deep copy of Arrays that are returned via Arguments. This ensures that the new object created points to its own buffers. This also indicates a divergence with the original object and the one created by Arguments; however, there is currently no standard to define this behavior.
  • Loading branch information
Thomas Moore (CHAKRA) committed Jan 5, 2018
1 parent 985a82f commit 40e45fc
Show file tree
Hide file tree
Showing 12 changed files with 112 additions and 60 deletions.
8 changes: 4 additions & 4 deletions lib/Backend/BailOut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,7 +1006,7 @@ BailOutRecord::RestoreValue(IR::BailOutKind bailOutKind, Js::JavascriptCallStack
if (boxStackInstance)
{
Js::Var oldValue = value;
value = Js::JavascriptOperators::BoxStackInstance(oldValue, scriptContext, /* allowStackFunction */ true);
value = Js::JavascriptOperators::BoxStackInstance(oldValue, scriptContext, /* allowStackFunction */ true, /* deepCopy */ false);

if (oldValue != value)
{
Expand Down Expand Up @@ -1275,7 +1275,7 @@ BailOutRecord::BailOutInlinedHelper(Js::JavascriptCallStackLayout * layout, Bail
if (inlineeFrameRecord)
{
InlinedFrameLayout* outerMostFrame = (InlinedFrameLayout *)(((uint8 *)Js::JavascriptCallStackLayout::ToFramePointer(layout)) - entryPointInfo->frameHeight);
inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout);
inlineeFrameRecord->RestoreFrames(functionBody, outerMostFrame, layout, false /* deepCopy */);
}
}

Expand Down Expand Up @@ -1480,7 +1480,7 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
{
const Js::Var arg = args.Values[i];
BAILOUT_VERBOSE_TRACE(executeFunction, bailOutKind, _u("BailOut: Argument #%3u: value: 0x%p"), i, arg);
const Js::Var boxedArg = Js::JavascriptOperators::BoxStackInstance(arg, functionScriptContext, true);
const Js::Var boxedArg = Js::JavascriptOperators::BoxStackInstance(arg, functionScriptContext, /* allowStackFunction */ true, /* deepCopy */ false);
if(boxedArg != arg)
{
args.Values[i] = boxedArg;
Expand Down Expand Up @@ -1775,7 +1775,7 @@ BailOutRecord::BailOutHelper(Js::JavascriptCallStackLayout * layout, Js::ScriptF
aReturn = Js::JavascriptFunction::FinishConstructor(aReturn, args.Values[0], function);

Js::Var oldValue = aReturn;
aReturn = Js::JavascriptOperators::BoxStackInstance(oldValue, functionScriptContext, /* allowStackFunction */ true);
aReturn = Js::JavascriptOperators::BoxStackInstance(oldValue, functionScriptContext, /* allowStackFunction */ true, /* deepCopy */ false);
#if ENABLE_DEBUG_CONFIG_OPTIONS
if (oldValue != aReturn)
{
Expand Down
17 changes: 10 additions & 7 deletions lib/Backend/InlineeFrameInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,14 @@ void InlineeFrameRecord::Finalize(Func* inlinee, uint32 currentOffset)
Assert(this->inlineDepth != 0);
}

void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *inlinedFrame, Js::JavascriptCallStackLayout * layout) const
void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *inlinedFrame, Js::JavascriptCallStackLayout * layout, bool deepCopy) const
{
Assert(this->inlineDepth != 0);
Assert(inlineeStartOffset != 0);

BAILOUT_VERBOSE_TRACE(functionBody, _u("Restore function object: "));
Js::Var varFunction = this->Restore(this->functionOffset, /*isFloat64*/ false, /*isInt32*/ false, layout, functionBody);
// No deepCopy needed for just the function
Js::Var varFunction = this->Restore(this->functionOffset, /*isFloat64*/ false, /*isInt32*/ false, layout, functionBody, /*deepCopy*/ false);
Assert(Js::ScriptFunction::Is(varFunction));

Js::ScriptFunction* function = Js::ScriptFunction::FromVar(varFunction);
Expand All @@ -219,7 +220,9 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
bool isInt32 = losslessInt32Args.Test(i) != 0;
BAILOUT_VERBOSE_TRACE(functionBody, _u("Restore argument %d: "), i);

Js::Var var = this->Restore(this->argOffsets[i], isFloat64, isInt32, layout, functionBody);
// Forward deepCopy flag for the arguments in case their data must be guaranteed
// to have its own lifetime
Js::Var var = this->Restore(this->argOffsets[i], isFloat64, isInt32, layout, functionBody, deepCopy);
#if DBG
if (!Js::TaggedNumber::Is(var))
{
Expand All @@ -233,7 +236,7 @@ void InlineeFrameRecord::Restore(Js::FunctionBody* functionBody, InlinedFrameLay
BAILOUT_FLUSH(functionBody);
}

void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostFrame, Js::JavascriptCallStackLayout* callstack)
void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostFrame, Js::JavascriptCallStackLayout* callstack, bool deepCopy)
{
InlineeFrameRecord* innerMostRecord = this;
class AutoReverse
Expand Down Expand Up @@ -271,7 +274,7 @@ void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFr

while (currentRecord)
{
currentRecord->Restore(functionBody, currentFrame, callstack);
currentRecord->Restore(functionBody, currentFrame, callstack, deepCopy);
currentRecord = currentRecord->parent;
currentFrame = currentFrame->Next();
}
Expand All @@ -280,7 +283,7 @@ void InlineeFrameRecord::RestoreFrames(Js::FunctionBody* functionBody, InlinedFr
currentFrame->callInfo.Count = 0;
}

Js::Var InlineeFrameRecord::Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody) const
Js::Var InlineeFrameRecord::Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody, bool deepCopy) const
{
Js::Var value;
bool boxStackInstance = true;
Expand Down Expand Up @@ -322,7 +325,7 @@ Js::Var InlineeFrameRecord::Restore(int offset, bool isFloat64, bool isInt32, Js
if (boxStackInstance)
{
Js::Var oldValue = value;
value = Js::JavascriptOperators::BoxStackInstance(oldValue, functionBody->GetScriptContext(), /* allowStackFunction */ true);
value = Js::JavascriptOperators::BoxStackInstance(oldValue, functionBody->GetScriptContext(), /* allowStackFunction */ true, deepCopy);

#if ENABLE_DEBUG_CONFIG_OPTIONS
if (oldValue != value)
Expand Down
6 changes: 3 additions & 3 deletions lib/Backend/InlineeFrameInfo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ struct InlineeFrameRecord
}

void PopulateParent(Func* func);
void RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostInlinee, Js::JavascriptCallStackLayout* callstack);
void RestoreFrames(Js::FunctionBody* functionBody, InlinedFrameLayout* outerMostInlinee, Js::JavascriptCallStackLayout* callstack, bool deepCopy);
void Finalize(Func* inlinee, uint currentOffset);
#if DBG_DUMP
void Dump() const;
Expand All @@ -123,8 +123,8 @@ struct InlineeFrameRecord
}

private:
void Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *outerMostFrame, Js::JavascriptCallStackLayout * layout) const;
Js::Var Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody) const;
void Restore(Js::FunctionBody* functionBody, InlinedFrameLayout *outerMostFrame, Js::JavascriptCallStackLayout * layout, bool deepCopy) const;
Js::Var Restore(int offset, bool isFloat64, bool isInt32, Js::JavascriptCallStackLayout * layout, Js::FunctionBody* functionBody, bool deepCopy) const;
InlineeFrameRecord* Reverse();
};

Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Debug/DiagObjectModel.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ namespace Js
{
activeScopeObject->SetPropertyWithAttributes(
resolveObject.propId,
JavascriptOperators::BoxStackInstance(resolveObject.obj, scriptContext), //The value escapes, box if necessary.
JavascriptOperators::BoxStackInstance(resolveObject.obj, scriptContext, /* allowStackFunction */ false, /* deepCopy */ false), //The value escapes, box if necessary.
resolveObject.isConst ? PropertyConstDefaults : PropertyDynamicTypeDefaults,
nullptr);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Debug/DiagStackFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ namespace Js
{
activeScopeObject->SetPropertyWithAttributes(
Js::PropertyIds::_this,
JavascriptOperators::BoxStackInstance(varThis, scriptContext), //The value escapes, box if necessary.
JavascriptOperators::BoxStackInstance(varThis, scriptContext, /* allowStackFunction */ false, /* deepCopy */ false), //The value escapes, box if necessary.
PropertyConstDefaults,
nullptr);
#if DBG
Expand Down
8 changes: 4 additions & 4 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9790,7 +9790,7 @@ namespace Js
}

Js::Var
JavascriptOperators::BoxStackInstance(Js::Var instance, ScriptContext * scriptContext, bool allowStackFunction)
JavascriptOperators::BoxStackInstance(Js::Var instance, ScriptContext * scriptContext, bool allowStackFunction, bool deepCopy)
{
if (!ThreadContext::IsOnStack(instance) || (allowStackFunction && !TaggedNumber::Is(instance) && (*(int*)instance & 1)))
{
Expand All @@ -9812,11 +9812,11 @@ namespace Js
case Js::TypeIds_Object:
return DynamicObject::BoxStackInstance(DynamicObject::FromVar(instance));
case Js::TypeIds_Array:
return JavascriptArray::BoxStackInstance(JavascriptArray::FromVar(instance));
return JavascriptArray::BoxStackInstance(JavascriptArray::FromVar(instance), deepCopy);
case Js::TypeIds_NativeIntArray:
return JavascriptNativeIntArray::BoxStackInstance(JavascriptNativeIntArray::FromVar(instance));
return JavascriptNativeIntArray::BoxStackInstance(JavascriptNativeIntArray::FromVar(instance), deepCopy);
case Js::TypeIds_NativeFloatArray:
return JavascriptNativeFloatArray::BoxStackInstance(JavascriptNativeFloatArray::FromVar(instance));
return JavascriptNativeFloatArray::BoxStackInstance(JavascriptNativeFloatArray::FromVar(instance), deepCopy);
case Js::TypeIds_Function:
Assert(allowStackFunction);
// Stack functions are deal with not mar mark them, but by nested function escape analysis
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Language/JavascriptOperators.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ namespace Js
#endif
static RecyclableObject *GetCallableObjectOrThrow(const Var callee, ScriptContext *const scriptContext);

static Js::Var BoxStackInstance(Js::Var value, ScriptContext * scriptContext, bool allowStackFunction = false);
static Js::Var BoxStackInstance(Js::Var value, ScriptContext * scriptContext, bool allowStackFunction, bool deepCopy);
static BOOL PropertyReferenceWalkUnscopable(Var instance, RecyclableObject** propertyObject, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext);
static BOOL PropertyReferenceWalk(Var instance, RecyclableObject** propertyObject, PropertyId propertyId, Var* value, PropertyValueInfo* info, ScriptContext* requestContext);

Expand Down
32 changes: 23 additions & 9 deletions lib/Runtime/Language/JavascriptStackWalker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ namespace Js
// are inlined frames on the stack the InlineeCallInfo of the first inlined frame
// has the native offset of the current physical frame.
Assert(!*inlinee);
InlinedFrameWalker::FromPhysicalFrame(tmpFrameWalker, currentFrame, ScriptFunction::FromVar(parentFunction), PreviousInterpreterFrameIsFromBailout(), loopNum, this, useInternalFrameInfo);
InlinedFrameWalker::FromPhysicalFrame(tmpFrameWalker, currentFrame, ScriptFunction::FromVar(parentFunction), PreviousInterpreterFrameIsFromBailout(), loopNum, this, useInternalFrameInfo, false /*noAlloc*/, false /*deepCopy*/);
inlineeOffset = tmpFrameWalker.GetBottomMostInlineeOffset();
tmpFrameWalker.Close();
}
Expand Down Expand Up @@ -555,7 +555,8 @@ namespace Js
}

bool hasInlinedFramesOnStack = InlinedFrameWalker::FromPhysicalFrame(inlinedFrameWalker, currentFrame,
ScriptFunction::FromVar(function), true /*fromBailout*/, loopNum, this, false /*useInternalFrameInfo*/);
ScriptFunction::FromVar(function), true /*fromBailout*/, loopNum, this, false /*useInternalFrameInfo*/, false /*noAlloc*/, this->deepCopyForArgs);

if (hasInlinedFramesOnStack)
{
// We're now back in the state where currentFrame == physical frame of the inliner, but
Expand Down Expand Up @@ -602,7 +603,18 @@ namespace Js
// Check whether there are inlined frames nested in this native frame. The corresponding check for
// a jitted loop body frame should have been done in CheckJavascriptFrame
Assert(lastInternalFrameInfo.codeAddress == nullptr);
if (InlinedFrameWalker::FromPhysicalFrame(inlinedFrameWalker, currentFrame, ScriptFunction::FromVar(function)))
bool inlinedFramesFound = InlinedFrameWalker::FromPhysicalFrame(
inlinedFrameWalker,
currentFrame,
ScriptFunction::FromVar(function),
false, // fromBailout
-1, // loopNum
nullptr,// walker
false, // useInternalFrameInfo
false, // noAlloc
this->deepCopyForArgs
);
if (inlinedFramesFound)
{
this->inlinedFramesBeingWalked = inlinedFrameWalker.Next(inlinedFrameCallInfo);
this->hasInlinedFramesOnStack = true;
Expand All @@ -621,7 +633,8 @@ namespace Js
_NOINLINE
JavascriptStackWalker::JavascriptStackWalker(ScriptContext * scriptContext, bool useEERContext, PVOID returnAddress, bool _forceFullWalk /*=false*/) :
inlinedFrameCallInfo(CallFlags_None, 0), shouldDetectPartiallyInitializedInterpreterFrame(true), forceFullWalk(_forceFullWalk),
previousInterpreterFrameIsFromBailout(false), previousInterpreterFrameIsForLoopBody(false), hasInlinedFramesOnStack(false)
previousInterpreterFrameIsFromBailout(false), previousInterpreterFrameIsForLoopBody(false), hasInlinedFramesOnStack(false),
deepCopyForArgs(false)
{
if (scriptContext == NULL)
{
Expand Down Expand Up @@ -917,7 +930,7 @@ namespace Js
Assert((this->interpreterFrame->GetFlags() & Js::InterpreterStackFrameFlags_FromBailOut) != 0);
InlinedFrameWalker tmpFrameWalker;
Assert(InlinedFrameWalker::FromPhysicalFrame(tmpFrameWalker, currentFrame, Js::ScriptFunction::FromVar(argv[JavascriptFunctionArgIndex_Function]),
true /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, true /*noAlloc*/));
true /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, true /*noAlloc*/, false /*deepCopy*/));
tmpFrameWalker.Close();
}
#endif //DBG
Expand Down Expand Up @@ -964,9 +977,10 @@ namespace Js
{
if (includeInlineFrames &&
InlinedFrameWalker::FromPhysicalFrame(inlinedFrameWalker, currentFrame, Js::ScriptFunction::FromVar(argv[JavascriptFunctionArgIndex_Function]),
false /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/))
false /*fromBailout*/, this->tempInterpreterFrame->GetCurrentLoopNum(), this, false /*useInternalFrameInfo*/, false /*noAlloc*/, this->deepCopyForArgs))
{
// Found inlined frames in a jitted loop body. We dont want to skip the inlined frames; walk all of them before setting codeAddress on lastInternalFrameInfo.
// DeepCopy here because, if there is an inlinee in a loop body, FromPhysicalFrame won't be called from UpdateFrame
this->inlinedFramesBeingWalked = inlinedFrameWalker.Next(inlinedFrameCallInfo);
this->hasInlinedFramesOnStack = true;
Assert(inlinedFramesBeingWalked);
Expand Down Expand Up @@ -1208,7 +1222,7 @@ namespace Js

#if ENABLE_NATIVE_CODEGEN
bool InlinedFrameWalker::FromPhysicalFrame(InlinedFrameWalker& self, StackFrame& physicalFrame, Js::ScriptFunction *parent, bool fromBailout,
int loopNum, const JavascriptStackWalker * const stackWalker, bool useInternalFrameInfo, bool noAlloc)
int loopNum, const JavascriptStackWalker * const stackWalker, bool useInternalFrameInfo, bool noAlloc, bool deepCopy)
{
bool inlinedFramesFound = false;
FunctionBody* parentFunctionBody = parent->GetFunctionBody();
Expand Down Expand Up @@ -1261,7 +1275,7 @@ namespace Js

if (record)
{
record->RestoreFrames(parent->GetFunctionBody(), outerMostFrame, JavascriptCallStackLayout::FromFramePointer(framePointer));
record->RestoreFrames(parent->GetFunctionBody(), outerMostFrame, JavascriptCallStackLayout::FromFramePointer(framePointer), deepCopy);
}
}

Expand Down Expand Up @@ -1347,7 +1361,7 @@ namespace Js

for (size_t i = 0; i < argCount; i++)
{
args[i] = Js::JavascriptOperators::BoxStackInstance(args[i], scriptContext);
args[i] = Js::JavascriptOperators::BoxStackInstance(args[i], scriptContext, false /*allowStackFunction*/, false /*deepCopy*/);
}
}

Expand Down
7 changes: 5 additions & 2 deletions lib/Runtime/Language/JavascriptStackWalker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ namespace Js
Assert(currentIndex == -1);
}

static bool FromPhysicalFrame(InlinedFrameWalker& self, StackFrame& physicalFrame, Js::ScriptFunction *parent, bool fromBailout = false,
int loopNum = -1, const JavascriptStackWalker * const walker = nullptr, bool useInternalFrameInfo = false, bool noAlloc = false);
static bool FromPhysicalFrame(InlinedFrameWalker& self, StackFrame& physicalFrame, Js::ScriptFunction *parent, bool fromBailout,
int loopNum, const JavascriptStackWalker * const walker, bool useInternalFrameInfo, bool noAlloc, bool deepCopy);
void Close();
bool Next(CallInfo& callInfo);
size_t GetArgc() const;
Expand Down Expand Up @@ -304,6 +304,8 @@ namespace Js
{
return previousInterpreterFrameIsFromBailout;
}

void SetDeepCopyForArguments() { deepCopyForArgs = true; }
#if DBG
static bool ValidateTopJitFrame(Js::ScriptContext* scriptContext);
#endif
Expand All @@ -328,6 +330,7 @@ namespace Js
bool previousInterpreterFrameIsFromBailout : 1;
bool previousInterpreterFrameIsForLoopBody : 1;
bool forceFullWalk : 1; // ignoring hasCaller
bool deepCopyForArgs : 1; // indicates when Var's data should be deep-copied when gathering Arguments for the frame

Var GetThisFromFrame() const; // returns 'this' object from the physical frame
Var GetCurrentArgumentsObject() const; // returns arguments object from the current frame, which may be virtual (belonging to an inlinee)
Expand Down
Loading

0 comments on commit 40e45fc

Please sign in to comment.