[Webkit-unassigned] [Bug 73083] Fix the Frame Leak Attack

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73083





--- Comment #37 from Darin Adler <darin at apple.com>  2012-04-03 17:25:53 PST ---
(From update of attachment 135438)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.