Closed Bug 1794061 Opened 3 years ago Closed 3 years ago

Crash in [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait]

Categories

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

Product:
Core
Component:
Networking: HTTP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
108 Branch
Tracking Flags:
Tracking Status
firefox-esr102 107+ fixed
firefox105 --- wontfix
firefox106 --- wontfix
firefox107 + fixed
firefox108 + fixed

People

(Reporter: mccr8, Assigned: kershaw)

References

Details

(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [necko-triaged][post-critsmash-triage][adv-main107+r][adv-esr102.5+r])

Crash Data

Attachments

(1 file)

Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko
48 bytes, text/x-phabricator-request
dmeehan
: approval-mozilla-beta+
pascalc
: approval-mozilla-release-
dmeehan
: approval-mozilla-esr102+
tjr
: sec-approval+
Details | Review

Crash report: https://crash-stats.mozilla.org/report/index/10c8bbcc-bcb9-4444-9f3f-289540221006

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 XUL nsCOMPtr_base::assign_assuming_AddRef xpcom/base/nsCOMPtr.h:372
0 XUL nsCOMPtr_base::assign_with_AddRef xpcom/base/nsCOMPtr.cpp:40
1 XUL nsCOMPtr<nsIOutputStreamCallback>::operator= xpcom/base/nsCOMPtr.h:690
1 XUL mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait netwerk/protocol/http/TLSTransportLayer.cpp:270
2 XUL mozilla::net::TLSTransportLayer::Close netwerk/protocol/http/TLSTransportLayer.cpp:457
3 XUL mozilla::net::WebSocketChannel::CleanupConnection netwerk/protocol/websocket/WebSocketChannel.cpp:2283
4 XUL mozilla::net::WebSocketChannel::DoStopSession netwerk/protocol/websocket/WebSocketChannel.cpp
5 XUL mozilla::net::WebSocketChannel::Notify netwerk/protocol/websocket/WebSocketChannel.cpp
6 XUL {virtual override thunk} netwerk/protocol/websocket/WebSocketChannel.cpp
7 XUL nsTimerImpl::Fire const xpcom/threads/nsTimerImpl.cpp:657

There's a decent number of these crashes. The ones I looked at are crashing on poison values, have WebSocketChannel::DoStopSession() on the stack, and are crashing on the main thread.

Priority: -- → P1
Whiteboard: [necko-triaged][necko-priority-queue]
Assignee: nobody → kershaw
Status: NEW → ASSIGNED

This signature looks like the same issue: bp-8da3fae9-1551-4a7e-a047-40cd90221007

Crash Signature: [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait] → [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait] [@ nsCOMPtr_base::assign_with_AddRef | mozilla::net::TLSTransportLayer::OutputStreamWr…

I did a search for protosignatures containing WebSocketChannel::CleanupConnection which turned up a number of variant signatures.

bp-cbb0d3e0-c4c7-4977-8483-3f10a0221007
bp-c6cab42b-8149-4c4d-9253-540260221007

[@ mozilla::net::WebSocketChannel::CleanupConnection ] looks like a null deref, unlike the other signatures. bp-e696a3a8-d3c2-487b-ae92-385e90221004

Crash Signature: mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait ] → mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait ] [@ nsCOMPtr_base::assign_with_AddRef | mozilla::net::TLSTransportLayer::InputStreamWrapper::AsyncWait ] [@ mozilla::net::TLSTransportLayer::Close ] [@ mozilla::net::WebSocketChannel::Clean…

bp-7f25a461-6ff5-43f5-b4f7-e05050221007
bp-0dca3c2b-3309-4ad1-af16-e3a740221001

Crash Signature: mozilla::net::WebSocketChannel::CleanupConnection ] → mozilla::net::WebSocketChannel::CleanupConnection ] [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::InputStreamWrapper::AsyncWait ] [@ nsCOMPtr_base::assign_assuming…
status-firefox105: --- → wontfix
status-firefox106: --- → affected
status-firefox107: --- → affected

esr102 looks like it has the same issue as the patch is fixing, at a glance, but I can't find any UAFs with WebSocketChannel::CleanupConnection in the proto stack, just a couple of hangs, such as: bp-d8b72aaa-56ef-4e72-a26e-125460221007

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Unlikely, since this is a racing 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?:
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This patch is easy and it makes sure WebSocketChannel::CleanupConnection work as expected.
  • Is Android affected?: Yes
Attachment #9297547 - Flags: sec-approval?

Hi Tom,

Could you take a look at the sec-approval request?
I think it'd be good to land this in this cycle.

Thanks.

Flags: needinfo?(tom)

The last uplifts were Oct 6, from a security perspective an internally discovered race issue generally doesn't need an emergency uplift which is why I hadn't considered it earlier. At this point we are in RC builds so if it is a big stability concern please escalate to RelMan, otherwise we can take it next cycle.

Flags: needinfo?(tom)

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

Approved to land and uplift once beta branches

Attachment #9297547 - Flags: sec-approval? → sec-approval+
Whiteboard: [necko-triaged][necko-priority-queue] → [necko-triaged]

WebSocketChannel::CleanupConnection should run on IO thread, r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/1c029b0ffc31502257687bce117d21804d34c8ef
https://hg.mozilla.org/mozilla-central/rev/1c029b0ffc31

Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox108: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 108 Branch

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

Beta/Release Uplift Approval Request

  • User impact if declined: 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: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a straightforward change.
  • String changes made/needed: N/A
  • Is Android affected?: Yes
Attachment #9297547 - Flags: approval-mozilla-beta?

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

Approved for 107.0b3.

Attachment #9297547 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Please nominate this for ESR102 approval when you get a chance.

tracking-firefox108: --- → +
tracking-firefox-esr102: --- → 107+
Flags: needinfo?(kershaw)

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Could crash.
  • Fix Landed on Version: 108
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): A straightforward patch to avoid thread racing.
Flags: needinfo?(kershaw)
Attachment #9297547 - Flags: approval-mozilla-esr102?

I think we might need to uplift this to release as well, since this may be also the root cause of Bug 1796886.

Crash Signature: [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait] [@ nsCOMPtr_base::assign_with_AddRef | mozilla::net::TLSTransportLayer::OutputStreamWr… → [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait]

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

Beta/Release Uplift Approval Request

  • User impact if declined: Crash
  • Is this code covered by automated tests?: Yes
  • 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): A straightforward patch to avoid racing.
  • String changes made/needed: N/A
  • Is Android affected?: Yes
Attachment #9297547 - Flags: approval-mozilla-release?
See Also: → 1796886

Comment on attachment 9297547 [details]
Bug 1794061 - WebSocketChannel::CleanupConnection should run on IO thread, r=#necko

Approved for 102.5esr.

Attachment #9297547 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+

If not all supported branches, which bug introduced the flaw?: None

Maybe the code you fixed hasn't changed in a while, but the crashing from it is clearly a regression in Release 105. Do we know what changed to cause that?

Flags: needinfo?(kershaw)

I'm assuming comment 17 didn't mean to revert the additional signatures mccr8 added

Crash Signature: [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait] → [@ nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr_base::assign_with_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::OutputStreamWrapper::AsyncWait] [@ nsCOMPtr_base::assign_with_AddRef | mozilla::net::TLSTransportLayer::OutputStreamWr…

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

If not all supported branches, which bug introduced the flaw?: None

Maybe the code you fixed hasn't changed in a while, but the crashing from it is clearly a regression in Release 105. Do we know what changed to cause that?

Sorry for not explaining this clearly.
This regression is caused by bug 1772201, which introduced TLSTransportLayer in 105. TLSTransportLayer is not thread-safe, so accessing it from main thread causing this issue.

Flags: needinfo?(kershaw)
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-critsmash-triage]
Blocks: 1794356
Blocks: 1796886
Duplicate of this bug: 1794355
Attachment #9297547 - Flags: approval-mozilla-release? → approval-mozilla-release-
Whiteboard: [necko-triaged][post-critsmash-triage] → [necko-triaged][post-critsmash-triage][adv-main107+r]
Whiteboard: [necko-triaged][post-critsmash-triage][adv-main107+r] → [necko-triaged][post-critsmash-triage][adv-main107+r][adv-esr102.5+r]
Duplicate of this bug: 1796886
Duplicate of this bug: 1794356

Copying crash signatures from duplicate bugs.

Crash Signature: nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr<T>::assign_assuming_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::Close ] → nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr<T>::assign_assuming_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::Close ] [@ mozilla::net::TLSTransportLayer::Close::<T>::operator()]

Copying crash signatures from duplicate bugs.

Crash Signature: nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr<T>::assign_assuming_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::Close ] [@ mozilla::net::TLSTransportLayer::Close::<T>::operator()] → nsCOMPtr_base::assign_assuming_AddRef | nsCOMPtr<T>::assign_assuming_AddRef | nsCOMPtr<T>::operator= | mozilla::net::TLSTransportLayer::Close ] [@ mozilla::net::TLSTransportLayer::Close::<T>::operator()] [@ mozilla::net::Http2Session::CloseStream]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: