Bug 118871 – Pages should not be able to abuse users inside beforeunload handlers

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 207037 [details]
Patch v1

[Alexey Proskuryakov]

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.

[Build Bot]

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

[Build Bot]

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

[Ryosuke Niwa]

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.

[Sam Weinig]

(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.

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

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.

[Brady Eidson]

(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  =/

[Mike West]

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.

[Brady Eidson]

Created attachment 207149 [details]
Patch v2 - Tests pass in WK1 and WK2 locally, I think...

[Brady Eidson]

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)

[Alexey Proskuryakov]

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.

[Brady Eidson]

(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.

[Brady Eidson]

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

[Brady Eidson]

http://trac.webkit.org/changeset/152941

[Darin Adler]

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?