Bug 113387 - X-Frame-Options: Multiple headers are ignored completely.
Summary: X-Frame-Options: Multiple headers are ignored completely.
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:
 
Reported: 2013-03-27 04:02 PDT by Mike West
Modified: 2013-03-28 01:57 PDT (History)
6 users (show)

See Also:


Attachments
Patch (23.26 KB, patch)
2013-03-27 06:24 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (23.27 KB, patch)
2013-03-28 01:32 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-27 04:02:11 PDT
http://www.veracode.com/blog/2013/03/security-headers-on-the-top-1000000-websites-march-2013-report/ notes that a small number of sites send multiple 'X-Frame-Options' headers. We're currently ignoring those completely, which probably isn't the safest behavior.

Gecko has settled on parsing each header, using the value if they're all the same, and defaulting to 'DENY' if they conflict, which seems like a better solution[1]. I'd suggest that WebKit do the same.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=761655
Comment 1 Mike West 2013-03-27 06:15:57 PDT
Also filed downstream as https://code.google.com/p/chromium/issues/detail?id=145659.
Comment 2 Mike West 2013-03-27 06:24:52 PDT
Created attachment 195300 [details]
Patch
Comment 3 Mike West 2013-03-27 10:29:35 PDT
Bots seem happy.

Nate, would you mind taking a look at this?
Comment 4 Nate Chapin 2013-03-27 10:39:01 PDT
Comment on attachment 195300 [details]
Patch

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

One style nit...

> Source/WebCore/loader/FrameLoader.cpp:2984
> +    default:
> +        m_frame->document()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Invalid 'X-Frame-Options' header encountered when loading '" + url.elidedString() + "': '" + content + "' is not a recognized directive. The header will be ignored.", requestIdentifier);
> +        return false;

I think it's more common in WebKit (or at least the parts I frequent) to explicitly state all cases and have the default be ASSERT_NOT_REACHED().
Comment 5 Mike West 2013-03-28 01:32:45 PDT
Created attachment 195500 [details]
Patch for landing
Comment 6 Mike West 2013-03-28 01:33:32 PDT
(In reply to comment #4)
> (From update of attachment 195300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195300&action=review
> 
> One style nit...
> 
> > Source/WebCore/loader/FrameLoader.cpp:2984
> > +    default:
> > +        m_frame->document()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Invalid 'X-Frame-Options' header encountered when loading '" + url.elidedString() + "': '" + content + "' is not a recognized directive. The header will be ignored.", requestIdentifier);
> > +        return false;
> 
> I think it's more common in WebKit (or at least the parts I frequent) to explicitly state all cases and have the default be ASSERT_NOT_REACHED().

Thanks Nate. I've taken care of that in the patch up for the CQ.
Comment 7 WebKit Review Bot 2013-03-28 01:57:16 PDT
Comment on attachment 195500 [details]
Patch for landing

Clearing flags on attachment: 195500

Committed r147086: <http://trac.webkit.org/changeset/147086>
Comment 8 WebKit Review Bot 2013-03-28 01:57:20 PDT
All reviewed patches have been landed.  Closing bug.