Skip to content

Commit 24c4d7d

Browse files
digitalinfinitydigitalinfinity
committed
This change contains combined fixes for CVE-2016-3350, CVE-2016-3377 and a defense in depth change in the CustomHeap
 Arguments symbol is uninitialized when a function definition with the name arguments occur in the body in non-split scope When a function definition with the name arguments occurs in the body it makrs the function as arguments creation is not needed. The arguments is initialized only at the beginning of the body. So when arguments is used in the param scope it will be unitialized. Also if arguments symbol is captured in the param scope we should split the scope as it can be overwritten in the body. CustomHeap - FreeAllocation - Bug fix Premise - The allocations under interest are the jit page allocations made by the CustomHeap. - When all bits in page's free bit vector are set, FreeAllocation API in CustomHeap behaves incorrectly - It will set a page's protection to RWX and returns. Fix - Refactored FreeAllocation API in CustomHeap - Merged two separate if conditions to a single if condition. - Added entry condition checks to fail fast. - Removed virtual keyword in a function and cached freebitVector count - Adding more release time checks - Added TestAnyInRange API [MSRC34310]Array.prototype.map() type confusion Type confusion when DirectSetItemAt() accesses a native int array return by a user-defined [@@species] constructor. Fix by replacing with a virtual SetItem() call.
1 parent 72dd87a commit 24c4d7d

10 files changed

Lines changed: 443 additions & 49 deletions

File tree

‎lib/Common/DataStructures/UnitBitVector.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,11 @@ class BVUnitT
287287
T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;
288288
return (this->word & mask) == mask;
289289
}
290+
BOOLEAN TestAnyInRange(const BVIndex index, uint length) const
291+
{
292+
T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;
293+
return (this->word & mask) != 0;
294+
}
290295
void SetRange(const BVIndex index, uint length)
291296
{
292297
T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;

‎lib/Common/Memory/CustomHeap.cpp‎

Lines changed: 53 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,7 @@ void Heap::FreeLargeObjects()
485485
#endif
486486
this->codePageAllocators->Release(allocation.address, allocation.GetPageCount(), allocation.largeObjectAllocation.segment);
487487

488-
largeObjectIter.RemoveCurrent(this->auxiliaryAllocator);
488+
largeObjectIter.RemoveCurrent(this->auxiliaryAllocator);
489489
}
490490
NEXT_DLISTBASE_ENTRY_EDITING;
491491
}
@@ -519,7 +519,13 @@ bool Heap::AllocInPage(Page* page, size_t bytes, ushort pdataCount, ushort xdata
519519

520520
uint length = GetChunkSizeForBytes(bytes);
521521
BVIndex index = GetFreeIndexForPage(page, bytes);
522-
Assert(index != BVInvalidIndex);
522+
523+
if (index == BVInvalidIndex)
524+
{
525+
CustomHeap_BadPageState_fatal_error((ULONG_PTR)this);
526+
return false;
527+
}
528+
523529
char* address = page->address + Page::Alignment * index;
524530

525531
#if PDATA_ENABLED
@@ -562,6 +568,13 @@ bool Heap::AllocInPage(Page* page, size_t bytes, ushort pdataCount, ushort xdata
562568
this->freeObjectSize -= bytes;
563569
#endif
564570

571+
//Section of the Page should already be freed.
572+
if (!page->freeBitVector.TestRange(index, length))
573+
{
574+
CustomHeap_BadPageState_fatal_error((ULONG_PTR)this);
575+
return false;
576+
}
577+
565578
page->freeBitVector.ClearRange(index, length);
566579
VerboseHeapTrace(_u("ChunkSize: %d, Index: %d, Free bit vector in page: "), length, index);
567580

@@ -729,18 +742,19 @@ bool Heap::FreeAllocation(Allocation* object)
729742
unsigned int length = GetChunkSizeForBytes(object->size);
730743
BVIndex index = GetIndexInPage(page, object->address);
731744

732-
#if DBG
733-
// Make sure that it's not already been freed
734-
for (BVIndex i = index; i < length; i++)
745+
uint freeBitsCount = page->freeBitVector.Count();
746+
747+
// Make sure that the section under interest or the whole page has not already been freed
748+
if (page->IsEmpty() || page->freeBitVector.TestAnyInRange(index, length))
735749
{
736-
Assert(!page->freeBitVector.Test(i));
750+
CustomHeap_BadPageState_fatal_error((ULONG_PTR)this);
751+
return false;
737752
}
738-
#endif
739753

740754
if (page->inFullList)
741755
{
742756
VerboseHeapTrace(_u("Recycling page 0x%p because address 0x%p of size %d was freed\n"), page->address, object->address, object->size);
743-
757+
744758
// If the object being freed is equal to the page size, we're
745759
// going to remove it anyway so don't add it to a bucket
746760
if (object->size != pageSize)
@@ -777,14 +791,43 @@ bool Heap::FreeAllocation(Allocation* object)
777791
// If the page is about to become empty then we should not need
778792
// to set it to executable and we don't expect to restore the
779793
// previous protection settings.
780-
if (page->freeBitVector.Count() == BVUnit::BitsPerWord - length)
794+
if (freeBitsCount == BVUnit::BitsPerWord - length)
781795
{
782796
EnsureAllocationWriteable(object);
797+
FreeAllocationHelper(object, index, length);
798+
Assert(page->IsEmpty());
799+
800+
this->buckets[page->currentBucket].RemoveElement(this->auxiliaryAllocator, page);
801+
return false;
783802
}
784803
else
785804
{
786805
EnsureAllocationExecuteWriteable(object);
806+
807+
FreeAllocationHelper(object, index, length);
808+
809+
// after freeing part of the page, the page should be in PAGE_EXECUTE_READWRITE protection, and turning to PAGE_EXECUTE (always with TARGETS_NO_UPDATE state)
810+
811+
DWORD protectFlags = 0;
812+
813+
if (AutoSystemInfo::Data.IsCFGEnabled())
814+
{
815+
protectFlags = PAGE_EXECUTE_RO_TARGETS_NO_UPDATE;
816+
}
817+
else
818+
{
819+
protectFlags = PAGE_EXECUTE;
820+
}
821+
822+
this->codePageAllocators->ProtectPages(page->address, 1, segment, protectFlags, PAGE_EXECUTE_READWRITE);
823+
824+
return true;
787825
}
826+
}
827+
828+
void Heap::FreeAllocationHelper(Allocation* object, BVIndex index, uint length)
829+
{
830+
Page* page = object->page;
788831

789832
// Fill the old buffer with debug breaks
790833
CustomHeap::FillDebugBreak((BYTE *)object->address, object->size);
@@ -796,6 +839,7 @@ bool Heap::FreeAllocation(Allocation* object)
796839
VerboseHeapTrace(_u("\n"));
797840

798841
page->freeBitVector.SetRange(index, length);
842+
799843
VerboseHeapTrace(_u("Free bit vector in page: "), length, index);
800844
#if VERBOSE_HEAP
801845
page->freeBitVector.DumpWord();
@@ -808,29 +852,6 @@ bool Heap::FreeAllocation(Allocation* object)
808852
#endif
809853

810854
this->auxiliaryAllocator->Free(object, sizeof(Allocation));
811-
812-
if (page->IsEmpty())
813-
{
814-
this->buckets[page->currentBucket].RemoveElement(this->auxiliaryAllocator, page);
815-
return false;
816-
}
817-
else // after freeing part of the page, the page should be in PAGE_EXECUTE_READWRITE protection, and turning to PAGE_EXECUTE (always with TARGETS_NO_UPDATE state)
818-
{
819-
DWORD protectFlags = 0;
820-
821-
if (AutoSystemInfo::Data.IsCFGEnabled())
822-
{
823-
protectFlags = PAGE_EXECUTE_RO_TARGETS_NO_UPDATE;
824-
}
825-
else
826-
{
827-
protectFlags = PAGE_EXECUTE;
828-
}
829-
830-
this->codePageAllocators->ProtectPages(page->address, 1, segment, protectFlags, PAGE_EXECUTE_READWRITE);
831-
832-
return true;
833-
}
834855
}
835856

836857
void Heap::FreeDecommittedBuckets()

‎lib/Common/Memory/CustomHeap.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,7 @@ class Heap
504504
void FreeBucket(DListBase<Page>* bucket, bool freeOnlyEmptyPages);
505505
void FreePage(Page* page);
506506
bool FreeAllocation(Allocation* allocation);
507+
void FreeAllocationHelper(Allocation * allocation, BVIndex index, uint length);
507508

508509
#if PDATA_ENABLED
509510
void FreeXdata(XDataAllocation* xdata, void* segment);

‎lib/Common/Memory/PageAllocator.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -518,8 +518,8 @@ class PageAllocatorBase
518518
#if DBG_DUMP
519519
virtual void DumpStats() const;
520520
#endif
521-
virtual PageSegmentBase<TVirtualAlloc> * AddPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList);
522-
static PageSegmentBase<TVirtualAlloc> * AllocPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList,
521+
PageSegmentBase<TVirtualAlloc> * AddPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList);
522+
static PageSegmentBase<TVirtualAlloc> * AllocPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList,
523523
PageAllocatorBase<TVirtualAlloc> * pageAllocator, bool committed, bool allocated);
524524

525525
// Zero Pages

‎lib/Parser/Parse.cpp‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -816,6 +816,7 @@ Symbol* Parser::AddDeclForPid(ParseNodePtr pnode, IdentPtr pid, SymbolType symbo
816816
&& blockInfo->pnodeBlock->sxBlock.blockType == PnodeBlockType::Function
817817
&& blockInfo->pBlockInfoOuter != nullptr
818818
&& blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter
819+
&& blockInfo->pnodeBlock->sxBlock.scope->GetScopeType() != ScopeType_FuncExpr
819820
&& blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope())
820821
{
821822
blockInfo = blockInfo->pBlockInfoOuter;
@@ -5029,6 +5030,13 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare
50295030
}
50305031
return false;
50315032
});
5033+
5034+
if (wellKnownPropertyPids.arguments->GetTopRef() && wellKnownPropertyPids.arguments->GetTopRef()->GetScopeId() > pnodeFnc->sxFnc.pnodeScopes->sxBlock.blockId)
5035+
{
5036+
Assert(pnodeFnc->sxFnc.UsesArguments());
5037+
// Arguments symbol is captured in the param scope
5038+
paramScope->SetCannotMergeWithBodyScope();
5039+
}
50325040
}
50335041
}
50345042
}
@@ -5895,13 +5903,6 @@ bool Parser::ParseFncNames(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncParent, u
58955903
pnodeFnc->sxFnc.pid = m_phtbl->PidHashNameLen(
58965904
m_pscan->PchBase() + ichMinNames, ichLimNames - ichMinNames);
58975905
}
5898-
5899-
if(pnodeFnc->sxFnc.pid == wellKnownPropertyPids.arguments && fDeclaration && pnodeFncParent)
5900-
{
5901-
// This function declaration (or function expression in compat modes) overrides the built-in arguments object of the
5902-
// parent function
5903-
pnodeFncParent->grfpn |= PNodeFlags::fpnArguments_overriddenByDecl;
5904-
}
59055906
}
59065907

59075908
return true;
@@ -6670,10 +6671,11 @@ void Parser::AddArgumentsNodeToVars(ParseNodePtr pnodeFnc)
66706671
}
66716672
else
66726673
{
6674+
ParseNodePtr argNode = nullptr;
66736675
if(m_ppnodeVar == &pnodeFnc->sxFnc.pnodeVars)
66746676
{
66756677
// There were no var declarations in the function
6676-
CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc)->grfpn |= PNodeFlags::fpnArguments;
6678+
argNode = CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc);
66776679
}
66786680
else
66796681
{
@@ -6684,10 +6686,18 @@ void Parser::AddArgumentsNodeToVars(ParseNodePtr pnodeFnc)
66846686
// object until it is replaced with something else.
66856687
ParseNodePtr *const ppnodeVarSave = m_ppnodeVar;
66866688
m_ppnodeVar = &pnodeFnc->sxFnc.pnodeVars;
6687-
CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc)->grfpn |= PNodeFlags::fpnArguments;
6689+
argNode = CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc);
66886690
m_ppnodeVar = ppnodeVarSave;
66896691
}
66906692

6693+
Assert(argNode);
6694+
argNode->grfpn |= PNodeFlags::fpnArguments;
6695+
6696+
// When a function definition with the name arguments occurs in the body the declaration of the arguments symbol will
6697+
// be set to that function declaration. We should change it to arguments declaration from the param scope as it may be
6698+
// used in the param scope and we have to load the arguments.
6699+
argNode->sxVar.sym->SetDecl(argNode);
6700+
66916701
pnodeFnc->sxFnc.SetHasReferenceableBuiltInArguments(true);
66926702
}
66936703
}

‎lib/Runtime/ByteCode/ByteCodeEmitter.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3693,7 +3693,9 @@ void EnsureFncDeclScopeSlot(ParseNode *pnodeFnc, FuncInfo *funcInfo)
36933693
{
36943694
Assert(pnodeFnc->sxFnc.pnodeName->nop == knopVarDecl);
36953695
Symbol *sym = pnodeFnc->sxFnc.pnodeName->sxVar.sym;
3696-
if (sym)
3696+
// If this function is shadowing the arguments symbol in body then skip it.
3697+
// We will allocate scope slot for the arguments symbol during EmitLocalPropInit.
3698+
if (sym && !sym->GetIsArguments())
36973699
{
36983700
sym->EnsureScopeSlot(funcInfo);
36993701
}

‎lib/Runtime/Library/JavascriptArray.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9019,7 +9019,7 @@ namespace Js
90199019

90209020
if (newArr)
90219021
{
9022-
newArr->DirectSetItemAt(k, mappedValue);
9022+
newArr->SetItem(k, mappedValue, PropertyOperation_None);
90239023
}
90249024
else
90259025
{

‎test/es6/ES6Species-bugs.js‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,41 @@ var tests = [
2727
assert.throws(function() { Array.prototype.splice.call(arr, 0, 3); }, TypeError, "TypeError when constructor[Symbol.species] is not constructor", "Function 'constructor[Symbol.species]' is not a constructor");
2828
}
2929
},
30+
{
31+
name: "Type confusion in Array.prototype.map()",
32+
body: function () {
33+
function test(){
34+
CollectGarbage();
35+
36+
var n = [];
37+
for (var i = 0; i < 0x10; i++)
38+
n.push([0x12345678, 0x12345678, 0x12345678, 0x12345678]);
39+
40+
class fake extends Object {
41+
static get [Symbol.species]() { return function() { return n[5]; }; };
42+
}
43+
44+
var f = function(a){ return a; }
45+
46+
var x = ["fluorescence", 0, 0, 0x41414141];
47+
var y = new Proxy(x, {
48+
get: function(t, p, r) {
49+
return (p == "constructor") ? fake : x[p];
50+
}
51+
});
52+
53+
// oob write
54+
Array.prototype.map.apply(y, [f]);
55+
56+
for (var i = 0; i < 0x10; i++)
57+
n[i][0] = 0x42424242;
58+
59+
}
60+
61+
test();
62+
63+
}
64+
},
3065
];
3166

3267
testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });

0 commit comments

Comments
 (0)