Crash in mozilla::net::TLSFilterTransaction::WriteSegments
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox64 | --- | unaffected |
firefox65 | + | fixed |
firefox66 | + | fixed |
People
(Reporter: philipp, Assigned: dragana)
References
Details
(4 keywords, Whiteboard: [necko-triaged][post-cristsmash-triage])
Crash Data
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Review |
This bug was filed from the Socorro interface and is report bp-33c061c7-7dad-4450-9af6-6029c0181219. ============================================================= Top 10 frames of crashing thread: 0 xul.dll mozilla::net::TLSFilterTransaction::WriteSegments netwerk/protocol/http/TunnelUtils.cpp:374 1 xul.dll mozilla::net::nsAHttpTransaction::WriteSegmentsAgain netwerk/protocol/http/nsAHttpTransaction.h:100 2 xul.dll mozilla::net::nsHttpConnection::OnInputStreamReady netwerk/protocol/http/nsHttpConnection.cpp:2418 3 xul.dll void mozilla::net::SpdyConnectTransaction::CreateShimError netwerk/protocol/http/TunnelUtils.cpp:1216 4 xul.dll nsresult mozilla::net::SpdyConnectTransaction::WriteDataToBuffer netwerk/protocol/http/TunnelUtils.cpp:1241 5 xul.dll mozilla::net::SpdyConnectTransaction::WriteSegments netwerk/protocol/http/TunnelUtils.cpp:1272 6 xul.dll mozilla::net::Http2Stream::WriteSegments netwerk/protocol/http/Http2Stream.cpp:307 7 xul.dll mozilla::net::Http2Session::WriteSegmentsAgain netwerk/protocol/http/Http2Session.cpp:3402 8 xul.dll mozilla::net::nsHttpConnection::OnInputStreamReady netwerk/protocol/http/nsHttpConnection.cpp:2418 9 xul.dll void mozilla::net::nsSocketInputStream::OnSocketReady netwerk/base/nsSocketTransport2.cpp:277 ============================================================= these crashes are regressing cross-platform in firefox 65 and generally look sec sensitive. the first crash report was registered from nightly build 20181101220058.
Comment 1•5 years ago
|
||
Hi Dragana, can you please take a look at this new crash in 65? Thanks :)
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
I looked at the first crash: mozilla::net::TLSFilterTransaction::WriteSegments. It crash in the middle of a function and it crash as if TLSFilterTransaction was been destroyed in the middle of executing TLSFilterTransaction::WriteSegments. nsHttpConnection holds reference to a TLSFilterTransaction instance (as far as I know this is only reference to the TLSFilterTransaction instance). What is happening: An error happens during an execution of SpdyConnectTransaction::WriteDataToBuffer. Therefore, SpdyConnectTransaction::CreateShimError[1] is called which sets the mTunnelStreamIn’s status to an error code and calls SpdyConnectTransaction.mTunneledConn::OnInputStreamReady(mTunnelStreamIn) that calls SpdyConnectTransaction.mTunneledConn::OnSocketReadable() that calls TLSFilterTransaction::WriteSegment(). We crash in TLSFilterTransaction::WriteSegment [2]. Just before the line that crashes, line TunnelUtils.cpp#370 calls mTransaction->WriteSegments, this function calls TLSFilterTransaction::OnWriteSegment, that calls PR_Read[3] which goes all the way to SpdyConnectTransaction.mTunneledConn.mTunnelStreamIn->Read() and returns the error set in CreateShimError. After PR_Read TLSFilterTransaction::Close is called [4] which calls SpdyConnectTransaction::Close[5]. This will call CreateShimError again [6]. This will call SpdyConnectTransaction.mTunneledConn::OnInputStreamReady(mTunnelStreamIn), SpdyConnectTransaction.mTunneledConn::OnSocketReadable() and TLSFilterTransaction::WriteSegment() again, but this will not call mTransaction->WriteSegments because mTransaction is nullptr, it will only return an error which will cause nsHttpConnection to be closed in OnInputStreamReady. This will set mTLSFilter to nullptr and cause TLSFilterTransaction to be destroyed. What I do not know is why this did not crash before websocket over http/2 implementation. Maybe my analysis here are wrong. I will make a patch that tries to mitigate calling CreateShimError and we can see if its resolves the crash. [1] https://hg.mozilla.org/releases/mozilla-beta/annotate/dfe78db5e4ae6de6052b22e0e2d7ced375a0110c/netwerk/protocol/http/TunnelUtils.cpp#l1241 [2] https://hg.mozilla.org/releases/mozilla-beta/annotate/dfe78db5e4ae6de6052b22e0e2d7ced375a0110c/netwerk/protocol/http/TunnelUtils.cpp#l374 [3] https://hg.mozilla.org/releases/mozilla-beta/annotate/dfe78db5e4ae6de6052b22e0e2d7ced375a0110c/netwerk/protocol/http/TunnelUtils.cpp#l284 [4] https://hg.mozilla.org/releases/mozilla-beta/annotate/dfe78db5e4ae6de6052b22e0e2d7ced375a0110c/netwerk/protocol/http/TunnelUtils.cpp#l291 [5] https://hg.mozilla.org/releases/mozilla-beta/annotate/dfe78db5e4ae6de6052b22e0e2d7ced375a0110c/netwerk/protocol/http/TunnelUtils.cpp#l142 [6] https://hg.mozilla.org/releases/mozilla-beta/annotate/dfe78db5e4ae6de6052b22e0e2d7ced375a0110c/netwerk/protocol/http/TunnelUtils.cpp#l1353
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #3) > Created attachment 9033179 [details] > Check if we reenter CreateShimError. r=mayhemer This is a patch to check hypothesis in comment #2.
Reporter | ||
Updated•5 years ago
|
![]() |
||
Updated•5 years ago
|
![]() |
||
Updated•5 years ago
|
Comment 5•5 years ago
|
||
Is this ready for a sec-approval request? We've got about a week and a half left for this to land in time to make it into 65.
Comment 6•5 years ago
|
||
:mayhemer should we get the sec-review without dragana? She's out for a couple more days.
Assignee | ||
Comment 7•4 years ago
|
||
Comment on attachment 9033179 [details]
Check if we reenter CreateShimError. r=mayhemer
[Security Approval Request]
How easily could an exploit be constructed based on the patch?: The patch check whether we are reentering a function. Looking how the function is call, someone could figure out what goes wrong.
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?: 65
If not all supported branches, which bug introduced the flaw?: Bug 1434137
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. The patch is small and strait forward.
Comment 8•4 years ago
|
||
Comment on attachment 9033179 [details]
Check if we reenter CreateShimError. r=mayhemer
Sec-approval+ for trunk. We'll want a beta patch made and nominated quickly so we can get this into Beta as well in time for...betas.
dragana or mayhemer can you nominate a beta patch for uplift? If you can tonight, it could still make it into tomorrow's beta 10 build.
Comment 10•4 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/740f1995b128
This grafts cleanly as-landed. Just needs an approval request.
![]() |
||
Comment 11•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
Comment on attachment 9033179 [details]
Check if we reenter CreateShimError. r=mayhemer
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: Bug 1434137
User impact if declined: crash.
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: No
Needs manual test from QE?: No
If yes, steps to reproduce: there are no step 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): The patch is small and strait forward.
String changes made/needed:
Comment 13•4 years ago
|
||
Comment on attachment 9033179 [details]
Check if we reenter CreateShimError. r=mayhemer
[Triage Comment]
Fixes a sec-high crash. Approved for 65.0b11.
Comment 14•4 years ago
|
||
uplift |
Updated•4 years ago
|
Filed bug 1532323 for the remaining crashes.
Updated•4 years ago
|
Description
•