Bug 184149

Summary: Do CSP checks in the network process because redirect responses are security sensitive
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: beidson, dbates, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184980

Description Ryosuke Niwa 2018-03-29 13:16:39 PDT
We should be checking CSP in the network process in process-per-origin.
Comment 1 Radar WebKit Bug Importer 2018-03-29 13:17:10 PDT
<rdar://problem/39011045>
Comment 2 Daniel Bates 2018-04-21 16:08:43 PDT
(In reply to Ryosuke Niwa from comment #0)
> We should be checking CSP in the network process in process-per-origin.

Elaborating further the purpose of this bug is to perform CSP checks that operate on the HTTP response (e.g. frame-ancestor directive) or an HTTP redirect request in the network process. One of the benefits of performing such checks in the network process is that it avoids the need to send the HTTP response to the web content process for such analysis as the response may be for a cross-origin resource.
Comment 3 youenn fablet 2018-04-21 16:25:09 PDT
Note that some CSP checks are already done in NetworkProcess for some loads (sync xhr, beacon, media loads).

Frame-ancestor might be need specific handle, as well as CSP violation messages and reports.
Comment 4 Daniel Bates 2018-04-24 17:03:48 PDT
(In reply to Daniel Bates from comment #2)
> (In reply to Ryosuke Niwa from comment #0)
> > We should be checking CSP in the network process in process-per-origin.
> 
> Elaborating further the purpose of this bug is to perform CSP checks that
> operate on the HTTP response (e.g. frame-ancestor directive) or an HTTP
> redirect request in the network process. One of the benefits of performing
> such checks in the network process is that it avoids the need to send the
> HTTP response to the web content process for such analysis as the response
> may be for a cross-origin resource.

Deliberating on this bug some more, there are no security benefits to having all CSP checks in the network process for redirects as the initial request had to be allowed by the page's CSP (in the WebContent process) and it can be shown by inductive argument that all intermediary redirects had to be allowed by the page's CSP. There is only a security benefit to checking the frame-ancestor directive in the network process. Fixing this bug may improve performance as we could cancel a redirected load without consulting the WebContent process. However this is not stated purpose of this bug per comment 0 and it is not obvious how much a performance win moving the checks would be.
Comment 5 youenn fablet 2018-04-24 20:51:11 PDT
I think doing CSP checks in network process is going in the right direction. Maybe not the most urgent thing to do but still good to do.

This has gains in terms of efficiency and security.
This will also allow simplifying the model of the loading code.

Ideally, we should only expose to WebProcess what fetch exposes, meaning whether a response is redirected or not. Currently we are exposing all redirect URLs, which might contain sensitive information.
Agreed that we are not there yet, so finalizing CSP checks in Network process might not be the highest priority now.
Comment 6 Daniel Bates 2018-04-25 06:39:01 PDT
(In reply to youenn fablet from comment #5)
> I think doing CSP checks in network process is going in the right direction.
> Maybe not the most urgent thing to do but still good to do.
> 
> This has gains in terms of efficiency and security.

Can you please elaborate on security?

> This will also allow simplifying the model of the loading code.
> 

I am unclear how this will allow us to simplify the model of loading code given that we will need to keep the same checks in the Web process for legacy WebKit and hence need to avoid performing the same check twice if we add checks in the network process as well as need to either pass script execution state to the network process or have it call back to the web process in order to provide source code line information when sending the violation report and dispatching the DOM SecurityPolicyViolatioj event.
Comment 7 Daniel Bates 2018-04-25 10:41:14 PDT
Youenn Fablet and I discussed this bug today (04/25). Youenn expressed an interest in standardizing on IPC messaging semantics between the network process and web content process that avoids passing HTTP response data to the web content process until the last possible moment (*). If we want to do this then we will need to duplicate in the network process all the CSP redirection checks we do in the web content process. Elaborating further, we need to duplicate these checks because we will always need to keep code in WebCore to do the CSP checks for redirections to continue to support CSP in WebKit Legacy. Here are some of the options I see with regards to moving more CSP logic to the network process and the advantages and disadvantages:

Option 1: Only duplicate CSP frame-ancestor check in network process. All other CSP checks are performed in web content process.
    Advantage: With the exception of frame-ancestor all other CSP checks are centralized in one process, the web content process; => minimal code duplication.
    Disadvantage: Must duplicate CSP frame-ancestor check in network process. Added IPC cost as network process must message web content process on each redirect request to ask if the load is allowed by the page's CSP policy.

Option 2: Duplicate frame-ancestor check and all CSP checks applied to redirect requests in the network process.
    Advantage: Network process does not need to message web content process on each redirect request to ask if the load is allowed by the page's CSP policy; => avoid IPC.
    Disadvantage: Must duplicate all CSP checks for redirects in network process to avoid the need to message web content process on each redirect request to ask if the load is allowed by the page's CSP policy. (We may be able to extract some or all of the CSP checks, at least the checks in  CachedResourceLoader, into a common functions that is used both by the network process and WebCore. It is non-trivial to do this refactor).

The following options are conditional on the removal of WebKit Legacy:

Option 3: Move all CSP checks (read: including the initial request) to the network process.
    Advantage: Centralizes all CSP checks for subresource loads to the network process.
    Disadvantage: Pay IPC cost for the initial request (since we send every request for CSP processing to the network process). Adds IPC from the web content process to network process so tell it the CSP policy when specified in an HTML meta element. Adds IPC from the network process to the web content process to dispatch the SecurityPolicyViolation event for each CSP violation (may be able to batch such dispatch).

Option 4: Move all CSP checks (read: including the initial request) to the network process and teach network process to parse CSP policy specified in HTML meta element.
    Advantage: Centralizes all CSP checks for subresource loads and CSP parsing to the network process.
    Disadvantage: Pay IPC cost for the initial request (since we send every request for CSP processing to the network process). Adds IPC from the network process to the web content process to block eval execution. Adds IPC from the network process to the web content process to dispatch the SecurityPolicyViolation event for each CSP violation (may be able to batch such dispatch).

(*) This is motivated by the desire to have a process-per-origin and only have cross-origin data in the same address space when such cross-origin data has passed all security checks and hence must be delivered to the web content process in order to avoid breaking the web.
Comment 8 Daniel Bates 2018-04-25 10:49:59 PDT
(In reply to Daniel Bates from comment #7)
> Option 2: Duplicate frame-ancestor check and all CSP checks applied to
> redirect requests in the network process.
>     Advantage: Network process does not need to message web content process
> on each redirect request to ask if the load is allowed by the page's CSP
> policy; => avoid IPC.
>     Disadvantage: Must duplicate all CSP checks for redirects in network
> process to avoid the need to message web content process on each redirect
> request to ask if the load is allowed by the page's CSP policy. (We may be
> able to extract some or all of the CSP checks, at least the checks in 
> CachedResourceLoader, into a common functions that is used both by the
> network process and WebCore. It is non-trivial to do this refactor).
> 

Filed bug #184980 to track this effort.