Framesniffing defense is too aggressive.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Thomas Sepez]

Follow on from bug 73083 comment #49 From Paul Stone 2012-04-10 09:10:34 PST:

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]

Created attachment 136760 [details]
Test case from Paul.

[Thomas Sepez]

Created attachment 136761 [details]
One possible approach for discussion.

(Tests still need to be hammered into submission if we go with something like this). 
Appears to work properly against Paul's example.

[Adam Barth]

Comment on attachment 136761 [details]
One possible approach for discussion.

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

> Source/WebCore/rendering/RenderLayer.cpp:1680
> +                        // Block parent scroll one time only.

Why only once?

[Thomas Sepez]

> Why only once?

Only scrolls initiated by fragment navigations in frameloader are unsafe.  Frameloader will set this, but we need some way to clear it out for subsequent use.  Doing so when we hit it seemed pragmatic.

[Adam Barth]

I see, so we set the boundary when we kick off the scroll and then remove it once we hit it?  Hum...  Should we have some sort of ASSERT that makes sure we don't accidentally leave it in?

[James Robinson]

Comment on attachment 136761 [details]
One possible approach for discussion.

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

Other than needing some layout test coverage this seems to be on track.

>> Source/WebCore/rendering/RenderLayer.cpp:1680
>> +                        // Block parent scroll one time only.
> 
> Why only once?

Could we instead clear this flag with a call in FrameLoader::scrollToFragmentWithParentBoundary after view->scrollToFragment() ?

[Thomas Sepez]

> Could we instead clear this flag with a call in FrameLoader::scrollToFragmentWithParentBoundary after view->scrollToFragment() ?

That is much cleaner, and I thought about doing it that way.  But here's why I held off

There's a case in FrameView::maintainScrollPositionAtAnchor() where we take a detour into Layout() rather than falling into the (no-argument form of) FrameView::ScrollToAnchor().  It wasn't clear to me how long this path would postpone the actual scroll.  Could it return to FrameLoader, clear the flag (in your proposal), and then have the scroll implied by the anchor fire some time later (like after some other style sheet load)?


Also, I wanted to log a console message (once) when this occurs, so that anyone opening the error console would know this is deliberate (vs. a silent breakage).  I got the feeling that logging from something like RenderLayer would be seriously frowned upon.  How to arrange for that to happen?

[Thomas Sepez]

Ok.  I don't think I can make the logging happen in a way that matches visual expectations - in Paul's test case, for example, there is a failure to propagate the scroll across the boundary, but we really don't care because the top frame didn't have to scroll.  Short of asking the frame that didn't scroll whether it or its parents would have had to scroll ... seems hard.  And probably too much of a hit for sites that simulate postmessage with fragment navs.

I'll also go with the reset in the FrameLoader as James suggested as the first fix for simplicity.  The path though Layout() may likely be hard to trigger reliably for exploitation.

[Thomas Sepez]

Created attachment 137127 [details]
Another possible patch for discussion.

[Thomas Sepez]

Created attachment 137174 [details]
Patch

Patch plus reworked tests.  Adds a refptr per conversation with Julien.

[David Kilzer (:ddkilzer)]

<rdar://problem/11250009>

[James Robinson]

Comment on attachment 137174 [details]
Patch

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

This looks pretty good to me. One style nitpick.

> Source/WebCore/loader/FrameLoader.cpp:2693
> +    if (boundaryFrame.get())

You can (and WebKit code typically does) null-check smart pointers by just doing:

if (boundaryFrame)

since RefPtr/OwnPtr implement operator bool()

> LayoutTests/http/tests/navigation/anchor-frames-same-origin.html:1
> +<html>

For new tests, we typically add a <!DOCTYPE html> declaration to put the page in standards (as opposed to quirks) mode unless we specifically want to check a quirks behavior. most of the time it doesn't make a difference, but it's useful to have the lack of a doctype mean that there is something quirks-mode-specific going on

[Thomas Sepez]

Created attachment 137369 [details]
Patch, fix nits.

Thanks James, heres' the same patch with the nits cleaned up.

[Gustavo Noronha (kov)]

Comment on attachment 137369 [details]
Patch, fix nits.

Attachment 137369 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12416240

[Thomas Sepez]

Build error is in dependencies.  If I reset cq=? does it try again?

[James Robinson]

(In reply to comment #15)
> Build error is in dependencies.  If I reset cq=? does it try again?

Sadly no, you have to upload a new patch to get the EWS to run again.

[James Robinson]

Comment on attachment 137369 [details]
Patch, fix nits.

Looks like we're good now

[WebKit Review Bot]

Comment on attachment 137369 [details]
Patch, fix nits.

Clearing flags on attachment: 137369

Committed r114406: <http://trac.webkit.org/changeset/114406>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

This commit added tests which fail on all WebKit2 test bots, namely http/tests/navigation/anchor-frames-same-origin.html. 

Filed <https://bugs.webkit.org/show_bug.cgi?id=84227> to track the broken test.