Bug 185410 – Check X-Frame-Options and CSP frame-ancestors in network process

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

[Daniel Bates]

Check X-Frame-Options and CSP frame-ancestors in network process.

[Daniel Bates]

<rdar://problem/37733934>

[Daniel Bates]

Created attachment 339795 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 339795 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:770
> +    if (!m_frame->settings().networkProcessCSPFrameAncestorsCheckingEnabled() || !RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess()) {

I think we should add a new method to PlatformStrategies instead of using settings to behave differently in WK1 and WK2.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:486
> +    if (ancestorOrigins.size() == 1)
> +        return true; // The top-most frame is always allowed.

Instead of adding a comment like this, why don't we use a local boolean variable with a descriptive name?
e.g.
bool isNavigatingTopLevelFrame = ancestorOrigins.size() == 1;
if (isNavigatingTopLevelFrame)
    return true;

> Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:93
> +        URL origin { URL { }, (*it)->toString() };

Why are we converting SecurityOrigin > String > URL?
What's the practical implication of this conversion?
I think we should either add a comment here or add a member function with a descriptive name to SecurityOrigin
referencing https://w3c.github.io/webappsec-csp/#frame-ancestors-navigation-response

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:407
> +    XFrameOptionsDisposition disposition = parseXFrameOptionsHeader(xFrameOptions);

Seems like we don't need this local variable. It's only used in switch.

[Daniel Bates]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 339795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339795&action=review
> 
> > Source/WebCore/loader/DocumentLoader.cpp:770
> > +    if (!m_frame->settings().networkProcessCSPFrameAncestorsCheckingEnabled() || !RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess()) {
> 
> I think we should add a new method to PlatformStrategies instead of using
> settings to behave differently in WK1 and WK2.
> 

Filed bug #185412 to address this.

> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:486
> > +    if (ancestorOrigins.size() == 1)
> > +        return true; // The top-most frame is always allowed.
> 
> Instead of adding a comment like this, why don't we use a local boolean
> variable with a descriptive name?
> e.g.
> bool isNavigatingTopLevelFrame = ancestorOrigins.size() == 1;
> if (isNavigatingTopLevelFrame)
>     return true;
> 

I will use your suggestion though I will call the local variable isTopLevelFrame since this algorithm doesn't know about navigations. It only knows about frames.

> > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:93
> > +        URL origin { URL { }, (*it)->toString() };
> 
> Why are we converting SecurityOrigin > String > URL?
> What's the practical implication of this conversion?
> I think we should either add a comment here or add a member function with a
> descriptive name to SecurityOrigin
> referencing
> https://w3c.github.io/webappsec-csp/#frame-ancestors-navigation-response
> 

Will extract out this conversion into a convenience function and add a comment above it:

// Used to compute the comparison URL when checking frame-ancestors. We do this weird conversion so that child
// frames of a page with a unique origin (e.g. about:blank) are not blocked due to their frame-ancestors policy
// and do not need to add the parent's URL to their policy. The latter could allow the child page to be framed
// by anyone. See <https://github.com/w3c/webappsec/issues/311> for more details.
static inline URL urlFromOrigin(const SecurityOrigin& origin)
{
    return { URL { }, origin.toString() };
}

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:407
> > +    XFrameOptionsDisposition disposition = parseXFrameOptionsHeader(xFrameOptions);
> 
> Seems like we don't need this local variable. It's only used in switch.

Will inline parseXFrameOptionsHeader(xFrameOptions) into the switch and remove the local variable before landing.

[Daniel Bates]

Committed r231479: <https://trac.webkit.org/changeset/231479>

[youenn fablet]

http/tests/appcache/x-frame-options-prevents-framing.php started to time out after this patch landed.

[youenn fablet]

Comment on attachment 339795 [details]
Patch

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

>>> Source/WebCore/loader/DocumentLoader.cpp:770
>>> +    if (!m_frame->settings().networkProcessCSPFrameAncestorsCheckingEnabled() || !RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess()) {
>> 
>> I think we should add a new method to PlatformStrategies instead of using settings to behave differently in WK1 and WK2.
> 
> Filed bug #185412 to address this.

LoaderStrategy has isDoingLoadingSecurityChecks for that purpose.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:481
> +    if (error.isNull() && isMainResource() && shouldInterruptLoadForCSPFrameAncestorsOrXFrameOptions(m_response)) {

In case X-Frame-Options and From-Origin are both set, From-Origin will currently win over X-Frame-Options.
Maybe it should be reverse. Might be worth waiting for the From-Origin spec before changing that.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1099
> +{

The way it was done for other checks like CORS checks was to send back an error to WebProcess.
The WebProcess was then responsible to do or not do the JS console logging.
If we go the addConsoleMessage road, we should probably use that consistently, which would mean that whenever NetworkResourceLoader receives an error from NetworkLoad/NetworkLoadChecker/itself, NetworkResourceLoader would need to determine whether to log it or not.
I quite like NetworkProcess being simple and just returning either a response or an error but this is probably a matter of taste.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1103
> +void NetworkResourceLoader::sendCSPViolationReport(URL&& reportURL, Ref<FormData>&& report)

The way it is implemented for DocumentThreadableLoader is to send back an error to WebProcess.
Then, we reuse the existing WebProcess handling to generate and report the CSP violation report.
The idea was to add in the future some CSP specific ResourceError, maybe using the domain of ResourceError.

I am not quite sure what is best there.
Anyway, we should make the CSP violation handling consistent for all loads/loaders.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:116
> +    bool isMainFrameLoad() const { return isMainResource() && m_parameters.frameAncestorOrigins.size() == 1; }

I believe NetworkResourceLoadParameters has an isMainFrameNavigation boolean for that purpose.

[Ryan Haddad]

(In reply to youenn fablet from comment #6)
> http/tests/appcache/x-frame-options-prevents-framing.php started to time out
> after this patch landed.

This also caused http/tests/quicklook/csp-header-ignored.html to start failing on iOS:
https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r231487%20(4843)/results.html

[Daniel Bates]

(In reply to Ryan Haddad from comment #8)
> (In reply to youenn fablet from comment #6)
> > http/tests/appcache/x-frame-options-prevents-framing.php started to time out
> > after this patch landed.
> 
> This also caused http/tests/quicklook/csp-header-ignored.html to start
> failing on iOS:
> https://build.webkit.org/results/
> Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r231487%20(4843)/
> results.html

For now, marked the test as Failure in <https://trac.webkit.org/changeset/231500>. Will follow up with a change that removes the test from TestExpectations as part of the fix for bug #185442.

[Ryan Haddad]

(In reply to Daniel Bates from comment #9)
> For now, marked the test as Failure in
> <https://trac.webkit.org/changeset/231500>. Will follow up with a change
> that removes the test from TestExpectations as part of the fix for bug
> #185442.

We will also need to address the timeout Youenn called out (http/tests/appcache/x-frame-options-prevents-framing.php).

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fappcache%2Fx-frame-options-prevents-framing.php

[Daniel Bates]

(In reply to Ryan Haddad from comment #10)
> (In reply to Daniel Bates from comment #9)
> > For now, marked the test as Failure in
> > <https://trac.webkit.org/changeset/231500>. Will follow up with a change
> > that removes the test from TestExpectations as part of the fix for bug
> > #185442.
> 
> We will also need to address the timeout Youenn called out
> (http/tests/appcache/x-frame-options-prevents-framing.php).
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fappcache%2Fx-frame-options-
> prevents-framing.php

For now, skipped the test in WebKit2 in <https://trac.webkit.org/changeset/231509>. Will address in bug #185443.