1760944 - (CVE-2022-31745) Disabling shift optimization asserts in js::jit::AssertValidObjectPtr

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[lukas.bernhard]

Steps to reproduce:

In a non-vanilla version of spidermonkey, I discovered an assertion violation. This bug is filed as the modification to spidermonkey is probably not the root-cause but instead exposes an underlying issue affecting the vanilla engine as well.

When disabling the array shift optimizations in NativeObject::{tryShiftDenseElements,tryUnshiftDenseElements} by letting these two function immediately return false, one of the jit-tests (js/src/jit-test/tests/basic/shifted-elements6.js) crashes in js::jit::AssertValidObjectPtr.
The crash happens in a small percentage of executions only, hence reproducing the issue might require some repetition.

As @iain pointed out: reproducing the issue in rr is possible if the rr flag --chaos is used.

Comment 1

[Iain Ireland [:iain]]

Looked into this. I've worked out what's happening, but somebody who knows the GC better than I do will have to figure out the fix.

We fill an array with objects. In a loop, we call shift on the array to remove the objects one by one.
At some point in this loop:

• we start a new major gc
• we pop the array from the mark stack and start marking its children
• with a single child left to mark, our budget runs out
• we push a range (start: 146, end: 147) onto the mark stack
• we return to scripted code and call shift again
• in moveDenseElements, we shift everything over by one (emitting pre-barriers on all elements except the last)
• we decrement the initialized length by 1, to 146
• we call GCSlice
• we pop the range off the mark stack
• there are no elements after 146 left to mark, so we skip the range
• the last element is never marked.
• eventually we finalize and poison the last element
• later on we try accessing the poisoned element and crash.

This normally doesn't happen because the shifted elements optimization kicks in instead.

I think the bug is probably in moveDenseElements?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #1)
Thanks for the analysis.

• in moveDenseElements, we shift everything over by one (emitting pre-barriers on all elements except the last)

This is a classic incremental marking bug. We move a pointer behind the marking wavefront but don't trigger a barrier on the previous location.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #1)

• we decrement the initialized length by 1, to 146

Looking at the code, this should trigger the required barrier - NativeObject::setDenseInitializedLengthInternal calls prepareElementRangeForOverwrite. I wonder what's going wrong?

Comment 4

[Iain Ireland [:iain]]

Oh, I might know the answer to that. In this case, the dense initialized length is only being updated after we return to jit code.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Iain Ireland [:iain] from comment #4)
Aha, that looks like the problem.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1760944 - Update array length in ArrayShiftMoveElements. r?iain!,jonco!

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9269223 [details]
Bug 1760944 - Update array length in ArrayShiftMoveElements. r?iain!,jonco!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This fixes some code that was incorrect but this was hidden by a different code path. I think this issue isn't exploitable at the moment.
• 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
• 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?: Should apply
• How likely is this patch to cause regressions; how much testing does it need?: Low risk

Comment 8

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:jandem, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 9

[Jan de Mooij [:jandem]]

The buggy code path is only reachable if initializedLength == 1 and that case is barriered in JIT code, so I think this is a sec-moderate rather than a sec-high.

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9269223 [details]
Bug 1760944 - Update array length in ArrayShiftMoveElements. r?iain!,jonco!

sec-approval not needed for moderate

Comment 11

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/5424da694d9274f37d8a6c9777fa9e29bf98389e

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5424da694d92

Comment 13

[Jan de Mooij [:jandem]]

This can ride the trains.

Comment 14

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Really not sure how to write this one up....