CVE-2017-0152: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: A remote code execution vulnerability exists in the way affected Microsoft scripting engine render when handling objects in memory in Microsoft browsers. The vulnerability could corrupt memory in such a way that an attacker could execute arbitrary code in the context of the current user. An attacker who successfully exploited the vulnerability could gain the same user rights as the current user, aka "Scripting Engine Memory Corruption Vulnerability."
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 8.1
Published: 2017-07-17T13:18Z
Repository: https://github.com/Microsoft/ChakraCore
Language: C++
Files changed: lib/Parser/Parse.cpp, lib/Runtime/ByteCode/ByteCodeEmitter.cpp, test/es6/default-splitscope.js
Commit date: 2017-03-16 15:40:16 -0700
Commit message: [CVE-2017-0152] MSFT: 10592731 : Issue with Function name capturing in param scope

In a function expression with name, where the name is captured in one
of the param scope functions, if there is a function or var declaration
with the same name as the function expression name we were marking the
function expression name as shadowed. In non-eval case this causes
issue because the name symbol won't get added to the body. This change is to
fix it in such a way if the name is captured in the param scope then we
split the param and body scope such that the name symbol is added to the
param scope not body scope.

Diff: diff --git a/lib/Parser/Parse.cpp b/lib/Parser/Parse.cpp
index 683a8fdf4..7bf964c09 100644
--- a/lib/Parser/Parse.cpp
+++ b/lib/Parser/Parse.cpp
@@ -5196,10 +5196,23 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho
                         Assert(pnodeFnc->sxFnc.UsesArguments());
                         // Arguments symbol is captured in the param scope
                         paramScope->SetCannotMergeWithBodyScope();
                     }
                 }
+                if (paramScope->GetCanMergeWithBodyScope() && !fDeclaration && pnodeFnc->sxFnc.pnodeName != nullptr)
+                {
+                    Symbol* funcSym = pnodeFnc->sxFnc.pnodeName->sxVar.sym;
+                    if (funcSym->GetPid()->GetTopRef()->GetFuncScopeId() > pnodeFnc->sxFnc.functionId)
+                    {
+                        // This is a function expression with name captured in the param scope. In non-eval, non-split cases the function
+                        // name symbol is added to the body scope to make it accessible in the body. But if there is a function or var
+                        // declaration with the same name in the body then adding to the body will fail. So in this case we have to add
+                        // the name symbol to the param scope by splitting it.
+                        paramScope->SetCannotMergeWithBodyScope();
+                    }
+                }
+
             }
         }
 
         if (!fLambda && paramScope != nullptr && !paramScope->GetCanMergeWithBodyScope()
             && (pnodeFnc->sxFnc.UsesArguments() || pnodeFnc->grfpn & fpnArguments_overriddenByDecl))
diff --git a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
index 673b6c619..665dbe7b1 100644
--- a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
+++ b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
@@ -3317,19 +3317,22 @@ void ByteCodeGenerator::EmitOneFunction(ParseNode *pnode)
 
         if (paramScope && !paramScope->GetCanMergeWithBodyScope())
         {
             // Emit bytecode to copy the initial values from param names to their corresponding body bindings.
             // We have to do this after the rest param is marked as false for need declaration.
+            Symbol* funcSym = funcInfo->root->sxFnc.GetFuncSymbol();
             paramScope->ForEachSymbol([&](Symbol* param) {
                 Symbol* varSym = funcInfo->GetBodyScope()->FindLocalSymbol(param->GetName());
                 Assert(varSym || pnode->sxFnc.pnodeName->sxVar.sym == param);
                 Assert(param->GetIsArguments() || param->IsInSlot(funcInfo));
                 if (param->GetIsArguments() && !funcInfo->GetHasArguments())
                 {
                     // Do not copy the arguments to the body if it is not used
                 }
-                else if (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister))
+                else if ((funcSym == nullptr || funcSym != param)    // Do not copy the symbol over to body as the function expression symbol
+                                                                     // is expected to stay inside the function expression scope
+                    && (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister)))
                 {
                     // Simulating EmitPropLoad here. We can't directly call the method as we have to use the param scope specifically.
                     // Walking the scope chain is not possible at this time.
                     Js::RegSlot tempReg = funcInfo->AcquireTmpRegister();
                     Js::PropertyId slot = param->EnsureScopeSlot(funcInfo);
diff --git a/test/es6/default-splitscope.js b/test/es6/default-splitscope.js
index 0a5da26a8..021ebce8d 100644
--- a/test/es6/default-splitscope.js
+++ b/test/es6/default-splitscope.js
@@ -157,10 +157,26 @@ var tests = [
             }
             var a = function () { return f12(20); };
             return a;
         }
         assert.areEqual(10, f11()(), "Recursive call to the function from the body scope returns the right value when eval is there in the body");
+
+        function f13() {
+            var a = function jnvgfg(sfgnmj = function ccunlk() { jnvgfg(undefined, 1); }, b) {
+                if (b) {
+                    assert.areEqual(undefined, jnvgfg, "This refers to the instance in the body and the value of the function expression is not copied over");
+                }
+                var jnvgfg = 10;
+                if (!b) {
+                    sfgnmj();
+                    return 100;
+                }
+            };
+            assert.areEqual(100, a(), "After the recursion the right value is returned by the split scoped function");
+        };
+        f13();
+
     } 
  }, 
  { 
     name: "Split parameter scope in member functions", 
     body: function () { 

Vulnerable code:                 else if (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister))
Fixed code:                 if (paramScope->GetCanMergeWithBodyScope() && !fDeclaration && pnodeFnc->sxFnc.pnodeName != nullptr)
                {
                    Symbol* funcSym = pnodeFnc->sxFnc.pnodeName->sxVar.sym;
                    if (funcSym->GetPid()->GetTopRef()->GetFuncScopeId() > pnodeFnc->sxFnc.functionId)
                    {
                        // This is a function expression with name captured in the param scope. In non-eval, non-split cases the function
                        // name symbol is added to the body scope to make it accessible in the body. But if there is a function or var
                        // declaration with the same name in the body then adding to the body will fail. So in this case we have to add
                        // the name symbol to the param scope by splitting it.
                        paramScope->SetCannotMergeWithBodyScope();
                    }
                }

            Symbol* funcSym = funcInfo->root->sxFnc.GetFuncSymbol();
                else if ((funcSym == nullptr || funcSym != param)    // Do not copy the symbol over to body as the function expression symbol
                                                                     // is expected to stay inside the function expression scope
                    && (varSym && varSym->GetSymbolType() == STVariable && (varSym->IsInSlot(funcInfo) || varSym->GetLocation() != Js::Constants::NoRegister)))

        function f13() {
            var a = function jnvgfg(sfgnmj = function ccunlk() { jnvgfg(undefined, 1); }, b) {
                if (b) {
                    assert.areEqual(undefined, jnvgfg, "This refers to the instance in the body and the value of the function expression is not copied over");
                }
                var jnvgfg = 10;
                if (!b) {
                    sfgnmj();
                    return 100;
                }
            };
            assert.areEqual(100, a(), "After the recursion the right value is returned by the split scoped function");
        };
        f13();