636374 - Don't show multiple dialogs when a page has multiple frames with onbeforeunload

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (3 onbeforeunload dialogs)

When multiple frames use onbeforeunload warnings, Firefox should only show the warning once.  Especially now that the warnings are identical (see bug 588292).

Scammy popup ads from Encyclopedia Dramatica might be taking advantage of this to make themselves hard to close.  Or maybe they're just exploiting bug 391834 and bug 560767 in a way that causes multiple onbeforeunload dialogs.

Comment 5

[mike]

This vulnerability is being actively exploited in the wild.  This bug needs to be elevated.

http://security.stackexchange.com/questions/46270/site-preventing-user-from-closing-tab-closing-browser

Comment 7

[keytothevan]

This bug was reported on 2011-02-23,
Just now I'm back redirected to a fraudulent Web site that exploits this error. Every time when I watch porn. And porn belong to life and keep the traffic of the internet upright.
Why is the group of Mozilla does not try to solve the problem? It's really a simple problem the query to zulasen only once.
I would not want to be naughty and I also admit that I am not the expert.

Comment 11

[Florian Bender]

Curits, can you please have a look at that? IMO, this should be rather straightforward: Simply ignore any further dialogues when the user clicked "Leave page".

Comment 12

[Jesse Ruderman]

Tim, are you interested in fixing this frequently-exploited bug?

Comment 13

[Frederik Braun [:freddy]]

According to this SUMO article, this is indeed an annoyance for a decent amount of our users: https://support.mozilla.org/en-US/questions/961803

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Jesse Ruderman from comment #12)
> Tim, are you interested in fixing this frequently-exploited bug?

I'm interested, yes, but I don't think I'll be able to pick this up in the foreseeable future. I'll put it on the back burner, so if/when I sit down to hammer out a fix for bug 616853, I'll also take a look at this.

Comment 16

[Florian Bender]

Another approach (that Chrome probably does) is: 

Run all onbeforeunload(), queue up return values. After the last unbeforeunload ran, display one message containing all return values and ask the user to confirm leaving the page. 

Chrome fails horribly with pages like those mentioned in Bug 934083 because the dialog grows bigger than the screen (on my machine) with multiple identical messages thereby hiding the confirm/abort buttons (and without any way to reach them – though you can push them via keyboard, thankfully). 

So a better UX than Chrome would be: Only remember unique messages (i.e. drop any message that was queued up before), and put the messages into a textfield (at least when there are more than 3) that overflows with scrolling (so the prompt does not grow bigger than necessary). As an additional precautionary measure (avoid DoS vector / excessive memory consumption), ignore any further message after the first 20 or so (which is _very_ liberal).

I think that solution should be easier to implement, and does not depend on fixing Bug 616853 first.

Comment 17

[Benjamin Kerensa [:bkerensa]]

Do we need an upgraded sec rating for this?

Comment 18

[mike]

How many other bugs in Mozilla are being actively exploited?   We are seeing a lot of activity here,  a lot of duplicate bug ids.  With this kind of activity I think the sec rating should be upgraded.

Comment 19

[Frederik Braun [:freddy]]

Our sec-ratings do not change when something is exploited or not. (See also https://wiki.mozilla.org/Security_Severity_Ratings)

But I agree that we should prioritize this.

Comment 20

[mike]

@Frederik How fortunate.  You make bugs like this publicly searchable,  and then leave them open for years with no intention of fixing them.

Attention Blackhats!  Want to abuse Firefox users???  Move aside exploit-db and metasploit,  come to bugzilla.mozilla.org  for all your Firefox sploits!

Comment 21

[Frederik Braun [:freddy]]

I might have been not too clear... Priority != Severity. But let's stay on topic and leave the remaining comments for fixing the issue.

Comment 22

[Daniel Veditz [:dveditz]]

(In reply to Benjamin Kerensa [:bkerensa] from comment #17)
> Do we need an upgraded sec rating for this?

No: the security team dearly wants the development team to fix this, it's both abusable and abused. However it's not a direct malware vector.

Comment 24

[:Gijs (he/him)]

Attachment: 636374-dont-show-multiple-prompts

I'm not really sure about this patch, but it does seem to work. I considered some other approaches:

- always finding the root viewer using the docshell parent chain. This gets complicated with consecutive/interleaving unloads within the same root, as the thing that's unloading (and has subframes) might not necessarily be the root of the docshell parent chain, AIUI.
- second outparam to indicate whether the prompt was done. Not even sure if xpcom supports that; in any case it would be impossible to use from JS.

Boris, could you look at the approach I've used and check whether this would be acceptable?

Comment 25

[:Gijs (he/him)]

I suppose one other option would be making the method return a status code rather than the bool it currently does, of the form

2 - nothing happened to the event
1 - we prompted and the user said we should unload
0 - we prompted and the user said we shouldn't unload

Not sure if that's preferable to adding the param...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8349539 [details] [diff] [review]
636374-dont-show-multiple-prompts

Conceptually, this seems fine, but you do need to fire the beforeunload events on the kids no matter what.  You just want to skip the prompting if we've already prompted.

Comment 28

[:Gijs (he/him)]

Attachment: don't show multiple onbeforeunload prompts

OK. This patch does the right thing, I think. Don't really expect/need review before January, but setting it so I don't forget. :-)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8351018 [details] [diff] [review]
don't show multiple onbeforeunload prompts

The IDL should probably document that passing in null means the viewer that this is being called on is the one being unloaded, right?

>+   * Called by calls to permitUnload on descendant content viewers to inform

I don't think this belongs on nsIContentViewer.  Just add an accessor on nsDocumentViewer and static_cast?  That's the only impl of nsIContentViewer anyway.

Alternately, add a noscript version of PermitUnload that takes a native bool pointer and call that when doing the recursive calls, with a pointer to your on-stack boolean?

r=me with that fixed.

Comment 32

[:Gijs (he/him)]

Attachment: don't show multiple onbeforeunload prompts,

Went with the bool option because the patch is so much cleaner this way... Re-requesting review because I am afraid of C++ and idl and this patch is dramatically different. Actually, reasonably confident about the correctness, just not about the order of the parameters of the noscript method... I would have thought inout would normally go after in, but also that non-optional goes before optional. Conundrum. Did I guess right? (fairly exhaustive try run for good measure, even if I don't think we have automated tests for this: https://tbpl.mozilla.org/?tree=Try&rev=0bde305f64cb )

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8355926 [details] [diff] [review]
don't show multiple onbeforeunload prompts,

I think you should just nix the [optional], which is meaningless for [noscript] bits anyway, and then put the inout last.

Also, mark the method as nostdcall and then just "nsresult" instead of NS_IMETHODIMP.

r=me with that.  This seems much simpler!

Comment 34

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8355926 [details] [diff] [review]
> don't show multiple onbeforeunload prompts,
> 
> I think you should just nix the [optional], which is meaningless for
> [noscript] bits anyway, and then put the inout last.
> 
> Also, mark the method as nostdcall and then just "nsresult" instead of
> NS_IMETHODIMP.
> 
> r=me with that.  This seems much simpler!

Whee!

http://hg.mozilla.org/integration/mozilla-inbound/rev/83bc132d37f3

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83bc132d37f3

Comment 36

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Comment on attachment 8355926 [details] [diff] [review]
> don't show multiple onbeforeunload prompts,
> 
> I think you should just nix the [optional], which is meaningless for
> [noscript] bits anyway, and then put the inout last.
> 
> Also, mark the method as nostdcall and then just "nsresult" instead of
> NS_IMETHODIMP.
> 
> r=me with that.  This seems much simpler!

So... do we do changes which have interface (idl) impact at all on aurora? (I searched the web for a bit but could find no reference either for or against it)

I'm asking because I'm contemplating asking for uplift approval, but don't know whether that'd be OK in this case? What with all the dupes getting this to go places sooner rather than later is probably a good idea, but I don't know how that sits with our binary compat and such...

Comment 37

[Boris Zbarsky [:bzbarsky]]

If you want to do this with binary compat, you could add a new interface and hang the method off it...

Comment 38

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

IDL changes on Aurora are OK.

Comment 39

[:Gijs (he/him)]

Comment on attachment 8355926 [details] [diff] [review]
don't show multiple onbeforeunload prompts,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: pages can 'take users hostage' by throwing never-ending streams of "are you sure you want to leave this page" dialogs in their faces which cannot be effectively closed.
Testing completed (on m-c, etc.): been on m-c for a while; manual testing
Risk to taking this patch (and alternatives if risky): Relatively low, but there is a small IDL addition. Another mitigating alternative would be a patch for bug 616853, but at this point it's not sure that that will be ready in time. Even if it is, this patch will provide additional benefit to users.
String or IDL/UUID changes made by this patch: one [noscript,nostdcall] IDL addition. Didn't impact in-tree consumers, will be unlikely to impact other consumers unless they implement the interface themselves or do binary linking

Comment 40

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #37)
> If you want to do this with binary compat, you could add a new interface and
> hang the method off it...

Hmm. For now, let's at least get this to Aurora, unless you feel strongly that we should do a beta-specific patch with a separate interface. (Am I right in thinking that we wouldn't need to have/'keep' the temp interface for aurora/central?) If you do think we should do a beta patch, please let me know and I can try and get it done this weekend - Paris work week next week, so not sure how much I'll be available then, and this doesn't seem like something we want to last-minute-land on beta...

Comment 41

[Boris Zbarsky [:bzbarsky]]

I wouldn't worry about beta for now.  Let's get this landed on m-c and aurora and then figure out beta.

Comment 42

[Bur]

Wouldn't it be possible to simply add a checkbox "don't show this dialog again" to this kind of dialog box similar to what is done with other JS dialog boxes?

That way no matter how someone outsmarts Fx and spawns endless prompts the user will be able to block them.

Comment 43

[Florian Bender]

(In reply to Bur from comment #42)
> Wouldn't it be possible to simply add a checkbox "don't show this dialog
> again" to this kind of dialog box similar to what is done with other JS
> dialog boxes?

Please file a new bug for that. If this is pursued, I'd preselect the checkbox as it makes more sense to only show the dialog once (in this case, at least).

Comment 44

[marco_pal]

(In reply to Bur from comment #42)
> Wouldn't it be possible to simply add a checkbox "don't show this dialog
> again" to this kind of dialog box similar to what is done with other JS
> dialog boxes?
> 
> That way no matter how someone outsmarts Fx and spawns endless prompts the
> user will be able to block them.

But in which case it would be useful for a user to being requested again after he has already chosen to close the tab? I can't think of anyone...
The other dialog boxes can show different information in each one at least, so there is a checkbox

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0032a5a180ff

Comment 46

[Bur]

(In reply to marco_pal from comment #44)
> But in which case it would be useful for a user to being requested again
> after he has already chosen to close the tab? I can't think of anyone...
> The other dialog boxes can show different information in each one at least,
> so there is a checkbox

Actually, never. But apparently it's possible to abuse this JS feature so my reasoning was that this checkbox would prevent all cases in which it's abused. On the other hand, then those guys would probably work around this checkbox tricking Fx into thinking it hasn't been ticked...

Maybe the best solution would be an option to permanently disable this popup. This would be hard to work around.

Comment 47

[Florian Bender]

What about Beta now? Do you want to do that? I believe this might be good to have in the next release because of the number of incidents reported, and it's a big problem for computer-illiterate users who don't know how to force-quit their browser (and for others where the virus has disabled the task manager). 

I think you need to prepare a new patch (at least for the IDL stuff) for beta only if you want to do this.

Comment 48

[:Gijs (he/him)]

(In reply to Bur from comment #46)
> Maybe the best solution would be an option to permanently disable this
> popup. This would be hard to work around.

There is a pref now, see bug 950336.

(In reply to Florian Bender from comment #47)
> What about Beta now? Do you want to do that? I believe this might be good to
> have in the next release because of the number of incidents reported, and
> it's a big problem for computer-illiterate users who don't know how to
> force-quit their browser (and for others where the virus has disabled the
> task manager). 
> 
> I think you need to prepare a new patch (at least for the IDL stuff) for
> beta only if you want to do this.

I'm not 100% convinced. It's a lot of effort and it's getting to be late in the beta cycle. On the other hand, bug 616853 isn't even done yet and will be an even bigger change in terms of behaviour, so maybe this bug is a better bet for uplift. Boris?

Comment 49

[Boris Zbarsky [:bzbarsky]]

This is certainly a much safer uplift than bug 616853!  So if we uplift anything, it should be this bug.

Comment 50

[:Gijs (he/him)]

Attachment: don't show multiple onbeforeunload prompts,

So this is the beta patch I made. It seems to work, but please doublecheck my interface implementation fu, which isn't very good...

Comment 51

[u279076]

Does this have in-testsuite coverage?

Comment 52

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #51)
> Does this have in-testsuite coverage?

No, because AFAIK we don't have a way to interact with these dialogs in tests...

Comment 53

[u279076]

(In reply to :Gijs Kruitbosch from comment #52)
> No, because AFAIK we don't have a way to interact with these dialogs in tests...

Would using Encyclopedia Dramatica be a good way to manually verify this is fixed? If so, maybe we can make a minimized testcase for testing in Mozmill?

Comment 54

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #53)
> (In reply to :Gijs Kruitbosch from comment #52)
> > No, because AFAIK we don't have a way to interact with these dialogs in tests...
> 
> Would using Encyclopedia Dramatica be a good way to manually verify this is
> fixed? If so, maybe we can make a minimized testcase for testing in Mozmill?

I think the attached minimal testcase is good enough. This throws 3 dialogs without the patch, and only 1 with the patch applied.

Comment 55

[u279076]

Flagging for manual verification with the attached testcase.

Henrik, do you think this is something that could be automated with one of the QA frameworks?

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to :Gijs Kruitbosch from comment #52)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #51)
> > Does this have in-testsuite coverage?
> 
> No, because AFAIK we don't have a way to interact with these dialogs in
> tests...

Handling of modal dialogs can be done by overriding the prompt service with your own mockup as how it can be seen here:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/test_bug61098.html?force=1#151

So I would still say this should be in-testsuite.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8361575 [details] [diff] [review]
don't show multiple onbeforeunload prompts,

This is exactly right.  r=me, and thank you!

Comment 58

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) from comment #56)
> (In reply to :Gijs Kruitbosch from comment #52)
> > (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #51)
> > > Does this have in-testsuite coverage?
> > 
> > No, because AFAIK we don't have a way to interact with these dialogs in
> > tests...
> 
> Handling of modal dialogs can be done by overriding the prompt service with
> your own mockup as how it can be seen here:
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/
> test_bug61098.html?force=1#151
> 
> So I would still say this should be in-testsuite.

I've been told by bsmedberg-via-jaws that this isn't actually usable. However, there's an alternative approach using the window watcher. I'll pursue that, but I'm meant to be shipping this frontend project right now, so I might take a bit to get to writing the test...

Comment 59

[:Gijs (he/him)]

Comment on attachment 8361575 [details] [diff] [review]
don't show multiple onbeforeunload prompts,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: pages can 'take users hostage' by throwing never-ending streams of "are you sure you want to leave this page" dialogs in their faces which cannot be effectively closed.
Testing completed (on m-c, etc.): been on m-c for more than a week; manual testing
Risk to taking this patch (and alternatives if risky): Relatively low, but we had to add a separate IDL for beta so as not to change the existing interface. Another mitigating alternative would be a patch for bug 616853, but that looks like it won't be ready in time and is probably riskier to uplift. Even if it is, this patch will provide additional benefit to users.
String or IDL/UUID changes made by this patch: one new non-scriptable IDL with one [nostdcall] method. Didn't impact in-tree consumers, shouldn't impact out-of-tree consumers either.

Comment 60

[bhavana bajaj [:bajaj]]

(In reply to :Gijs Kruitbosch from comment #59)
> Comment on attachment 8361575 [details] [diff] [review]
> don't show multiple onbeforeunload prompts,
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): n/a
> User impact if declined: pages can 'take users hostage' by throwing
> never-ending streams of "are you sure you want to leave this page" dialogs
> in their faces which cannot be effectively closed.
> Testing completed (on m-c, etc.): been on m-c for more than a week; manual
> testing
> Risk to taking this patch (and alternatives if risky): Relatively low, but
> we had to add a separate IDL for beta so as not to change the existing
> interface. Another mitigating alternative would be a patch for bug 616853,
> but that looks like it won't be ready in time and is probably riskier to
> uplift. Even if it is, this patch will provide additional benefit to users.

Thanks for separate IDL change to accommodate this on beta. Confirmed with bsmedberg/jorgev that this should be fine.
> String or IDL/UUID changes made by this patch: one new non-scriptable IDL
> with one [nostdcall] method. Didn't impact in-tree consumers, shouldn't
> impact out-of-tree consumers either.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3108eef95746

Comment 64

[Florian Bender]

Mardeg, the target milestone is for the initial landing and not for any backports (that's what the flags are for). I reset the field.


I think this deserves a relnote à la: "Firefox now prevents malicious scripts from blocking the browser when closing a tab or window."

Comment 65

[:Gijs (he/him)]

(In reply to Florian Bender from comment #64)
> I think this deserves a relnote à la: "Firefox now prevents malicious
> scripts from blocking the browser when closing a tab or window."

We've fixed a specific case regarding multiple frames. There are other ways this can happen, so I don't think we should state something that general.

The ideal fix is to have the dialog be tab-modal, which we're working on in bug 616853, but that's complicated.

Comment 66

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0

Reproduce with Nightly from 2014-01-06 with the attached testcase: 3 dialogs are displayed.
Verified as fixed with Firefox 27 beta 8 (Build ID: 20140120132616) and latest Aurora (Build ID: 20140121004017): only one dialog is displayed.

Comment 68

[bhavana bajaj [:bajaj]]

(In reply to :Gijs Kruitbosch from comment #65)
> (In reply to Florian Bender from comment #64)
> > I think this deserves a relnote à la: "Firefox now prevents malicious
> > scripts from blocking the browser when closing a tab or window."
> 
> We've fixed a specific case regarding multiple frames. There are other ways
> this can happen, so I don't think we should state something that general.
> 
> The ideal fix is to have the dialog be tab-modal, which we're working on in
> bug 616853, but that's complicated.

Removing relnote for now, lets get it on once bug 616853 is ready.

Comment 69

[Florian Bender]

(In reply to bhavana bajaj [:bajaj] from comment #68)
> Removing relnote for now, lets get it on once bug 616853 is ready.

So let it be.

Comment 72

[Paul Silaghi, QA [:pauly]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #66)
> Reproduce with Nightly from 2014-01-06 with the attached testcase: 3 dialogs
> are displayed.
> Verified as fixed with Firefox 27 beta 8 (Build ID: 20140120132616) and
> latest Aurora (Build ID: 20140121004017): only one dialog is displayed.
Verified fixed FF 29b2, 31.0a1 (2014-03-25), Win 7 x64