1685260 - 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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

+++ 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.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1685260: Add TDZCheckCache. r=yulia!

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1685260: Add test case for tdz check in optional chaining. r=yulia!

Depends on D100883

Comment 3

[André Bargull [:anba]]

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:

Comment 4

[Julien Cristau [:jcristau]]

Please request sec-approval on this patch.

Comment 5

[André Bargull [:anba]]

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.

Comment 6

[Daniel Veditz [:dveditz]]

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)

Comment 7

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/82d8d42717dfbd163ba475d5515bd48ff0ab3f53

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82d8d42717df

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 78.7esr.

Comment 10

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36e6fd83e81d

Comment 11

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/a9b12f3e794f

Comment 12

[BugBot [:suhaib / :marco]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c617ed604e36
Add test case for tdz check in optional chaining. r=yulia

Comment 14

[Sandor Molnar[:smolnar]]

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