Bug 183911 – Warn against cookie access in the WebContent process using ProcessPrivilege assertions

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

[Brent Fulgham]

Use the new ProcessPrivilege concept (see Bug 183806) to enforce our goal of prohibiting Cookie API access inside the WebContent process.

[Radar WebKit Bug Importer]

<rdar://problem/38762306>

[Brent Fulgham]

Created attachment 336344 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebKit/WebProcess/WebProcess.cpp:223:  Too many spaces inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/network/NetworkStorageSession.h:94:  The parameter name "cookieAPI" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 28 files


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

[Brent Fulgham]

Created attachment 336345 [details]
Patch

[Brent Fulgham]

Created attachment 336350 [details]
Patch

[Brent Fulgham]

Created attachment 336375 [details]
Patch

[Brent Fulgham]

WebContent really only uses NetworkStorageSession objects to know which Session ID to use when talking to the Network process.

It would be nice if NetworkStorageSession had two flavors -- one for use in the WebContent process, which didn't have any networking features in it, and another for the Network process, which could own the various platform networking API objects.

[youenn fablet]

Comment on attachment 336375 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:73
> +    enum class CookieAPI { Allowed, NotAllowed };

Maybe CookieAPIAccess would be clearer?

> Source/WebCore/platform/network/NetworkStorageSession.h:94
> +    NetworkStorageSession(PAL::SessionID, RetainPtr<CFURLStorageSessionRef>&&, RetainPtr<CFHTTPCookieStorageRef>&&, CookieAPI);

With this API, it seems that we could have in the same process some NetworkStorageSession that have Cookie API access and some NetworkStorageSession that do not have Cookie API access. Is it what we want?

If not, I wonder whether, instead of passing a parameter, we could have a process wide singleton telling whether CookieAPI access is allowed.
Then we could check it at the right places without passing this value.

Something like a static CookieAPI::isAllowed() and a corresponding setter that would also check the assertion.
Or directly using the hasPriviledge function.

> Source/WebCore/platform/network/NetworkStorageSession.h:95
> +    NetworkStorageSession(PAL::SessionID);

explicit

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:82
> +    m_platformCookieStorage = platformCookieStorage ? WTFMove(platformCookieStorage) : cookieAPI == CookieAPI::Allowed ? cookieStorage() : nullptr;

If cookieAPI access is not allowed, should platformCookieStorage be null? Should we assert it?

[Brent Fulgham]

Created attachment 336407 [details]
Patch

[Brent Fulgham]

Created attachment 336433 [details]
Patch

[youenn fablet]

Comment on attachment 336433 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:248
> +    WebCore::NetworkStorageSession::permitProcessToUseCookieAPI(true);

We could probably use std::call_once here.
Is it soon enough though?
Can there be a way to use some Cookie API that would need that permission before creating a WebProcessPool?

[Brent Fulgham]

Created attachment 336437 [details]
Patch

[youenn fablet]

Comment on attachment 336437 [details]
Patch

r=me.
It would be nice to check that if we create a website data store, get a cookie store and modify it, we do not hit those assertions. The WebProcessPool relaxing code is probably happening too late in that scenario.
An API test would tell us.

[EWS Watchlist]

Comment on attachment 336437 [details]
Patch

Attachment 336437 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7081747

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 336449 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brent Fulgham]

Created attachment 336523 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 336523 [details]
Patch for landing

Clearing flags on attachment: 336523

Committed r229978: <https://trac.webkit.org/changeset/229978>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.