Bug 113387 – X-Frame-Options: Multiple headers are ignored completely.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

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

[Mike West]

Also filed downstream as https://code.google.com/p/chromium/issues/detail?id=145659.

[Mike West]

Created attachment 195300 [details]
Patch

[Mike West]

Bots seem happy.

Nate, would you mind taking a look at this?

[Nate Chapin]

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().

[Mike West]

Created attachment 195500 [details]
Patch for landing

[Mike West]

(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.

[WebKit Review Bot]

Comment on attachment 195500 [details]
Patch for landing

Clearing flags on attachment: 195500

Committed r147086: <http://trac.webkit.org/changeset/147086>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.