1845205 - Use-after-free crash in [@ MOZ_Z_inflateStateCheck] from PMCECompression::Inflate() from WebSocket

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/10d7a75e-0c8a-4dfd-859f-143e50230723

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libmozglue.so  MOZ_Z_inflateStateCheck  modules/zlib/src/inflate.c:113
0  libmozglue.so  MOZ_Z_inflate  modules/zlib/src/inflate.c:648
1  libxul.so  mozilla::net::PMCECompression::Inflate  netwerk/protocol/websocket/WebSocketChannel.cpp:881
2  libxul.so  mozilla::net::WebSocketChannel::ProcessInput  netwerk/protocol/websocket/WebSocketChannel.cpp:1643
3  libxul.so  mozilla::net::WebSocketChannel::OnInputStreamReady  netwerk/protocol/websocket/WebSocketChannel.cpp:4117
4  libxul.so  mozilla::net::RunOnThread::Run  netwerk/base/nsPreloadedStream.cpp:104
5  libxul.so  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1234
5  libxul.so  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:479
6  libxul.so  mozilla::net::nsSocketTransportService::Run  netwerk/base/nsSocketTransportService2.cpp:1198
7  libxul.so  {virtual override thunk}  netwerk/base/nsSocketTransportService2.cpp

There's a few different UAF crashes happening from PMCECompression::Inflate, in various zlib methods. I don't know if this is actionable or not. The crash volume is very low.

Other signatures:
[@ MOZ_Z_inflate_table ]: bp-aeb25610-cdf1-46d9-99e4-5fe190230718
[@ memcpy | MOZ_Z_updatewindow ] bp-84905787-4059-4cc9-8864-4af240230610
[@ MOZ_Z_updatewindow ] bp-53b7a96d-885f-4ac9-9975-518000230511
[@ MOZ_Z_inflate ] bp-2cf96e12-394d-4c0a-a9ef-14be50230601
[@ MOZ_Z_inflate_fast ] bp-bc9faf39-06ae-4753-ac17-4d2fd0230514

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

The e5e5 crashes seem to be mostly WebSocket (about 1/2 of the websocket crashes from these signatures have e5e5).

I'll note that there are 10-20x more crashes from these signatures that are not websocket, and most of those are some type of wildptr or illegal instruction (a few are nullptr). Only one in the last 6 months had an e5e5. This seems high for bitflips (and they don't smell like that), so there maybe some underlying issue with the inflate/etc code (or misuse of the API)

Let's restrict this bug to only WebSocket variants

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1845205: WebSocket cleanup r=kershaw

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Leave this open, as the patch likely won't solve all or many of these UAFs (I think it will help WebSocket BeginOpen UAFS)

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9346266 [details]
Bug 1845205: WebSocket cleanup r=kershaw

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very hard
• 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?: No
• If not, how different, hard to create, and risky will they be?: trivial, no risk
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions; worst regression would be some type of leak. Passes ./mach try auto.
• Is Android affected?: Yes

Comment 5

[Phabricator Automation]

Comment on attachment 9346266 [details]
Bug 1845205: WebSocket cleanup r=kershaw

Revision D184828 was moved to bug 1846080. Setting attachment 9346266 [details] to obsolete.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Moved the BeginOpen() patch to a new bug: bug 1846080

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Looking at https://crash-stats.mozilla.org/report/index/aeb25610-cdf1-46d9-99e4-5fe190230718

We're called from nsInputStreamReadyEvent::Run(), which holds an nsCOMPtr<nsIInputStreamCallback> to the WebSocketChannel. we call down through some layers, then call MOZ_Z_inflate(), and UAF in MOZ_Z_inflate_table. mInflator is part of class PMCECompression, which is a UniquePtr<PMCECompression> in WebSocketChannel:

// Set on MainThread in OnStartRequest (before mDataStarted), used on IO
// Thread (after mDataStarted), cleared in DoStopSession on IOThread or
// on MainThread (if mDataStarted == false)
(or Set in OnTransportAvailableInternal() or HandleExtensions(), both MainThread only). It's only used from the IOThread (socket thread)

So my assumption is that since we're crashing on SocketThread here, presumably with mDataStarted=true, that after we loaded the mPMCECompressor into a register, MainThread went and nulled out the mPCMECompressor (or replaced it) and deleted the allocation SocketThread was about to dereference. I'm not sure how we end up doing that (presumably DoStopSession happens on Main while we're processing data on SocketThread).

We could lock around mPMCECompressor access, perhaps using a single-writer mutex (only MainThread writes to mPMCECompressor)

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Pretty hard. Clear that we're adding a mutex, but the window to hit this is really hard.
• 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?: No
• If not, how different, hard to create, and risky will they be?: trivial, very little risk
• How likely is this patch to cause regressions; how much testing does it need?: Very unlikely. Nothing called under the new mutex would take another mutex or wait that I know of.
• Is Android affected?: Yes

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw

Approved to land and uplift

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d698aca1dec
Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw,necko-reviewers

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d698aca1dec

Comment 13

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

The patch landed in nightly and beta is affected.
:jesup, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox117 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw

Beta/Release Uplift Approval Request

• User impact if declined: Sec risk
• 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): Adds a mutex; no code within the mutexes waits so minimal risk of deadlock
• String changes made/needed: none
• Is Android affected?: Yes

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw

Approved for 117.0b4.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/8a57026acfa7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw

Approved for 115.2esr and 102.15esr.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/db5d25ac2f21

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/0e90259cee58

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/87cae5c8755e