[CVE-2017-8740] Fix bad byte code gen for 'with'. The original (conservative) fix for this issue relied on marking scopes as containing 'with'. But because block scopes are created lazily, we can miss the opportunity to mark a scope. Instead, implementing a more accurate fix that marks symbols that are referenced from within 'with' statements as needing scope objects if they are closure-captured.

Author: pleath

lib/Parser/Hash.cpp

@@ -131,18 +131,6 @@ void HashTbl::Grow()
 #endif
 }
 
-void HashTbl::ClearPidRefStacks()
-{
-    // Clear pidrefstack pointers from all existing pid's.
-    for (uint i = 0; i < m_luMask; i++)
-    {
-        for (IdentPtr pid = m_prgpidName[i]; pid; pid = pid->m_pidNext)
-        {
-            pid->m_pidRefStack = nullptr;
-        }
-    }
-}
-
 #if DEBUG
 uint HashTbl::CountAndVerifyItems(IdentPtr *buckets, uint bucketCount, uint mask)
 {

lib/Parser/Hash.h

@@ -393,7 +393,18 @@ class HashTbl
     NoReleaseAllocator* GetAllocator() {return &m_noReleaseAllocator;}
 
     bool Contains(_In_reads_(cch) LPCOLESTR prgch, int32 cch);
-    void ClearPidRefStacks();
+
+    template<typename Fn>
+    void VisitPids(Fn fn)
+    {
+        for (uint i = 0; i <= m_luMask; i++)
+        {
+            for (IdentPtr pid = m_prgpidName[i]; pid; pid = pid->m_pidNext)
+            {
+                fn(pid);
+            }
+        }
+    }
 
 private:
     NoReleaseAllocator m_noReleaseAllocator;            // to allocate identifiers

lib/Parser/Parse.cpp

@@ -1726,6 +1726,10 @@ void Parser::BindPidRefsInScope(IdentPtr pid, Symbol *sym, int blockId, uint max
         {
             Assert(ref->GetFuncScopeId() > funcId);
             sym->SetHasNonLocalReference();
+            if (ref->IsDynamicBinding())
+            {
+                sym->SetNeedsScopeObject();
+            }
         }
 
         if (ref->IsFuncAssignment())
@@ -1855,15 +1859,13 @@ void Parser::PopDynamicBlock()
         return;
     }
     Assert(m_currentDynamicBlock);
-    for (BlockInfoStack *blockInfo = m_currentBlockInfo; blockInfo; blockInfo = blockInfo->pBlockInfoOuter)
-    {
-        for (ParseNodePtr pnodeDecl = blockInfo->pnodeBlock->sxBlock.pnodeLexVars;
-             pnodeDecl;
-             pnodeDecl = pnodeDecl->sxVar.pnodeNext)
+
+    m_phtbl->VisitPids([&](IdentPtr pid) {
+        for (PidRefStack *ref = pid->GetTopRef(); ref && ref->GetScopeId() >= blockId; ref = ref->prev)
         {
-            this->SetPidRefsInScopeDynamic(pnodeDecl->sxVar.pid, blockId);
+            ref->SetDynamicBinding();
         }
-    }
+    });
 
     m_currentDynamicBlock = m_currentDynamicBlock->prev;
 }
@@ -9932,10 +9934,6 @@ ParseNodePtr Parser::ParseStatement()
         {
             GetCurrentFunctionNode()->sxFnc.SetHasWithStmt(); // Used by DeferNested
         }
-        for (Scope *scope = this->m_currentScope; scope; scope = scope->GetEnclosingScope())
-        {
-            scope->SetContainsWith();
-        }
 
         ichMin = m_pscan->IchMinTok();
         ChkNxtTok(tkLParen, ERRnoLparen);
@@ -10773,7 +10771,7 @@ void Parser::FinishDeferredFunction(ParseNodePtr pnodeScopeList)
             auto addArgsToScope = [&](ParseNodePtr pnodeArg) {
                 if (pnodeArg->IsVarLetOrConst())
                 {
-                    PidRefStack *ref = this->FindOrAddPidRef(pnodeArg->sxVar.pid, blockId, funcId);//this->PushPidRef(pnodeArg->sxVar.pid);
+                    PidRefStack *ref = this->FindOrAddPidRef(pnodeArg->sxVar.pid, blockId, funcId);
                     pnodeArg->sxVar.symRef = ref->GetSymRef();
                     if (ref->GetSym() != nullptr)
                     {
@@ -11194,7 +11192,7 @@ ParseNodePtr Parser::Parse(LPCUTF8 pszSrc, size_t offset, size_t length, charcou
         FinishParseBlock(pnodeGlobalBlock);
 
         // Clear out references to undeclared identifiers.
-        m_phtbl->ClearPidRefStacks();
+        m_phtbl->VisitPids([&](IdentPtr pid) { pid->SetTopRef(nullptr); });
 
         // Restore global scope and blockinfo stacks preparatory to reparsing deferred functions.
         PushScope(pnodeGlobalBlock->sxBlock.scope);

lib/Runtime/ByteCode/ByteCodeGenerator.cpp

@@ -1780,6 +1780,11 @@ Symbol * ByteCodeGenerator::AddSymbolToScope(Scope *scope, const char16 *key, in
 
     Assert(sym && sym->GetScope() && (sym->GetScope() == scope || sym->GetScope()->GetScopeType() == ScopeType_Parameter));
 
+    if (sym->NeedsScopeObject())
+    {
+        scope->SetIsObject();
+    }
+
     return sym;
 }
 
@@ -2794,34 +2799,6 @@ FuncInfo* PostVisitFunction(ParseNode* pnode, ByteCodeGenerator* byteCodeGenerat
                     byteCodeGenerator->AssignParamSlotsRegister();
                 }
 
-                if (top->GetBodyScope()->ContainsWith() && 
-                    (top->GetBodyScope()->GetHasOwnLocalInClosure() ||
-                     (top->GetParamScope()->GetHasOwnLocalInClosure() &&
-                      top->IsBodyAndParamScopeMerged())))
-                {
-                    // Parent scopes may contain symbols called inside the with.
-                    // Current implementation needs the symScope isObject.
-
-                    top->GetBodyScope()->SetIsObject();
-                    if (top->byteCodeFunction->IsFunctionBody())
-                    {
-                        // Record this for future use in the no-refresh debugging.
-                        top->byteCodeFunction->GetFunctionBody()->SetHasSetIsObject(true);
-                    }
-                }
-
-                if (top->GetParamScope()->ContainsWith() &&
-                    (top->GetParamScope()->GetHasOwnLocalInClosure() &&
-                     !top->IsBodyAndParamScopeMerged()))
-                {
-                    top->GetParamScope()->SetIsObject();
-                    if (top->byteCodeFunction->IsFunctionBody())
-                    {
-                        // Record this for future use in the no-refresh debugging.
-                        top->byteCodeFunction->GetFunctionBody()->SetHasSetIsObject(true);
-                    }
-                }
-
                 if (byteCodeGenerator->NeedObjectAsFunctionScope(top, top->root)
                     || top->bodyScope->GetIsObject()
                     || top->paramScope->GetIsObject())
@@ -3152,11 +3129,6 @@ void ByteCodeGenerator::ProcessScopeWithCapturedSym(Scope *scope)
 {
     Assert(scope->GetHasOwnLocalInClosure());
 
-    if (scope->ContainsWith() && scope->GetScopeType() != ScopeType_Global)
-    {
-        scope->SetIsObject();
-    }
-
     // (Note: if any catch var is closure-captured, we won't merge the catch scope with the function scope.
     // So don't mark the function scope "has local in closure".)
     FuncInfo *func = scope->GetFunc();

lib/Runtime/ByteCode/Scope.h

@@ -41,7 +41,6 @@ class Scope
     BYTE canMergeWithBodyScope : 1;
     BYTE hasLocalInClosure : 1;
     BYTE isBlockInLoop : 1;
-    BYTE containsWith : 1;
 public:
 #if DBG
     BYTE isRestored : 1;
@@ -61,7 +60,6 @@ class Scope
         canMergeWithBodyScope(true),
         hasLocalInClosure(false),
         isBlockInLoop(false),
-        containsWith(false),
         location(Js::Constants::NoRegister),
         m_symList(nullptr),
         m_count(0),
@@ -264,9 +262,6 @@ class Scope
     void SetIsBlockInLoop(bool is = true) { isBlockInLoop = is; }
     bool IsBlockInLoop() const { return isBlockInLoop; }
 
-    void SetContainsWith(bool does = true) { containsWith = does; }
-    bool ContainsWith() const { return containsWith; }
-
     bool HasInnerScopeIndex() const { return innerScopeIndex != (uint)-1; }
     uint GetInnerScopeIndex() const { return innerScopeIndex; }
     void SetInnerScopeIndex(uint index) { innerScopeIndex = index; }

lib/Runtime/ByteCode/Symbol.h

@@ -47,6 +47,7 @@ class Symbol
     BYTE isModuleExportStorage : 1; // If true, this symbol should be stored in the global scope export storage array.
     BYTE isModuleImport : 1; // If true, this symbol is the local name of a module import statement
     BYTE isUsedInLdElem : 1;
+    BYTE needsScopeObject : 1;
 
     // These are get and set a lot, don't put it in bit fields, we are exceeding the number of bits anyway
     bool hasFuncAssignment;
@@ -84,6 +85,7 @@ class Symbol
         isModuleExportStorage(false),
         isModuleImport(false),
         isUsedInLdElem(false),
+        needsScopeObject(false),
         moduleIndex(Js::Constants::NoProperty)
     {
         SetSymbolType(symbolType);
@@ -184,6 +186,16 @@ class Symbol
         return isUsedInLdElem;
     }
 
+    void SetNeedsScopeObject(bool does = true)
+    {
+        needsScopeObject = does;
+    }
+
+    bool NeedsScopeObject() const
+    {
+        return needsScopeObject;
+    }
+
     void SetModuleIndex(Js::PropertyId index)
     {
         moduleIndex = index;

test/Basics/With-defer-block-scope.js

@@ -0,0 +1,27 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+let h = function f(a0 = (function () {
+    a0;
+    a1;
+    a2;
+    a3;
+    a4;
+    a5;
+    a6;
+    a7 = 0x99999; // oob write
+
+    with ({});
+})(), a1, a2, a3, a4, a5, a6, a7) {
+    function g() {
+        f;
+    }
+};
+
+for (let i = 0; i < 0x10000; i++) {
+h();
+}
+
+WScript.Echo('pass');

test/Basics/rlexe.xml

@@ -272,6 +272,12 @@
       <compile-flags>-force:deferparse</compile-flags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>With-defer-block-scope.js</files>
+      <compile-flags>-force:deferparse</compile-flags>
+    </default>
+  </test>
   <test>
     <default>
       <files>withBug940841.js</files>