Bug 183806

Summary: Create an assertion mechanism to ensure proper WebCore use in each WebKit Process
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, ddkilzer, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 183911    
Attachments:
Description Flags
Patch none

Description Brent Fulgham 2018-03-20 21:41:54 PDT
Part of our security improvements involve better isolation between the different WebKit processes (UIProcess, WebContent, Networking, Storage, etc.).

We need an assertion language we can use to protect certain critical APIs and code paths against accidental misuse.

This patch adds a new enum type meant to represent different categories of program logic that we do not want used in the wrong process.

Initially, this consists of the following:

1. None -- this process cannot use any specially privileged operations.
2. CanAccessRawCookies -- this protects access to the system cookie store. The WebContent process should only ever have cookies as filtered and meted out by the NetworkProcess.
3. CanAccessCredentials -- access to the system security credentials and keychain should not be allowed in the WebContent process.
4. CanCommunicateWithWindowServer -- WindowServer access (on macOS) should never be allowed in the WebContent process. Other platforms may have similar powerful APIs that need protection as well.
5. All -- This process may use all privileged operations. This should really only be present in the UIProcess.

This first patch just creates these types and makes them available. New assertions using these values will be added as we complete our work ensuring proper process isolation.
Comment 1 Radar WebKit Bug Importer 2018-03-20 21:42:36 PDT
<rdar://problem/38694251>
Comment 2 Brent Fulgham 2018-03-20 21:54:54 PDT
Usage:

In a method we want to protect, we can add an assertion describing the process privileges needed to execute the code:

For example, for cookie access we might use this:

    ASSERT(WTF::hasProcessPrivilege(WTF::ProcessPrivilege::CanAccessRawCookies));

At the launch of the UIProcess we would use this method to ensure all privileges are available:

    WTF::setProcessPrivileges(WTF::ProcessPrivilege::All);

In the network process, during platform initialization, we would use something like this:

    WTF::setProcessPrivileges(WTF::ProcessPrivilege::CanAccessRawCookies | WTF::ProcessPrivilege::CanAccessCredentials);

In the WebContent process, we would not set any privileges. We could just leave it as the default initialization, or use this:

    WTF::setProcessPrivileges(WTF::ProcessPrivilege::None);

Later, when we attempt to execute the initial code, we would expect an assertion for WebContent process, while Network and UIProcess pass the assertion.
Comment 3 Brent Fulgham 2018-03-20 22:19:37 PDT
Created attachment 336178 [details]
Patch
Comment 4 Ryosuke Niwa 2018-03-21 20:21:19 PDT
Comment on attachment 336178 [details]
Patch

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

r=me with the use of OptionSet.

> Source/WTF/wtf/ProcessPrivilege.h:39
> +WTF_EXPORT void setProcessPrivileges(unsigned /* bitmap of privileges */);
> +WTF_EXPORT bool hasProcessPrivilege(ProcessPrivilege);

Please use OptionSet<ProcessPrivilege>.
Comment 5 Brent Fulgham 2018-03-21 21:35:11 PDT
(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 336178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336178&action=review
> 
> r=me with the use of OptionSet.
> 
> > Source/WTF/wtf/ProcessPrivilege.h:39
> > +WTF_EXPORT void setProcessPrivileges(unsigned /* bitmap of privileges */);
> > +WTF_EXPORT bool hasProcessPrivilege(ProcessPrivilege);
> 
> Please use OptionSet<ProcessPrivilege>.

Will do.
Comment 6 Brent Fulgham 2018-03-21 21:35:15 PDT
Committed r229845: <https://trac.webkit.org/changeset/229845>