Open Bug 2078793 Opened 1 month ago Updated 1 month ago

Wrong |this| (undefined instead of the arguments object) for arguments.length() calls in BytecodeEmitter::emitCalleeAndThis

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

UNCONFIRMED

People

(Reporter: bugmon, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(3 files)

BytecodeEmitter::emitCalleeAndThis and BytecodeEmitter::emitOptionalCalleeAndThis handle a ParseNodeKind::ArgumentsLength callee with CallOrNewEmitter::prepareForOtherCallee() instead of the property-callee path used for ParseNodeKind::DotExpr. The callee is therefore treated as a plain value and CallOrNewEmitter::emitThis emits JSOp::Undefined for |this|, so when arguments.length has been overwritten with a function, calling arguments.length() runs it with |this| undefined (the global object in a sloppy callee) rather than the arguments object that ECMA-262 EvaluateCall requires for a property reference.

A web page author can make SpiderMonkey compute a result that differs from the specification and from V8 by calling a function stored in arguments.length as arguments.length(). The demonstrated effect is a wrong |this| value in script the page itself supplies; no memory corruption, assertion failure or cross-origin effect was observed, and the behaviour is identical under --no-jit-backend and --ion-eager.

Build Info

Affected Code

File: js/src/frontend/BytecodeEmitter.cpp, line 8296-8306

    case ParseNodeKind::ArgumentsLength: {
      MOZ_ASSERT(emitterMode != BytecodeEmitter::SelfHosting);
      if (!cone.prepareForOtherCallee()) {
        return false;
      }
      if (!emitArgumentsLength()) {
        //          [stack] ARGUMENTS_LENGTH
        return false;
      }
      break;
    }

File: js/src/frontend/BytecodeEmitter.cpp, line 8187-8197

    case ParseNodeKind::ArgumentsLength: {
      MOZ_ASSERT(emitterMode != BytecodeEmitter::SelfHosting);
      if (!cone.prepareForOtherCallee()) {
        return false;
      }
      if (!emitArgumentsLength()) {
        //          [stack] ARGUMENTS_LENGTH
        return false;
      }
      break;
    }

File: js/src/frontend/BytecodeEmitter.cpp, line 1701-1719

bool BytecodeEmitter::emitArgumentsLength() {
  if (sc->isFunctionBox() &&
      sc->asFunctionBox()->isEligibleForArgumentsLength() &&
      !sc->asFunctionBox()->needsArgsObj()) {
    return emit1(JSOp::ArgumentsLength);
  }

  PropOpEmitter poe(this, PropOpEmitter::Kind::Get,
                    PropOpEmitter::ObjKind::Other);
  if (!poe.prepareForObj()) {
    return false;
  }

  NameOpEmitter noe(this, TaggedParserAtomIndex::WellKnown::arguments(),
                    NameOpEmitter::Kind::Get);
  if (!noe.emitGet()) {
    return false;
  }
  return poe.emitGet(TaggedParserAtomIndex::WellKnown::length());

File: js/src/frontend/CallOrNewEmitter.cpp, line 177-193

    case State::OtherCallee:
      needsThis = true;
      break;
    default:;
  }
  if (needsThis) {
    if (isNew() || isSuperCall()) {
      if (!bce_->emit1(JSOp::IsConstructing)) {
        //          [stack] CALLEE IS_CONSTRUCTING
        return false;
      }
    } else {
      if (!bce_->emit1(JSOp::Undefined)) {
        //          [stack] CALLEE THIS
        return false;
      }
    }
  }

File: js/src/frontend/Parser.cpp, line 11039-11051

  if (handler_.isArgumentsName(lhs) && handler_.isLengthName(name)) {
    MOZ_ASSERT(pc_->numberOfArgumentsNames > 0);
    pc_->numberOfArgumentsNames--;
    // Resumed generators and async functions have numActualArgs == 0.
    // See e.g. InterpreterStack::createGeneratorResumeFrame and its call to
    // initCallFrame.
    if (pc_->isGeneratorOrAsync()) {
      pc_->sc()->setIneligibleForArgumentsLength();
    }
    return handler_.newArgumentsLength(lhs, name);
  }

  return handler_.newPropertyAccess(lhs, name);

The parser creates a dedicated ArgumentsLength node for the non-optional form arguments.length (js/src/frontend/Parser.cpp:11039-11048). When that node is a call's callee, both callee emitters put the CallOrNewEmitter in the OtherCallee state and emit the value with emitArgumentsLength(). In the fallback path of emitArgumentsLength() (function ineligible for the optimisation, e.g. because arguments.length was assigned) the value is a real arguments.length property read, which can be callable, but the base object is not kept as |this|. CallOrNewEmitter::emitThis then pushes Undefined for OtherCallee. The DotExpr case directly below uses prepareForPropCallee, which keeps the base object as |this|.

Exploit Chain

  1. Script defines a function that assigns a callable to arguments.length, which makes the function ineligible for the JSOp::ArgumentsLength optimisation so the property read returns that callable.
  2. The same function calls it as arguments.length(); the parser has produced a ParseNodeKind::ArgumentsLength callee node.
  3. BytecodeEmitter::emitCalleeAndThis emits the callee via prepareForOtherCallee()/emitArgumentsLength(), and CallOrNewEmitter::emitThis pushes Undefined as |this|.
  4. The callee observes |this| as undefined (strict) or the global object (sloppy) instead of the arguments object; the testcase's === arguments check fails and it throws, while V8 passes.

Steps to Reproduce

  1. Build the SpiderMonkey debug shell from an unmodified tree with /app/mozconfigs/mozconfig.js.linux.debug (no source patches, no prefs).
  2. Run testcase.js with js_shell_differential_evaluator: shell_a=/firefox/obj-js-debug/dist/bin/js (spidermonkey), shell_b=/app/tools/d8/d8 (v8), no extra flags. Result: differs:true, SpiderMonkey exit 3, d8 exit 0.
  3. Run again with flags_a=["--differential-testing"]. Result: differs:true, SpiderMonkey exit 3, d8 exit 0.
  4. Optional control: the same shell with --no-jit-backend versus --ion-eager --no-threads gives differs:false with both sides exiting 3, showing the wrong result is tier-independent.

Security Impact

  • Severity: Low
  • Attacker capability: Page script can observe a |this| value of undefined (or the global object in sloppy code) inside a function it stored in arguments.length and invoked as arguments.length(), where the specification requires the arguments object. This is a language-semantics deviation confined to the script's own realm; no memory corruption, assertion failure, or access to data the script could not already reach was demonstrated.
  • Preconditions: None beyond running script; the function must assign a callable to arguments.length and call it via the literal arguments.length() form (also reproduced by the researcher for the optional-call and tagged-template forms, which I did not re-run).

ASAN Report

No sanitizer report: this is a js_differential finding (exit-status divergence), reproduced by the verifier on a clean tree.

Run 1 (no extra flags): differs=true
  shell_a argv: /firefox/obj-js-debug/dist/bin/js --fuzzing-safe /tmp/fx_audit_jsdiff_g_j65y2_/testcase.js  -> exit_code 3
  shell_b argv: /app/tools/d8/d8 /tmp/fx_audit_jsdiff_g_j65y2_/testcase.js  -> exit_code 0
  SpiderMonkey stderr:
    /tmp/fx_audit_jsdiff_g_j65y2_/testcase.js:6:9 Error: wrong this value for arguments.length() call
    Stack:
      @/tmp/fx_audit_jsdiff_g_j65y2_/testcase.js:6:9
  d8 stderr/stdout: empty

Run 2 (--differential-testing on SpiderMonkey): differs=true
  shell_a argv: /firefox/obj-js-debug/dist/bin/js --fuzzing-safe --differential-testing /tmp/fx_audit_jsdiff_4ifk9za2/testcase.js  -> exit_code 3
  shell_b argv: /app/tools/d8/d8 /tmp/fx_audit_jsdiff_4ifk9za2/testcase.js  -> exit_code 0
  SpiderMonkey stderr:
    /tmp/fx_audit_jsdiff_4ifk9za2/testcase.js:6:9 Error: wrong this value for arguments.length() call
    Stack:
      @/tmp/fx_audit_jsdiff_4ifk9za2/testcase.js:6:1
  d8 stderr/stdout: empty

Control A (verifier-written: arguments["length"]() must return arguments; strict arguments.length() must return undefined): differs=true in the opposite direction, SpiderMonkey exit 0, d8 exit 1 with "Error: control: strict this not undefined". This shows SpiderMonkey handles the computed-key form correctly and passes undefined as |this| for the dotted form in a strict function, while V8 passes the arguments object.

Control B (same testcase, SpiderMonkey --no-jit-backend vs --ion-eager --no-threads): differs=false, both exit 3. The wrong result is produced in every tier, consistent with a bytecode-emitter defect.
Attached patch fix.patch — — Splinter Review
Attached file crash_stack.txt —
Attached file testcase.js —

This bug was filed automatically by an AI-powered security testing tool. The analysis, testcase, and any suggested patches were generated by an AI model and have NOT been verified by a human.

Please review all details carefully before triaging. The issue was confirmed by an independent verifier, but for non-ASAN issues, the verifier has been made less strict. Additionally, the root cause analysis and suggested fix may contain errors.

Group: core-security → javascript-core-security
Blocks: sm-security
Severity: -- → S3
Priority: -- → P3

This is a correctness issue, but it's not security-sensitive.

Group: javascript-core-security
Keywords: sec-other
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: