Bug 118871 - Pages should not be able to abuse users inside beforeunload handlers
Summary: Pages should not be able to abuse users inside beforeunload handlers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-07-18 16:40 PDT by Brady Eidson
Modified: 2013-07-31 12:11 PDT (History)
9 users (show)

See Also:


Attachments
Patch v1 (25.71 KB, patch)
2013-07-18 17:11 PDT, Brady Eidson
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (623.10 KB, application/zip)
2013-07-18 18:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (689.36 KB, application/zip)
2013-07-18 19:58 PDT, Build Bot
no flags Details
Patch v2 - Tests pass in WK1 and WK2 locally, I think... (27.99 KB, patch)
2013-07-19 14:49 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-07-18 16:40:17 PDT
Pages should not be able to abuse users inside beforeunload handlers.

Abusive techniques include showing various forms of modal dialogs inside beforeunload and using iframes.

Some other browsers don't allow modal dialogs inside beforeunload (like alert, confirm, prompt, showModalDialog), and I think WebKit shouldn't by default.

Also, if multiple iframes all try to display a beforeunload confirmation dialog, that seems like spam - Only one should be allowed to be shown.

Finally, iframes from different origins probably shouldn't be allowed to show even one beforeunload confirmation.

This is in radar as <rdar://problem/14475779>
Comment 1 Brady Eidson 2013-07-18 17:11:45 PDT
Created attachment 207037 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2013-07-18 17:55:09 PDT
Comment on attachment 207037 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=207037&action=review

> Source/WebCore/ChangeLog:3
> +        Need a short description (OOPS!).

Yes.

> Source/WebCore/loader/FrameLoader.cpp:2760
> -bool FrameLoader::fireBeforeUnloadEvent(Chrome& chrome)
> +bool FrameLoader::shouldCloseFiringBeforeUnloadEvent(Chrome& chrome, FrameLoader* navigatingFrameLoader)

The new name is difficult to parse, maybe handleBeforeUnloadEvent would be accurate and descriptive enough?

> Source/WebCore/loader/FrameLoader.cpp:2777
>      if (!beforeUnloadEvent->defaultPrevented())

Below this, there is a document->defaultEventHandler() call. Can it do anything that we'd want to bracket with setFrameHandlingBeforeUnloadDismissal/clearFrameHandlingBeforeUnloadDismissal?

> Source/WebCore/loader/FrameLoader.cpp:2798
> +                m_frame->page()->console()->addMessage(JSMessageSource, ErrorMessageLevel, "Blocked attempt to show beforeunload confirmation dialog on behalf of a frame with different security origin from the navigating frame.  Protocols, domains, and ports must match.");

Two spaces?

> Source/WebCore/page/DOMWindow.cpp:1037
> +    // Pages are not allowed to cause modal alerts during BeforeUnload dispatch.

Not sure if I like the capitalization of "BeforeUnload", it's not how the event named.

> Source/WebCore/page/DOMWindow.cpp:2017
> +        printErrorMessage("Use of window.showModalDialog is not allowed during beforeunload event dispatch.");

Add window.print()? :-)

> Source/WebCore/page/Page.cpp:1582
> +void Page::setFrameHandlingBeforeUnloadDismissal(Frame* frame)
> +{
> +    ASSERT(!m_framesHandlingBeforeUnloadDismissal.contains(frame));
> +    m_framesHandlingBeforeUnloadDismissal.add(frame);
> +}
> +
> +void Page::clearFrameHandlingBeforeUnloadDismissal(Frame* frame)
> +{
> +    ASSERT(m_framesHandlingBeforeUnloadDismissal.contains(frame));
> +    m_framesHandlingBeforeUnloadDismissal.remove(frame);
> +}

Can this be a simple number?

"setFrameHandlingBeforeUnloadDismissal" doesn't parse very well.
Comment 3 Build Bot 2013-07-18 18:18:36 PDT
Comment on attachment 207037 [details]
Patch v1

Attachment 207037 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1130538

New failing tests:
fast/events/before-unload-remove-itself.html
fast/loader/page-dismissal-modal-dialogs.html
fast/events/onbeforeunload-focused-iframe.html
fast/events/onunload-clears-onbeforeunload.html
fast/events/before-unload-adopt-within-subframes.html
fast/dom/null-page-show-modal-dialog-crash.html
http/tests/loading/iframe-beforeunload-dialog-matching-ancestor-securityorigin.html
Comment 4 Build Bot 2013-07-18 18:18:38 PDT
Created attachment 207044 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 5 Ryosuke Niwa 2013-07-18 18:29:10 PDT
Comment on attachment 207037 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=207037&action=review

> LayoutTests/ChangeLog:4
> +        Need a short description (OOPS!).
> +        <rdar://problem/14475779> and https://bugs.webkit.org/show_bug.cgi?id=118871.

Please update the bug title.
Comment 6 Sam Weinig 2013-07-18 19:57:25 PDT
(In reply to comment #3)
> (From update of attachment 207037 [details])
> Attachment 207037 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/1130538
> 
> New failing tests:
> fast/events/before-unload-remove-itself.html
> fast/loader/page-dismissal-modal-dialogs.html
> fast/events/onbeforeunload-focused-iframe.html
> fast/events/onunload-clears-onbeforeunload.html
> fast/events/before-unload-adopt-within-subframes.html
> fast/dom/null-page-show-modal-dialog-crash.html
> http/tests/loading/iframe-beforeunload-dialog-matching-ancestor-securityorigin.html

These should probably be investigated.
Comment 7 Build Bot 2013-07-18 19:58:33 PDT
Comment on attachment 207037 [details]
Patch v1

Attachment 207037 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1110734

New failing tests:
fast/dom/null-page-show-modal-dialog-crash.html
fast/events/before-unload-with-subframes.html
fast/events/before-unload-adopt-within-subframes.html
fast/events/onbeforeunload-focused-iframe.html
fast/events/onunload-clears-onbeforeunload.html
Comment 8 Build Bot 2013-07-18 19:58:35 PDT
Created attachment 207053 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 9 Brady Eidson 2013-07-18 22:45:08 PDT
I was in the middle of running the entire suite when I uploaded the patch, as I wanted to get the review ball rolling.

The failures will almost certainly be due to the fact that alert() can no longer be used for poor-man's logging inside beforeunload, or other reliance on the behavior that is changed.

I will, of course, investigate the test failures before pursuing landing this.
Comment 10 Brady Eidson 2013-07-18 22:47:47 PDT
(In reply to comment #7)
> (From update of attachment 207037 [details])
> Attachment 207037 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/1110734
> 
> New failing tests:
> fast/dom/null-page-show-modal-dialog-crash.html
> fast/events/before-unload-with-subframes.html
> fast/events/before-unload-adopt-within-subframes.html
> fast/events/onbeforeunload-focused-iframe.html
> fast/events/onunload-clears-onbeforeunload.html

These links don't actually go to a results page where you can see the diff between expected and actual  =/
Comment 11 Mike West 2013-07-19 01:32:43 PDT
Comment on attachment 207037 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=207037&action=review

I like this patch, and I have one drive-by comment.

>> Source/WebCore/loader/FrameLoader.cpp:2798
>> +                m_frame->page()->console()->addMessage(JSMessageSource, ErrorMessageLevel, "Blocked attempt to show beforeunload confirmation dialog on behalf of a frame with different security origin from the navigating frame.  Protocols, domains, and ports must match.");
> 
> Two spaces?

Also, you can avoid including 'PageConsole.h' if you add these messages to the console via 'm_frame->document()->addConsoleMessage(...)'. See the 'X-Frame-Options' console messages in this file for an example.
Comment 12 Brady Eidson 2013-07-19 14:49:09 PDT
Created attachment 207149 [details]
Patch v2 - Tests pass in WK1 and WK2 locally, I think...
Comment 13 Brady Eidson 2013-07-19 21:51:21 PDT
Mac EWS seems to have gotten hung up.  I'll look at landing tomorrow and manually watch for fallout (as mentioned, no failures locally, either WK1 or WK2)
Comment 14 Alexey Proskuryakov 2013-07-19 22:37:43 PDT
Comment on attachment 207149 [details]
Patch v2 - Tests pass in WK1 and WK2 locally, I think...

View in context: https://bugs.webkit.org/attachment.cgi?id=207149&action=review

> Source/WebCore/loader/FrameLoader.cpp:2759
> +bool FrameLoader::handleBeforeUnloadEvent(Chrome& chrome, FrameLoader* navigatingFrameLoader)

Looking at this again, I want to suggest the name "frameLoaderBeingNavigated". What do you think?

> Source/WebCore/loader/FrameLoader.cpp:2769
> +    
>  

This extra blank line appears accidental.

> Source/WebCore/loader/FrameLoader.cpp:2775
> +    // We store the frame's page in a local variable because the frame might get detached inside dispatchEvent.
> +    Page* page = m_frame->page();
> +    ASSERT(page);

Should it also be protected with a RefPtr?

I don't think that this ASSERT is useful - it doesn't help catch a null pointer, we'll get a very clear explanation of what happened when crashing on a dereference.
Comment 15 Brady Eidson 2013-07-19 23:34:57 PDT
(In reply to comment #14)
> (From update of attachment 207149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207149&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2759
> > +bool FrameLoader::handleBeforeUnloadEvent(Chrome& chrome, FrameLoader* navigatingFrameLoader)
> 
> Looking at this again, I want to suggest the name "frameLoaderBeingNavigated". What do you think?

I'm okay with this.

> > Source/WebCore/loader/FrameLoader.cpp:2769
> > +    
> >  
> 
> This extra blank line appears accidental.

Indeed!

> > Source/WebCore/loader/FrameLoader.cpp:2775
> > +    // We store the frame's page in a local variable because the frame might get detached inside dispatchEvent.
> > +    Page* page = m_frame->page();
> > +    ASSERT(page);
> 
> Should it also be protected with a RefPtr?

Pages are not ref counted.  I think the only action that can possibly involved destruction of a Page would require a cycle of the runloop.

> I don't think that this ASSERT is useful - it doesn't help catch a null pointer, we'll get a very clear explanation of what happened when crashing on a dereference.

You're right.
Comment 16 Brady Eidson 2013-07-19 23:38:38 PDT
wk2 passed, and I think wk1 should be fine...  gonna land this then go to bed.

http://24.media.tumblr.com/tumblr_lzys30jtSU1rqvy12o1_500.jpg
Comment 17 Brady Eidson 2013-07-19 23:43:06 PDT
http://trac.webkit.org/changeset/152941
Comment 18 Darin Adler 2013-07-31 12:11:14 PDT
Comment on attachment 207149 [details]
Patch v2 - Tests pass in WK1 and WK2 locally, I think...

View in context: https://bugs.webkit.org/attachment.cgi?id=207149&action=review

>>> Source/WebCore/loader/FrameLoader.cpp:2775
>>> +    ASSERT(page);
>> 
>> Should it also be protected with a RefPtr?
>> 
>> I don't think that this ASSERT is useful - it doesn't help catch a null pointer, we'll get a very clear explanation of what happened when crashing on a dereference.
> 
> Pages are not ref counted.  I think the only action that can possibly involved destruction of a Page would require a cycle of the runloop.

I am concerned about verifying this hypothesis. We are using the page pointer after dispatching to arbitrary JavaScript. Is it guaranteed that the page object will still be around?