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

VERIFIED FIXED in Firefox 37

Status

()

--
critical
VERIFIED FIXED
4 years ago
a year ago

People

(Reporter: Virtual, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, 4 keywords)

unspecified
mozilla38
csectype-dos, csectype-spoof, nightly-community, ux-control
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 wontfix, firefox36 wontfix, firefox37 verified, firefox38 verified)

Details

(URL)

Attachments

(3 attachments)

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.
Sounds like a DoS-type issue, no need to make it security-sensitive.
Group: core-security
Component: Security → Security
Product: Firefox → Core
@ :Gijs Kruitbosch - Can you look on this as you were working on similar case in bug #636374 or maybe know who can? Thanks
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: csectype-dos
(Assignee)

Comment 7

4 years ago
How are you sure 1117342 is a dupe?
Flags: needinfo?(BernesB)
(Assignee)

Comment 8

4 years ago
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?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 9

4 years ago
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

4 years ago
Created attachment 8553700 [details]
badsite-french-version.7z
@ :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.
Flags: needinfo?(BernesB) → needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

4 years ago
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?
(Assignee)

Comment 13

4 years ago
(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?
Flags: needinfo?(bzbarsky)
Not in the short term, sorry.  I'm completely swamped.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 15

4 years ago
Created attachment 8553756 [details] [diff] [review]
disallow link clicks during beforeunload,

This also breaks hypothetical links during print preview - which AFAICT shouldn't be working anyway, so...
Attachment #8553756 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 16

4 years ago
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.
Component: Security → Document Navigation
Depends on: 1123986
Flags: needinfo?(gijskruitbosch+bugs)
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.

Updated

4 years ago
Attachment #8553756 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

4 years ago
(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!
(Assignee)

Updated

4 years ago
Iteration: --- → 38.1 - 26 Jan
Points: --- → 3
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
(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! :)
https://hg.mozilla.org/mozilla-central/rev/ef538bd60b1c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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?
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8553756 - Flags: approval-mozilla-beta?
Attachment #8553756 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

4 years ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8553756 - Flags: approval-mozilla-beta?
(Assignee)

Comment 25

4 years ago
(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.
(Assignee)

Updated

4 years ago
status-firefox36: affected → wontfix
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+
Attachment #8553756 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Status: RESOLVED → VERIFIED
status-firefox37: fixed → verified
status-firefox38: fixed → verified
Please see #Comment 29, it's the issue I was talking in bug #1206411 and bug #1205678.
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 31

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Updated

2 years ago
See Also: → bug 1214805
You need to log in before you can comment on or make changes to this bug.