1684020 - (CVE-2021-23954) 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

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

switch (0) {
    case e &&= x:
    case x % 0:
        let x;
}
var e;

Run with --fuzzing-safe --no-threads --no-baseline --no-ion:

Assertion failure: next == JSOp::CheckThis || next == JSOp::CheckReturn || next == JSOp::CheckThisReinit || next == JSOp::CheckLexical, at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:3715

Thread 1 "js-dbg-64-linux" received signal SIGSEGV, Segmentation fault.
Interpret (cx=<optimized out>, cx@entry=0x7ffff6924000, state=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:3714
3714	        MOZ_ASSERT(next == JSOp::CheckThis || next == JSOp::CheckReturn ||
(gdb) bt
#0  Interpret (cx=<optimized out>, cx@entry=0x7ffff6924000, state=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:3714
#1  0x0000555556c1d398 in js::RunScript (cx=cx@entry=0x7ffff6924000, state=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:473
#2  0x0000555556c33173 in js::ExecuteKernel (cx=cx@entry=0x7ffff6924000, script=..., script@entry=..., envChainArg=envChainArg@entry=..., 
    newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:839
#3  0x0000555556c335e0 in js::Execute (cx=cx@entry=0x7ffff6924000, script=..., envChain=..., rval=..., rval@entry=...)
    at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:871
#4  0x0000555556de07e2 in ExecuteScript (cx=cx@entry=0x7ffff6924000, envChain=..., script=..., rval=rval@entry=...)
    at /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:424
#5  0x0000555556de09c2 in JS_ExecuteScript (cx=cx@entry=0x7ffff6924000, scriptArg=scriptArg@entry=...)
    at /home/skygentoo/trees/mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:457
#6  0x0000555556b7c147 in RunFile (cx=cx@entry=0x7ffff6924000, filename=0x5b71ff8cccd27 <error: Cannot access memory at address 0x5b71ff8cccd27>, 
    filename@entry=0x7ffff7756020 "\230$\255\373\344\344\344", <incomplete sequence \344>, file=<optimized out>, file@entry=0x7ffff7756020, 
    compileMethod=<optimized out>, compileMethod@entry=CompileUtf8::DontInflate, compileOnly=false)
    at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:982
#7  0x0000555556b7b829 in Process (cx=cx@entry=0x7ffff6924000, filename=<optimized out>, forceTTY=false, kind=kind@entry=FileScript)
    at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:1573
#8  0x0000555556b44701 in ProcessArgs (cx=0x7ffff6924000, op=0x7fffffffd7e0) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:10378
#9  Shell (cx=0x7ffff6924000, op=<optimized out>, op@entry=0x7fffffffd7e0, envp=<optimized out>)
    at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11119
#10 0x0000555556b3d92e in main (argc=6, argv=<optimized out>, envp=<optimized out>) at /home/skygentoo/trees/mozilla-central/js/src/shell/js.cpp:11918
(gdb)

Run with --fuzzing-safe --no-threads --no-baseline --no-ion --blinterp-warmup-threshold=0:

Assertion failure: v.isSymbol() || v.isBigInt(), at /home/skygentoo/trees/mozilla-central/js/src/jsnum.cpp:1944

Thread 1 "js-dbg-64-linux" received signal SIGSEGV, Segmentation fault.
js::ToNumberSlow (cx=<optimized out>, cx@entry=0x7ffff6924000, v_=..., out=out@entry=0x7fffffffb670)
    at /home/skygentoo/trees/mozilla-central/js/src/jsnum.cpp:1944
1944	  MOZ_ASSERT(v.isSymbol() || v.isBigInt());
(gdb) bt
#0  js::ToNumberSlow (cx=<optimized out>, cx@entry=0x7ffff6924000, v_=..., out=out@entry=0x7fffffffb670)
    at /home/skygentoo/trees/mozilla-central/js/src/jsnum.cpp:1944
#1  0x0000555556d337c7 in js::ToNumber (cx=0x7ffff6924000, vp=...) at /home/skygentoo/trees/mozilla-central/js/src/jsnum.h:220
#2  js::ToNumericSlow (cx=cx@entry=0x7ffff6924000, vp=vp@entry=...) at /home/skygentoo/trees/mozilla-central/js/src/jsnum.cpp:1975
#3  0x0000555556c37cbb in js::ToNumeric (cx=0x7ffff6924000, vp=...) at /home/skygentoo/trees/mozilla-central/js/src/jsnum.h:236
#4  ModOperation (cx=cx@entry=0x7ffff6924000, lhs=lhs@entry=..., rhs=rhs@entry=..., res=...)
    at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:1540
#5  0x0000555556c37c17 in js::ModValues (cx=0x7ffff7bb19a0 <_IO_stdfile_2_lock>, cx@entry=0x7ffff6924000, lhs=..., lhs@entry=..., rhs=..., rhs@entry=..., 
    res=..., res@entry=...) at /home/skygentoo/trees/mozilla-central/js/src/vm/Interpreter.cpp:4833
#6  0x0000555557633d7d in js::jit::DoBinaryArithFallback (cx=0x7ffff6924000, frame=0x7fffffffbaa0, stub=0x7ffff69fa110, lhs=..., rhs=..., ret=...)
    at /home/skygentoo/trees/mozilla-central/js/src/jit/BaselineIC.cpp:2465
#7  0x00000bfd04b2ed73 in ?? ()
#8  0x00007fffffffbae8 in ?? ()
#9  0x00007fffffffba38 in ?? ()
#10 0x00007fffffffbae8 in ?? ()
#11 0xfff9800000000000 in ?? ()
#12 0x0000555558121c50 in js::jit::vmFunctions ()
#13 0x00000bfd04b38e39 in ?? ()
#14 0x0000000000009821 in ?? ()
#15 0x00007fffffffbaa0 in ?? ()
#16 0x00007ffff69fa110 in ?? ()
#17 0xfffa80000000000b in ?? ()
#18 0xfff8800000000000 in ?? ()
#19 0xfff8800000000000 in ?? ()
#20 0xfffa80000000000b in ?? ()
#21 0xfff8800000000000 in ?? ()
#22 0xfff8800000000000 in ?? ()
#23 0xfffa80000000000b in ?? ()
#24 0x00001f1f7d59b060 in ?? ()
#25 0x00007ffff695ba72 in ?? ()
#26 0x00007ffff67f36e8 in ?? ()
#27 0x00001f1f7d579040 in ?? ()
#28 0x00007ffff67f3678 in ?? ()
#29 0x00007ffff6924000 in ?? ()
#30 0x00007fffffffbb00 in ?? ()
#31 0x0000007800000002 in ?? ()
#32 0x0000000000000000 in ?? ()
(gdb)

Run with the respective flags above, compile with AR=ar sh ./configure --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests, tested on m-c rev f725a528bb4c.

Not sure if this is s-s yet.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/17566bf1e9cd
user:        André Bargull
date:        Wed Apr 15 09:15:22 2020 +0000
summary:     Bug 1629106 - Part 1: Implement Logical Assignment Operators proposal. r=yulia

Andre, this is probably yours?

Comment 2

[André Bargull [:anba]]

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

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1684020: Add test case for tdz check in short-circuit assignment. r=yulia!

Depends on D100787

Comment 4

[Andrew McCreight [:mccr8]]

This needs a security rating before landing. Is this a security issue? What does the fix do? What does the failure of the assertion in comment 0 mean and what might go wrong in a non-debug build? Thanks.

Comment 5

[André Bargull [:anba]]

This bug escapes the JS_UNINITIALIZED_LEXICAL magic value to user code. Other bugs (bug 1254164 and bug 1612636) which let magic values escape to user code were rated sec-high, so I guess this also applies here. There can be various effects, for example:

{ var True = true; True ||= b; +b; let b; }

simply throws TypeError: can't convert symbol to number, but

{ var True = true; True ||= b; !!b; let b; }

will always crash in ToBooleanSlow because we try to interpret the MagicValue as an ObjectValue.

The fix ensures we use a new TDZ (temporal dead zone) cache when emitting byte code for short-circuit assignments (||=, &&=, and ??=). This is necessary because the short-circuit nature of these assignment expressions doesn't guarantee that the right-hand side will always be evaluated. So for any following expressions we can't omit uninitialised lexical binding checks for lexical bindings which are present on the right-hand side of the short-circuit assignment.

b; // Needs to emit CheckLexical, because `b` may not be initialised.
b; // We can omit CheckLexical, because a preceding expression is guaranteed to have executed CheckLexical.
let b;

a ||= b; // Emits CheckLexical for `b`, but only executes it when `a` is false.
b; // The preceding CheckLexical may not have been executed, so CheckLexical is needed here, too.
let b;

Comment 6

[Andrew McCreight [:mccr8]]

Thanks for the explanation. It sounds like some kind of type confusion for a fixed small constant which doesn't sound super dangerous necessarily, but I'll conservatively mark it sec-high because we have lots of different JS values.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #6)

It sounds like some kind of type confusion for a fixed small constant which doesn't sound super dangerous necessarily,

That's what I was thinking as well. Any dereference (object, string, BigInt) will be a guaranteed crash and all cases reported in this bug seem harmless AFAICT. Because Values are used in so many places it's hard to say anything with 100% certainty though.

Comment 8

[André Bargull [:anba]]

Comment on attachment 9195420 [details]
Bug 1684020: 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 9

[Julien Cristau [:jcristau]]

Please request sec-approval on this patch.

Comment 10

[André Bargull [:anba]]

Comment on attachment 9195420 [details]
Bug 1684020: 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 short-circuit assignment) 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 (77+)
• 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 #8.

Comment 11

[Daniel Veditz [:dveditz]]

Comment 7 makes this sound more like sec-moderate

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 9195420 [details]
Bug 1684020: Add TDZCheckCache. r=yulia!

sec-approval+, a=dveditz for beta. I'll leave ESR for now because I don't know if we have a preferred landing period on ESR these days (though I think we should take this safe fix).

Comment 13

[Daniel Veditz [:dveditz]]

Don't check in the testcase yet, though. Setting the in-testsuite? flag as a reminder to land later

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/fb3c410ea9732eaa74444b7f0b13cdef31f7b582

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9195420 [details]
Bug 1684020: Add TDZCheckCache. r=yulia!

Approved for 78.7esr.

Comment 17

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3284497e16ef

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/4b86e0a9ac95

Comment 19

[Daniel Veditz [:dveditz]]

The bounty committee thought my sec-moderate was too optimistic and wants to err on the side of pessimism like earlier bugs.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

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 21

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 22

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5c2bada6c51
Add test case for tdz check in short-circuit assignment. r=yulia

Comment 24

[Sandor Molnar[:smolnar]]

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