1515459 - Crash in mozilla::net::TLSFilterTransaction::WriteSegments

Status: RESOLVED FIXED

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

Description

[[:philipp]]

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

[Ryan VanderMeulen [:RyanVM]]

Hi Dragana, can you please take a look at this new crash in 65? Thanks :)

Comment 2

[Dragana Damjanovic [:dragana]]

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

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: Check if we reenter CreateShimError. r=mayhemer

Comment 4

[Dragana Damjanovic [:dragana]]

(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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

[Selena Deckelmann :selenamarie :selena]

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

Comment 7

[Dragana Damjanovic [:dragana]]

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

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[Ryan VanderMeulen [:RyanVM]]

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

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

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 12

[Dragana Damjanovic [:dragana]]

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

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da0b07a8386c

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Filed bug 1532323 for the remaining crashes.