Closed Bug 1544792 Opened 2 years ago Closed 2 years ago

Spidermonkey: definite properties are incorrectly computed in some cases, leading to uninitialized memory access when unboxed objects are enabled

Categories

(Core :: JavaScript Engine, defect, P1)

66 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- disabled
firefox66 --- disabled
firefox67 + fixed
firefox68 + fixed

People

(Reporter: saelo, Assigned: jandem)

References

()

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [disclosure deadline July 16, 2019])

Attachments

(2 files)

For constructors, Spidermonkey implements a "definite property analysis" [1] to compute which properties will definitely exist on the constructed objects. Spidermonkey then directly allocates the constructed objects with the final Shape. As such, at the entrypoint of the constructor the constructed objects will already "look like" they have all the properties that are only installed throughout the constructor. This mechanism e.g. makes it possible to omit some Shape updates in JITed code. See also https://bugs.chromium.org/p/project-zero/issues/detail?id=1791 for another short explanation of this mechanism.

The definite property analysis must ensure that "predefining" the properties in such a way will not be visible to the running script. In particular, it can only mark properties as definite if they aren't read or otherwise accessed before the assignment.

In the following JavaScript program, discovered through fuzzing and then manually modified, Spidermonkey appears to incorrectly handle such a scenario:

l = undefined;

function v10() {
    let v15 = 0;
    try {
        const v16 = v15.foobar();
    } catch(v17) {
        l = this.uninitialized;
    }
    this.uninitialized = 1337;
}

for (let v36 = 0; v36 < 100; v36++) {
    const v38 = new v10();
    if (l !== undefined) {
        console.log("Success: 0x" + l.toString(16));
        break;
    }
}

When run on a local Spidermonkey built from the beta branch or in Firefox 66.0.3 with javascript.options.unboxed_objects set to true in about:config, it will eventually output something like:

Success: 0x2d2d2d2d

Here, the definite property analysis concluded that .uninitialized is definitely assigned to the constructed objects and not accessed before it is assigned (which is wrong). In particular, it seems that the catch block is entirely ignored by the analysis as it is not present in the Ion graph representation of v10 on which the analysis is performed. As such, when reading .uninitialized in the catch block, uninitialized memory (which seems to be initialized with 0x2d in debug builds) is read from this and later printed to stdout. If the line this.uninitialized = 1337; is modified to instead assign a double value (e.g. this.uninitialized = 13.37;), then an assertion failure can be observed:

Assertion failure: isDouble(), at js/src/build_DBG.OBJ/dist/include/js/Value.h:450

As unboxed properties can also store JSObject pointers, this bug can likely be turned into memory corruption as well. However, since this requires unboxed object, which have recently been disabled by default and appear to be fully removed soon, it likely only affects non-standard configurations of FireFox. If unboxed objects are disabled (e.g. through --no-unboxed-objects), then the analysis will still be incorrect and determine that .uninitialized can be "predefined". This can be observed by changing l = this.uninitialized; to l = this.hasOwnProperty('uninitialized'); which will incorrectly return true. In that case, the property slots seem to be initialized with undefined though, so no memory safety violation occurs. However, I have not verified that they will always be initialized in that way. Furthermore, it might be possible to confuse property type inference in that case, but I have also not attempted that.

Please note: this bug is subject to a 90 day disclosure deadline. After 90 days elapse or a patch has been made broadly available (whichever is earlier), the bug report will become visible to the public.

With any fix, please give credit for identifying the vulnerability to Samuel Groß of Google Project Zero.

[1] https://github.com/mozilla/gecko-dev/blob/83bea62461d1e30aebef5077462fafb256368e9e/js/src/jit/IonAnalysis.cpp#L4511

Group: firefox-core-security → javascript-core-security
Component: Security → JavaScript Engine
Product: Firefox → Core

I've attached the original sample triggered by my fuzzer for completeness. It ended up reading the property by spreading this.

Unboxed objects have been removed in bug 1505574, which landed in Firefox 68, and are off by default in other branches. Is there something we should do here, Jan?

Flags: needinfo?(jdemooij)
Status: UNCONFIRMED → NEW
Ever confirmed: true

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

Unboxed objects have been removed in bug 1505574, which landed in Firefox 68, and are off by default in other branches. Is there something we should do here, Jan?

We definitely need to fix the analysis. Without unboxed objects that might be a non-security-sensitive correctness bug, as Samuel noted. I'll look into it more.

Whiteboard: [disclosure deadline July 16, 2019]

Assigning to Jan since we're waiting for his analysis

Assignee: nobody → jdemooij

Calling this "sec-moderate" for old versions because you had to flip a hidden pref to be vulnerable.

Ion does not compile the catch block so the analysis fails to account for code
there.

I think sec-moderate makes sense here.

Flags: needinfo?(jdemooij)

Thanks for the great bug report btw. Nice find.

Status: NEW → ASSIGNED

(In reply to Jan de Mooij [:jandem] from comment #8)

Thanks for the great bug report btw. Nice find.

Thank you!

Priority: -- → P1

Comment on attachment 9061864 [details]
Bug 1544792 - Abort on try-catch blocks when doing definite properties analysis. r?nbp!

Beta/Release Uplift Approval Request

  • User impact if declined: Correctness issues. Patch is so simple that I think it's worth taking.
  • 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): Pretty trivial patch to disable an optimization if the function has try-catch blocks.
  • String changes made/needed: None
Attachment #9061864 - Flags: approval-mozilla-beta?
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment on attachment 9061864 [details]
Bug 1544792 - Abort on try-catch blocks when doing definite properties analysis. r?nbp!

Uplift approved for the beta branch, thanks.

Attachment #9061864 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I just want to double check since the deadline for this issue is approaching: is this issue is fully fixed by now?

(In reply to Samuel Groß from comment #15)

I just want to double check since the deadline for this issue is approaching: is this issue is fully fixed by now?

Yes the patch that landed in 67 and 68 fixes the try-catch + definite-properties-analysis issue reported here. As far as we know this was a correctness issue and not exploitable after we disabled unboxed objects on all branches.

(In reply to Jan de Mooij [:jandem] from comment #16)

(In reply to Samuel Groß from comment #15)

I just want to double check since the deadline for this issue is approaching: is this issue is fully fixed by now?

Yes the patch that landed in 67 and 68 fixes the try-catch + definite-properties-analysis issue reported here. As far as we know this was a correctness issue and not exploitable after we disabled unboxed objects on all branches.

Great, thanks!

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