1523249 - FTP can open endless number of blocking popups, making Firefox unusable

Status: VERIFIED FIXED

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

Description

[Hanno Boeck]

Attachment: firefox-manypopups.webm

FTP login and error popups are blocking (i.e. make the main window unusable).

By causing a lot of FTP popups a site can practically make the browser unusable. I'll attach a video showing how this looks.

This is a simple setup with an FTP server allowing anoynmous login (I use proftpd, every one should do) and an HTML file containing lots of img tags loading images from an invalid directory.

I create the HTML with this:
for i in $(seq 1 10000); do echo "<img src='../$i/$i.png'>"; done

In this example I get a mixture of directory errors and login fields. I don't know exactly why the login fields show up, I guess the FTP server reacts somehow to being overloaded with requests.

There's no other option than killing the browser to escape such a situation. It's "only a DoS", but a pretty annoying one.

(Ceterum Censeo FTP esse delendam.)

Comment 1

[Johann Hofmann [:johannh]]

Thanks for reporting!

This doesn't seem like a known DOS to me. Of course the auth dialog is already infamous (bug 377496) but I suppose this isn't strictly the same issue. I'll add it to the list.

Comment 2

[Hanno Boeck]

bug #1362050 is also related.

(Shared underlying problem is that FTP is allowed to create window modal popups.)

Comment 3

[Johann Hofmann [:johannh]]

(In reply to Hanno Boeck from comment #2)

bug #1362050 is also related.

(Shared underlying problem is that FTP is allowed to create window modal popups.)

Digging one level the deeper the problem, IMO, is that anything untrusted is allowed to create window modal popups, and we are planning to change that but it's harder than one might imagine.

Comment 4

[Hanno Boeck]

Before I forget: I plan to disclose this 90 days from now if no fix is shipped earlier.

Comment 5

[:Gijs (out for now; he/him)]

Attachment: Bug 1523249, r?valentin

Comment 6

[Andrew McCreight [:mccr8]]

Assigning to Gijs because he wrote a patch.

Comment 7

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

https://hg.mozilla.org/integration/autoland/rev/2ce5cbe9bdf2d173dd19771adbfe3a5d43e7e639
https://hg.mozilla.org/mozilla-central/rev/2ce5cbe9bdf2

Comment 8

[:Gijs (out for now; he/him)]

I'm tempted to suggest we uplift this to beta, as it's a straightforward enough change and there's a lot of beta bake time left. Dan, Valentin, what do you think?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Note that an uplift patch minus the string changes would need to be created if you want to nominate this for Beta.

Comment 10

[Daniel Veditz [:dveditz]]

Sure, this looks fine to uplift. Since the only "string change" is deleting one it would be trivial to leave netwerk/locales/en-US/necko.properties untouched. Won't hurt anything to have an unnecessary string in it.

Comment 11

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

(In reply to Daniel Veditz [:dveditz] from comment #10)

Sure, this looks fine to uplift. Since the only "string change" is deleting one it would be trivial to leave netwerk/locales/en-US/necko.properties untouched. Won't hurt anything to have an unnecessary string in it.

Agreed, I think that would be good.

Comment 12

[:Gijs (out for now; he/him)]

Attachment: Beta patch for uplift

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

n/a

User impact if declined

Annoying FTP alerts

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

See comment #0

List of other uplifts needed

n/a

Risk to taking this patch

Low

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

This just removes all the alerts.

String changes made/needed

No (just leaving the dead string around in this patch)

Comment 13

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

Andrei, can your team verify before uplift?

Comment 14

[Anca Soncutean, Desktop QA]

I’ve managed to reproduce this issue, using the information provided in comment 0, with Nightly 67.0a1 (2019-01-28) on Windows 10 x64.
On my end, this is still reproducible with the latest Nightly 67.0a1 (2019-02-04), popups can’t be dismissed, browser gets unusable.
In order to create the FTP server, I used FileZilla.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #14)

I’ve managed to reproduce this issue, using the information provided in comment 0, with Nightly 67.0a1 (2019-01-28) on Windows 10 x64.
On my end, this is still reproducible with the latest Nightly 67.0a1 (2019-02-04), popups can’t be dismissed, browser gets unusable.

What popups are you seeing? Can you create a screencast?

Comment 16

[Anca Soncutean, Desktop QA]

Attachment: screencast issue.gif

Sorry for the late response (time zone inconvenience). Here is the screencast with the issue on the latest Nightly.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #16)

Created attachment 9041400 [details]
screencast issue.gif

Sorry for the late response (time zone inconvenience). Here is the screencast with the issue on the latest Nightly.

OK. So only the auth prompt issue remains; I didn't intend to fix that in this bug, only the alerts (that have nothing but text and an OK button). However, I would have expected bug 377496 to address the auth prompts. Can you file a separate bug for the auth prompts, make it block this bug and bug 377496, and needinfo :johannh ? Thank you.

Comment 18

[Anca Soncutean, Desktop QA]

I will mark this issue as verified fix on Fx 67, since it targets only the FTP alerts segment (no longer reproducible on the latest Nightly build) and the remaining concern related to auth prompts problem is treated separately in bug 1525267.

Comment 19

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

Gijs, should we still uplift this patch on its own? Just making sure we don't need to wait for a fix for bug 1525267.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #19)

Gijs, should we still uplift this patch on its own? Just making sure we don't need to wait for a fix for bug 1525267.

I think the patch should probably be uplifted now given it's an improvement over the status quo, it also fixes bug 1362050, and because I'm confident Johann will get to 1525267 "soon". :-)

Comment 21

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

Comment on attachment 9040410 [details] [diff] [review]
Beta patch for uplift

Fix for annoying popup issue, verified in nightly. 
Let's uplift for beta 6.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0630d18a7d26

Comment 23

[Anca Soncutean, Desktop QA]

The FTP alerts no longer occur with Beta 66.0b6 on Windows 10 x64. I had some difficulties in setting the proper environment on Mac. Can I consider this issue verified, although I can confirm the fix only on one platform?

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #23)

The FTP alerts no longer occur with Beta 66.0b6 on Windows 10 x64. I had some difficulties in setting the proper environment on Mac. Can I consider this issue verified, although I can confirm the fix only on one platform?

Yes, the ftp and alert implementation is shared, and the patch is basically just code removal, so this should be good now. Thanks!