Bug 64556 (CVE-2011-2845)

Summary: Fragment navigations should interrupt a provisional load of a different document
Product: WebKit Reporter: Charles Reis <creis>
Component: Page LoadingAssignee: Charles Reis <creis>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, ayao, beidson, creis, darin, dglazkov, eric, fishd, japhet, mihaip, ossy, sreeram, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Charles Reis 2011-07-14 13:21:13 PDT
If a provisional navigation to a different document is in progress but a fragment scroll in the current page commits, we should cancel the provisional navigation.

Letting the provisional navigation continue to load and commit after the fragment scroll is a bit strange in the normal case, but dangerous to the session history if it's a back/forward navigation.  Suppose the user is going forward but interrupts it with a fragment navigation.  When the fragment navigation commits, it clears the forward history, even though the forward navigation is still in progress.

I have a fairly straightforward fix prepared, which calls StopAllLoaders if there's a provisional loader for a different document in FrameLoader::continueFragmentScrollAfterNavigationPolicy.
Comment 1 Charles Reis 2011-07-14 13:31:10 PDT
Created attachment 100853 [details]
Patch
Comment 2 WebKit Review Bot 2011-07-14 13:49:47 PDT
Comment on attachment 100853 [details]
Patch

Attachment 100853 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9059319

New failing tests:
fast/loader/page-dismissal-modal-dialogs.html
Comment 3 WebKit Review Bot 2011-07-14 13:49:52 PDT
Created attachment 100854 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 4 Alexey Proskuryakov 2011-07-14 15:14:45 PDT
The bug seems valid to me, but stopping all loaders doesn't sound right. The current document may have its own loaders, which don't need to be stopped for fragment navigation.
Comment 5 Charles Reis 2011-07-14 15:46:02 PDT
(In reply to comment #4)
> The bug seems valid to me, but stopping all loaders doesn't sound right. The current document may have its own loaders, which don't need to be stopped for fragment navigation.

Yes, and stopAllLoaders also seems to have led to a crash in page-dismissal-modal-dialogs.html (which is happening when we interact with m_documentLoader in checkLoadCompleteForFrame).

Is there a better way to stop just the provisional load?  Presumably we should do it for the whole frame tree (much like stopAllLoaders), but not stop m_documentLoader.  There's clearProvisionalLoad(), but that doesn't deal with the rest of the frame tree.  (Maybe that's ok?)
Comment 6 Mihai Parparita 2011-09-14 16:05:12 PDT
Created attachment 107415 [details]
Patch
Comment 7 Mihai Parparita 2011-09-14 16:09:55 PDT
I've updated Charlie's patch to only stop provisional loaders (via a recursive FrameLoader::stopAllProvisionalLoaders method).

That made fast/loader/page-dismissal-modal-dialogs.html not crash, but it would still fail. This was because the order of events was:
1. main frame "load" fires
2. main frame sets location to resources/pass-and-notify-done.html (the provisional load)
3. main frame "beforeunload" fires
4. Event handler simulates a click on the <a href="#"> anchor node
5. Beyond invoking the showMessages function, since that doesn't preventDefault the event, this triggers a same-document navigation, thus canceling the provisional load.

Since it seemed like having the anchor node be a real link wasn't what was being tested, I've just changed it to a <span>, so that the event handler still gets triggered (which shows an alert during page unload, which is what the test is testing for), but we don't cause a same-document navigation anymore.

+Sreeram in case I'm misunderstanding the purpose of the test and the <a href="#"> part is necessary
Comment 8 Mihai Parparita 2011-09-14 16:10:54 PDT
Cc-ing more people who know FrameLoader.
Comment 9 Alexey Proskuryakov 2011-09-14 16:12:55 PDT
Can a regression test be made for this?

Stopping provisional loaders in subframes when a top frame navigates to an anchor is surprising, too.
Comment 10 Mihai Parparita 2011-09-14 16:25:00 PDT
(In reply to comment #9)
> Can a regression test be made for this?

http/tests/navigation/navigation-interrupted-by-fragment.html tests this, sorry for not mentioning it in the ChangeLog.

> Stopping provisional loaders in subframes when a top frame navigates to an anchor is surprising, too.

Hmm, now that I think about it, yes, I don't see any reason for stopping provisional loads in subframes.  Changed to just stop the current frame's provisional loader.
Comment 11 Mihai Parparita 2011-09-14 16:25:32 PDT
Created attachment 107419 [details]
Patch
Comment 12 Alexey Proskuryakov 2011-09-14 16:38:24 PDT
Comment on attachment 107419 [details]
Patch

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

Thanks, I have no further comment. Others CC'ed on the bug would make better reviewers for actual code change. Considering whether the urls being compared are original or final ones in case of redirect seems important.

> Source/WebCore/loader/FrameLoader.cpp:2708
> +    // If we have a provisional request for a different document, a fragment scroll should cancel it.

It's not entirely obvious why a provisional request for the same document should not be canceled.
Comment 13 Adam Barth 2011-09-14 16:45:10 PDT
Comment on attachment 107419 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2712
> +    // If we have a provisional request for a different document, a fragment scroll should cancel it.
> +    if (m_provisionalDocumentLoader && !equalIgnoringFragmentIdentifier(m_provisionalDocumentLoader->request().url(), request.url())) {
> +        m_provisionalDocumentLoader->stopLoading();
> +        setProvisionalDocumentLoader(0);
> +    }

This part makes sense to me.

> Source/WebCore/loader/HistoryController.cpp:531
> +    // The provisional item may represent a different pending navigation.
> +    // Don't commit it if it isn't a same document navigation.
> +    if (m_currentItem && !m_currentItem->shouldDoSameDocumentNavigationTo(m_provisionalItem.get()))
> +        return;

This part I'm less confident in, mostly because I don't understand HistoryController very well.
Comment 14 Mihai Parparita 2011-09-14 18:07:44 PDT
(In reply to comment #13)
> > Source/WebCore/loader/HistoryController.cpp:531
> > +    // The provisional item may represent a different pending navigation.
> > +    // Don't commit it if it isn't a same document navigation.
> > +    if (m_currentItem && !m_currentItem->shouldDoSameDocumentNavigationTo(m_provisionalItem.get()))
> > +        return;
> 
> This part I'm less confident in, mostly because I don't understand HistoryController very well.

This was in Charlie's original patch, I didn't look at it too closely originally. This happens when there's a provisional history item set (triggered via history.back() or anything else that ends up in FrameLoader::loadDifferentDocumentItem) but then we abort that provisional load because of a fragment change. This didn't have a test originally, I've added http/tests/history/back-with-fragment-change.php to the patch to exercise this behavior. If we do overwrite the current item with the different document provisional one (that belongs to the aborted load), then the back-forward list would end up being:

        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
curr->  http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**

Instead of the expected:

        http://127.0.0.1:8000/history/back-with-fragment-change.php  **nav target**
        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html  **nav target**
curr->  http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
Comment 15 Mihai Parparita 2011-09-14 18:08:27 PDT
Created attachment 107432 [details]
Patch
Comment 16 Sreeram Ramachandran 2011-09-14 19:20:19 PDT
(In reply to comment #7)
> Since it seemed like having the anchor node be a real link wasn't what was being tested, I've just changed it to a <span>, so that the event handler still gets triggered (which shows an alert during page unload, which is what the test is testing for), but we don't cause a same-document navigation anymore.
> 
> +Sreeram in case I'm misunderstanding the purpose of the test and the <a href="#"> part is necessary

LGTM. The anchor isn't important. Any method that executes JS in the context of the other frame works. The simplest way I found was to dispatch a click event to an element in the other frame.. A span works just as well as an anchor for this purpose. Thanks for taking care of the test!
Comment 17 WebKit Review Bot 2011-09-15 05:12:52 PDT
Comment on attachment 107432 [details]
Patch

Attachment 107432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9685040

New failing tests:
http/tests/history/back-with-fragment-change.php
Comment 18 Mihai Parparita 2011-09-15 09:38:34 PDT
(In reply to comment #17)
> New failing tests:
> http/tests/history/back-with-fragment-change.php

I thought the EWS would attach results from failing runs to the bug. Does it no longer do that?
Comment 19 Eric Seidel (no email) 2011-09-15 10:47:56 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > New failing tests:
> > http/tests/history/back-with-fragment-change.php
> 
> I thought the EWS would attach results from failing runs to the bug. Does it no longer do that?

I'm told it was disabled due to space concerns.
Comment 20 Adam Barth 2011-09-15 17:03:11 PDT
Comment on attachment 107432 [details]
Patch

Ok.  I'm not 100% confident in the HistoryController part of the change, but it looks fine as far as I can tell.
Comment 21 Mihai Parparita 2011-09-15 17:08:48 PDT
It looks like http/tests/history/back-with-fragment-change.php fails with release builds (which is what EWS uses. I assume there's some a timing bug in the test, where the load starts already by the time the fragment change is issued, so it's not considered provisional. I'll see if I can tweak it to make it more reliable before landing.
Comment 22 Mihai Parparita 2011-09-15 18:03:04 PDT
Created attachment 107576 [details]
Patch for landing
Comment 23 Mihai Parparita 2011-09-15 18:04:46 PDT
Actually, the problem with the test was that back-with-fragment-change.php was getting cached, so the load history.back() was called was not taking 2 seconds as expected (it was only a matter of luck that it passed with debug builds). Adding the appropriate HTTP headers to disable caching made it pass in both build types.
Comment 24 WebKit Review Bot 2011-09-15 19:15:53 PDT
Comment on attachment 107576 [details]
Patch for landing

Clearing flags on attachment: 107576

Committed r95259: <http://trac.webkit.org/changeset/95259>
Comment 25 WebKit Review Bot 2011-09-15 19:16:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Csaba Osztrogon√°c 2011-09-16 08:28:12 PDT
This test still fails on SL, on GTK and on Qt ...

--- /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/history/back-with-fragment-change-expected.txt 
+++ /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/http/tests/history/back-with-fragment-change-actual.txt 
@@ -4,6 +4,5 @@
 
 ============== Back Forward List ==============
         http://127.0.0.1:8000/history/back-with-fragment-change.php  **nav target**
-        http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html  **nav target**
 curr->  http://127.0.0.1:8000/history/resources/back-with-fragment-change-target.html#foo  **nav target**
 ===============================================
Comment 27 Mihai Parparita 2011-09-16 08:38:28 PDT
Looking into those failures. Windows fails too, presumably because of HTTP server differences: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r95289%20(16342)/http/tests/history/back-with-fragment-change-diffs.txt
Comment 28 Mihai Parparita 2011-09-16 13:43:08 PDT
Leaving this as closed, bug 68278 is open for the failures on other platforms.
Comment 29 David Kilzer (:ddkilzer) 2011-10-21 15:50:08 PDT
<rdar://problem/10327710>
Comment 30 Brady Eidson 2012-03-27 16:52:02 PDT
This caused a regression in Safari choosing the different comics at the top of http://www.uclick.com/client/nyt/db/

Covered in <rdar://problem/11019576>
Comment 31 Mihai Parparita 2012-03-28 16:56:32 PDT
(In reply to comment #30)
> This caused a regression in Safari choosing the different comics at the top of http://www.uclick.com/client/nyt/db/

The comic switching UI boils down to: <a href="#" onclick="window.location = '...'">...</a>. So the regression is expected given the change that was made.

Charlie's original concerns when he filed the bug were about fragment scrolls during back/forward navigation. Charlie, was it http://crbug.com/86758 that prompted this (which was an address bar spoofing security bug triggered by back/forward navigation)?

Maybe it makes sense to revise the fix such that we only cancel provisional navigation during back/forward navigation only.
Comment 32 Charles Reis 2012-06-26 14:30:41 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > This caused a regression in Safari choosing the different comics at the top of http://www.uclick.com/client/nyt/db/
> 
> The comic switching UI boils down to: <a href="#" onclick="window.location = '...'">...</a>. So the regression is expected given the change that was made.
> 
> Charlie's original concerns when he filed the bug were about fragment scrolls during back/forward navigation. Charlie, was it http://crbug.com/86758 that prompted this (which was an address bar spoofing security bug triggered by back/forward navigation)?
> 
> Maybe it makes sense to revise the fix such that we only cancel provisional navigation during back/forward navigation only.

Yes, that was the original bug, which was about using back and forward to cause a URL spoof.  I take it the "#" on the link click is preempting the window.location change?  I'm not 100% sure it's safe to ignore new navigations and only have this apply to back/forward.  It depends on whether the correct URL is displayed if we allow the provisional new navigation to commit.