1116977 - Looped in closing trap site and unable to fast close it and prevent from creating a new tabs

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Going to some prank and trap sites like the one mentioned URL I'm getting into infinite loop in closing tabs and I'm unable to fast close it and prevent from creating a new tabs when I close the other one.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Attachment: Compressed site.7z

Comment 2

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I also reported it on StopBadware here - https://www.stopbadware.org/report-badware

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Sounds like a DoS-type issue, no need to make it security-sensitive.

Comment 4

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

A new working address - http://gfnyjxfg.cfoexecutives.com/europol/hvMo-TnnVRpdPph3CRBR88L3-EnfVrzZPsLKd_/IDzvGN1Mu0ANRrv21TXc/WG2ksbInCJmic2KdZBPDvLOa4hBA~~

Comment 5

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@ :Gijs Kruitbosch - Can you look on this as you were working on similar case in bug #636374 or maybe know who can? Thanks

Comment 7

[:Gijs (he/him)]

How are you sure 1117342 is a dupe?

Comment 8

[:Gijs (he/him)]

When I load the remote URL, I get redirected to msn.com. When I load the site in your archive over file:/// or from http://localhost/, I get a single beforeunload prompt, but clicking 'leave page' works in one go.

What am I missing?

Comment 9

[Segismond]

I confirm 1117342 is a dupe.

This URL is working with french IP too : http://gfnyjxfg.cfoexecutives.com/europol/hvMo-TnnVRpdPph3CRBR88L3-EnfVrzZPsLKd_/IDzvGN1Mu0ANRrv21TXc/WG2ksbInCJmic2KdZBPDvLOa4hBA~~

Archive missing two .js files

jquery.min.js (not the real JQuery) = http://pastebin.com/cTReXxBd

public.min.js = http://pastebin.com/3nu0F1wg

Comment 10

[Segismond]

Attachment: badsite-french-version.7z

Comment 11

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

@ :Gijs Kruitbosch - Please open the newest site posted in comment #4 as others are probably taken down. Site archive as I see isn't fully as it didn't downloaded any scripts.

Comment 12

[:Gijs (he/him)]

I'm busy with other things right now; it looks like the new window creation stuff is being done by calling link.click() on an <a> with target="_blank" from within the beforeunload handler (although I'm struggling to get source for that handler, which would be quite useful).

It's also using focusout handlers and other evil things to trigger unloads in iframes on the page which then pop up beforeunload dialogs... but I'm not sure what we can do about that. :-(

I guess we should make onLinkClick check the docshell if navigation is allowed at that point.

Boris, do you have time to check if there's other things here that need addressing?

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> I'm busy with other things right now; it looks like the new window creation
> stuff is being done by calling link.click() on an <a> with target="_blank"
> from within the beforeunload handler (although I'm struggling to get source
> for that handler, which would be quite useful).
> 
> It's also using focusout handlers and other evil things to trigger unloads
> in iframes on the page which then pop up beforeunload dialogs... but I'm not
> sure what we can do about that. :-(
> 
> I guess we should make onLinkClick check the docshell if navigation is
> allowed at that point.
> 
> Boris, do you have time to check if there's other things here that need
> addressing?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Not in the short term, sorry.  I'm completely swamped.

Comment 15

[:Gijs (he/him)]

Attachment: disallow link clicks during beforeunload,

This also breaks hypothetical links during print preview - which AFAICT shouldn't be working anyway, so...

Comment 16

[:Gijs (he/him)]

Evil pages are evil.

The patch on the bug makes the pages a lot less convincing because they can't just spawn new things all the time, and so the pages are much more easily closed - although part of me is sure they'll find some other way to bypass our attempts to block them abusing this web feature.

This is starting to feel a bit like whack-a-mole; maybe we should either move towards just fixing bug 1123986, or try to be smarter about them in general. We could try keeping track of the last N times we saw a dialog like this on a per-chrome-window basis, and if you see more than X in Y seconds, we could pop up a notification and allow you to disable them for the next 5 minutes, or something...

Ideas welcome.

Comment 17

[:Gijs (he/him)]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I wonder if we should do the check only if aIsTrusted is false
(and propagate aIsTrusted to OnLinkClickSync too).
And what do other browsers do here?

But even then..., based on the other use of OnLinkClickSync() this should be fine.

Comment 19

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #18)
> I wonder if we should do the check only if aIsTrusted is false
> (and propagate aIsTrusted to OnLinkClickSync too).
> And what do other browsers do here?

I've not checked this time, but it says in the spec that we're allowed to ignore prompts and window.open calls from beforeunload/unload, so this seems even less controversial than bug 1046022. :-)

> But even then..., based on the other use of OnLinkClickSync() this should be
> fine.

Thanks for the quick review!

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef538bd60b1c

Comment 21

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to :Gijs Kruitbosch from comment #16)
> Evil pages are evil.
Poor standards are the worst ;)

Why this standard isn't checking that you made some changes on exit, like for example simply writing text in Notepad:
-if you write something, you will be prompted to save it,
-if you won't write anything, you won't be prompted to save it as it isn't anything to save because no changes were made.

First of all, this standard is kinda obsolete. As even in GMail when you compose an email, your changes are saved in real time. So why we even need today these beforeunload/onbeforeunload dialogs, as in most cases they annoy the users and give bad guys chance to create a malicious and prank sites to abuse the users.



(In reply to :Gijs Kruitbosch from comment #16)
> We could try keeping track of the last N times we saw a dialog like
> this on a per-chrome-window basis, and if you see more than X in Y seconds,
> we could pop up a notification and allow you to disable them for the next 5
> minutes, or something...

I think this will lead to the same dialogs with hidden numbers, so it won't be considered by browser as same,
so again, abuse will start.



(In reply to :Gijs Kruitbosch from comment #16)
> Ideas welcome.
Stop supporting this bad and obsolete standard. ;)
As site and server can save user work in real time.



Thank you very much guys for your time and very fast fix and review! :)

Comment 22

[Phil Ringnalda (:philor)]

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

Comment 23

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Comment on attachment 8553756 [details] [diff] [review]
disallow link clicks during beforeunload,

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Availability to close tabs and prevent to create a new tabs on malicious and prank sites with ransomware.
[Describe test coverage new/current, TreeHerder]: This code should be tested through our existing tests.
[Risks and why]: This should be pretty low risk and what's more gain is bigger than risk.
[String/UUID change made/needed]: N/A



@ :Gijs Kruitbosch - Please double check it, thanks. Shouldn't we also push it on release?

Comment 24

[:Gijs (he/him)]

Comment on attachment 8553756 [details] [diff] [review]
disallow link clicks during beforeunload,

(In reply to Virtual_ManPL [:Virtual] from comment #23)
> Comment on attachment 8553756 [details] [diff] [review]
> disallow link clicks during beforeunload,
> 
> Approval Request Comment
> [Feature/regressing bug #]: N/A
> [User impact if declined]: Availability to close tabs and prevent to create
> a new tabs on malicious and prank sites with ransomware.
> [Describe test coverage new/current, TreeHerder]: This code should be tested
> through our existing tests.
> [Risks and why]: This should be pretty low risk and what's more gain is
> bigger than risk.
> [String/UUID change made/needed]: N/A
> 
> 
> 
> @ :Gijs Kruitbosch - Please double check it, thanks. Shouldn't we also push
> it on release?

Please leave requesting branch approval to path authors / reviewers.

And, no, this is definitely not something we can push on release. The current behaviour is not a regression compared to previous Firefox builds. Interfering with link clicks and web standards is not something we do lightly, so I am not sure it makes sense to push it to beta halfway through a cycle - I'm going to clear beta?.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Sites can popup new tabs from onbeforeunload handlers (see also bug 391834).
[Describe test coverage new/current, TreeHerder]: The patch adds a test.
[Risks and why]: This patch stops link clicks during beforeunload/unload events from working. In principle, this should only affect malicious sites that programmatically trigger these from the events in question (because of JS' run-to-completion execution model), but I'd prefer to tread carefully.
It'll also explicitly "break" link clicks in print preview mode, where as best I can tell they don't work anyway, so that shouldn't matter.
[String/UUID change made/needed]: No.

Comment 25

[:Gijs (he/him)]

(In reply to Virtual_ManPL [:Virtual] from comment #21)
> (In reply to :Gijs Kruitbosch from comment #16)
> > Evil pages are evil.
> Poor standards are the worst ;)
> 
> Why this standard isn't checking that you made some changes on exit, like
> for example simply writing text in Notepad:
> -if you write something, you will be prompted to save it,
> -if you won't write anything, you won't be prompted to save it as it isn't
> anything to save because no changes were made.

bug 636905

> First of all, this standard is kinda obsolete. As even in GMail when you
> compose an email, your changes are saved in real time. So why we even need
> today these beforeunload/onbeforeunload dialogs, as in most cases they annoy
> the users and give bad guys chance to create a malicious and prank sites to
> abuse the users.

That is your experience. It is not universal. There are pages that use these dialogs for legitimate purposes ("even", for instance, Facebook and some Google applications do this in some circumstances). 

> (In reply to :Gijs Kruitbosch from comment #16)
> > We could try keeping track of the last N times we saw a dialog like
> > this on a per-chrome-window basis, and if you see more than X in Y seconds,
> > we could pop up a notification and allow you to disable them for the next 5
> > minutes, or something...
> 
> I think this will lead to the same dialogs with hidden numbers, so it won't
> be considered by browser as same,
> so again, abuse will start.

Err, no. The point is that if people abuse alert/confirm prompts, after a number of these prompts you get a checkbox "prevent this page from showing more dialogs" (bug 61098), which we could do for beforeunload as well.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8553756 [details] [diff] [review]
disallow link clicks during beforeunload,

Gijs and I spoke about this change on irc. While there is some risk, he thinks the risk to breaking clicks is pretty theoretical. While he is not going to advocate pushing this to Beta, taking it at this point in the Aurora cycle seems reasonable. Aurora+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/127079a6ab72

Comment 28

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

(In reply to :Gijs Kruitbosch from comment #24)
> Please leave requesting branch approval to path authors / reviewers.
OK. Sorry about that, but you didn't do that at first, so I do this only to make your job easier and faster as I asked for feedback.

(In reply to :Gijs Kruitbosch from comment #25)
> That is your experience. It is not universal.
Yes, I'm talking about only from my perspective and experience with Firefox, but I think I'm not the only one having problems with this "feature".
If Firefox had better protection from the start or bugs about beforeunload/onbeforeunload will be decider faster what to do about it, maybe I will have other experience.



Still, thank you very much guys for fast fix, review and uplift.

Comment 30

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Please see #Comment 29, it's the issue I was talking in bug #1206411 and bug #1205678.

Comment 31

[:Gijs (he/him)]

(In reply to Virtual_ManPL [:Virtual] from comment #30)
> Please see #Comment 29, it's the issue I was talking in bug #1206411 and bug
> #1205678.

I already looked at it, but for me it redirects to a random ad - nothing that tries to stop me closing the browser.

If you can still see this, please save a copy of the page using save as... html complete, or link to the final page post-redirects.