[JSC] get_by_id_with_this + ProxyObject can leak JSScope objects

https://bugs.webkit.org/show_bug.cgi?id=267425 <rdar://120777816> Reviewed by Yusuke Suzuki and Justin Michaud. According to the spec [1], `var base = { foo }; with (base) foo();` should be called with `this` value of `base`, which is why FunctionCallResolveNode moves resolved scope to thisRegister(). That is arguably a bad design, and there is an effort [2] to abolish using JSScope as `this` value. When `this` value is accessed by JS code, it's being sanitized via ToThis (JSScope replaced with `undefined`), yet not in case of `super.property` access calling into ProxyObject `get` trap, which passes raw `this` value as receiver parameter, leaking JSScope to be exploited. For performance reasons, we can't call toThis() whenever `get_by_id_with_this` is used, so this change introduces @tothis() intrinsic specifically for ProxyObject IC helpers, tweaks DFG to respect `m_srcDst`, and also fixes baseline code. Inlineability of ProxyObject IC helpers was verified to remain unaffected (`performProxyObjectGet` is smaller then 120 while other helpers were already exceeding inline size limit). [1]: https://tc39.es/ecma262/#sec-evaluatecall (step 1.b.iii) [2]: https://bugs.webkit.org/show_bug.cgi?id=225397 * JSTests/stress/regress-120777816.js: Added. * Source/JavaScriptCore/builtins/ProxyHelpers.js: (linkTimeConstant.performProxyObjectGet): (linkTimeConstant.performProxyObjectGetByVal): (linkTimeConstant.performProxyObjectSetSloppy): (linkTimeConstant.performProxyObjectSetStrict): * Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h: * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::emitToThis): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: (JSC::BytecodeGenerator::emitToThis): * Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp: (JSC::BytecodeIntrinsicNode::emit_intrinsic_toThis): * Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: (JSC::DFG::ByteCodeParser::parseBlock): (JSC::DFG::ByteCodeParser::getThis): Deleted. (JSC::DFG::ByteCodeParser::setThis): Deleted. * Source/JavaScriptCore/runtime/ProxyObject.cpp: (JSC::performProxyGet): (JSC::ProxyObject::performPut): Originally-landed-as: 272448.103@safari-7618-branch (e3a7580). rdar://124556295 Canonical link: https://commits.webkit.org/276104@main
WebKit · Mar 14, 2024 · ceb7e89 · ceb7e89
1 parent 8b2e509
commit ceb7e89
Show file tree

Hide file tree

Showing 8 changed files with 119 additions and 22 deletions.
diff --git a/JSTests/stress/regress-120777816.js b/JSTests/stress/regress-120777816.js
@@ -0,0 +1,96 @@
+function assert(x) {
+    if (!x)
+        throw new Error("Bad assertion!");
+}
+
+(function() {
+    function tryToLeakThisViaGetById() {
+        class Leaker {
+            leak() {
+                return super.foo;
+            }
+        }
+
+        Leaker.prototype.__proto__ = new Proxy({}, {
+            get(target, propertyName, receiver) {
+                return receiver;
+            }
+        });
+
+        const foo = 42;
+        const {leak} = Leaker.prototype;
+
+        return (() => leak())();
+    }
+
+    function tryToLeakThisViaGetByVal() {
+        class Leaker {
+            leak() {
+                return super[Math.random() < 0.5 ? "foo" : "bar"];
+            }
+        }
+
+        Leaker.prototype.__proto__ = new Proxy({}, {
+            get(target, propertyName, receiver) {
+                return receiver;
+            }
+        });
+
+        const foo = 42;
+        const bar = 84;
+        const {leak} = Leaker.prototype;
+
+        return (() => leak())();
+    }
+
+    function tryToLeakThisViaSetById() {
+        let receiver;
+        class Leaker {
+            leak() {
+                super.foo = {};
+                return receiver;
+            }
+        }
+        Leaker.prototype.__proto__ = new Proxy({}, {
+            set(target, propertyName, value, __receiver) {
+                receiver = __receiver;
+                return true;
+            }
+        });
+
+        const foo = 42;
+        const {leak} = Leaker.prototype;
+
+        return (() => leak())();
+    }
+
+    function tryToLeakThisViaSetByVal() {
+        let receiver;
+        class Leaker {
+            leak() {
+                super[Math.random() < 0.5 ? "foo" : "bar"] = {};
+                return receiver;
+            }
+        }
+
+        Leaker.prototype.__proto__ = new Proxy({}, {
+            set(target, propertyName, value, __receiver) {
+                receiver = __receiver;
+                return true;
+            }
+        });
+
+        const foo = 42;
+        const bar = 84;
+        const {leak} = Leaker.prototype;
+
+        return (() => leak())();
+    }
+
+    for (var i = 0; i < 1e5; i++) {
+        assert(tryToLeakThisViaGetById() === undefined);
+        assert(tryToLeakThisViaGetByVal() === undefined);
+        assert(tryToLeakThisViaSetById() === undefined);
+        assert(tryToLeakThisViaSetByVal() === undefined);
+    }
+})();
diff --git a/Source/JavaScriptCore/builtins/ProxyHelpers.js b/Source/JavaScriptCore/builtins/ProxyHelpers.js
@@ -68,7 +68,7 @@ function performProxyObjectGet(propertyName, receiver)
     if (!@isCallable(trap))
         @throwTypeError("'get' property of a Proxy's handler should be callable");
 
-    var trapResult = trap.@call(handler, target, propertyName, receiver);
+    var trapResult = trap.@call(handler, target, propertyName, @toThis(receiver));
 
     if (@mustValidateResultOfProxyGetAndSetTraps(target))
         @handleProxyGetTrapResult(trapResult, target, propertyName);
@@ -95,7 +95,7 @@ function performProxyObjectGetByVal(propertyName, receiver)
     if (!@isCallable(trap))
         @throwTypeError("'get' property of a Proxy's handler should be callable");
 
-    var trapResult = trap.@call(handler, target, propertyName, receiver);
+    var trapResult = trap.@call(handler, target, propertyName, @toThis(receiver));
 
     if (@mustValidateResultOfProxyGetAndSetTraps(target))
         @handleProxyGetTrapResult(trapResult, target, propertyName);
@@ -123,7 +123,7 @@ function performProxyObjectSetSloppy(propertyName, receiver, value)
     if (!@isCallable(trap))
         @throwTypeError("'set' property of a Proxy's handler should be callable");
 
-    if (!trap.@call(handler, target, propertyName, value, receiver))
+    if (!trap.@call(handler, target, propertyName, value, @toThis(receiver)))
         return;
 
     if (@mustValidateResultOfProxyGetAndSetTraps(target))
@@ -150,7 +150,7 @@ function performProxyObjectSetStrict(propertyName, receiver, value)
     if (!@isCallable(trap))
         @throwTypeError("'set' property of a Proxy's handler should be callable");
 
-    if (!trap.@call(handler, target, propertyName, value, receiver))
+    if (!trap.@call(handler, target, propertyName, value, @toThis(receiver)))
         @throwTypeError("Proxy object's 'set' trap returned falsy value for property '" + @String(propertyName) + "'");
 
     if (@mustValidateResultOfProxyGetAndSetTraps(target))

diff --git a/Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h b/Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h
@@ -99,6 +99,7 @@ enum class LinkTimeConstant : int32_t;
     macro(toString) \
     macro(toPropertyKey) \
     macro(toObject) \
+    macro(toThis) \
     macro(mustValidateResultOfProxyGetAndSetTraps) \
     macro(mustValidateResultOfProxyTrapsExceptGetAndSet) \
     macro(newArrayWithSize) \

diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
@@ -5632,9 +5632,10 @@ void StaticPropertyAnalysis::record()
     }
 }
 
-void BytecodeGenerator::emitToThis()
+RegisterID* BytecodeGenerator::emitToThis(RegisterID* srcDst)
 {
-    OpToThis::emit(this, kill(&m_thisRegister), ecmaMode(), nextValueProfileIndex());
+    OpToThis::emit(this, kill(srcDst), ecmaMode(), nextValueProfileIndex());
+    return srcDst;
 }
 
 ForInContext* BytecodeGenerator::findForInContext(RegisterID* property)

diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
@@ -752,6 +752,7 @@ namespace JSC {
         RegisterID* emitToNumeric(RegisterID* dst, RegisterID* src);
         RegisterID* emitToString(RegisterID* dst, RegisterID* src);
         RegisterID* emitToObject(RegisterID* dst, RegisterID* src, const Identifier& message);
+        RegisterID* emitToThis(RegisterID* srcDst);
         RegisterID* emitInc(RegisterID* srcDst);
         RegisterID* emitDec(RegisterID* srcDst);
 
@@ -1080,7 +1081,7 @@ namespace JSC {
         bool isNewTargetUsedInInnerArrowFunction();
         bool isArgumentsUsedInInnerArrowFunction();
 
-        void emitToThis();
+        void emitToThis() { emitToThis(&m_thisRegister); }
 
         RegisterID* emitMove(RegisterID* dst, RegisterID* src);
 

diff --git a/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp b/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
@@ -2023,6 +2023,15 @@ RegisterID* BytecodeIntrinsicNode::emit_intrinsic_toObject(BytecodeGenerator& ge
     return generator.move(dst, generator.emitToObject(temp.get(), src.get(), generator.vm().propertyNames->emptyIdentifier));
 }
 
+RegisterID* BytecodeIntrinsicNode::emit_intrinsic_toThis(BytecodeGenerator& generator, RegisterID* dst)
+{
+    ArgumentListNode* node = m_args->m_listNode;
+    RefPtr<RegisterID> src = generator.emitNode(node);
+    ASSERT(!node->m_next);
+
+    return generator.move(dst, generator.emitToThis(src.get()));
+}
+
 RegisterID* BytecodeIntrinsicNode::emit_intrinsic_idWithProfile(BytecodeGenerator& generator, RegisterID* dst)
 {
     ArgumentListNode* node = m_args->m_listNode;

diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
@@ -769,17 +769,6 @@ class ByteCodeParser {
         return jsConstant(m_graph.freeze(constantValue));
     }
 
-    // Helper functions to get/set the this value.
-    Node* getThis()
-    {
-        return get(m_inlineStackTop->m_codeBlock->thisRegister());
-    }
-
-    void setThis(Node* value)
-    {
-        set(m_inlineStackTop->m_codeBlock->thisRegister(), value);
-    }
-
     InlineCallFrame* inlineCallFrame()
     {
         return m_inlineStackTop->m_inlineCallFrame;
@@ -6232,8 +6221,8 @@ void ByteCodeParser::parseBlock(unsigned limit)
         }
 
         case op_to_this: {
-            Node* op1 = getThis();
             auto bytecode = currentInstruction->as<OpToThis>();
+            Node* op1 = get(VirtualRegister(bytecode.m_srcDst));
             auto& metadata = bytecode.metadata(codeBlock);
             StructureID cachedStructureID = metadata.m_cachedStructureID;
             Structure* cachedStructure = nullptr;
@@ -6245,7 +6234,7 @@ void ByteCodeParser::parseBlock(unsigned limit)
                 || cachedStructure->classInfoForCells()->isSubClassOf(JSScope::info())
                 || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
                 || (op1->op() == GetLocal && op1->variableAccessData()->structureCheckHoistingFailed())) {
-                setThis(addToGraph(ToThis, OpInfo(bytecode.m_ecmaMode), OpInfo(getPrediction()), op1));
+                set(bytecode.m_srcDst, addToGraph(ToThis, OpInfo(bytecode.m_ecmaMode), OpInfo(getPrediction()), op1));
             } else {
                 addToGraph(
                     CheckStructure,

diff --git a/Source/JavaScriptCore/runtime/ProxyObject.cpp b/Source/JavaScriptCore/runtime/ProxyObject.cpp
@@ -187,7 +187,7 @@ static JSValue performProxyGet(JSGlobalObject* globalObject, ProxyObject* proxyO
     MarkedArgumentBuffer arguments;
     arguments.append(target);
     arguments.append(identifierToSafePublicJSValue(vm, Identifier::fromUid(vm, propertyName.uid())));
-    arguments.append(receiver);
+    arguments.append(receiver.toThis(globalObject, ECMAMode::strict()));
     ASSERT(!arguments.hasOverflowed());
     JSValue trapResult = call(globalObject, getHandler, callData, handler, arguments);
     RETURN_IF_EXCEPTION(scope, { });
@@ -517,7 +517,7 @@ bool ProxyObject::performPut(JSGlobalObject* globalObject, JSValue putValue, JSV
     arguments.append(target);
     arguments.append(identifierToSafePublicJSValue(vm, Identifier::fromUid(vm, propertyName.uid())));
     arguments.append(putValue);
-    arguments.append(thisValue);
+    arguments.append(thisValue.toThis(globalObject, ECMAMode::strict()));
     ASSERT(!arguments.hasOverflowed());
     JSValue trapResult = call(globalObject, setMethod, callData, handler, arguments);
     RETURN_IF_EXCEPTION(scope, false);