Bug 185567 – X-Frame-Options: SAMEORIGIN needs to check all ancestor frames

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

X-Frame-Options: SAMEORIGIN needs to check all ancestor frames.

[Daniel Bates]

<rdar://problem/40175008>

[Daniel Bates]

Mozilla bug: <https://bugzilla.mozilla.org/show_bug.cgi?id=725490>

[Daniel Bates]

Chrome issue: <https://bugs.chromium.org/p/chromium/issues/detail?id=250309>

[Daniel Bates]

RFC: <https://tools.ietf.org/html/rfc7034>

[Daniel Bates]

Created attachment 340230 [details]
Patch and layout tests

[EWS Watchlist]

Attachment 340230 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious  [changelog/unwantedsecurityterms] [3]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 340230 [details]
Patch and layout tests

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

r=me, but I have a few minor complaints about the patch I hope you can resolve before landing.

> Source/WebCore/loader/FrameLoader.cpp:3423
> +                return true;

Ha! It's funny it was every written as a break, considering that the loop wasn't doing anything meaningful! :-)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:419
> +        if (!SecurityOrigin::create(url)->isSameSchemeHostPort(*m_parameters.sourceOrigin))

Do we need to create the SecurityOrigin twice?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:421
> +        for (auto& origin : m_parameters.frameAncestorOrigins) {

It's weird to have origin defined in two scopes, even though it apparently works. Could you name one the "sourceOrigin" or "topLevelOrigin" or something?

[Daniel Bates]

Comment on attachment 340230 [details]
Patch and layout tests

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

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:419
>> +        if (!SecurityOrigin::create(url)->isSameSchemeHostPort(*m_parameters.sourceOrigin))
> 
> Do we need to create the SecurityOrigin twice?

Oops! Will fix before landing.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:421
>> +        for (auto& origin : m_parameters.frameAncestorOrigins) {
> 
> It's weird to have origin defined in two scopes, even though it apparently works. Could you name one the "sourceOrigin" or "topLevelOrigin" or something?

Oops! I hope you do not mind that I rename the loop variable ancestorOrigin to resolve this ambiguity.

[Brent Fulgham]

Comment on attachment 340230 [details]
Patch and layout tests

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

>>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:421
>>> +        for (auto& origin : m_parameters.frameAncestorOrigins) {
>> 
>> It's weird to have origin defined in two scopes, even though it apparently works. Could you name one the "sourceOrigin" or "topLevelOrigin" or something?
> 
> Oops! I hope you do not mind that I rename the loop variable ancestorOrigin to resolve this ambiguity.

That sounds like a great name!

[Daniel Bates]

Created attachment 340239 [details]
For EWS

[EWS Watchlist]

Attachment 340239 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WebCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious, malicious, malicious  [changelog/unwantedsecurityterms] [3]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Committed r231730: <https://trac.webkit.org/changeset/231730>