1641211 - Crash in [@ nsTArray_Impl<T>::AppendElementInternal<T> | mozilla::dom::ContentParent::GetUsedBrowserProcess]

Status: VERIFIED FIXED

(Core :: DOM: Content Processes, defect, P1)

Description

[Simon Giesecke [:sg] [he/him]]

This bug is for crash report bp-64118ec8-115a-4698-bfc7-ff2ac0200527.

Top 10 frames of crashing thread:

0 xul.dll nsTArray_Impl<RefPtr<imgRequestProxy>, nsTArrayInfallibleAllocator>::AppendElementInternal<nsTArrayInfallibleAllocator, imgRequestProxy*&> xpcom/ds/nsTArray.h:2512
1 xul.dll static mozilla::dom::ContentParent::GetUsedBrowserProcess dom/ipc/ContentParent.cpp:823
2 xul.dll static mozilla::dom::ContentParent::GetNewOrUsedBrowserProcessInternal dom/ipc/ContentParent.cpp:892
3 xul.dll static mozilla::dom::ContentParent::GetNewOrUsedBrowserProcess dom/ipc/ContentParent.cpp:979
4 xul.dll static mozilla::dom::ContentParent::CreateBrowser dom/ipc/ContentParent.cpp:1221
5 xul.dll nsFrameLoader::TryRemoteBrowserInternal dom/base/nsFrameLoader.cpp:2579
6 xul.dll nsFrameLoader::TryRemoteBrowser dom/base/nsFrameLoader.cpp:2641
7 xul.dll nsFrameLoader::ShowRemoteFrame dom/base/nsFrameLoader.cpp:1018
8 xul.dll nsFrameLoader::Show dom/base/nsFrameLoader.cpp:889
9 xul.dll nsSubDocumentFrame::ShowViewer layout/generic/nsSubDocumentFrame.cpp:192

Reports started with Nightly build id 20200514094044 and then somewhat more consistently with build id 20200520213050. Most reports are from Windows, but one is from OS X as well (https://crash-stats.mozilla.org/report/index/800307b4-50af-4fea-b7e5-b99c30200515).

Comment 1

[Chris Peterson [:cpeterson]]

This looks like a UAF, so marking as a possible sec issue.

Assigning to kmag.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1641211: Part 1 - Add stronger checks for ContentParent pool membership consistency. r=nika

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1641211: Part 2 - Stop using raw pointers for ContentParent static members. r=nika

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 9152845 [details]
Bug 1641211: Part 1 - Add stronger checks for ContentParent pool membership consistency. r=nika

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. Assuming that this is a UAF, it should only ever happen at some unspecified point after a process has shut down and a new one is needed. This doesn't happen very often in practice outside of Fission, and the apparent UAF is even less common. Even in the rare instances that it does happen, crafting a targeted exploit would not be easy, if at all possible.
• 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?: It appears to exist in 77 onwards
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not very difficult, or very risky, but it would probably be better to wait until we have a proper fix, rather than just stricter assertions to narrow down the source of the problem.
• How likely is this patch to cause regressions; how much testing does it need?: Not likely. It simply adds stricter assertions which, if they fail, imply that a UAF would otherwise happen at some point down the road.

Comment 5

[Julien Cristau [:jcristau]]

Based on the crash volume in beta marking this as a blocker for 78.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 9152845 [details]
Bug 1641211: Part 1 - Add stronger checks for ContentParent pool membership consistency. r=nika

sec-approval+

Comment 7

[Julien Cristau [:jcristau]]

Kris, as we're entering the last week of beta what are our options for this bug? Is there something we can back out of beta to get rid of these crashes?

Comment 8

[Kris Maglione [:kmag]]

(In reply to Julien Cristau [:jcristau] from comment #7)

Kris, as we're entering the last week of beta what are our options for this bug? Is there something we can back out of beta to get rid of these crashes?

Not that I can think of. We still don't know what's causing this. The best candidate I can think of would be disabling async content process launch, but I don't really know if that would make a difference.

Comment 9

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/integration/autoland/rev/5b9a66cd196e4169b5d499322c98b196e13c946b
https://hg.mozilla.org/integration/autoland/rev/cc7f7f9597a853d0e16000bd1a4a6c5569012e7d

Comment 10

[Calixte Denizet (:calixte)]

There are 3 crashes in nightly 20200617215206 with signature mozilla::dom::ContentParent::AssertNotInPool and the moz_crash_reason is MOZ_RELEASE_ASSERT(!mIsInPool).

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/5b9a66cd196e
https://hg.mozilla.org/mozilla-central/rev/cc7f7f9597a8

Comment 12

[Chris Peterson [:cpeterson]]

I just hit this MOZ_RELEASE_ASSERT(!mIsInPool) twice in 79 Nightly build 2020-06-18 on Windows 10:

bp-a6be687b-3ded-4d39-9928-97d0e0200618
bp-63003951-c67a-44e1-945b-ba2e80200618

STR:

1. Open about:crashcontent with Fission enabled.
2. Quickly switch to a different tab before the "Gah. Your tab just crashed." crash reporter page is displayed.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1641211: Clear crashed processes from the e10s preallocator. r=jesup

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 9157787 [details]
Bug 1641211: Clear crashed processes from the e10s preallocator. r=jesup

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. This can only be triggered when a web content process crashes at a time when we have fewer than the maximum number of content processes and don't have an existing preallocated process. Even then, the exploit would need to happen in a content process and still have enough control over the parent process to manipulate the freed memory.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: 78
• If not all supported branches, which bug introduced the flaw?: Unknown
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not difficult and not risky.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It adds some diagnostic assertions, but they won't affect release builds, and even if they did, they would only trip in cases where they would otherwise lead to a UAF.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 9157787 [details]
Bug 1641211: Clear crashed processes from the e10s preallocator. r=jesup

sec-approval+

Comment 19

[Kris Maglione [:kmag]]

Attachment: Bug 1641211: Clear crashed processes from the e10s preallocator. r=jesup

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/630708946a69d288dd7f372ccf4d1429ef60ee10

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 9158306 [details]
Bug 1641211: Clear crashed processes from the e10s preallocator. r=jesup

Beta/Release Uplift Approval Request

• User impact if declined: This bug sometimes causes the parent process to crash shortly after a content process crashes. There may also be other UAF-related effects other than crashes.
• Is this code covered by automated tests?: No
• 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): It's a relatively simple change to remove a cached reference to a content process after it crashes so that we don't try to reuse it.
• String changes made/needed:

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9158306 [details]
Bug 1641211: Clear crashed processes from the e10s preallocator. r=jesup

Approved for 78.0rc1 (and esr78 by extension). Let's wait to resolve the bug until it's merged to m-c, though :-). Thanks for the patch!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/af00ea74f8fb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/630708946a69

Comment 25

[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 26

[Cristian Fogel [:cfogel]]

As per comment 12, reproduced instantly on Windows 10(1903) with 79.0a1(20200618212828).
Crash no longer encountered with 79.0a1 (2020-06-25), 78.0, 78.0esr(20200625154640).
Based on this, marking the bug as verified.