1459666 - Browser crashes via FTP

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect, P2)

Description

[Dmitrii Bezborodov]

Attachment: screenshot.tar.gz

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

OS: Debian 9
Browser: 59.0.2 64bit and 52.7.4 ESR 64bit

1. Open in tab ftp://cyberghost.pro:80/pro.php (any exec file which not on server)
2. After a while all the tabs crashed in 52.7.4 ESR version and only active tabs crashed in 59.0.2 version

For example, may be use redirect for reproduction - https://cyberghost.pro/tor/r.php






Actual results:

After try open FTP link on non default port and with the indication of certain types of files such as php, doc, xls  (any exec type file) there is a crashed browser.


Expected results:

Browser must inform you that ftp is not used at this address.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This reproduces for me on Linux, but not Win10. Valentin, can you please take a look? It's also unclear to me why this was filed as a security-sensitive bug?

Comment 2

[Georg Koppen]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> It's also unclear to me why this was filed as a
> security-sensitive bug?

That's been my suggestion as I did not have time to look into this yesterday and was not sure about the severity of this issue. It seems we hit

Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))) || rv == NS_ERROR_NOT_AVAILABLE, at /var/tmp/build/firefox-deaa82b4f8ab/netwerk/protocol/ftp/FTPChannelParent.cpp:646

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Good news: with the latest nightly this doesn't crash anymore. (bug 1436311)
Bad news: infinite loop :)

Comment 4

[Valentin Gosu [:valentin] (he/him)]

The bug is indeed caused by the tcp connection not sending back any data, and just being closed right away.
So we get something like this:
FTPChannelChild::DoOnStartRequest
FTPChannelChild::DoOnStopRequest -> nsUnknownDecoder::OnStopRequest -> (data is empty) -> nsUnknownDecoder::FireListenerNotifications -> nsDocumentOpenInfo::OnStartRequest -> ExternalHelperAppChild::OnStartRequest -> ExternalHelperAppChild::DivertToParent -> FTPChannelChild::DivertToParent.

However, in nsIDivertableChannel.idl the description for divertToParent() is "divertToParent is called in the child process. It can only be called during OnStartRequest()."

Enforcing this condition seems to be enough to avoid an infinite loop. The crash was fixed by bug 1436311.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Ensure that DivertToParent is only called during OnStartRequest

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 8979355 [details] [diff] [review]
Ensure that DivertToParent is only called during OnStartRequest

Review of attachment 8979355 [details] [diff] [review]:
-----------------------------------------------------------------

thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb4b8d4daad
Ensure that DivertToParent is only called during OnStartRequest r=dragana

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 6cb4b8d4daad (bug 1459666) for build bustages on /netwerk/protocol/ftp/FTPChannelChild.cpp

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/104f1728e3bcb12d677effe725a58f92e1709074

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=898c5c1920bd221336841f74857fce2fe9c3e8cb

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=180241949&repo=mozilla-inbound&lineNumber=16676

Comment 9

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe8eab2539185df20fe89c0c4561a760bd84d2b

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Attachment: Ensure that DivertToParent is only called during OnStartRequest

The bug was caused by the tcp connection not sending back any data, and just being closed right away.
So we get something like this:
FTPChannelChild::DoOnStartRequest
FTPChannelChild::DoOnStopRequest -> nsUnknownDecoder::OnStopRequest -> (data is empty) -> nsUnknownDecoder::FireListenerNotifications -> nsDocumentOpenInfo::OnStartRequest -> ExternalHelperAppChild::OnStartRequest -> ExternalHelperAppChild::DivertToParent -> FTPChannelChild::DivertToParent.

However, in nsIDivertableChannel.idl the description for divertToParent() is "divertToParent is called in the child process. It can only be called during OnStartRequest()."

Enforcing this condition seems to be enough to avoid an infinite loop. The crash was fixed by bug 1436311.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8303c9587a1445920ea4c99937f995bc2d7c72df
Bug 1459666 - Ensure that DivertToParent is only called during OnStartRequest r=dragana

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8303c9587a14

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I think we should let this ride the trains given where we are in the cycle. If you feel strongly otherwise, feel free to nominate the patch for uplift with your reasoning for why.

Comment 14

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using a older version of Nightly (2018-05-07) on Ubuntu 16.04 x64. As soon as the page was loaded, the tab crashed.
I tried again using the latest Nightly and Beta 62.0b7 on the same platform and the tab didn't crash anymore. However, I didn't get any warnings that the ftp is not used at this address either. Is that expected behaviour?

Comment 15

[Valentin Gosu [:valentin] (he/him)]

If you tested with ftp://cyberghost.pro:80/pro.php then all you should see is a blank page and no crashes.

Comment 16

[Oana Botisan, Desktop QA]

According to comment 14 and comment 15, I will mark this bug as verified.