Last Comment Bug 391834 - Don't allow alert/confirm/prompt in onbeforeunload, onunload and onpagehide
: Don't allow alert/confirm/prompt in onbeforeunload, onunload and onpagehide
Status: RESOLVED FIXED
[sg:want P4]
: dev-doc-needed, sec-want
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- enhancement with 7 votes (vote)
: mozilla17
Assigned To: Neil Deakin
:
Mentors:
http://www.internetisseriousbusiness....
: 402257 410247 (view as bug list)
Depends on: 969787 784142 803022 827084 856977
Blocks: eviltraps 588292 613800 664586 CVE-2012-4200 792458
  Show dependency treegraph
 
Reported: 2007-08-11 08:13 PDT by Volkmar Kostka
Modified: 2014-02-12 07:01 PST (History)
42 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
onunload.html (205 bytes, text/html)
2010-09-20 19:28 PDT, Biju
no flags Details
OnlyOnBeforeUnload.html (107 bytes, text/html)
2010-11-20 17:45 PST, Biju
no flags Details
test page (165 bytes, text/html)
2011-12-06 14:58 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
WIP (24.79 KB, patch)
2011-12-06 15:01 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Review
Part 1 - cleanup of dialog abuse handling (16.27 KB, patch)
2012-06-22 10:43 PDT, Neil Deakin
no flags Details | Diff | Review
Part 2 - add cabability to prevent further dialogs while unloading (17.43 KB, patch)
2012-06-22 10:44 PDT, Neil Deakin
no flags Details | Diff | Review
Part 1 - cleanup of dialog abuse handling (16.22 KB, patch)
2012-06-27 11:44 PDT, Neil Deakin
bugs: review-
Details | Diff | Review
Part 2 - add cabability to prevent further dialogs while unloading (17.51 KB, patch)
2012-06-28 16:54 PDT, Neil Deakin
bugs: review-
Details | Diff | Review
Part 1.2 - cleanup of dialog abuse handling (15.78 KB, patch)
2012-08-08 08:33 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 2.2 - add cabability to prevent further dialogs while unloading (17.60 KB, patch)
2012-08-08 08:34 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 2.3 - add cabability to prevent further dialogs while unloading, with additional iframe test (18.85 KB, patch)
2012-08-10 18:54 PDT, Neil Deakin
no flags Details | Diff | Review

Description Volkmar Kostka 2007-08-11 08:13:45 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007081005 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/2007081005 Minefield/3.0a8pre

Using onbeforeunload with a loop can render the browser useless.

As example:
<body onbeforeunload="for ( var i = 0; i < 999999; i++) { alert('Alert'); }
return false;">

If JS is enabled and you tries top close the tab you run into an endless loop with an alert which blocks the complete browser.
It may be even possible that you can trigger it automatically with a redirect clause.
It is not a bug just malicious code. IE6 is also affected but i could not get it to work with Opera.


Reproducible: Always

Steps to Reproduce:
1) Visit the mentioned site with JS on.
2) Try to close the tab.

Actual Results:  
You get an long chain of alerts and the tab is not closed.

Expected Results:  
Allow to ignore the return value from onbeforeunload and allow to close the tab nevertheless.

If you do not know how to fix it (disable JS before closing the tab) you are not even able to get rid of it when sessionstore is enabled. If the thing with redirect is possible you have to abandon your stored session to use your browser again.
Comment 1 Jesse Ruderman 2007-08-11 17:27:22 PDT
Being able to get out of an infinite loop of alerts is covered by other bugs (bug 123913, bug 61098, bug 59314), so I'm changing the summary of this bug to focus on the onbeforeunload part.
Comment 2 Spencer Selander [greenknight] 2007-08-12 03:52:21 PDT
I was able to reproduce this on the Branch:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070811 BonEcho/2.0.0.7pre ID:2007081103

Nasty - a malicious hacker could really make hay with this.
Comment 3 Ria Klaassen (not reading all bugmail) 2007-08-12 04:48:47 PDT
I wonder in what cases websites will use this? What could be the advantage for them? After such an experience visitors with avoid the website like the plague.
Comment 4 Volkmar Kostka 2007-08-12 04:59:23 PDT
It is just malicious code.
Let's say someone was able to hack a web server and places this code on any page. If there are internal web pages which are required for the day-to-day use this will wreck havoc in that company.
Sort of DoS attack.
Comment 5 Jesse Ruderman 2007-11-02 17:37:34 PDT
*** Bug 402257 has been marked as a duplicate of this bug. ***
Comment 7 Jesse Ruderman 2007-12-30 11:41:17 PST
*** Bug 410247 has been marked as a duplicate of this bug. ***
Comment 8 WADA 2008-01-04 01:27:15 PST
Just an idea.
(A) For DoS by infinite dialog loop
    Button/option to "Stop (malicious) JavaScript Execution" or "Stop next
    alert/confirm/prompt" in alert/confirm/prompt dialog
(B) For DoS by infinite loop
    (B-1) "Stop (malicious) JavaScript Execution" menu or button,
          with highest task priority among all of Back-end/UI/User-Script tasks
    (B-2) Option to limit loop counter(e.g. site_script.max_loop_count),
          for site specified JavaScript(and/or for Script by extension)
(C) For DoS by "very-small/ZERO value" of setInterval/setTimeout
    Minimum interval/timeout option
(D) For Dos by recursive&infinite call/include
    (D-1) Option of max-depth for iframe, link-rel-style, object,
          Script-generated-Script-Tag, object etc. 
    (D-2) Loop check for recursive include
Comment 9 Jesse Ruderman 2008-01-04 18:36:13 PST
Making it possible to abort scripts from the alert() dialog is bug 61098.  That's mostly orthogonal to this bug.
Comment 10 Tim McCormack 2008-02-15 11:02:44 PST
alert() should be allowed in on(before)unload code. Many sites use it to prevent the user from losing data by accidentally closing the tab.

The decision of what to do with abusive loops is the subject of bug 61098.
Comment 11 Jesse Ruderman 2008-02-15 16:02:06 PST
Many sites *return a string* from onbeforeunload, which triggers a dialog that lets users stay on the page.  alert() doesn't let users stay on the page, so I don't see why sites would do that.  Do you know of sites that do?
Comment 12 Tim McCormack 2008-02-15 17:06:07 PST
@Jesse: D'oh! Okay, in that case I have no objection to disabling all modal interface components in onunload/onbeforeunload.
Comment 13 Al Billings [:abillings] 2008-03-14 00:21:12 PDT
The "rickroll" bug. We should fix this. :-)
Comment 14 strata_ranger 2009-07-26 08:12:57 PDT
Onunload/onbeforeunload can have legitimate uses, yes, but they're also popular (very popular?) vectors for malicious websites to launch Javascript code and/or XSS with.

If the latter potential outweighs the former (and I would suggest it does, if not clearly so) then this is a definite hazard that must be made a user configurable option, e.g. (Tools > Options > Javascript > Advanced) or at least via about:config.

I was recently hit by a random prompt("Are you sure you want to navigate away from this page?") (exact words) while browsing across the Deviantart domain (normally a safe domain in this respect), which looped until I hit the "Cancel" (as in "stay on this page") button, upon which it redirected my tab to somewhere in the MySpace domain.  I shut off Javascript using another window to mitigate any further damage, but being unable to prevent this sort of attack in the first place is more frustrating than the attack itself.
Comment 15 Biju 2010-09-20 19:28:22 PDT
Created attachment 477005 [details]
onunload.html
Comment 16 [not reading bugmail] 2010-11-15 22:23:23 PST
(In reply to comment #11)
> Many sites *return a string* from onbeforeunload, which triggers a dialog that
> lets users stay on the page.  

For example another product: JIRA (similar to bugzilla) uses this when I use Chrome. The dialog shows up when I try to leave a page that I was updating to do something else, but didn't want to lose my changes.
Comment 17 Jesse Ruderman 2010-11-15 23:21:08 PST
This bug isn't about removing the return-a-string "are you sure you want to leave" dialog. It's about removing the ability to pop up *other* dialogs from the beforeunload event, which are problematic.

If you want to argue about whether we should have onbeforeunload at all, go to bug 578828 ;)
Comment 18 Biju 2010-11-20 17:45:10 PST
Created attachment 492119 [details]
OnlyOnBeforeUnload.html
Comment 19 Jesse Ruderman 2011-03-07 16:29:37 PST
Making this worse, these alerts are old-style modal rather than tab-modal :(

data:text/html,<body onbeforeunload=alert(5)>
Comment 20 Biju 2011-03-07 20:51:10 PST
(In reply to comment #19)
> Making this worse, these alerts are old-style modal rather than tab-modal :(
culprit Bug 613800
Comment 21 David [:auscompgeek] 2011-04-11 21:38:59 PDT
Suggesting alternatives to this bug:

* Disallow (infinite) loops in onBeforeUnload, onUnload and onPageHide
* Disallow alert/confirm/prompt in onUnload and onPageHide only (as long as infinite loops are disallowed in onBeforeUnload)
* Disallow all infinite loops with only alert/confirm/prompt
* Don't fix this bug at all, instead relying on tab-modal prompts (bad for UX)
Comment 22 Biju 2011-04-13 19:19:43 PDT
(In reply to comment #21)
Already disallow infinite loops implemented in Fx 4
This bug is clearly disallowing and message from web developer on page close.
Comment 23 dindog 2011-04-25 19:09:10 PDT
It's that possible to disable some specific sites or all onbeforeunload pop-up?

Sometime found it's really annoying
Comment 24 Sergey Zh 2011-07-24 08:32:48 PDT
The proper fix against malicious javascript is to make alert/confirm non-modal (tab-modal)!

Currently confirm is modal when called from beforeunload, and not modal in other cases. Removing confirm from onbeforeunload would break the ability of web application to ask user to save his data before leaving (which he might have spent a lot of time to enter). This would reduce appeal of recommending mozilla to users of such web application.
Comment 25 Jesse Ruderman 2011-07-24 11:20:40 PDT
Sergey, I think you're misunderstanding the onbeforeunload API. There's a confirm-like dialog that's part of the onbeforeunload API itself; this is about blocking additional alerts/confirms/prompts.

I see you've also commented in bug 578828; thanks.
Comment 26 Zibi Braniecki [:gandalf][:zibi] 2011-08-20 02:43:07 PDT
webkit landed it https://bugs.webkit.org/show_bug.cgi?id=56397
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-06 14:56:48 PST
I tested Chrome 15.0.874.121m, Opera 11.52, IE 9, and Safari 5.1.2 (Mac).

IE 9 is the only browser that seems to support onbeforeunload.

All of them seem to ignore alert() from onunload/onpagehide, except for Safari, which ignores them on tab close but not on tab navigation, and IE 9, which allows alert() from onunload on navigation (but not tab close).
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-06 14:58:48 PST
Created attachment 579471 [details]
test page
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-06 15:01:11 PST
Created attachment 579473 [details] [diff] [review]
WIP

This has some other stuff in it that may not be necessary, but I wanted to back this up.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-16 15:59:07 PDT
The HTML spec has specified how this should work in more detail since I last looked into this (see http://html5.org/r/6966 ). I think implementing that behavior exactly would require some more invasive changes, but it would probably be a cleaner and more robust solution.
Comment 31 Neil Deakin 2012-06-12 10:21:46 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30)
> The HTML spec has specified how this should work in more detail since I last
> looked into this (see http://html5.org/r/6966 ). I think implementing that
> behavior exactly would require some more invasive changes, but it would
> probably be a cleaner and more robust solution.

What changes?

The algorithm described in the spec seems to match what the code does, apart from the counter and state stuff which seems to have changed since this comment and is now vague about what it means.

Your patch above seems to fix the bug though. What's left to do here?
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-20 11:53:31 PDT
Thanks for looking that stuff over. Good to hear that it matches what's specced pretty closely.

Much of the changes to nsGlobalWindow (basically, all changes except from the addition of mDialogsPermanentlyDisabled) are cleanup, that might be best split into its own preliminary patch.

The tabbrowser/nsIContentViewer/nsDOMWindowUtils changes are the meat of the functional change (one thing I forgot in the patch: nsDOMWindowUtils::PreventFurtherDialogs will need a security check). And then we'll need some tests for this (ideally based on the HTML spec).
Comment 33 Neil Deakin 2012-06-21 13:08:14 PDT
So I updated the patches but some tests fail when run as a batch, although not when run individually. This is because the code within nsGlobalWindow::DialogsAreBeingAbused is checking to see if the time since the last dialog was shown is less than 3 seconds, and displaying a 'prevent additional dialogs' dialog.

The last dialog was from an different earlier test that happened to run less than 3 seconds ago.

The tests worked before because showModalDialog didn't perform this timestamp check.

What is the correct behaviour here? Should the timestamp be reset when changing pages, or should the tests be changed to wait for a bit, or should the tests change the 'time between dialogs' preference temporarily to some very high value?
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-21 18:16:32 PDT
Neil and I discussed this on IRC - the timestamp should certainly not be persisting across page loads.

Looking over this further, though, I'm not sure how this is actually happeening: the state seems to be kept on the inner window (which shouldn't persist across different page navigations). Perhaps the problem is actually that we're using the top-most window - I don't actually understand why we do that, at least not for mLastDialogQuitTime/mDialogAbuseCount.
Comment 35 Neil Deakin 2012-06-22 07:39:56 PDT
I would assume that the top window is used so that a series of successive dialogs cannot be opened from different child frames. In this case, the top window is the test harness page (http://mochi.test:8888/tests/dom/tests/mochitest/bugs/)

We could change this to set the time on the parent frame from the same domain, but it's not clear to me that it isn't intentionally set of the top page to avoid user annoyance, since in a situation where multiple prompts appear successively, it doesn't seem like the user would care what page or frame they were from.
Comment 36 Neil Deakin 2012-06-22 10:43:28 PDT
Created attachment 635802 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling
Comment 37 Neil Deakin 2012-06-22 10:44:25 PDT
Created attachment 635804 [details] [diff] [review]
Part 2 - add cabability to prevent further dialogs while unloading
Comment 38 Neil Deakin 2012-06-22 10:46:19 PDT
These are the patches so far, with the remaining issue being the broken tests as described above.
Comment 39 Neil Deakin 2012-06-27 11:44:04 PDT
Created attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling

This version moves the check at the end of DialogsAreBeingAbused so that tests no longer fail. Theres a leak with these patches now, or still, that I need to investigate.
Comment 40 Neil Deakin 2012-06-28 16:54:52 PDT
Created attachment 637733 [details] [diff] [review]
Part 2 - add cabability to prevent further dialogs while unloading

Add security check and fix test to remove listener so a leak doesn't occur.
Comment 41 Neil Deakin 2012-08-03 06:30:05 PDT
Comment on attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling

Gavin hinted that smaug might be willing to review this instead.
Comment 42 Olli Pettay [:smaug] 2012-08-05 15:34:42 PDT
Comment on attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling

> bool
>-nsGlobalWindow::DialogOpenAttempted()
>-{
>-  nsGlobalWindow *topWindow = GetScriptableTop();
>+nsGlobalWindow::DialogsAreBlocked(bool *beingAbused)
* goes with the type, and parameters should be named aParameterName


...hmm, this patch is quite tricky to review. I need some sleep before r-/+
Comment 43 Olli Pettay [:smaug] 2012-08-07 03:59:02 PDT
Comment on attachment 637197 [details] [diff] [review]
Part 1 - cleanup of dialog abuse handling


> bool
>-nsGlobalWindow::DialogOpenAttempted()
>-{
>-  nsGlobalWindow *topWindow = GetScriptableTop();
>+nsGlobalWindow::DialogsAreBlocked(bool *beingAbused)
>+{
>+  *beingAbused = false;
>+
>+  nsGlobalWindow * topWindow = GetTopInnerWin();
So you change behavior here.
GetScriptableTop -> GetTopInnerWin


> 
>-  if (AreDialogsBlocked() || !ConfirmDialogAllowed())
>+  bool needToPromptForAbuse;
>+  if (DialogsAreBlocked(&needToPromptForAbuse))
>+    return NS_ERROR_NOT_AVAILABLE;
if (expr) {
  stmt;
}

>+  bool needToPromptForAbuse;
>+  if (DialogsAreBlocked(&needToPromptForAbuse))
>+    return NS_ERROR_NOT_AVAILABLE;
ditto
Comment 44 Olli Pettay [:smaug] 2012-08-07 04:13:58 PDT
Comment on attachment 637733 [details] [diff] [review]
Part 2 - add cabability to prevent further dialogs while unloading

>+nsGlobalWindow::PreventFurtherDialogs(bool aPermanent)
> {
>   nsGlobalWindow *topWindow = GetScriptableTop();
>   if (!topWindow) {
>     NS_ERROR("PreventFurtherDialogs() called without a top window?");
>     return;
>   }
> 
>   topWindow = topWindow->GetCurrentInnerWindowInternal();
I don't see where aPermanent is used.
Comment 45 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-07 11:38:53 PDT
(In reply to Olli Pettay [:smaug] from comment #44)
> >   topWindow = topWindow->GetCurrentInnerWindowInternal();
> I don't see where aPermanent is used.

nsDOMWindowUtils::PreventFurtherDialogs() calls nsGlobalWindow::PreventFurtherDialogs(true). Perhaps the functions should have different names, since they slightly different things.
Comment 46 Neil Deakin 2012-08-07 11:53:31 PDT
> >+nsGlobalWindow::PreventFurtherDialogs(bool aPermanent)
> > {
> >   nsGlobalWindow *topWindow = GetScriptableTop();
> >   if (!topWindow) {
> >     NS_ERROR("PreventFurtherDialogs() called without a top window?");
> >     return;
> >   }
> > 
> >   topWindow = topWindow->GetCurrentInnerWindowInternal();
> I don't see where aPermanent is used.

Actually, the lines that used this didn't make it into the patch. I'll make a new patch soon.
Comment 47 Neil Deakin 2012-08-08 08:33:40 PDT
Created attachment 650138 [details] [diff] [review]
Part 1.2 - cleanup of dialog abuse handling
Comment 48 Neil Deakin 2012-08-08 08:34:26 PDT
Created attachment 650139 [details] [diff] [review]
Part 2.2 - add cabability to prevent further dialogs while unloading
Comment 49 Olli Pettay [:smaug] 2012-08-08 09:24:36 PDT
Comment on attachment 650138 [details] [diff] [review]
Part 1.2 - cleanup of dialog abuse handling


> bool
>-nsGlobalWindow::DialogOpenAttempted()
>-{
>+nsGlobalWindow::DialogsAreBlocked(bool *beingAbused)
bool* aBeingAbused


>+  bool DialogsAreBlocked(bool *beingAbused);
bool* aBeingAbused
Comment 50 Olli Pettay [:smaug] 2012-08-08 09:33:37 PDT
Comment on attachment 650139 [details] [diff] [review]
Part 2.2 - add cabability to prevent further dialogs while unloading

We need tests for iframes.
Comment 51 Neil Deakin 2012-08-08 10:40:19 PDT
(In reply to Olli Pettay [:smaug] from comment #50)
> Comment on attachment 650139 [details] [diff] [review]
> Part 2.2 - add cabability to prevent further dialogs while unloading
> 
> We need tests for iframes.

What do you think needs to be tested here?
Comment 52 Olli Pettay [:smaug] 2012-08-08 10:48:20 PDT
Same things which tested for the top level content page.
Comment 53 Neil Deakin 2012-08-10 18:54:15 PDT
Created attachment 651072 [details] [diff] [review]
Part 2.3 - add cabability to prevent further dialogs while unloading, with additional iframe test
Comment 55 Justin Dolske [:Dolske] 2012-08-14 19:06:30 PDT
Hooray! \o/
Comment 56 Florian Bender 2012-10-17 15:59:09 PDT
Crashes in vBulletin 4 editors. Could reproduce this on contao-community.org: I wanted to answer, entered a response but wanted to go back and check the original message (two-finger-swipe), message showed for a second, and boom. 

Shall I file a new bug?


This is what I got to see from the crash report: 

AdapterDeviceID: 0x 166
AdapterVendorID: 0x8086
Add-ons: kitsuneymg%40gmail.com:1.0.6,macos-keychain%40fitzell.ca:1.1.3,youtubeunblocker%40unblocker.yt:0.2.0,%7B45d8ff86-d909-11db-9705-005056c00008%7D:1.1.0,%7Baede9b05-c23c-479b-a90e-9146ed62d377%7D:1.2.1,firefontfamily%40firebugextensions.org:0.1.2,artur.dubovoy%40gmail.com:3.7.1,%7B73a6fe31-595d-460b-a920-fcc0f8843232%7D:2.5.7,%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D:17.0,%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D:2.1.2,firebug%40software.joehewitt.com:1.10.4
BuildID: 20121010150351
Comments: crashing when unload: when in vBulletin 4 (on contao-community.de) editor with some text entered, navigating back via gesture will show prompt() asking the user if he wants to discard his text and navigate away from the editor – message will appear and Fx immediately crashes

will post a bug. 
CrashTime: 1350512527
EMCheckCompatibility: true
Email: ***
FramePoisonBase: 7ffffffff0dea000
FramePoisonSize: 4096
InstallTime: 1350329730
Notes: AdapterVendorID: 0x8086, AdapterDeviceID: 0x 166GL Context? GL Context+ GL Layers? GL Layers+ WebGL? WebGL+ 
ProductID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
ProductName: Firefox
ReleaseChannel: beta
StartupTime: 1350372636
Theme: classic/1.0
Throttleable: 1
URL: https://www.contao-community.de/newreply.php?p=227322&noquote=1
Vendor: Mozilla
Version: 17.0
Comment 57 Dão Gottwald [:dao] 2012-10-18 00:07:40 PDT
(In reply to Florian Bender from comment #56)
> Shall I file a new bug?

Yes.
Comment 58 Chen Zhixiang 2013-11-12 07:53:05 PST
I hate this feature! It is an evil function to do modal confirm dialog in page unload event.
Could Mozilla/FF provide a way to disable it? Much Much Thanks.

Note You need to log in before you can comment on or make changes to this bug.