Closed Bug 1685260 Opened 4 years ago Closed 4 years ago

Assertion failure: next == JSOp::CheckThis || next == JSOp::CheckReturn || next == JSOp::CheckThisReinit || next == JSOp::CheckLexical, at vm/Interpreter.cpp:3715 or Assertion failure: v.isSymbol() || v.isBigInt(), at jsnum.cpp:1944

Categories

(Core :: JavaScript Engine, defect)

defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox-esr78 85+ fixed
firefox84 --- wontfix
firefox85 + fixed
firefox86 + fixed

People

(Reporter: anba, Assigned: anba, NeedInfo)

References

(Regression)

Details

(Keywords: regression, sec-high, testcase, Whiteboard: [sec-survey][adv-main85+r][adv-esr78.7+r])

Attachments

(2 files)

+++ This bug was initially created as a clone of Bug #1684020 +++

{
  var a = null;
  a?.[b];
  !!b;
  let b;
}

Crashes the same way as bug 1684020.

Assignee: nobody → andrebargull
Status: NEW → ASSIGNED

Depends on D100883

Comment on attachment 9195591 [details]
Bug 1685260: Add TDZCheckCache. r=yulia!

Beta/Release Uplift Approval Request

  • User impact if declined: The JS_UNINITIALIZED_LEXICAL magic value is exposed to user code, which can lead to wrong JavaScript behaviour and crashes. It's unclear if this issue can be used for anything worse and because JS::Value is used in many places in SpiderMonkey/Gecko, it's impossible to check every line of code processing JS::Value.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Single line patch to use a fresh TDZCheckCache. TDZCheckCache keeps track of definitely accessed variables and creating a new one is a safe operation, for example for every arm of an if-statement, a new TDZCheckCache is used.
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: See beta uplift request.
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): See beta uplift request.
  • String or UUID changes made by this patch:
Attachment #9195591 - Flags: approval-mozilla-esr78?
Attachment #9195591 - Flags: approval-mozilla-beta?

Please request sec-approval on this patch.

Flags: needinfo?(andrebargull)

Comment on attachment 9195591 [details]
Bug 1685260: Add TDZCheckCache. r=yulia!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Probably easily because it's a single line change. That line added TDZCheckCache which makes it also easy to see where we had a problem: "TDZ" is the abbreviation for "temporal dead zone", which refers to the zone before a declaration of a lexical binding (let, const, etc.) where any access to a lexical binding must throw a ReferenceError.
    So when taking into account where the line was added (bytecode emitter for optional chaining) and what was added (TDZCheckCache), it should be easy to determine what was broken before.
  • 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 older supported branches are affected by this flaw?: all (74+)
  • 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?:
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely, see comment #3.
Flags: needinfo?(andrebargull)
Attachment #9195591 - Flags: sec-approval?
Group: core-security → javascript-core-security
Flags: in-testsuite?

Comment on attachment 9195591 [details]
Bug 1685260: Add TDZCheckCache. r=yulia!

sec-approval+, a=dveditz for landing on beta. Please don't land the test until after we ship the fix (in-testsuite? flag set)

Attachment #9195591 - Flags: sec-approval?
Attachment #9195591 - Flags: sec-approval+
Attachment #9195591 - Flags: approval-mozilla-beta?
Attachment #9195591 - Flags: approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

Comment on attachment 9195591 [details]
Bug 1685260: Add TDZCheckCache. r=yulia!

Approved for 78.7esr.

Attachment #9195591 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(andrebargull)
Whiteboard: [sec-survey]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [sec-survey] → [sec-survey][adv-main85+r]
Whiteboard: [sec-survey][adv-main85+r] → [sec-survey][adv-main85+r][adv-esr78.7+r]
Group: core-security-release
Pushed by andre.bargull@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c617ed604e36
Add test case for tdz check in optional chaining. r=yulia
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: