Closed Bug 1515459 Opened 6 years ago Closed 6 years ago

Crash in mozilla::net::TLSFilterTransaction::WriteSegments

Categories

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

Product:
Core
Component:
Networking: HTTP
Version:
65 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
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)

Check if we reenter CreateShimError. r=mayhemer
(deleted), text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
abillings
: sec-approval+
Details
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.
Hi Dragana, can you please take a look at this new crash in 65? Thanks :)
tracking-firefox65: --- → +
tracking-firefox66: --- → +
Flags: needinfo?(dd.mozilla)
Keywords: sec-high
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(dd.mozilla)
Blocks: 1434137
No longer blocks: 1434137
Crash Signature: [@ mozilla::net::TLSFilterTransaction::WriteSegments] → [@ mozilla::net::TLSFilterTransaction::WriteSegments] [@ mozilla::net::TLSFilterTransaction::OnReadSegment ]
Blocks: 1434137
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
(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.
Crash Signature: [@ mozilla::net::TLSFilterTransaction::WriteSegments] [@ mozilla::net::TLSFilterTransaction::OnReadSegment ] → [@ mozilla::net::TLSFilterTransaction::WriteSegments] [@ mozilla::net::TLSFilterTransaction::OnReadSegment] [@ mozilla::net::nsHttpTransaction::ReadSegments]
Whiteboard: [necko-triaged]
Priority: -- → P1

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.

Flags: needinfo?(dd.mozilla)

:mayhemer should we get the sec-review without dragana? She's out for a couple more days.

Flags: needinfo?(honzab.moz)

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.

Flags: needinfo?(dd.mozilla)
Attachment #9033179 - Flags: sec-approval?

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.

Attachment #9033179 - Flags: sec-approval? → sec-approval+

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.

Flags: needinfo?(dd.mozilla)

https://hg.mozilla.org/integration/autoland/rev/740f1995b128

This grafts cleanly as-landed. Just needs an approval request.

https://hg.mozilla.org/mozilla-central/rev/740f1995b128

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Group: network-core-security → core-security-release

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:

Flags: needinfo?(dd.mozilla)
Attachment #9033179 - Flags: approval-mozilla-beta?

Comment on attachment 9033179 [details]
Check if we reenter CreateShimError. r=mayhemer

[Triage Comment]
Fixes a sec-high crash. Approved for 65.0b11.

Flags: needinfo?(honzab.moz)
Attachment #9033179 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [necko-triaged] → [necko-triaged][post-cristsmash-triage]

Filed bug 1532323 for the remaining crashes.

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: