2027982 - TOCTOU type confusion in [@ js::ProxyHas] — ToPropertyKey can transplant proxy to NativeObject before unchecked ProxyObject cast

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bugmon [:jkratzer for issues]]

Summary

Time-of-check-to-time-of-use type confusion in js::ProxyHas (js/src/proxy/Proxy.cpp:420-428). The function calls ToPropertyKey(cx, idVal, &id) which can invoke arbitrary JavaScript via toString/@@toPrimitive, then passes the object to Proxy::has which performs proxy->as<ProxyObject>().handler() without re-validating the class. During the ToPropertyKey side-effect window, JS_TransplantObject can swap the proxy's contents in-place with a NativeObject, causing the handler() accessor to read the NativeObject::elements_ field and treat it as a BaseProxyHandler*.

Affected Code

File: js/src/proxy/Proxy.cpp, lines 420-428 and 383-390

bool js::ProxyHas(JSContext* cx, HandleObject proxy, HandleValue idVal,
                  bool* result) {
  RootedId id(cx);
  if (!ToPropertyKey(cx, idVal, &id)) {   // <-- runs arbitrary JS
    return false;
  }

  return Proxy::has(cx, proxy, id, result);  // <-- proxy may no longer be a ProxyObject
}

bool Proxy::has(JSContext* cx, HandleObject proxy, HandleId id, bool* bp) {
  AutoCheckRecursionLimit recursion(cx);
  if (!recursion.check(cx)) {
    return false;
  }
  const BaseProxyHandler* handler = proxy->as<ProxyObject>().handler();  // <-- unchecked cast
  *bp = false;
  AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::GET, true);  // <-- crash

The Baseline IC stub HasProp.ProxyElement emits guardIsProxy(objId) followed by proxyHasPropResult(objId, keyId, hasOwn). The guard validates the object is a proxy before entering the VM function, but ToPropertyKey runs inside the VM function and can invalidate that invariant via JS_TransplantObject. When the destination compartment already holds a CCW to the transplant source, JS_TransplantObject reuses the existing CCW's GC cell and swaps its class/shape/slots in-place to become the native object, leaving stale HandleObject references pointing at a non-proxy.

After the swap, proxy->as<ProxyObject>().handler() reads from offsetof(ProxyObject, data.handler) which overlaps with NativeObject::elements_. The elements_ pointer points into the 24-byte emptyElementsHeader global. AutoEnterPolicy then calls handler->hasSecurityPolicy() which reads mHasSecurityPolicy at offset 33 from the bogus handler pointer — 9 bytes past the end of emptyElementsHeader, triggering ASAN global-buffer-overflow.

The same vulnerable pattern exists in:

• js::ProxyHasOwn (lines 461-469)
• js::ProxyGetPropertyByValue (lines 541-550)
• js::ProxySetPropertyByValue

Correct Pattern

bool js::ProxyHas(JSContext* cx, HandleObject proxy, HandleValue idVal,
                  bool* result) {
  RootedId id(cx);
  if (!ToPropertyKey(cx, idVal, &id)) {
    return false;
  }

  // ToPropertyKey can run script which may transplant |proxy| in-place.
  // Re-check the class and fall back to the generic path if it changed.
  if (MOZ_UNLIKELY(!proxy->is<ProxyObject>())) {
    return HasProperty(cx, proxy, id, result);
  }

  return Proxy::has(cx, proxy, id, result);
}

Exploit Chain

1. Create a FakeDOMObject and obtain a transplant closure via transplantableObject({object: d}).
2. Create a new compartment global g and export d into it — g now holds a cross-compartment wrapper (CCW, a ProxyObject) to d.
3. In g, warm up function f(o, k) { return k in o; } with the CCW and an object key, causing Baseline to attach the HasProp.ProxyElement IC stub (guardIsProxy + proxyHasPropResult).
4. Set trigger = true and call f(ccw, key). The IC stub's guardIsProxy passes and calls VM function ProxyHas(cx, ccw, keyVal, &result).
5. ProxyHas calls ToPropertyKey(cx, keyVal, &id) → invokes key.toString() → calls transplant(globalThis).
6. JS_TransplantObject finds an existing CCW in the destination compartment and swaps its guts in-place with the native FakeDOMObject — the GC cell that proxy handles now has a NativeObject class.
7. ToPropertyKey returns. ProxyHas calls Proxy::has(cx, proxy, id, result).
8. Proxy::has executes proxy->as<ProxyObject>().handler() reading what is now NativeObject::elements_ (pointer to emptyElementsHeader + sizeof(ObjectElements)).
9. AutoEnterPolicy calls handler->hasSecurityPolicy() reading 1 byte at offset 33 from the bogus handler → 9 bytes past emptyElementsHeader → ASAN global-buffer-overflow.

Security Impact

Severity: High. Type confusion between ProxyObject and NativeObject with attacker-influenced control flow.

Attacker capability: With a non-empty elements array, elements_ points into the GC heap rather than the static emptyElementsHeader. The attacker controls the contents of dense elements (arbitrary JS Values), so the fake BaseProxyHandler vtable, mFamily, mHasPrototype, and mHasSecurityPolicy are attacker-controlled. If mHasSecurityPolicy reads as true, AutoEnterPolicy calls the virtual handler->enter(...) through an attacker-controlled vtable pointer, yielding arbitrary code execution in the content process.

Preconditions: Requires a mechanism to trigger JS_TransplantObject from script during a toString callback. In the JS shell this is transplantableObject. In the browser, JS_TransplantObject is invoked by document.adoptNode() and cross-global navigation — whether these can be triggered synchronously from a toString callback on a CCW'd DOM node requires further browser-side investigation.

ASAN Report

==195847==ERROR: AddressSanitizer: global-buffer-overflow on address 0x59b719994a21 at pc 0x59b71cd598e6 bp 0x7ffd7f104c60 sp 0x7ffd7f104c58
READ of size 1 at 0x59b719994a21 thread T0
    #0 0x59b71cd598e5 in js::BaseProxyHandler::hasSecurityPolicy() const /firefox/obj-x86_64-pc-linux-gnu/dist/include/js/Proxy.h:212:43
    #1 0x59b71cd598e5 in js::AutoEnterPolicy::AutoEnterPolicy(JSContext*, js::BaseProxyHandler const*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, unsigned int, bool) /firefox/obj-x86_64-pc-linux-gnu/dist/include/js/Proxy.h:633:22
    #2 0x59b71cd506d9 in js::Proxy::has(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, bool*) /firefox/js/src/proxy/Proxy.cpp:390:19
    #3 0x59b71cd50d89 in js::ProxyHas(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, bool*) /firefox/js/src/proxy/Proxy.cpp:427:10
    #4 0x151a9dadf2a6  (<unknown module>)

0x59b719994a21 is located 31 bytes before global variable 'emptyElementsHeaderShared' defined in '/firefox/js/src/vm/NativeObject.cpp:57' (0x59b719994a40) of size 24
0x59b719994a21 is located 9 bytes after global variable 'emptyElementsHeader' defined in '/firefox/js/src/vm/NativeObject.cpp:51' (0x59b719994a00) of size 24
SUMMARY: AddressSanitizer: global-buffer-overflow /firefox/obj-x86_64-pc-linux-gnu/dist/include/js/Proxy.h:212:43 in js::BaseProxyHandler::hasSecurityPolicy() const

Suggested Fix

Add a class re-check after ToPropertyKey in all four Proxy*ByValue helpers that are called from JIT IC stubs:

bool js::ProxyHas(JSContext* cx, HandleObject proxy, HandleValue idVal,
                  bool* result) {
  RootedId id(cx);
  if (!ToPropertyKey(cx, idVal, &id)) {
    return false;
  }
  if (MOZ_UNLIKELY(!proxy->is<ProxyObject>())) {
    return HasProperty(cx, proxy, id, result);
  }
  return Proxy::has(cx, proxy, id, result);
}

Apply the same guard to ProxyHasOwn, ProxyGetPropertyByValue, and ProxySetPropertyByValue. Alternatively, hoist ToPropertyKey out of the VM helpers into the IC stub so the guardIsProxy check runs after key conversion.

Comment 1

[Bugmon [:jkratzer for issues]]

Attachment: poc.js

Comment 2

[Bugmon [:jkratzer for issues]]

Attachment: crash_stack.txt

Comment 4

[Jan de Mooij [:jandem]]

This is a really good find. I'm a bit worried about similar bugs lurking in the code. We should at least check the proxy code in this area.

Comment 5

[Christian Holler (:decoder)]

Attachment: Re-check proxy class after ToPropertyKey in JIT VM helpers

Comment 6

[Christian Holler (:decoder)]

Prefs check: No prefs required. The shell transplantableObject helper simulates JS_TransplantObject, which the browser invokes via document.adoptNode() / cross-global navigation. The vulnerable ProxyHas path through the HasProp.ProxyElement Baseline IC is production code. Supported configuration, [prefs-checked] added.

sec-high is correct: Type confusion where NativeObject::elements_ is read as BaseProxyHandler*. With a populated elements array this points into attacker-controlled GC heap, giving a fake vtable for the virtual handler->enter() call in AutoEnterPolicy.

Fix: Reproduced the ASAN global-buffer-overflow with --baseline-eager. Added a proxy->is<ProxyObject>() re-check after ToPropertyKey in all four JIT-reachable helpers, falling back to the generic VM path (HasProperty / HasOwnProperty / GetProperty / SetProperty) if the class changed. The check is MOZ_UNLIKELY — one predictable branch on a cold path, no overhead when the key is already an atom (ToPropertyKey doesn't call into JS then).

Verified: original PoC and variants for all four helpers (k in o, Object.hasOwn(o,k), o[k], o[k]=v) no longer crash; normal proxy operations and CCW access unaffected.

Re: comment 4 (similar bugs) — these four are the only ToPropertyKey call sites in Proxy.cpp. The ByName siblings (ProxyGetProperty, ProxySetProperty) take a pre-converted HandleId and the IC guards the proxy class after any key conversion, so they're not affected by this specific TOCTOU.

This is the analysis tool's suggested fix. Feel welcome to adopt it as a starting point and evolve it as needed to meet our coding standards.

Comment 7

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20260331210827-1369941f425d.
Unable to bisect testcase (Testcase reproduces on start build!):

Start: 6fb14d9268d3e32063cc78c1c5246980488aec70 (20260102212001)
End: 1369941f425dab52c6a0a3de6b14d9de93376a04 (20260331210827)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False, searchfox=False, afl=False)

Comment 8

[Iain Ireland [:iain]]

I got Claude to do an audit focusing on side-effecting operations other than ToPropertyKey. It identified PropertyEnumerator::enumerateProxyProperties as another potentially similar case. However, when I asked it to write a testcase, it thought for 20 minutes and concluded that it was actually not a problem. The explanation, which I find convincing, is that the potentially side-effecting operations in enumerateProxyProperties are the proxy's own handlers. But scripted proxies created by users are not transplantable, and transplantable DOM proxies have handlers that don't run arbitrary JS code. In the original testcase, ToPropertyKey is called on the key being looked up, not the proxy that we will look it up in, so it's possible to inject arbitrary behaviour.

Given that, I think Claude's initial proposed fix is sufficient. It looks like these four functions are the only (current) instances of this problem.

It does feel like there's still a bit of a footgun here. Native objects suddenly becoming proxies seems unavoidable, but maybe we can prevent proxies from becoming native objects. It's actually not entirely clear to me why the proxy here needs to be changed at all. I guess it's because of this code, where we see that the destination compartment already has a wrapper for the object we're transplanting? I wonder if it would be better if we somehow turned that wrapper into a same-compartment wrapper instead of overwriting it. IIUC, that would prevent proxy->native transitions; maybe it's not worth it, given that native->proxy transitions are going to be possible either way.

Comment 9

[Iain Ireland [:iain]]

Attachment: (secure)

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 2027982: Add test and comments r=jandem

Comment 11

[Iain Ireland [:iain]]

Comment on attachment 9563331 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The attacker would have to realize that the issue is transplantable objects being swapped by the ToPropertyKey call. Once the bug is identified, it doesn't seem too hard to exploit. I asked Claude to look at the patch and figure out the exploit; it didn't immediately identify the real issue, but I didn't push very hard.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch should apply cleanly to all branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This patch only changes behaviour in cases where the bug is being triggered.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9563331 [details]
(secure)

Approved to land and request uplift

Comment 14

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Sec-high bug
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This patch only changes behaviour in cases where the bug is being triggered.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 15

[Iain Ireland [:iain]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D291614

Comment 16

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: Sec-high bug
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This patch only changes behaviour in cases where the bug is being triggered.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 17

[Iain Ireland [:iain]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D291614

Comment 18

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: Sec-high bug
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This patch only changes behaviour in cases where the bug is being triggered.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 19

[Iain Ireland [:iain]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D291614

Comment 20

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: Sec-high bug
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This patch only changes behaviour in cases where the bug is being triggered.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 21

[Iain Ireland [:iain]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D291614

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/63fc4e84a8e7
https://hg.mozilla.org/integration/autoland/rev/c29515d5f859
Add non-proxy fallback r=jandem

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c29515d5f859

Comment 24

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20260408091222-c618caf8d6e8.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 25

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/ff97b3ade90e
https://hg.mozilla.org/releases/mozilla-beta/rev/d70ca8b4c3cd

Comment 26

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/7ce024b5e0b5

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/c554f68c2d8b