2024905 - Type confusion via bytecode stack misalignment in [@ DecoratorEmitter::emitCreateDecoratorContextObject] when decorator key is a computed property containing a function expression

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bugmon [:jkratzer for issues]]

Summary

Vulnerability type: Type confusion (JS::Value bits used as NativeObject* pointer)

Affected file: js/src/frontend/DecoratorEmitter.cpp, line 1320 (root cause); js/src/frontend/FunctionEmitter.cpp, lines 104–108 (broken invariant)

Root cause: DecoratorEmitter::emitCreateDecoratorContextObject re-emits a computed property key ParseNode subtree that was already emitted once by BytecodeEmitter::emitPropertyList. When this subtree contains a FunctionNode, the second emission hits the wasEmittedByEnclosingScript() fast-path in BytecodeEmitter::emitFunction, which calls FunctionEmitter::emitAgain(). For non-Annex-B, non-hoisted function expressions, emitAgain() returns true without emitting any bytecode — no value is pushed onto the expression stack. The subsequent JSOp::ToPropertyKey (emitted by emitComputedPropertyName) consumes the wrong stack slot, and the one-slot deficit propagates through the rest of the bytecode. At the outer array literal, JSOp::InitElemArray reads a misaligned slot and treats its raw bits as an object pointer.

Precondition: Requires the build to be configured with --enable-decorators (default: off; js/moz.configure:122). This code is not compiled into production Firefox.

Affected Code

File: js/src/frontend/DecoratorEmitter.cpp, lines 1315–1323

if (key->is<NameNode>()) {
  if (!bce_->emitStringOp(JSOp::String, key->as<NameNode>().atom())) {
    return false;
  }
} else {
  if (!emitPropertyKey(key)) {    // Re-emits the ParseNode tree that was
    return false;                 // already emitted at BytecodeEmitter.cpp:9792
  }
}

emitPropertyKey → emitComputedPropertyName:

bool BytecodeEmitter::emitComputedPropertyName(UnaryNode* computedPropName) {
  MOZ_ASSERT(computedPropName->isKind(ParseNodeKind::ComputedName));
  return emitTree(computedPropName->kid()) && emit1(JSOp::ToPropertyKey);
  //     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ re-emits the FunctionNode
}

File: js/src/frontend/BytecodeEmitter.cpp, lines 6190–6195

if (funbox->wasEmittedByEnclosingScript()) {
  if (!fe.emitAgain()) {
    //            [stack]
    return false;
  }
  MOZ_ASSERT(funNode->functionIsHoisted());  // debug-only! not enforced in release
}

File: js/src/frontend/FunctionEmitter.cpp, lines 96–109

bool FunctionEmitter::emitAgain() {
  MOZ_ASSERT(state_ == State::Start);
  MOZ_ASSERT(funbox_->wasEmittedByEnclosingScript());

  //                [stack]

  // Annex B block-scoped functions are hoisted like any other assignment
  // that assigns the function to the outer 'var' binding.
  if (!funbox_->isAnnexB) {
#ifdef DEBUG
    state_ = State::End;
#endif
    return true;   // Returns WITHOUT emitting anything — no JSOp::Lambda, nothing pushed
  }

Why this is vulnerable: The decorators proposal requires the computed property key to be evaluated once and reused for both the method installation and the decorator context's name property. The current implementation re-evaluates the key expression by re-emitting the same ParseNode tree. This is fundamentally unsound: ParseNode trees are designed for single emission, and nodes like FunctionNode carry state (wasEmittedByEnclosingScript) that makes re-emission a no-op. In debug builds, the MOZ_ASSERT(funNode->functionIsHoisted()) at BytecodeEmitter.cpp:6195 would catch this; in release builds, the bytecode is silently emitted with a stack deficit.

Correct Pattern

The computed key should be evaluated once and stored (e.g., via JSOp::Dup or a stack pick/unpick, or by storing to a temporary slot) and the stored value should be reused when building the decorator context object:

// At the original computed key emission site (BytecodeEmitter::emitPropertyList),
// when decorators are present, dup the key after ToPropertyKey and stash it:
//   [stack] ... KEY
//   Dup
//   [stack] ... KEY KEY
//   <save one copy for decorator context>

// In emitCreateDecoratorContextObject, instead of re-emitting the expression:
if (key->is<NameNode>()) {
  if (!bce_->emitStringOp(JSOp::String, key->as<NameNode>().atom())) {
    return false;
  }
} else if (key->isKind(ParseNodeKind::NumberExpr)) {
  // Number literals are safe to re-emit (no side effects, no state)
  if (!bce_->emitNumberOp(key->as<NumericLiteral>().value())) {
    return false;
  }
} else {
  // For ComputedName: retrieve the previously-computed key from the stack
  // (requires the caller to have stashed it with pickN/dupAt)
  if (!bce_->emitDupAt(/* depth of stashed key */)) {
    return false;
  }
}

Alternatively, the bytecode emitter should assert (via MOZ_RELEASE_ASSERT) that a FunctionNode in expression position is never reached with wasEmittedByEnclosingScript() == true.

Exploit Chain

1. Attacker supplies JS: [class { @dec [function(){}]() {} }]
2. Parser builds a ClassMemberList with a ClassMethod whose key is ComputedName(FunctionNode)
3. BytecodeEmitter::emitPropertyList calls emitTree(key->kid()) — the FunctionNode is emitted, JSOp::Lambda is pushed, and funbox->wasEmittedByEnclosingScript is set to true
4. After installing the method, decorator application begins. DecoratorEmitter::emitCallDecoratorForElement calls emitCreateDecoratorContextObject(kind, key, ...)
5. At DecoratorEmitter.cpp:1320, emitPropertyKey(key) is called for the non-NameNode key
6. This reaches emitComputedPropertyName → emitTree(funNode) → emitFunction
7. Since wasEmittedByEnclosingScript() is true, emitAgain() is called. The function is not Annex-B, so it returns true without emitting any bytecode — nothing is pushed
8. Back in emitComputedPropertyName, emit1(JSOp::ToPropertyKey) is emitted. At runtime this pops the wrong stack slot (the ObjectEmitter's partially-built context object) and converts it to a property key
9. The emitted bytecode now has one fewer push than the bytecode-emitter's stack model expects. Every subsequent operation that references stack slots is off by one
10. Class emission completes and the outer array literal emits JSOp::InitElemArray. At runtime, REGS.sp[-2] now reads into a slot containing raw bits 0x0000000000000000 (which is DoubleValue(+0.0))
11. Value::toObject() computes asBits_ ^ JSVAL_SHIFTED_TAG_OBJECT = 0 ^ 0xfffe000000000000 = (JSObject*)0xfffe000000000000 (unchecked in release; MOZ_ASSERT(isObject()) is debug-only)
12. This pointer is passed via obj.as<ArrayObject>() (unchecked cast) to InitElemArrayOperation
13. setDenseInitializedLength → getElementsHeader() reads this->elements_ at offset 0x10 → load from 0xfffe000000000010 → SEGV (non-canonical address)

Security Impact

Severity: sec-high (within --enable-decorators builds only)

Attacker capability: The bytecode stack misalignment is deterministic and depends on the emitted bytecode, which in turn depends on the attacker-controlled JS source. In this minimal testcase, the misaligned sp[-2] happens to read a zero-bits slot, yielding a non-canonical pointer and a safe crash. However, the attacker fully controls the surrounding JS structure (the class body, additional statements before/after, the outer expression context). By crafting the surrounding code, the attacker may be able to place an arbitrary ObjectValue in the slot that InitElemArray reads — in which case toObject() would correctly extract the payload pointer, and the subsequent unchecked as<ArrayObject>() cast would allow setDenseInitializedLength(index+1) and initDenseElement(index, val) to operate on a non-Array object. If the target object's elements_ field overlaps with attacker-influenced memory, this becomes a controlled write. Additionally, other opcodes (InitElemInc, property initializers) reached during the misaligned window may offer alternative exploitation paths.

Preconditions:

• Build configured with --enable-decorators (default off; not shipped in production Firefox)
• No runtime pref required — if compiled in, the syntax is parsed unconditionally
• No JIT required — reproduces in the bytecode interpreter on first execution

Mitigating factors: The feature is compile-time gated and not present in release Firefox. A debug build would catch this at MOZ_ASSERT(funNode->functionIsHoisted()) (BytecodeEmitter.cpp:6195).

ASAN Report

AddressSanitizer:DEADLYSIGNAL
=================================================================
==412717==ERROR: AddressSanitizer: SEGV on unknown address (pc 0x56c082ece2b5 bp 0x7fff2dd7a630 sp 0x7fff2dd79780 T0)
==412717==The signal is caused by a READ memory access.
==412717==Hint: this fault was caused by a dereference of a high value address (see register values below).  Disassemble the provided pc to learn which register was used.
    #0 0x56c082ece2b5 in js::NativeObject::getElementsHeader() const /firefox/js/src/vm/NativeObject.h:1470:41
    #1 0x56c082ece2b5 in js::NativeObject::setDenseInitializedLengthInternal(unsigned int) /firefox/js/src/vm/NativeObject.h:1532:37
    #2 0x56c082ece2b5 in js::NativeObject::setDenseInitializedLength(unsigned int) /firefox/js/src/vm/NativeObject.h:1539:5
    #3 0x56c082ece2b5 in js::InitElemArrayOperation(JSContext*, unsigned char*, JS::Handle<js::ArrayObject*>, JS::Handle<JS::Value>) /firefox/js/src/vm/Interpreter-inl.h:916:8
    #4 0x56c082ece2b5 in js::Interpret(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:3980:7
    #5 0x56c082eb82a6 in MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:384:10
    #6 0x56c082eb82a6 in js::RunScript(JSContext*, js::RunState&) /firefox/js/src/vm/Interpreter.cpp:460:13
    #7 0x56c082ebcee7 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /firefox/js/src/vm/Interpreter.cpp:850:10
    #8 0x56c0832400f2 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /firefox/js/src/vm/CompilationAndEvaluation.cpp:572:10
    #9 0x56c0813720b7 in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) /firefox/js/src/shell/js.cpp:1388:10
    #10 0x56c081371780 in Process(JSContext*, char const*, bool, FileKind) /firefox/js/src/shell/js.cpp
    #11 0x56c08131d385 in ProcessArgs(JSContext*, js::cli::OptionParser*) /firefox/js/src/shell/js.cpp:12095:10
    #12 0x56c08131d385 in Shell(JSContext*, js::cli::OptionParser*) /firefox/js/src/shell/js.cpp:12348:12
    #13 0x56c08130ac78 in main /firefox/js/src/shell/js.cpp:12754:12
    #14 0x7aeba5bd31c9  (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 8e9fd827446c24067541ac5390e6f527fb5947bb)
    #15 0x7aeba5bd328a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 8e9fd827446c24067541ac5390e6f527fb5947bb)
    #16 0x56c081213e18 in _start (/firefox/obj-x86_64-pc-linux-gnu/dist/bin/js+0x2db3e18) (BuildId: 3aba26634ffaecbc331026713f24abcf)

==412717==Register values:
rax = 0x0000000000000000  rbx = 0x000076e39bffe890  rcx = 0x0000000000000010  rdx = 0x0000780ba50e4f3b  
rdi = 0x000056c084de0978  rsi = 0x0000780ba50e4f3e  rbp = 0x00007fff2dd7a630  rsp = 0x00007fff2dd79780  
 r8 = 0x00000000fffe26d6   r9 = 0x000076e39bffe89f  r10 = 0x00000edc737ffd13  r11 = 0x00000edcf37f7d10  
r12 = 0x00007fff2dd7a3b0  r13 = 0x1fffc00000000002  r14 = 0xfffe000000000010  r15 = 0xfffe000000000000  
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /firefox/js/src/vm/NativeObject.h:1470:41 in js::NativeObject::getElementsHeader() const
==412717==ABORTING

Register decode: r15 = 0xfffe000000000000 = JSVAL_SHIFTED_TAG_OBJECT = (JSObject*)(0 ^ obj_tag) — the this pointer returned by toObject() on a zero-bits slot. r14 = 0xfffe000000000010 = r15 + offsetof(NativeObject, elements_) — the faulting load address.

Suggested Fix

Short-term defensive fix — upgrade the debug assertion to a release assertion so the bytecode emitter fails instead of producing corrupt bytecode:

// js/src/frontend/BytecodeEmitter.cpp:6190
if (funbox->wasEmittedByEnclosingScript()) {
  // emitAgain only handles hoisted functions (Annex B var assignment).
  // A function expression reaching here means the ParseNode tree is being
  // emitted twice, which is unsupported.
  if (!funNode->functionIsHoisted()) {
    MOZ_CRASH("FunctionNode in expression position emitted twice");
  }
  if (!fe.emitAgain()) {
    return false;
  }
}

Correct fix — the decorator emitter must not re-emit computed key expressions. At the first emission point in emitPropertyList, when the class member has decorators and a computed key, dup the key after ToPropertyKey and keep it on the stack (or store it in a reserved slot). In emitCreateDecoratorContextObject, retrieve the stashed key instead of calling emitPropertyKey(key):

// js/src/frontend/DecoratorEmitter.cpp:1315
if (key->is<NameNode>()) {
  if (!bce_->emitStringOp(JSOp::String, key->as<NameNode>().atom())) {
    return false;
  }
} else if (key->isKind(ParseNodeKind::NumberExpr)) {
  if (!bce_->emitNumberOp(key->as<NumericLiteral>().value())) {
    return false;
  }
} else {
  MOZ_ASSERT(key->isKind(ParseNodeKind::ComputedName));
  // Retrieve the pre-computed key stashed at emitPropertyList time.
  // Requires plumbing the stashed-key stack depth through the decorator
  // emitter call chain.
  if (!bce_->emitDupAt(computedKeyDepth)) {
    return false;
  }
}

This also fixes a spec-correctness bug: the current implementation evaluates the computed key expression twice (observable if the expression has side effects), which violates the decorators proposal semantics.

Comment 1

[Bugmon [:jkratzer for issues]]

Attachment: Crash stack trace

Comment 2

[Bugmon [:jkratzer for issues]]

Attachment: Testcase: tmpzxgxomhz.js

Comment 4

[Jan de Mooij [:jandem]]

This requires building with --enable-decorators.

Comment 5

[Tooru Fujisawa [:arai]]

The current property key handling seems to be completely wrong,
and it's difficult to modify the current implementation to work correctly, given there are multiple loops that iterates over property keys (for example for class fields), and the decorator's loop can appear before other loop, and keeping the property key on the stack across them requires almost full rewrite.

it will require either:

• merge all decorator-related and non-decorator-related handlings into single place and single loop
• put property keys in a temporary array, beforehand, and use the array for all remaining operations
• put all property keys on the stack, and dup it for each loop

Comment 6

[Tooru Fujisawa [:arai]]

tentative solution would be to just throw not-yet-implemented Error whenever computed property appears there.
At least the execution order and the number of exectuion are incorrect, and all other side-effectful computed properties also behaves incorrectly.

Comment 7

[Tooru Fujisawa [:arai]]

The following part also doesn't handle computed properties. the string concatenation must be performed at runtime.
And just running the public-auto-accessor.js testcase hits an assertion failure, due to the propAtom being null.

https://searchfox.org/firefox-main/rev/a0503ac3a1606e67261867bb5ba8fef385736c4c/js/src/frontend/Parser.cpp#7751-7754

// Step 3. Let privateStateDesc be the string-concatenation of name
// and " accessor storage".
StringBuilder privateStateDesc(fc_);
if (!privateStateDesc.append(this->parserAtoms(), propAtom)) {

And given that this part is executed regardless of the existence of decorators, throwing an error for computed properties isn't a reasonable option. (edit: actually, this is accessor-specific, which is a part of the decorator. so we can still throw error)
So probably we should drop the current implementation, and re-implement with the correct execution order and the semantics whenever necessary.

Comment 8

[Tooru Fujisawa [:arai]]

so far prepared a patch stack to remove the implementation, in https://phabricator.services.mozilla.com/D289058 as WIP.
but apparently it's not linked, possibly because of WIP + sec bug combination?
Anyway, I'll wait for others' opinion for the removal.

Comment 9

[BugBot [:suhaib / :marco]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:arai, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 10

[Tooru Fujisawa [:arai]]

This is disabled at build-time, and thus S3 or S4 is appropriate.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: (secure)

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: (secure)

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: (secure)

Comment 14

[Dan Minor [:dminor]]

In my opinion, if the implementation is that flawed, we should just remove it. If we come back to decorators, we can always reapply the parts that are still useful.

Comment 15

[Tooru Fujisawa [:arai]]

Some context from discussion.

I'll file two separate bugs for the followings:

• computed properties gets evaluated multiple times (the underlying issue of the comment #0, where property key node is emitted multiple times)
• accessor name concatenation fails on computed properties (comment #7's case)

Given that the decorator code is disabled at compile time and has no effect on the distributed Firefox binaries,
they can be public bugs.
(so, please don't dupe them against this bug)

For this bug, we could do one of the following:

• disable the build option
• throw "not-yet-implemented" error when computed property appears for decorator/accessor
• throw an error on --fuzzing-safe whenever decorator/accessor related keyword appears

Comment 16

[Tooru Fujisawa [:arai]]

filed bug 2025654 and bug 2025653

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: (secure)

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: (secure)

Comment 19

[Tooru Fujisawa [:arai]]

https://wiki.mozilla.org/Security_Severity_Ratings/Client
according to the severity rating, we can reduce the severity for unusual configuration.
this is build-time configuration and won't affect many users (only people who's building the browser by their own with custom configuration).

And thus I'm reducing the rating from sec-high to sec-moderate.
and given it's sec-moderate and disabled by build-time configuration, I'll skip the security approval process.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c62b269f9d24
https://hg.mozilla.org/integration/autoland/rev/a8cf453219de
Part 1: Make decorator/accessor not compatible with --fuzzing-safe. r=dminor
https://github.com/mozilla-firefox/firefox/commit/282e5e1de1bf
https://hg.mozilla.org/integration/autoland/rev/cba979f5bb3d
Part 2: Throw errors when decorators/accessors are used with computed properties. r=dminor

Comment 21

[Donal Meehan [:dmeehan]]

https://hg-edge.mozilla.org/mozilla-central/rev/a8cf453219de
https://hg.mozilla.org/mozilla-central/rev/cba979f5bb3d