Bug 73083 – Fix the Frame Leak Attack

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Paul Stone]

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.

[Paul Stone]

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.

[Lucas Forschler]

<rdar://problem/10497237>

[Thomas Sepez]

http://code.google.com/p/chromium/issues/detail?id=118633

[Thomas Sepez]

Seems like the only viable solution is to prevent fragment scrolling in the cross-origin case, which mimics what happens in FF.

[Thomas Sepez]

Created attachment 132630 [details]
Test case for blocking cross-origin fragment navigations which flunk at present.

[Paul Stone]

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.

[Adam Barth]

Do you know who worked on this change in Firefox?  We could ask them for advice.

[Paul Stone]

Jonas Sicking fixed it - see https://bugzilla.mozilla.org/show_bug.cgi?id=583889

[Thomas Sepez]

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.

[Adam Barth]

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

[Thomas Sepez]

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?

[Adam Barth]

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

[Thomas Sepez]

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?

[Adam Barth]

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.

[Thomas Sepez]

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

[Thomas Sepez]

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

[Chris Evans]

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.

[Thomas Sepez]

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.

[Thomas Sepez]

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.

[Thomas Sepez]

Remove security box as all info already public.

[Chris Evans]

Adam, any chance you could review?

[Adam Barth]

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?

[Eric Seidel (no email)]

If it's no longer security, it should move to WebKit.

[Sam Weinig]

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?

[Thomas Sepez]

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

[Thomas Sepez]

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

[Adam Barth]

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

[Thomas Sepez]

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 .

[Thomas Sepez]

Created attachment 135223 [details]
New Patch

I believe this patch addresses the above issues.

[Thomas Sepez]

Created attachment 135367 [details]
Pacth

Or would you like it better if we made some console noise to indicate this is intentional?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Thomas Sepez]

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.

[Thomas Sepez]

Created attachment 135438 [details]
Patch for review.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Darin Adler]

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?

[Thomas Sepez]

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

[Thomas Sepez]

Created attachment 135667 [details]
Patch addressing #38

[Thomas Sepez]

Created attachment 135677 [details]
Patch.

Frame *frame => Frame* frame.

[Thomas Sepez]

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.

[Adam Barth]

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.

[Thomas Sepez]

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

[Adam Barth]

Comment on attachment 136058 [details]
Patch for landing with ChangeLog edits.

Thanks.

[WebKit Review Bot]

Comment on attachment 136058 [details]
Patch for landing with ChangeLog edits.

Clearing flags on attachment: 136058

Committed r113503: <http://trac.webkit.org/changeset/113503>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ádám Kallai]

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.

[Ádám Kallai]

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

[Paul Stone]

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.

[Thomas Sepez]

Proper fix probably depends on blocking propagation to parent in RenderLayer::scrollRectToVisible()

[Thomas Sepez]

New bug for followup patch is https://bugs.webkit.org/show_bug.cgi?id=83721