1715318 - (CVE-2021-29982) Differential Testing: Different output during TypeError

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[lukas.bernhard]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

The following testcase produces different results on mozilla-central git commit 0e97d943bc746e4fd92902b21d3d5bebfef885a1 depending on whether ion is enabled or disabled.

function main() {
    let v1 = 0;
    function v18() {
        for (let v35 = 0; v35 < 100; v35++) {}

        try {
            async function* v37() {}
            class V47 extends v37 {}; 
            v1 += 1;
        }   
        catch (e) {
            print("caught: " + e); 
        }   
    };  
    v18();
    v18();  // second call may or may not reach catch handler, approx 50/50
    print(v1); // either 0 or 1
}
main();

Actual results:

Running the sample with ion disabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --baseline-warmup-threshold=10 --fuzzing-safe --differential-testing --no-ion diff.js) causes two exceptions to be thrown, v1 is 0 at the end of main().

If ion is enabled (obj-x86_64-pc-linux-gnu/dist/bin/js --no-threads --cpu-count=1 --ion-offthread-compile=off --baseline-warmup-threshold=10 --ion-warmup-threshold=100 --ion-check-range-analysis --ion-extra-checks --fuzzing-safe --differential-testing diff.js), the second execution of v18() may or may not (approx. 50/50) throw an exception. v1 is either 0 or 1 at the end of main().

Marking as security as a few differential execution bugs were flagged in the past.

Comment 1

[Jan de Mooij [:jandem]]

Sorry for the delay. I started looking into this today but had something urgent come up IRL.

The bug has to do with MacroAssembler::isCallableOrConstructor though, we call it with the same register for |obj| and |output| from Ion codegen and that's wrong because the function clobbers that register for the class check before we check the function flags (expecting the original value).

Great find, thanks for reporting this!

Comment 2

[Jan de Mooij [:jandem]]

Opening this up. We end up reading a single bit from one of the pointers in the function JSClass and interpreting that as "is constructor", so ASLR causes the differential behavior, but this isn't exploitable.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1715318 part 1 - Fix LCheckClassHeritage to not clobber the object register. r?iain!

isCallableOrConstructor first does a JSClass check which clobbers the output register.
If the JSClass is the JSFunction class, it then checks the function's is-constructor
flag. If we pass the same register for object and output, we end up reading a garbage
bit of the JSFunction JSClass and interpreting that as is-constructor.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1715318 part 2 - Use fallibleUnboxObject for LCheckClassHeritage. r?iain!

This is a bit simpler and faster.

Depends on D117634

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f9481bf5840
part 1 - Fix LCheckClassHeritage to not clobber the object register. r=iain
https://hg.mozilla.org/integration/autoland/rev/09af51138cf0
part 2 - Use fallibleUnboxObject for LCheckClassHeritage. r=iain

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7f9481bf5840
https://hg.mozilla.org/mozilla-central/rev/09af51138cf0

Comment 7

[Daniel Veditz [:dveditz]]

A one-bit leak is still a leak so we'll tag this as a low-severity security bug.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt