Closed Bug 1845205 Opened 1 years ago Closed 1 years ago

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

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
118 Branch
Tracking Status
firefox-esr102 117+ fixed
firefox-esr115 117+ fixed
firefox115 --- wontfix
firefox116 --- wontfix
firefox117 + fixed
firefox118 + fixed

People

(Reporter: mccr8, Assigned: jesup)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [necko-priority-queue][necko-triaged] [adv-main117+r] [adv-esr115.2+r] [adv-esr102.15+r])

Crash Data

Attachments

(1 file, 1 obsolete file)

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

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

Severity: -- → S2
Priority: -- → P2
Summary: Use-after-free crash in [@ MOZ_Z_inflateStateCheck] from PMCECompression::Inflate() → Use-after-free crash in [@ MOZ_Z_inflateStateCheck] from PMCECompression::Inflate() from WebSocket
Whiteboard: [necko-priority-queue][necko-triaged]
Assignee: nobody → rjesup
Status: NEW → ASSIGNED

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

Keywords: leave-open

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
Attachment #9346266 - Flags: sec-approval?

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

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

Attachment #9346266 - Attachment is obsolete: true
Attachment #9346266 - Flags: sec-approval?
Keywords: leave-open

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

See Also: → 1846080

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 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
Attachment #9346335 - Flags: sec-approval?

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

Approved to land and uplift

Attachment #9346335 - Flags: sec-approval? → sec-approval+
Pushed by rjesup@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d698aca1dec Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw,necko-reviewers
Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 years ago
Resolution: --- → FIXED
Target Milestone: --- → 118 Branch

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.

Flags: needinfo?(rjesup)

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
Flags: needinfo?(rjesup)
Attachment #9346335 - Flags: approval-mozilla-beta?

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

Approved for 117.0b4.

Attachment #9346335 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1847298
QA Whiteboard: [post-critsmash-triage]

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

Approved for 115.2esr and 102.15esr.

Attachment #9346335 - Flags: approval-mozilla-esr115+
Attachment #9346335 - Flags: approval-mozilla-esr102+
Whiteboard: [necko-priority-queue][necko-triaged] → [necko-priority-queue][necko-triaged] [adv-main117+r] [adv-esr115.2+r] [adv-esr102.15+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: