Bug 112670

Summary: FeatureObserver: Measure X-Frame-Options usage.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 85558    
Attachments:
Description Flags
Patch
none
Patch for landing none

Mike West
Reported 2013-03-19 01:28:47 PDT
Given the work in public-webappsec@ around the 'frame-options' CSP directive[1], it would be informative to learn a few things: 1. How often is 'X-Frame-Options' used? 2. How often is 'SAMEORIGIN' used? 3. How often is the top-only implementation of 'SAMEORIGIN' bypassed via an unexpected ancestor chain (e.g. example.com -> evil.com -> example.com)? For #3, see also Mozilla's[2] and public-webappsec@'s[3] discussion on the topic. [1]: http://www.w3.org/TR/UISafety/#frame-options [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=725490 [3]: http://lists.w3.org/Archives/Public/public-webappsec/2013Mar/0007.html
Attachments
Patch (6.44 KB, patch)
2013-03-19 01:51 PDT, Mike West 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.83 KB, patch)
2013-03-19 13:18 PDT, Mike West 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Mike West
Comment 1 2013-03-19 01:51:01 PDT
Created attachment 193759 [details] Patch
Mike West
Comment 2 2013-03-19 01:57:09 PDT
(In reply to comment #1) > Created an attachment (id=193759) [details] > Patch Hi Adam, as discussed over email, would you mind taking a look at this? The potentially objectionable part is the addition of Frame::isSameOriginWithAllAncestors(). We'll need some sort of mechanism to do this check if/when we implement the 'frame-origin' directive from the UI Safety draft, so adding it now seems reasonable, but I can see good arguments for keeping the patch self-contained inside FrameLoader. Likewise, Frame::* might be a bad place for the method. I'd considered something like 'static bool isFrameSameOriginWithAllAncestors(Frame*)' on SecurityOrigin, but sided with the current implementation for purely asthetic reasons. I'd appreciate your thoughts.
Adam Barth
Comment 3 2013-03-19 09:50:29 PDT
Comment on attachment 193759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193759&action=review > Source/WebCore/page/Frame.cpp:1051 > +bool Frame::isSameOriginWithAllAncestors() const This function implies that the "same-origin" relation is symmetric, which isn't the case. I'd prefer to line this function into FrameLoader::shouldInterruptLoadForXFrameOptions. It's not really a general-purpose function, and I wouldn't want other code to call it.
Mike West
Comment 4 2013-03-19 13:18:11 PDT
Created attachment 193907 [details] Patch for landing
Mike West
Comment 5 2013-03-19 13:19:42 PDT
(In reply to comment #3) > (From update of attachment 193759 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=193759&action=review > > > Source/WebCore/page/Frame.cpp:1051 > > +bool Frame::isSameOriginWithAllAncestors() const > > This function implies that the "same-origin" relation is symmetric, which isn't the case. > > I'd prefer to line this function into FrameLoader::shouldInterruptLoadForXFrameOptions. It's not really a general-purpose function, and I wouldn't want other code to call it. Inlined the function, and threw it into the CQ. Thanks for taking a look!
WebKit Review Bot
Comment 6 2013-03-19 14:14:46 PDT
Comment on attachment 193907 [details] Patch for landing Clearing flags on attachment: 193907 Committed r146257: <http://trac.webkit.org/changeset/146257>
WebKit Review Bot
Comment 7 2013-03-19 14:14:49 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.