Use-after-free crash in [@ MOZ_Z_inflateStateCheck] from PMCECompression::Inflate() from WebSocket
Categories
(Core :: Networking: WebSockets, defect, P2)
Tracking
()
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)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr102+
RyanVM
:
approval-mozilla-esr115+
tjr
:
sec-approval+
|
Details | Review |
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
Assignee | ||
Comment 1•1 years ago
|
||
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
Assignee | ||
Comment 2•1 years ago
|
||
Updated•1 years ago
|
Assignee | ||
Comment 3•1 years ago
|
||
Leave this open, as the patch likely won't solve all or many of these UAFs (I think it will help WebSocket BeginOpen UAFS)
Assignee | ||
Comment 4•1 years ago
|
||
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•1 years ago
|
||
Comment on attachment 9346266 [details]
Bug 1845205: WebSocket cleanup r=kershaw
Revision D184828 was moved to bug 1846080. Setting attachment 9346266 [details] to obsolete.
Assignee | ||
Updated•1 years ago
|
Assignee | ||
Comment 6•1 years ago
|
||
Moved the BeginOpen() patch to a new bug: bug 1846080
Assignee | ||
Comment 7•1 years ago
|
||
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)
Assignee | ||
Comment 8•1 years ago
|
||
Updated•1 years ago
|
Assignee | ||
Comment 9•1 years ago
|
||
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•1 years ago
|
||
Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw
Approved to land and uplift
Comment 11•1 years ago
|
||
Comment 12•1 years ago
|
||
Comment 13•1 years ago
|
||
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
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 14•1 years ago
|
||
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•1 years ago
|
||
Comment on attachment 9346335 [details]
Bug 1845205: Add locking around access to WebSocketChannel::mPMCECompressor r=kershaw
Approved for 117.0b4.
Comment 16•1 years ago
|
||
uplift |
Updated•1 years ago
|
Updated•1 year ago
|
Comment 17•1 year ago
|
||
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•1 year ago
|
||
uplift |
Updated•1 year ago
|
Comment 19•1 year ago
|
||
uplift |
Updated•1 year ago
|
Comment 20•1 year ago
|
||
uplift |
Updated•1 year ago
|
Updated•1 year ago
|
Description
•