Bug 112670 - FeatureObserver: Measure X-Frame-Options usage.
Summary: FeatureObserver: Measure X-Frame-Options usage.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 85558
  Show dependency treegraph
 
Reported: 2013-03-19 01:28 PDT by Mike West
Modified: 2013-03-19 14:14 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.44 KB, patch)
2013-03-19 01:51 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (4.83 KB, patch)
2013-03-19 13:18 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 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
Comment 1 Mike West 2013-03-19 01:51:01 PDT
Created attachment 193759 [details]
Patch
Comment 2 Mike West 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.
Comment 3 Adam Barth 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.
Comment 4 Mike West 2013-03-19 13:18:11 PDT
Created attachment 193907 [details]
Patch for landing
Comment 5 Mike West 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!
Comment 6 WebKit Review Bot 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>
Comment 7 WebKit Review Bot 2013-03-19 14:14:49 PDT
All reviewed patches have been landed.  Closing bug.