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

[bugzilla-daemon at webkit.org]

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





--- Comment #38 from Thomas Sepez <tsepez at chromium.org>  2012-04-04 11:14:41 PST ---
(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.

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