Bug 73083 (CVE-2012-2815)

Summary: Fix the Frame Leak Attack
Product: WebKit Reporter: Paul Stone <bugzilla>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cevans, dglazkov, eric, japhet, kadam, ossy, tsepez, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: https://bug583889.bugzilla.mozilla.org/attachment.cgi?id=465540
Attachments:
Description Flags
Test case for blocking cross-origin fragment navigations which flunk at present.
none
Test case + code fragment
none
Patch
tsepez: review-
New Patch
none
Pacth
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04
none
Patch for review.
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-04
none
Patch addressing #38
none
Patch.
none
Alternate patch
abarth: review+
Patch for landing with ChangeLog edits.
none
Broken testcase (works in Firefox) none

Description Paul Stone 2011-11-24 09:23:18 PST
The 'frame leak attack' allows an attacker to find out information about a web page by loading it in a nested iframe, navigating to various #fragments and determining if the page scrolled or not. Depending on the website, this can be used to leak sensitive information.

For more information on the frame leak attack, see https://media.blackhat.com/bh-us-10/whitepapers/Bursztein_Gourdin_Rydstedt/BlackHat-USA-2010-Bursztein-Bad-Memories-wp.pdf (page 18)
or 
http://www.contextis.com/research/white-papers/clickjacking/Context-Clickjacking_white_paper.pdf (page 16)

Mozilla fixed this in Firefox by preventing an outer page from scrolling if an inner frame is navigated (see https://bugzilla.mozilla.org/show_bug.cgi?id=583889). This has caused a few breakages on real-world sites (see https://bugzilla.mozilla.org/show_bug.cgi?id=638598). However, a slight refinement could be made by scrolling parent frames only if they belong to the same origin as the child frame.
Comment 1 Paul Stone 2011-11-24 09:24:24 PST
There's no need for this bug to be private, since this technique is publically known. I reported under 'security' so the right people would see it.
Comment 2 Lucas Forschler 2011-11-29 09:12:21 PST
<rdar://problem/10497237>
Comment 4 Thomas Sepez 2012-03-19 12:28:41 PDT
Seems like the only viable solution is to prevent fragment scrolling in the cross-origin case, which mimics what happens in FF.
Comment 5 Thomas Sepez 2012-03-19 12:36:45 PDT
Created attachment 132630 [details]
Test case for blocking cross-origin fragment navigations which flunk at present.
Comment 6 Paul Stone 2012-03-19 12:37:45 PDT
I don't think Mozilla's solution takes origin into account - it simply never scrolls a parent frame as a result of a child frame scrolling. However I believe that has caused some amount of sites to break.
Comment 7 Adam Barth 2012-03-19 13:09:35 PDT
Do you know who worked on this change in Firefox?  We could ask them for advice.
Comment 8 Paul Stone 2012-03-19 13:12:59 PDT
Jonas Sicking fixed it - see https://bugzilla.mozilla.org/show_bug.cgi?id=583889
Comment 9 Thomas Sepez 2012-03-19 15:00:39 PDT
Created attachment 132679 [details]
Test case + code fragment

Here's the proposed code change.   It looks like it may have broken some tests, so cleanup required before landing.
Comment 10 Adam Barth 2012-03-19 15:08:37 PDT
Comment on attachment 132679 [details]
Test case + code fragment

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

So, if you're a cross-origin iframe, then fragment navigations don't work at all?  For example, if you're inside the Digg bar (assuming that still exists), then table-of-contents links don't work?

> Source/WebCore/loader/FrameLoader.cpp:2785
> +    if (FrameView* view = m_frame->view()) {

Prefer early return.

> Source/WebCore/loader/FrameLoader.cpp:2789
> +            const Document* activeDocument = m_frame->document();

const Document* => Document*     (The const here is pretty meaningless.)

> Source/WebCore/loader/FrameLoader.cpp:2791
> +            if (activeDocument && parentDocument) {

This check should always pass.  Frame's always have documents.

> Source/WebCore/loader/FrameLoader.cpp:2794
> +                if (!activeSecurityOrigin->canAccess(parentSecurityOrigin))

I would just do this all on one line:

if (!m_frame->document()->securityOrigin()->canAccess(parent->document()->securityOrigin())
    ...
Comment 11 Thomas Sepez 2012-03-19 15:32:42 PDT
Ok.  The check for parent document mirrors one at FrameLoader.cpp:1510


       Document* ancestorDocument = ancestorFrame->document();
        if (!ancestorDocument)
            return true;

is this dead code that can be removed as well?
Comment 12 Adam Barth 2012-03-19 15:53:23 PDT
> is this dead code that can be removed as well?

I believe so.  It used to be that document() could be null, but not anymore.  There is still a tiny amount of code that needs to cope with a null document, but that's only while running FrameLoader::init().
Comment 13 Thomas Sepez 2012-03-19 16:05:39 PDT
I'm still concerned about landing this since the referenced FF bug has has some fallout.  What's the best way to see if this is a link click from within the frame vs. a navigation by the parent?
Comment 14 Adam Barth 2012-03-19 16:46:24 PDT
We don't have a good mechanism for that currently, but it's something that we've wanted in other contexts as well.  The idea would be to associate a SecurityOrigin object with FrameLoadRequests.  I see to remember that there's some subtly involved, but I don't remember the details.
Comment 15 Thomas Sepez 2012-03-19 16:49:08 PDT
Note that the FF folks won't consider accomodating this case because one could induce a click by clickjacking.  I'm not concerned because the more significant leaks require repeated probing (as in the linkedin example).
Comment 16 Thomas Sepez 2012-03-19 16:50:23 PDT
(In reply to comment #14)
> We don't have a good mechanism for that currently, but it's something that we've wanted in other contexts as well.  The idea would be to associate a SecurityOrigin object with FrameLoadRequests.  I see to remember that there's some subtly involved, but I don't remember the details.

Ah.  I like the idea.
Comment 17 Chris Evans 2012-03-19 18:14:21 PDT
It looks like this was fixed in FF4. As a tweak landed to a 20%+ share browser over a year ago, I'd not expect a significant compat problem at this stage? If it's simpler / quicker to just copy FF's approach, I'd vote for that as a first approach. We'd get immediate testing in Chrome's dev channel.
Comment 18 Thomas Sepez 2012-03-20 16:02:14 PDT
OK.  Let me doublecheck the tests and I'll upload a tidy version of this fix.

(In reply to comment #17)
> It looks like this was fixed in FF4. As a tweak landed to a 20%+ share browser over a year ago, I'd not expect a significant compat problem at this stage? If it's simpler / quicker to just copy FF's approach, I'd vote for that as a first approach. We'd get immediate testing in Chrome's dev channel.
Comment 19 Thomas Sepez 2012-03-21 14:50:55 PDT
Created attachment 133116 [details]
Patch

Patch.  Note that only suppressing the scroll when there is a fragment (vs empty to top) makes the existing tests happier without actually leaking any information.
Comment 20 Thomas Sepez 2012-03-21 14:52:34 PDT
Remove security box as all info already public.
Comment 21 Chris Evans 2012-03-30 02:25:26 PDT
Adam, any chance you could review?
Comment 22 Adam Barth 2012-03-30 13:06:46 PDT
Comment on attachment 133116 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2795
> +        Frame* parent = m_frame->tree()->parent();
> +        if (parent && !m_frame->document()->securityOrigin()->canAccess(parent->document()->securityOrigin()))
> +            return;

What happens if there is a nested iframe?  For example
A
 - B
    - C

where B and C are the same origin?  Won't scroll operations on C leak information to A even through B and C are the same origin?

Also, isn't information being leaked from the child to the parent?  In that case, it would seems like we should ask whether the parent can access the child.

Does this disable fragment scrolling entirely for iframes embedded in cross-origin pages?  Even if the navigation is initiated by the frame itself?  Is that what Firefox does too?
Comment 23 Eric Seidel (no email) 2012-03-30 15:21:34 PDT
If it's no longer security, it should move to WebKit.
Comment 24 Sam Weinig 2012-04-01 19:45:38 PDT
Comment on attachment 133116 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Fix the frame leak attack.
> +        https://bugs.webkit.org/show_bug.cgi?id=73083

This title is too aggressive for a change log.

> Source/WebCore/ChangeLog:9
> +        Block cross-origin iframe scroll to fragment behaviour to avoid leaking the
> +        presence or absence of ids on the page.  FF has done this for all iframes for
> +        a year now, but our change is more sophisticated in that it only does this in
> +        the dangerous cross-orgin case.

What is the reason for not matching Firefox? Has there been compat fallout?
Comment 25 Thomas Sepez 2012-04-02 10:21:38 PDT
(In reply to comment #24)
> (From update of attachment 133116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133116&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        Fix the frame leak attack.
> > +        https://bugs.webkit.org/show_bug.cgi?id=73083
> 
> This title is too aggressive for a change log.
> 

Ok, I'll make it more innocent sounding.  However, the whole issue has been blogged about extensively by the bug reporter and others, so I'm not sure we're really hiding anything.

> > Source/WebCore/ChangeLog:9
> > +        Block cross-origin iframe scroll to fragment behaviour to avoid leaking the
> > +        presence or absence of ids on the page.  FF has done this for all iframes for
> > +        a year now, but our change is more sophisticated in that it only does this in
> > +        the dangerous cross-orgin case.
> 
> What is the reason for not matching Firefox? Has there been compat fallout?

Yes, folks have been annoyed.  See https://bugzilla.mozilla.org/show_bug.cgi?id=638598 . I'd like to break as few sites as possible, and avoid having to revisit this should mozilla change their policy to exclude same-origin.
Comment 26 Thomas Sepez 2012-04-02 10:33:27 PDT
(In reply to comment #22)
> (From update of attachment 133116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133116&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2795
> > +        Frame* parent = m_frame->tree()->parent();
> > +        if (parent && !m_frame->document()->securityOrigin()->canAccess(parent->document()->securityOrigin()))
> > +            return;
> 
> What happens if there is a nested iframe?  For example
> A
>  - B
>     - C
> 
> where B and C are the same origin?  Won't scroll operations on C leak information to A even through B and C are the same origin?

We don't care about scroll operations apart from those resulting from navigation to anchors, and the attack is possible in general because A can initiate a navigation on B.  I don't think A can initiate a navigation on C -- that would be the iframe hijacking issue again, no?

> 
> Also, isn't information being leaked from the child to the parent?  In that case, it would seems like we should ask whether the parent can access the child.
> 
Ok, I guess this is not symmetric.  I'll cobble together such a change.

> Does this disable fragment scrolling entirely for iframes embedded in cross-origin pages?  Even if the navigation is initiated by the frame itself?  Is that what Firefox does too?

Yes, Yes, and Yes.

Here's a couple of live examples from the FF bug.

cross-origin iframe which does not navigate:
http://knoxrooms.sirsi.net/rooms/portal/page/21728_Reference

same-origin iframe which does not navigate:
http://firedictionary.com/test/parent.html

The second one should be functional even after our change.
Comment 27 Adam Barth 2012-04-02 11:36:32 PDT
(In reply to comment #26)
> (In reply to comment #22)
> > (From update of attachment 133116 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=133116&action=review
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:2795
> > > +        Frame* parent = m_frame->tree()->parent();
> > > +        if (parent && !m_frame->document()->securityOrigin()->canAccess(parent->document()->securityOrigin()))
> > > +            return;
> > 
> > What happens if there is a nested iframe?  For example
> > A
> >  - B
> >     - C
> > 
> > where B and C are the same origin?  Won't scroll operations on C leak information to A even through B and C are the same origin?
> 
> We don't care about scroll operations apart from those resulting from navigation to anchors, and the attack is possible in general because A can initiate a navigation on B.  I don't think A can initiate a navigation on C -- that would be the iframe hijacking issue again, no?

A can initiate a navigation on C.  In general, a frame can navigate any of it's descendants.
Comment 28 Thomas Sepez 2012-04-02 11:51:53 PDT
Ok, so the logic would be canAcessAllAncestors()?

> A can initiate a navigation on C.  In general, a frame can navigate any of it's descendants.

Also, I've noticed the firedictionary link is blocked (despite layout tests for this situation), so theres something that needs to be run to ground .
Comment 29 Thomas Sepez 2012-04-02 16:33:17 PDT
Created attachment 135223 [details]
New Patch

I believe this patch addresses the above issues.
Comment 30 Thomas Sepez 2012-04-03 11:08:42 PDT
Created attachment 135367 [details]
Pacth

Or would you like it better if we made some console noise to indicate this is intentional?
Comment 31 WebKit Review Bot 2012-04-03 12:48:16 PDT
Comment on attachment 135367 [details]
Pacth

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

New failing tests:
http/tests/security/xssAuditor/anchor-url-dom-write-location2.html
http/tests/security/xssAuditor/anchor-url-dom-write-location-inline-event-null-char.html
http/tests/navigation/anchor-frames-cross-origin.html
http/tests/security/xssAuditor/anchor-url-dom-write-location-javascript-URL.html
http/tests/security/xssAuditor/anchor-url-dom-write-location-inline-event.html
http/tests/security/xssAuditor/anchor-url-dom-write-location.html
http/tests/inspector/resource-parameters.html
Comment 32 WebKit Review Bot 2012-04-03 12:48:22 PDT
Created attachment 135400 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 33 Thomas Sepez 2012-04-03 12:58:32 PDT
Adam, 

Looks like the diffs are from the console message in the latest patch (which I also noted has a misspelling). Let me know if we want to keep the message; if so, i'll tidy up the tests and spelling, otherwise can go with patch n-1.
Comment 34 Thomas Sepez 2012-04-03 15:25:38 PDT
Created attachment 135438 [details]
Patch for review.
Comment 35 WebKit Review Bot 2012-04-03 16:49:34 PDT
Comment on attachment 135438 [details]
Patch for review.

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

New failing tests:
http/tests/security/xssAuditor/anchor-url-dom-write-location2.html
http/tests/security/xssAuditor/anchor-url-dom-write-location-inline-event-null-char.html
http/tests/navigation/anchor-frames-cross-origin.html
http/tests/security/xssAuditor/anchor-url-dom-write-location-javascript-URL.html
http/tests/security/xssAuditor/anchor-url-dom-write-location-inline-event.html
http/tests/security/xssAuditor/anchor-url-dom-write-location.html
http/tests/inspector/resource-parameters.html
Comment 36 WebKit Review Bot 2012-04-03 16:49:41 PDT
Created attachment 135455 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 37 Darin Adler 2012-04-03 17:25:53 PDT
Comment on attachment 135438 [details]
Patch for review.

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

> Source/WebCore/loader/FrameLoader.cpp:2673
> +    if (!url.fragmentIdentifier().isEmpty()) {

Why check this instead of using hasFragmentIdentifier()?

> Source/WebCore/loader/FrameLoader.cpp:2684
> +        // Block fragment scrolling when there is a cross-origin ancestor to avoid an information
> +        // leak known as framesniffing (although "scroll position sniffing" might be a more accurate
> +        // description of the issue).
> +        SecurityOrigin* childSecurityOrigin = m_frame->document()->securityOrigin();
> +        for (Frame* ancestorFrame = m_frame->tree()->parent(); ancestorFrame; ancestorFrame = ancestorFrame->tree()->parent()) {
> +            SecurityOrigin* ancestorSecurityOrigin = ancestorFrame->document()->securityOrigin();
> +            if (!(ancestorSecurityOrigin->canAccess(childSecurityOrigin) || (childSecurityOrigin->isLocal() && ancestorSecurityOrigin->isLocal()))) {
> +                m_frame->domWindow()->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Fragment navigation not allowed with cross-origin frames.");
> +                return;
> +            }
> +        }

I don’t understand why this frame’s security origin is being put in a local variable named “child security origin”.

The comment here leads me to believe we should have a function called hasCrossOriginAncestor. With that function, the code to add the message to the console would no longer be buried inside a for and an if. And the name of the function should help document what the code does and make it possible to make the comment even shorter or at least make the comment more clearly match the code.

I am not sure why there are exceptions here for local origins. That does not look right to me and so needs a comment explaining why it’s right. Another nice thing about putting the logic in a separate function is that there would be room for the comment without obscuring the logic.

> Source/WebCore/loader/FrameLoader.cpp:2687
> +    view->scrollToFragment(url);

It is a mistake to have two different scrollToFragment functions, one here and one in FrameView, one that is wrong to call, and one that is right to call. There’s nothing about “calling it on a loader” vs. “calling it on a view” that makes it clear to me why the two are different.

If we’re going to keep both we need them to have different names to make clear what the difference is.

For example, one could be called scrollToFragmentIfAllowed. But there may be some even better way of naming things.

> Source/WebCore/loader/FrameLoader.h:321
>      bool shouldScrollToAnchor(bool isFormSubmission, const String& httpMethod, FrameLoadType, const KURL&);
> +    void scrollToFragment(const KURL&);

This mix of terminology between “fragment” and “anchor” here is not so great.

It also seems strange to have a function named shouldScrollToAnchor, but then have a separate check for whether it’s OK to scroll to an anchor in the scrollToFragment function.

Maybe the bug can be fixed by adding more checks to the existing shouldScrollToAnchor function?
Comment 38 Thomas Sepez 2012-04-04 11:14:41 PDT
(In reply to comment #37)
> (From update of attachment 135438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135438&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2673
> > +    if (!url.fragmentIdentifier().isEmpty()) {
> 
> Why check this instead of using hasFragmentIdentifier()?
Thanks.  Much better.

> 
> > Source/WebCore/loader/FrameLoader.cpp:2684
> > +        // Block fragment scrolling when there is a cross-origin ancestor to avoid an information
> > +        // leak known as framesniffing (although "scroll position sniffing" might be a more accurate
> > +        // description of the issue).
> > +        SecurityOrigin* childSecurityOrigin = m_frame->document()->securityOrigin();
> > +        for (Frame* ancestorFrame = m_frame->tree()->parent(); ancestorFrame; ancestorFrame = ancestorFrame->tree()->parent()) {
> > +            SecurityOrigin* ancestorSecurityOrigin = ancestorFrame->document()->securityOrigin();
> > +            if (!(ancestorSecurityOrigin->canAccess(childSecurityOrigin) || (childSecurityOrigin->isLocal() && ancestorSecurityOrigin->isLocal()))) {
> > +                m_frame->domWindow()->console()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Fragment navigation not allowed with cross-origin frames.");
> > +                return;
> > +            }
> > +        }
> 
> I don’t understand why this frame’s security origin is being put in a local variable named “child security origin”.
> 
Will rename variable.  Some code used activeSecurityOrigin in a few places and I'll do the same.

> The comment here leads me to believe we should have a function called hasCrossOriginAncestor. 
 I had that in an initial revision.  Formerly, there was a function called canAccessAncestor, which I renamed to canAccessAnyAncestor(), and introduced canAccessEveryAncestor().  But after Adam refactored canAccessAncestor() into the Document class, it felt simpler to just collapse this into the single simple function.

> With that function, the code to add the message to the console would no longer be buried inside a for and an if.
Is this an issue given the simplicity of the function???

> And the name of the function should help document what the code does and make it possible to make the comment even shorter or at least make the comment more clearly match the code.
Ok, but the comment is intended as a "why" comment rather than a "what" or "how".  You could rename the function scrollToFragmentAvoidingFrameSniffing() i suppose.

> 
> I am not sure why there are exceptions here for local origins. That does not look right to me and so needs a comment explaining why it’s right. Another nice thing about putting the logic in a separate function is that there would be room for the comment without obscuring the logic.
The local origin exception originated in the old canAcessAncestor() function, and I kept it because there are protections against web pages framing local resources, so the attack doesn't exist in theory in the local case.  I'll yank it, but there may be cases that no longer work that could safely otherwise.

> 
> > Source/WebCore/loader/FrameLoader.cpp:2687
> > +    view->scrollToFragment(url);
> 
> It is a mistake to have two different scrollToFragment functions, one here and one in FrameView, one that is wrong to call, and one that is right to call. There’s nothing about “calling it on a loader” vs. “calling it on a view” that makes it clear to me why the two are different.
That would be fixed by changing the name above.  

> 
> If we’re going to keep both we need them to have different names to make clear what the difference is.
> For example, one could be called scrollToFragmentIfAllowed. But there may be some even better way of naming things.
Sure

> 
> > Source/WebCore/loader/FrameLoader.h:321
> >      bool shouldScrollToAnchor(bool isFormSubmission, const String& httpMethod, FrameLoadType, const KURL&);
> > +    void scrollToFragment(const KURL&);
> 
> This mix of terminology between “fragment” and “anchor” here is not so great.
Indeed.  You scroll to anchors based on fragments; you don't scroll to the fragment.  I'll make more consistent.

> 
> It also seems strange to have a function named shouldScrollToAnchor, but then have a separate check for whether it’s OK to scroll to an anchor in the scrollToFragment function.
I believe the "should" in this case means shouldShortCircuitLoadingAndScrollToAnchorInstead(). 
> 
> Maybe the bug can be fixed by adding more checks to the existing shouldScrollToAnchor function?
Perphaps. Return would have to be more than just a boolean, probably at least the following options:
Normal Navigation vs. Scroll only navigation vs. No navigation.  But I wouldn't want to rule out other steps besides the final scroll that need to take place even under the case where we block the last step.
Comment 39 Thomas Sepez 2012-04-04 13:43:34 PDT
Created attachment 135667 [details]
Patch addressing #38
Comment 40 Thomas Sepez 2012-04-04 14:10:01 PDT
Created attachment 135677 [details]
Patch.

Frame *frame => Frame* frame.
Comment 41 Thomas Sepez 2012-04-05 16:00:44 PDT
Created attachment 135923 [details]
Alternate patch

Alternate version of patch.  Moves the ancestor check implementation into Document to be more consistent with Adam's recent changes.  Resolve the Anchor/Fragment naming issue in favor of "fragment" to maintain consistency with frameview's notion of scrolltofragment (which takes URLS) and scrollToAnchor (which takes a string naming the anchor).  The operations in frameloader all manipulate URLS, so use "fragment".  Clean up some comment to keep consistent where there's an URL involved.
Comment 42 Adam Barth 2012-04-06 13:25:28 PDT
Comment on attachment 135923 [details]
Alternate patch

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

This patch looks great.  I would just add some more information to the ChangeLog.

> Source/WebCore/ChangeLog:9
> +        Match FF behaviour: FF has done this for all iframes for a year now, but our change
> +        is less disruptive in that it only does this in the cross-orgin case.

These two sentences confuse me.  Are we matching Firefox's behavior or not?  If we're not matching their behavior, we should explain why in more detail.
Comment 43 Thomas Sepez 2012-04-06 13:35:55 PDT
Created attachment 136058 [details]
Patch for landing with ChangeLog edits.

Rewrote change log.  Trying to walk a balance between describing what is happening vs. advertising "pwnage".
Comment 44 Adam Barth 2012-04-06 13:38:12 PDT
Comment on attachment 136058 [details]
Patch for landing with ChangeLog edits.

Thanks.
Comment 45 WebKit Review Bot 2012-04-06 15:15:20 PDT
Comment on attachment 136058 [details]
Patch for landing with ChangeLog edits.

Clearing flags on attachment: 136058

Committed r113503: <http://trac.webkit.org/changeset/113503>
Comment 46 WebKit Review Bot 2012-04-06 15:15:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 47 Ádám Kallai 2012-04-10 05:56:32 PDT
This test fails on Qt WK2 and Mac Lion WK2.

Diff:
--- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/http/tests/navigation/anchor-frames-cross-origin-expected.txt 
+++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/http/tests/navigation/anchor-frames-cross-origin-actual.txt 
@@ -1,23 +1,2 @@
 CONSOLE MESSAGE: Fragment navigation not allowed with cross-origin frames.
 
-
---------
-Frame: 'main'
---------
-This prevents a cross-origin information leak sometimes know as framesniffing.
-
-On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
-
-
-PASS document.body.offsetHeight > document.documentElement.clientHeight is true
-PASS document.body.scrollTop == 0 is true
-PASS document.body.scrollLeft == 0 is true
-PASS successfullyParsed is true
-
-TEST COMPLETE
-This is an anchor point named "anchor1.
-
---------
-Frame: 'footer'
---------
-

Could you check it? I'm going to skip this, until It is fixed.
Comment 48 Ádám Kallai 2012-04-10 07:50:46 PDT
I made new bug report for this failing test.
https://bugs.webkit.org/show_bug.cgi?id=83581

* http/tests/navigation/anchor-frames-cross-origin.html

Skip landed: http://trac.webkit.org/changeset/113713
Comment 49 Paul Stone 2012-04-10 09:10:34 PDT
Created attachment 136465 [details]
Broken testcase (works in Firefox)

I just tested this in the Chrome Canary builds, and it seems that the fix is a bit too agressive. When navigating to a fragment in a cross-origin frame, it prevents the frame itself from scrolling. The frame itself should scroll (there's no leak there, Firefox still allows this), but it should prevent any any ancestor frames from scrolling if they're cross-origin.

I've attached a simple testcase that works in Firefox, but is broken in the Canary build. I think this could break some websites  - for example API documentation that uses frames, where the table-of-contents pane is on a different (sub)domain than the main frame.
Comment 50 Thomas Sepez 2012-04-10 16:58:20 PDT
Proper fix probably depends on blocking propagation to parent in RenderLayer::scrollRectToVisible()
Comment 51 Thomas Sepez 2012-04-11 14:50:18 PDT
New bug for followup patch is https://bugs.webkit.org/show_bug.cgi?id=83721