This change contains combined fixes for CVE-2016-3350, CVE-2016-3377 and a defense in depth change in the CustomHeap

digitalinfinity · digitalinfinity · commit 24c4d7df8199 · 2016-09-15T10:38:28.000-07:00
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.
diff --git a/lib/Common/DataStructures/UnitBitVector.h b/lib/Common/DataStructures/UnitBitVector.h
@@ -287,6 +287,11 @@ class BVUnitT
         T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;
         return (this->word & mask) == mask;
     }
+    BOOLEAN TestAnyInRange(const BVIndex index, uint length) const
+    {
+        T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;
+        return (this->word & mask) != 0;
+    }
     void SetRange(const BVIndex index, uint length)
     {
         T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;
diff --git a/lib/Common/Memory/CustomHeap.cpp b/lib/Common/Memory/CustomHeap.cpp
@@ -485,7 +485,7 @@ void Heap::FreeLargeObjects()
 #endif
         this->codePageAllocators->Release(allocation.address, allocation.GetPageCount(), allocation.largeObjectAllocation.segment);
 
-            largeObjectIter.RemoveCurrent(this->auxiliaryAllocator);
+        largeObjectIter.RemoveCurrent(this->auxiliaryAllocator);
     }
     NEXT_DLISTBASE_ENTRY_EDITING;
 }
@@ -519,7 +519,13 @@ bool Heap::AllocInPage(Page* page, size_t bytes, ushort pdataCount, ushort xdata
 
     uint length = GetChunkSizeForBytes(bytes);
     BVIndex index = GetFreeIndexForPage(page, bytes);
-    Assert(index != BVInvalidIndex);
+    
+    if (index == BVInvalidIndex)
+    {
+        CustomHeap_BadPageState_fatal_error((ULONG_PTR)this);
+        return false;
+    }
+
     char* address = page->address + Page::Alignment * index;
 
 #if PDATA_ENABLED
@@ -562,6 +568,13 @@ bool Heap::AllocInPage(Page* page, size_t bytes, ushort pdataCount, ushort xdata
     this->freeObjectSize -= bytes;
 #endif
 
+    //Section of the Page should already be freed.
+    if (!page->freeBitVector.TestRange(index, length))
+    {
+        CustomHeap_BadPageState_fatal_error((ULONG_PTR)this);
+        return false;
+    }
+
     page->freeBitVector.ClearRange(index, length);
     VerboseHeapTrace(_u("ChunkSize: %d, Index: %d, Free bit vector in page: "), length, index);
 
@@ -729,18 +742,19 @@ bool Heap::FreeAllocation(Allocation* object)
     unsigned int length = GetChunkSizeForBytes(object->size);
     BVIndex index = GetIndexInPage(page, object->address);
 
-#if DBG
-    // Make sure that it's not already been freed
-    for (BVIndex i = index; i < length; i++)
+    uint freeBitsCount = page->freeBitVector.Count();
+
+    // Make sure that the section under interest or the whole page has not already been freed
+    if (page->IsEmpty() || page->freeBitVector.TestAnyInRange(index, length))
     {
-        Assert(!page->freeBitVector.Test(i));
+        CustomHeap_BadPageState_fatal_error((ULONG_PTR)this);
+        return false;
     }
-#endif
 
     if (page->inFullList)
     {
         VerboseHeapTrace(_u("Recycling page 0x%p because address 0x%p of size %d was freed\n"), page->address, object->address, object->size);
-
+       
         // If the object being freed is equal to the page size, we're
         // going to remove it anyway so don't add it to a bucket
         if (object->size != pageSize)
@@ -777,14 +791,43 @@ bool Heap::FreeAllocation(Allocation* object)
     // If the page is about to become empty then we should not need
     // to set it to executable and we don't expect to restore the
     // previous protection settings.
-    if (page->freeBitVector.Count() == BVUnit::BitsPerWord - length)
+    if (freeBitsCount == BVUnit::BitsPerWord - length)
     {
         EnsureAllocationWriteable(object);
+        FreeAllocationHelper(object, index, length);
+        Assert(page->IsEmpty());
+
+        this->buckets[page->currentBucket].RemoveElement(this->auxiliaryAllocator, page);
+        return false;
     }
     else
     {
         EnsureAllocationExecuteWriteable(object);
+
+        FreeAllocationHelper(object, index, length);
+
+        // 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)
+
+        DWORD protectFlags = 0;
+
+        if (AutoSystemInfo::Data.IsCFGEnabled())
+        {
+            protectFlags = PAGE_EXECUTE_RO_TARGETS_NO_UPDATE;
+        }
+        else
+        {
+            protectFlags = PAGE_EXECUTE;
+        }
+
+        this->codePageAllocators->ProtectPages(page->address, 1, segment, protectFlags, PAGE_EXECUTE_READWRITE);
+        
+        return true;
     }
+}
+
+void Heap::FreeAllocationHelper(Allocation* object, BVIndex index, uint length)
+{
+    Page* page = object->page;
 
     // Fill the old buffer with debug breaks
     CustomHeap::FillDebugBreak((BYTE *)object->address, object->size);
@@ -796,6 +839,7 @@ bool Heap::FreeAllocation(Allocation* object)
     VerboseHeapTrace(_u("\n"));
 
     page->freeBitVector.SetRange(index, length);
+
     VerboseHeapTrace(_u("Free bit vector in page: "), length, index);
 #if VERBOSE_HEAP
     page->freeBitVector.DumpWord();
@@ -808,29 +852,6 @@ bool Heap::FreeAllocation(Allocation* object)
 #endif
 
     this->auxiliaryAllocator->Free(object, sizeof(Allocation));
-
-    if (page->IsEmpty())
-    {
-        this->buckets[page->currentBucket].RemoveElement(this->auxiliaryAllocator, page);
-        return false;
-    }
-    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)
-    {
-        DWORD protectFlags = 0;
-
-        if (AutoSystemInfo::Data.IsCFGEnabled())
-        {
-            protectFlags = PAGE_EXECUTE_RO_TARGETS_NO_UPDATE;
-        }
-        else
-        {
-            protectFlags = PAGE_EXECUTE;
-        }
-
-        this->codePageAllocators->ProtectPages(page->address, 1, segment, protectFlags, PAGE_EXECUTE_READWRITE);
-        
-        return true;
-    }
 }
 
 void Heap::FreeDecommittedBuckets()
diff --git a/lib/Common/Memory/CustomHeap.h b/lib/Common/Memory/CustomHeap.h
@@ -504,6 +504,7 @@ class Heap
     void FreeBucket(DListBase<Page>* bucket, bool freeOnlyEmptyPages);
     void FreePage(Page* page);
     bool FreeAllocation(Allocation* allocation);
+    void FreeAllocationHelper(Allocation * allocation, BVIndex index, uint length);
 
 #if PDATA_ENABLED
     void FreeXdata(XDataAllocation* xdata, void* segment);
diff --git a/lib/Common/Memory/PageAllocator.h b/lib/Common/Memory/PageAllocator.h
@@ -518,8 +518,8 @@ class PageAllocatorBase
 #if DBG_DUMP
     virtual void DumpStats() const;
 #endif
-    virtual PageSegmentBase<TVirtualAlloc> * AddPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList);
-    static PageSegmentBase<TVirtualAlloc> * AllocPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList, 
+    PageSegmentBase<TVirtualAlloc> * AddPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList);
+    static PageSegmentBase<TVirtualAlloc> * AllocPageSegment(DListBase<PageSegmentBase<TVirtualAlloc>>& segmentList,
     PageAllocatorBase<TVirtualAlloc> * pageAllocator, bool committed, bool allocated);
 
     // Zero Pages
diff --git a/lib/Parser/Parse.cpp b/lib/Parser/Parse.cpp
@@ -816,6 +816,7 @@ Symbol* Parser::AddDeclForPid(ParseNodePtr pnode, IdentPtr pid, SymbolType symbo
         && blockInfo->pnodeBlock->sxBlock.blockType == PnodeBlockType::Function
         && blockInfo->pBlockInfoOuter != nullptr
         && blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter
+        && blockInfo->pnodeBlock->sxBlock.scope->GetScopeType() != ScopeType_FuncExpr
         && blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope())
     {
         blockInfo = blockInfo->pBlockInfoOuter;
@@ -5029,6 +5030,13 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare
                         }
                         return false;
                     });
+
+                    if (wellKnownPropertyPids.arguments->GetTopRef() && wellKnownPropertyPids.arguments->GetTopRef()->GetScopeId() > pnodeFnc->sxFnc.pnodeScopes->sxBlock.blockId)
+                    {
+                        Assert(pnodeFnc->sxFnc.UsesArguments());
+                        // Arguments symbol is captured in the param scope
+                        paramScope->SetCannotMergeWithBodyScope();
+                    }
                 }
             }
         }
@@ -5895,13 +5903,6 @@ bool Parser::ParseFncNames(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncParent, u
             pnodeFnc->sxFnc.pid = m_phtbl->PidHashNameLen(
                 m_pscan->PchBase() + ichMinNames, ichLimNames - ichMinNames);
         }
-
-        if(pnodeFnc->sxFnc.pid == wellKnownPropertyPids.arguments && fDeclaration && pnodeFncParent)
-        {
-            // This function declaration (or function expression in compat modes) overrides the built-in arguments object of the
-            // parent function
-            pnodeFncParent->grfpn |= PNodeFlags::fpnArguments_overriddenByDecl;
-        }
     }
 
     return true;
@@ -6670,10 +6671,11 @@ void Parser::AddArgumentsNodeToVars(ParseNodePtr pnodeFnc)
     }
     else
     {
+        ParseNodePtr argNode = nullptr;
         if(m_ppnodeVar == &pnodeFnc->sxFnc.pnodeVars)
         {
             // There were no var declarations in the function
-            CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc)->grfpn |= PNodeFlags::fpnArguments;
+            argNode = CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc);
         }
         else
         {
@@ -6684,10 +6686,18 @@ void Parser::AddArgumentsNodeToVars(ParseNodePtr pnodeFnc)
             // object until it is replaced with something else.
             ParseNodePtr *const ppnodeVarSave = m_ppnodeVar;
             m_ppnodeVar = &pnodeFnc->sxFnc.pnodeVars;
-            CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc)->grfpn |= PNodeFlags::fpnArguments;
+            argNode = CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc);
             m_ppnodeVar = ppnodeVarSave;
         }
 
+        Assert(argNode);
+        argNode->grfpn |= PNodeFlags::fpnArguments;
+
+        // When a function definition with the name arguments occurs in the body the declaration of the arguments symbol will
+        // be set to that function declaration. We should change it to arguments declaration from the param scope as it may be
+        // used in the param scope and we have to load the arguments.
+        argNode->sxVar.sym->SetDecl(argNode);
+
         pnodeFnc->sxFnc.SetHasReferenceableBuiltInArguments(true);
     }
 }
diff --git a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
@@ -3693,7 +3693,9 @@ void EnsureFncDeclScopeSlot(ParseNode *pnodeFnc, FuncInfo *funcInfo)
     {
         Assert(pnodeFnc->sxFnc.pnodeName->nop == knopVarDecl);
         Symbol *sym = pnodeFnc->sxFnc.pnodeName->sxVar.sym;
-        if (sym)
+        // If this function is shadowing the arguments symbol in body then skip it.
+        // We will allocate scope slot for the arguments symbol during EmitLocalPropInit.
+        if (sym && !sym->GetIsArguments())
         {
             sym->EnsureScopeSlot(funcInfo);
         }
diff --git a/lib/Runtime/Library/JavascriptArray.cpp b/lib/Runtime/Library/JavascriptArray.cpp
@@ -9019,7 +9019,7 @@ namespace Js
 
                     if (newArr)
                     {
-                        newArr->DirectSetItemAt(k, mappedValue);
+                        newArr->SetItem(k, mappedValue, PropertyOperation_None);
                     }
                     else
                     {
diff --git a/test/es6/ES6Species-bugs.js b/test/es6/ES6Species-bugs.js
@@ -27,6 +27,41 @@ var tests = [
             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");
         }
     },
+    {
+        name: "Type confusion in Array.prototype.map()",
+        body: function () {
+            function test(){
+                CollectGarbage();
+
+                var n = [];
+                for (var i = 0; i < 0x10; i++)
+                    n.push([0x12345678, 0x12345678, 0x12345678, 0x12345678]);
+
+                class fake extends Object {
+                    static get [Symbol.species]() { return function() { return n[5]; }; };
+                }
+
+                var f = function(a){ return a; }
+
+                var x = ["fluorescence", 0, 0, 0x41414141];
+                var y = new Proxy(x, {
+                    get: function(t, p, r) {
+                        return (p == "constructor") ? fake : x[p];
+                    }
+                });
+
+                // oob write
+                Array.prototype.map.apply(y, [f]);
+
+                for (var i = 0; i < 0x10; i++)
+                    n[i][0] = 0x42424242;
+
+            }
+
+            test();
+
+        }
+    },
 ];
 
 testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
diff --git a/test/es6/default-splitscope.js b/test/es6/default-splitscope.js
diff --git a/test/es6/default.js b/test/es6/default.js

Original file line number	Diff line number	Diff line change
`@@ -287,6 +287,11 @@ class BVUnitT`
`287`	`287`	`T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;`
`288`	`288`	`return (this->word & mask) == mask;`
`289`	`289`	`}`
	`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`	`+ }`
`290`	`295`	`void SetRange(const BVIndex index, uint length)`
`291`	`296`	`{`
`292`	`297`	`T mask = ((T)AllOnesMask) >> (BitsPerWord - length) << index;`
Original file line number	Diff line number	Diff line change
`@@ -816,6 +816,7 @@ Symbol* Parser::AddDeclForPid(ParseNodePtr pnode, IdentPtr pid, SymbolType symbo`
`816`	`816`	`&& blockInfo->pnodeBlock->sxBlock.blockType == PnodeBlockType::Function`
`817`	`817`	`&& blockInfo->pBlockInfoOuter != nullptr`
`818`	`818`	`&& blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.blockType == PnodeBlockType::Parameter`
	`819`	`+ && blockInfo->pnodeBlock->sxBlock.scope->GetScopeType() != ScopeType_FuncExpr`
`819`	`820`	`&& blockInfo->pBlockInfoOuter->pnodeBlock->sxBlock.scope->GetCanMergeWithBodyScope())`
`820`	`821`	`{`
`821`	`822`	`blockInfo = blockInfo->pBlockInfoOuter;`
`@@ -5029,6 +5030,13 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncPare`
`5029`	`5030`	`}`
`5030`	`5031`	`return false;`
`5031`	`5032`	`});`
	`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`	`+ }`
`5032`	`5040`	`}`
`5033`	`5041`	`}`
`5034`	`5042`	`}`
`@@ -5895,13 +5903,6 @@ bool Parser::ParseFncNames(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncParent, u`
`5895`	`5903`	`pnodeFnc->sxFnc.pid = m_phtbl->PidHashNameLen(`
`5896`	`5904`	`m_pscan->PchBase() + ichMinNames, ichLimNames - ichMinNames);`
`5897`	`5905`	`}`
`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`		`- }`
`5905`	`5906`	`}`
`5906`	`5907`
`5907`	`5908`	`return true;`
`@@ -6670,10 +6671,11 @@ void Parser::AddArgumentsNodeToVars(ParseNodePtr pnodeFnc)`
`6670`	`6671`	`}`
`6671`	`6672`	`else`
`6672`	`6673`	`{`
	`6674`	`+ ParseNodePtr argNode = nullptr;`
`6673`	`6675`	`if(m_ppnodeVar == &pnodeFnc->sxFnc.pnodeVars)`
`6674`	`6676`	`{`
`6675`	`6677`	`// 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);`
`6677`	`6679`	`}`
`6678`	`6680`	`else`
`6679`	`6681`	`{`
`@@ -6684,10 +6686,18 @@ void Parser::AddArgumentsNodeToVars(ParseNodePtr pnodeFnc)`
`6684`	`6686`	`// object until it is replaced with something else.`
`6685`	`6687`	`ParseNodePtr *const ppnodeVarSave = m_ppnodeVar;`
`6686`	`6688`	`m_ppnodeVar = &pnodeFnc->sxFnc.pnodeVars;`
`6687`		`- CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc)->grfpn \|= PNodeFlags::fpnArguments;`
	`6689`	`+ argNode = CreateVarDeclNode(wellKnownPropertyPids.arguments, STVariable, true, pnodeFnc);`
`6688`	`6690`	`m_ppnodeVar = ppnodeVarSave;`
`6689`	`6691`	`}`
`6690`	`6692`
	`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`	`+`
`6691`	`6701`	`pnodeFnc->sxFnc.SetHasReferenceableBuiltInArguments(true);`
`6692`	`6702`	`}`
`6693`	`6703`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3693,7 +3693,9 @@ void EnsureFncDeclScopeSlot(ParseNode pnodeFnc, FuncInfo funcInfo)`
`3693`	`3693`	`{`
`3694`	`3694`	`Assert(pnodeFnc->sxFnc.pnodeName->nop == knopVarDecl);`
`3695`	`3695`	`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())`
`3697`	`3699`	`{`
`3698`	`3700`	`sym->EnsureScopeSlot(funcInfo);`
`3699`	`3701`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9019,7 +9019,7 @@ namespace Js`
`9019`	`9019`
`9020`	`9020`	`if (newArr)`
`9021`	`9021`	`{`
`9022`		`- newArr->DirectSetItemAt(k, mappedValue);`
	`9022`	`+ newArr->SetItem(k, mappedValue, PropertyOperation_None);`
`9023`	`9023`	`}`
`9024`	`9024`	`else`
`9025`	`9025`	`{`