1520483 - Crash in mozilla::net::nsHttpTransaction::ReadSegments

Status: RESOLVED FIXED

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

Description

[Dragana Damjanovic [:dragana]]

I wanted to keep bug 1515459 open until I have resolved the other 2 crashes, but I forgot to set leave-open.

Crash mozilla::net::TLSFilterTransaction::OnReadSegment may have been resolved with bug 1515459, I will keep an eye on it. the crash rate is low so I can not say 100% if it has been resolved.

mozilla::net::nsHttpTransaction::ReadSegments has not been resolved.

Comment 1

[[:philipp]]

maybe i have put the [@ mozilla::net::nsHttpTransaction::ReadSegments] signature into bug 1515459 in error and it's a slightly different case, though it's rising during the 65.0b cycle as well.

correlations show that the presence of various vpn related addons is apparently contributing to that crash:
(36.67% in signature vs 00.56% overall) Addon "Browsec VPN" = true
(20.00% in signature vs 00.17% overall) Addon "ZenMate VPN for Firefox" = true
(15.56% in signature vs 00.53% overall) Addon "Hoxx VPN Proxy" = true

Comment 2

[Dragana Damjanovic [:dragana]]

(In reply to [:philipp] from comment #1)

maybe i have put the [@ mozilla::net::nsHttpTransaction::ReadSegments] signature into bug 1515459 in error and it's a slightly different case, though it's rising during the 65.0b cycle as well.

correlations show that the presence of various vpn related addons is apparently contributing to that crash:
(36.67% in signature vs 00.56% overall) Addon "Browsec VPN" = true
(20.00% in signature vs 00.17% overall) Addon "ZenMate VPN for Firefox" = true
(15.56% in signature vs 00.53% overall) Addon "Hoxx VPN Proxy" = true

That is fine. It is probably cause by the same change set but the fix will be different. Thanks for adding the crash signature.

Comment 3

[[:philipp]]

adding a similar signature that's also looking related to the usage of vpn extensions.

Comment 4

[Dragana Damjanovic [:dragana]]

Here is some docs about TLS tunnel for h1 (h2 is described in TunnelUtils.h):

Writing data to the socket(tcp socket):
nsHttpConnection::OnOutputStreamReady
nsHttpConnection::OnSocketWritable
TLSFilterTransaction::ReadSegments
nsHttpTransaction::ReadSegments
nsStringInputStream::ReadSegments
nsHttpTransaction::ReadRequestSegment
TLSFilterTransaction::OnReadSegment

1. this calls int32_t written = PR_Write(mFD, aData, aCount); which goes through nss and calls TLSFilterTransaction::FilterSend -> TLSFilterTransaction::FilterWrite ->TLSFilterTransaction::FilterOutput. In TLSFilterTransaction::FilterOutput we write the encrypted data into mEncryptedText.
2. After that it calls mSegmentReader->OnReadSegment with mEncryptedText as a parameter. This is actually nsHttpConnection::OnReadSegment

nsStringInputStream::ReadSegments does not propagate errors, therefore nsHttpConnection::OnReadSegment stores any socket error it experience in mSocketOutCondition. The problem is that if an error occurs during step 1) in TLSFilterTransaction::OnReadSegment we will not report it back. We cannot call Close() here because it will close the socket, tunnl and httpTransaction in the middle of a call. we need to propagate the error back and let nsHttpConnection close them all. We will store the error in TLSFilterTransaction and report it back in TLSFilterTransaction::ReadSegments.

Reading data from a the socket:

nsHttpConnection::OnInputStreamReady (real socket)
nsHttpConnection::OnSocketReadable
TLSFilterTransaction::WriteSegments
nsHttpTransaction::WriteSegments
nsPipeOutputStream::WriteSegments
nsHttpTransaction::WritePipeSegment
TLSFilterTransaction::OnWriteSegment -> some nss layers -> TLSFilterTransaction::FilterInput -> nsHttpConnection::OnWriteSegment -> mSocketIn->Read

Similar as the write path: nsPipeOutputStream::WriteSegments does not propagate errors therefore nsHttpConnection::OnWriteSegment stores any read error into mSocketInCondition. If error happens during nss calls this error will not be propagated. therefore we need to store the error in TLSFilterTransaction and return it back in TLSFilterTransaction::WriteSegments. The same as before we cannot call Close() here.

If nsHttpConnection::OnSocketReadable or nsHttpConnection::OnSocketWritable returns an error nsHttpConnection::OnInputStreamReady or nsHttpConnection::OnOutputStreamReady will close the tunnel.
In case of nsHttpConnection::OnTunnelNudged we do not close the tunnel, but we should.

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 9039846 [details]
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

Security Approval Request

How easily could an exploit be constructed based on the patch?

The patch shows what the problem is (also crash data as well). The nss layer must return an error for the problem to be triggered (that can be common). The bug only affects people with https proxies.

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?

release

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

Bug 1484947

Do you have backports for the affected branches?

No

If not, how different, hard to create, and risky will they be?

It is easy to create and it is not risky.

How likely is this patch to cause regressions; how much testing does it need?

I am not sure, I would say low.

Comment 7

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

Comment on attachment 9039846 [details]
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

sec-approval+ for trunk. We'll want a beta patch made and nominated as well.

Comment 8

[Dragana Damjanovic [:dragana]]

The patch applies clearly on beta.

Comment 9

[Dragana Damjanovic [:dragana]]

Comment on attachment 9039846 [details]
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1484947

User impact if declined

It causes a 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

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

I am not sure, I would say low. Although, the patch changes only the code path that causes a crash, it should not cause more problems than a crash.

String changes made/needed

Comment 10

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

Comment on attachment 9039846 [details]
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

Crash fix for sec issue. This should land for beta 6.

Comment 11

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

Comment on attachment 9039846 [details]
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

Actually - let's leave this in the approval queue till it lands on trunk.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/661ca61b9413

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This was backed out for Android build bustage.
https://hg.mozilla.org/integration/autoland/rev/ac53eefd531e

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226843468&repo=autoland

ERROR - /builds/worker/workspace/build/src/netwerk/protocol/http/TunnelUtils.cpp:323:9: error: use of undeclared identifier 'mReadSegmentBlocked'

Comment 14

[Dragana Damjanovic [:dragana]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

This was backed out for Android build bustage.
https://hg.mozilla.org/integration/autoland/rev/ac53eefd531e

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226843468&repo=autoland

ERROR - /builds/worker/workspace/build/src/netwerk/protocol/http/TunnelUtils.cpp:323:9: error: use of undeclared identifier 'mReadSegmentBlocked'

sorry for this. It is fixed.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/a4247f89d070

Comment 16

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

Backed out for bustage on build/src/obj-firefox/dist/include/mozilla/Logging.h:257 in Linux x64 base-toolchains, fuzzing and hazard builds:

https://hg.mozilla.org/integration/autoland/rev/0ede01588c31a512424f97ffefda0dd1ca8f7387

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&group_state=expanded&revision=a4247f89d070aea2df1636ad7b95498ad13a10b9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=227039074&repo=autoland

12:28.01 /builds/worker/checkouts/gecko/obj-analyzed/dist/include/mozilla/Logging.h:257:61: error: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'nsresult' [-Werror=format=]

Comment 17

[Dragana Damjanovic [:dragana]]

Sorry again :(

Comment 18

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

https://hg.mozilla.org/integration/autoland/rev/e52aa87c1cedbd9184d1bc25463833e191ffcac4

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/e52aa87c1ced

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9039846 [details]
Bug 1520483 - Return proper error if the nss layer encounters an error on the http tunnel. r=valentin,kershaw

[Triage Comment]
Fixes a sec bug, approved for 66.0b7.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/81b95ee7a2f3