1675868 - Crash in [@ mozilla::detail::SupportCheckedUnsafePtrImpl<T>::~SupportCheckedUnsafePtrImpl | mozilla::dom::MessagePortParent::~MessagePortParent]

Status: RESOLVED FIXED

(Core :: DOM: postMessage, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/f5eae3b9-69d1-433e-9745-3d0170201106

MOZ_CRASH Reason: MOZ_CRASH(Found dangling CheckedUnsafePtr)

Top 10 frames of crashing thread:

0 XUL mozilla::detail::SupportCheckedUnsafePtrImpl<mozilla::CrashOnDanglingCheckedUnsafePtr,  dom/quota/CheckedUnsafePtr.h:287
1 XUL mozilla::dom::MessagePortParent::~MessagePortParent dom/messagechannel/MessagePortParent.cpp:26
2 XUL mozilla::ipc::BackgroundParentImpl::DeallocPMessagePortParent ipc/glue/BackgroundParentImpl.cpp:1045
3 XUL mozilla::ipc::ActorLifecycleProxy::~ActorLifecycleProxy ipc/glue/ProtocolUtils.cpp:259
4 XUL mozilla::ipc::PBackgroundParent::ClearSubtree ipc/ipdl/PBackgroundParent.cpp:6829
5 XUL mozilla::ipc::PBackgroundParent::OnChannelError ipc/ipdl/PBackgroundParent.cpp:6494
6 XUL mozilla::detail::RunnableMethodImpl<mozilla::ipc::MessageChannel*, void  xpcom/threads/nsThreadUtils.h:1240
7 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1197
8 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:302
9 XUL MessageLoop::Run ipc/chromium/src/base/message_loop.cc:309

Comment 1

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

The severity field is not set for this bug.
:jstutte, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

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

This diagnostic check was added by Bug 1597481, but AFAIU it didn't undertake an attempt to fix the underlying issue. It is likely crashes of a non-diagnostic build in MessagePortParent::Close happen under the same circumstances, and we still get a low number of reports with that signature.

Jason, is the test case from the fuzzer from Bug 1597481 really fixed?

Comment 3

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

This also affects esr78. The volume is low so this doesn't show in the current stats, but there was, e.g., https://crash-stats.mozilla.org/report/index/82a6107f-b4bc-4565-8269-6a01d0200908

Comment 4

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

Ah, Bug 1597481 doesn't have a reproducer testcase attached at all, it says it needs to be reduced. Jason, is it possible to get a reproducer?

Comment 5

[Jason Kratzer [:jkratzer]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

Ah, Bug 1597481 doesn't have a reproducer testcase attached at all, it says it needs to be reduced. Jason, is it possible to get a reproducer?

Unfortunately not. Any testcases we had associated with bug 1597481 were automatically cleaned up once that bug was fixed.

Comment 6

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

Adding another signature, which is probably related.

Comment 7

[Daniel Veditz [:dveditz]]

Although the MOZ_CRASH ones are safe enough, the reason is scary and the fact that [@ mozilla::dom::MessagePortParent::Close ] signatures are all UAF write crashes backs that up.

Comment 8

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

(In reply to Daniel Veditz [:dveditz] from comment #7)

Although the MOZ_CRASH ones are safe enough, the reason is scary and the fact that [@ mozilla::dom::MessagePortParent::Close ] signatures are all UAF write crashes backs that up.

Yes, and the MOZ_CRASH ones are only in diagnostic builds. Release doesn't have these safety checks.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

From brief discussion at our team meeting right now, the general plan is:

• Simon and I will both try and look at the crashing stack a bit more to see if we can infer what might be happening. In particular, I'm thinking it's like this involves a MessagePort being transferred between threads as hypothesized at https://bugzilla.mozilla.org/show_bug.cgi?id=1577430#c2
• After understanding the crashes as best we can, try and provide guidance to the fuzzing team as to what might be necessary to get the fuzzers to reproduce. Right now my best guesses would be a MessagePort being transferred between at least 2 threads, potentially with messages in the channel that need to be re-transmitted back to the parent, and forcing the termination of the content process while that is happening. The order in which the PBackground channels are torn down in channel error might matter, so it might be necessary to try the MessagePort transferring in 2 directions. (Ex: page to worker to nested worker as the first path, and then nested worker to first-level worker to page. Dedicated workers are likely the best mechanism to get multiple different PBackground channels involved and to make sure that the content process manages to get ahead of the parent process.)

Comment 10

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

Attachment: Bug 1675868 - Store weak pointer to next parents. r=asuth

Comment 11

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

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #9)

From brief discussion at our team meeting right now, the general plan is:

• Simon and I will both try and look at the crashing stack a bit more to see if we can infer what might be happening. In particular, I'm thinking it's like this involves a MessagePort being transferred between threads as hypothesized at https://bugzilla.mozilla.org/show_bug.cgi?id=1577430#c2

While I haven't figured out how this happens, the attached patch should fix the issue by using a WeakPtr that is checked before dereferencing. If we understand all conditions that may lead to this better, we could revert this to a more efficient version again that doesn't need a WeakReference.

Comment 12

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

Comment on attachment 9193006 [details]
Bug 1675868 - Store weak pointer to next parents. r=asuth

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not sure, we don't have a way to reproduce this, and it seems to happen only in response to a channel error based on the existing reports. Since the patch introduces a WeakPtr, it might be deduced that this is in some way related to a UAF issue.
• 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?: I think the patch should apply cleanly to all supported branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It uses a safer WeakPtr rather than a CheckedUnsafePtr (that decays to a raw pointer in non-diagnostics build).

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9193006 [details]
Bug 1675868 - Store weak pointer to next parents. r=asuth

Approved to land. Because this is a speculative fix, should we uplift it? Maybe uplift it after the holidays?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/10d80a597cc85bf225393d44287017c653191f02

Our next Beta is coming in January anyway, so waiting until after the holidays to uplift sounds reasonable to me.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/10d80a597cc8

Comment 16

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

[Julien Cristau [:jcristau]]

Comment on attachment 9193006 [details]
Bug 1675868 - Store weak pointer to next parents. r=asuth

approved for 85.0b5

Comment 18

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74f373917103

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9193006 [details]
Bug 1675868 - Store weak pointer to next parents. r=asuth

No crashes on Beta since this was uplifted. Approved for 78.7esr.

Comment 20

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/da49ca43ea30

Comment 21

[Julien Cristau [:jcristau]]

Bustage fix:
https://hg.mozilla.org/releases/mozilla-esr78/rev/0e2cc6dc233cc74d5acbf1d3b55537a722608ce3
https://hg.mozilla.org/releases/mozilla-esr78/rev/7d9f093e6e002a72a64db8d966cd5a2aabbe68a6

Comment 22

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

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #16)

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.

I responded.