1750688 - Crash in [@ mozilla::net::WebSocketChannel::DoStopSession ]

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect, P1)

Description

[Kershaw Chang [:kershaw]]

This is from this crash report.
I hit this UAF crash during my daily use with socket process.

0 	XUL 	mozilla::net::WebSocketChannel::DoStopSession(nsresult) 	netwerk/protocol/websocket/WebSocketChannel.cpp:2375 	context
1 	XUL 	mozilla::net::WebSocketConnectionParent::RecvOnError(nsresult const&) 	netwerk/protocol/websocket/WebSocketConnectionParent.cpp:58 	cfi
2 	XUL 	mozilla::net::PWebSocketConnectionParent::OnMessageReceived(IPC::Message const&) 	ipc/ipdl/PWebSocketConnectionParent.cpp:258 	cfi
3 	XUL 	mozilla::ipc::PBackgroundParent::OnMessageReceived(IPC::Message const&) 	ipc/ipdl/PBackgroundParent.cpp:3418 	cfi
4 	XUL 	mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) 	ipc/glue/MessageChannel.cpp:1964 	cfi
5 	XUL 	mozilla::ipc::MessageChannel::MessageTask::Run() 	ipc/glue/MessageChannel.cpp:1851 	cfi
6 	XUL 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1189 	cfi
7 	XUL 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:330 	cfi
8 	XUL 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:306 	cfi

Comment 1

[Kershaw Chang [:kershaw]]

Obviously, there is a race at WebSocketChannel::mCancelable. I assume this crash happens because mCancelable is released on main thread at here and it gets accessed on background thread at here.

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Bug 1750688 - Use a lock to protect WebSocketChannel::mCancelable, r=#necko

Comment 3

[Kershaw Chang [:kershaw]]

Comment on attachment 9259478 [details]
Bug 1750688 - Use a lock to protect WebSocketChannel::mCancelable, r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Unlikely, since this is based on a race condition.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• 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?: This patch should be able to backport cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Low risk, since this patch only uses a lock to fix the race.

Comment 4

[Kershaw Chang [:kershaw]]

Note that this is a UAF, so I assume this is sec-high.

Comment 5

[Daniel Veditz [:dveditz]]

kershaw: if this is in the socket process is it fair to call older branches "disabled" rather than "affected"? That might affect what branches we need to fix this on. Are we shipping this by default soon? I'll call current branches "affected" for now.

Comment 6

[Kershaw Chang [:kershaw]]

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

kershaw: if this is in the socket process is it fair to call older branches "disabled" rather than "affected"? That might affect what branches we need to fix this on. Are we shipping this by default soon? I'll call current branches "affected" for now.

I think this is a longstanding issue, since this code has been there for years. It's just that enabling socket process makes this crash happen easily.

Note that we will not shipping socket process in the near future. Probably in 2022 H2.

Comment 7

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

Comment on attachment 9259478 [details]
Bug 1750688 - Use a lock to protect WebSocketChannel::mCancelable, r=#necko

Approved to land and request uplift

Comment 8

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

Use a lock to protect WebSocketChannel::mCancelable, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/228c43530cc1b0f7d9a7533e3ea5bbcf891303c9
https://hg.mozilla.org/mozilla-central/rev/228c43530cc1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

I see crash reports from Beta, Release, and ESR which are also attributed to this bug. Are those the same root cause or something else?

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

I see crash reports from Beta, Release, and ESR which are also attributed to this bug. Are those the same root cause or something else?

No, they are different issues.
I'll file a bug for this.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please still nominate this for Beta approval :)

Comment 12

[Kershaw Chang [:kershaw]]

Comment on attachment 9259478 [details]
Bug 1750688 - Use a lock to protect WebSocketChannel::mCancelable, r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Could crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: N/A
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch is straightforward, i.e., it only adds a lock to avoid a race.
• String changes made/needed: N/A

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9259478 [details]
Bug 1750688 - Use a lock to protect WebSocketChannel::mCancelable, r=#necko

Approved for 97.0b9.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/29b3edf0fb8c

Comment 15

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

[Kershaw Chang [:kershaw]]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #15)

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.

Done.