Bug 193749 – [WPE][GTK] Deprecate nonfunctional process limit APIs

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Since r240363 "Deprecate API to limit the maximum number of WebProcesses" several inherently-unsafe WebKitWebContext APIs no longer work:

webkit_web_context_set_web_process_count_limit
webkit_web_context_get_web_process_count_limit

The process count limit APIs are no longer safe because we assume websites that abuse Spectre are able to read arbitrary memory from the web process, which could allow one security origin to compromise sensitive data from another after the process count limit is reached. We should deprecate those without replacement. This is easy to do because it shouldn't affect the correctness of application code unless the application is doing something weird.

typedef enum {
    WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS,
    WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES,
} WebKitProcessModel;

webkit_web_context_set_process_model
webkit_web_context_get_process_model

These ones are trickier. r240363 allows the special case of one secondary process, which is intended for debugging, but could be useful by many native apps that render trusted content. So in theory, we could keep these functions working as-is and without deprecation, though we should change the default process model to WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES and make sure our documentation explains the speculative execution attacks that will be enabled by WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS. But the problem is, we have to set APIProcessPoolConfiguration::m_usesSingleWebProcess before creating the WebProcessPool, which is too soon to keep webkit_web_context_set_process_model() working. Users call webkit_web_context_set_process_model() after the WebKitWebContext has been constructed (at which time the WebProcessPool has been constructed) but before constructing any WebKitWebViews with it (at which time the first web process is launched and it becomes too late). So if we want to keep this working, we'd need to work with Brady to make some changes to allow setting the process limit to 1 in some other manner. Otherwise, deprecating seems reasonable too, and safer.

All of the above assumes we switch on PSON, which probably makes sense *after* branching 2.24. (Without PSON, multiple secondary process mode is not safe either.) So we could, for example, make these changes on trunk for now, but revert them and r240363 on the 2.24 branch.

Probably some API tests are broken, and will need to be updated.

In Epiphany, these are hidden settings, which I'll just remove because it's not safe for a web browser. We've kept them around so far because some people need it to limit memory usage, but Adrian is about to land some massive improvements there (as much as 5x web process memory reduction with content extensions) so they won't be needed anymore.

[Carlos Garcia Campos]

We are supposed to branch in two weeks, we could also branch right now after r240363, but still allow to land the pending features API in the branch for a while. Then we can decide what to do in trunk. We can probably deprecate both APIs, and keep the single process mode as a debugging option using an env var (you love them). What's exactly PSON?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #1)
> We are supposed to branch in two weeks, we could also branch right now after
> r240363, but still allow to land the pending features API in the branch for
> a while. Then we can decide what to do in trunk.

Either way seems fine.

> We can probably deprecate
> both APIs, and keep the single process mode as a debugging option using an
> env var (you love them).

If we add an env var, let's limit it to developer mode.

> What's exactly PSON?

Process swap on navigation.

Status quo: load google.com then apple.com in the same web view, only one web process is used, apple.com can potentially steal sensitive data from google.com because it can read process memory via speculative execution attack

PSON: load google.com then apple.com, new web process is launched for the same web view, danger is mitigated because the two security origins share different processes

Probably there will be problems, so not a great thing to enable at the end of the release cycle.

[Michael Catanzaro]

Created attachment 360313 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

We might want to change the deprecated tags to Deprecated: 2.26, if we revert r240363 on the 2.24 branch.

[Michael Catanzaro]

Carlos, what do you want to do with this...?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> We might want to change the deprecated tags to Deprecated: 2.26, if we
> revert r240363 on the 2.24 branch.

We decided to do this.

[Michael Catanzaro]

Created attachment 361057 [details]
Patch

[Michael Catanzaro]

Note: my patch does not deprecate WebKitProcessModel simply because that introduces a ton of deprecation warnings in WebKit's own build process. We might want to do that in a follow-up.

[Adrian Perez]

Comment on attachment 361057 [details]
Patch

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

Looks fine, just a small comment below with a suggestion.

> Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:77
>   *

I think that the deprecation could be added conditionally to avoid
the warnings while building WebKit, but still having them show for
applications built using 2.26.x -- something like the following could
work:

  #if !defined(WEBKIT2_COMPILATION)
  WEBKIT_DEPRECATED
  #endif
  typedef enum {
      ...
  } WebKitProcessModel;

[Michael Catanzaro]

Good idea.

[Carlos Garcia Campos]

Comment on attachment 361057 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:86
> - * You can define the #WebKitCacheModel and #WebKitProcessModel with
> - * webkit_web_context_set_cache_model() and
> - * webkit_web_context_set_process_model(), depending on the needs of
> + * You can define the #WebKitCacheModel with
> + * webkit_web_context_set_cache_model(), depending on the needs of

hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model.

[Michael Catanzaro]

Comment on attachment 361057 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:86
>> + * webkit_web_context_set_cache_model(), depending on the needs of
> 
> hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model.

Hm, I'm not sure, we might indeed add more process models in the future. But we do want the documentation to reflect that the process model is currently ignored and specifying it won't do anything. I'm open to suggestions as to how to handle the documentation.

[Michael Catanzaro]

Note my current patch does not deprecate WebKitProcessModel (although it is no longer used for any purpose).

[Michael Catanzaro]

I forgot to update the documentation of webkit_web_view_new_with_related_view(). Need to do that too, though I'm not really sure how since it's a security hole. Need to ask Apple about it.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #14)
> Note my current patch does not deprecate WebKitProcessModel (although it is
> no longer used for any purpose).

I would only deprecate the specific process model, not the functions get/set and document that when using the deprecated one, the value will be ignored and the default one will be used.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #16)
> I would only deprecate the specific process model, not the functions get/set
> and document that when using the deprecated one, the value will be ignored
> and the default one will be used.

Do you want to try submitting a patch? It just seems too weird to me. My suggestion is to deprecate the functions now, and just un-deprecate them in the future in the unlikely event we ever introduce a new process model. Seems more likely that we'll never do that. And if we do, applications will have to be changed anyway if they want to use the new process model. In the meantime, it doesn't make sense for applications to continue using these functions.

[Michael Catanzaro]

(Don't forget about this one.)

[Michael Catanzaro]

BTW this is why TestMultiprocess is broken currently.

[Michael Catanzaro]

Ping reviewers.

[Carlos Garcia Campos]

Comment on attachment 361057 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:86
>>> - * You can define the #WebKitCacheModel and #WebKitProcessModel with
>>> - * webkit_web_context_set_cache_model() and
>>> - * webkit_web_context_set_process_model(), depending on the needs of
>>> + * You can define the #WebKitCacheModel with
>>> + * webkit_web_context_set_cache_model(), depending on the needs of
>> 
>> hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model.
> 
> Hm, I'm not sure, we might indeed add more process models in the future. But we do want the documentation to reflect that the process model is currently ignored and specifying it won't do anything. I'm open to suggestions as to how to handle the documentation.

hmm, are we sure there won't be process models in the future? will PSON be default? maybe it's safer to only deprecate the specific single process model.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1533
> - * This method **must be called before any web process has been created**,
> - * as early as possible in your application. Calling it later will make
> - * your application crash.
> + * This function used to set the maximum number of web processes that
> + * could be created at the same time for the @context, but it is no
> + * longer safe to set such limits as process swapping is required to
> + * mitigate speculative execution vulnerabilities.

I prefer to keep the documentation unchanged and add a note saying it's deprecated and does nothing now.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-1560
> -
> -    if (context->priv->processCountLimit == limit)
> -        return;
> -
> -    context->priv->processCountLimit = limit;

And add a g_warning here too saying this does nothing.

> Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:72
> + * @WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES: Use one or more
> + *   processes for each #WebKitWebView, while still allowing for some

One or more web processes for each view? It's confusing, the fact that we can swap processes is an implementation detail, there's still always one web process per web view, in contrast to single process for all web views.

[Michael Catanzaro]

OK, I still think it's weird to have a non-deprecated function that doesn't do anything, but your solution is OK too so I'll try again to implement it.

(In reply to Carlos Garcia Campos from comment #21)
> > Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:72
> > + * @WEBKIT_PROCESS_MODEL_MULTIPLE_SECONDARY_PROCESSES: Use one or more
> > + *   processes for each #WebKitWebView, while still allowing for some
> 
> One or more web processes for each view? It's confusing, the fact that we
> can swap processes is an implementation detail, there's still always one web
> process per web view, in contrast to single process for all web views.

Hm, let's ask Chris (hey Chris!), but I think both your comment and my documentation are wrong. It's one process per security origin (right?). So a view will be backed by multiple web processes if it includes frames from different origins, right? The two separate origins are not permitted to run in the same process; otherwise, one origin can exfiltrate data from the other by abusing Spectre. (At least that's how Chrome is supposed to work. Chris, is that actually what you've implemented? Or is that harder?) And also multiple views can happily share the same process without issue as long as they share the same security origin, right? (Again, Chris, is that what you've implemented?)

[Michael Catanzaro]

Um, and then there's related views. Do related views from different origins still share the same process?

I guess using a separate process for frames and related views might be quite difficult to implement. I assume that's the end goal, but not sure if we're there yet?

[Michael Catanzaro]

Hey Chris, could you answer the questions in comment #22 and comment #23 please?

[Milan Crha]

Regarding comment #22: how would be a "security origin" distinguished?

Evolution uses iframe-s, these (and the main body as well) can reference different URIs, like mail://, evo-http://, evo-https://, gtk-stock://, ... all of them being handled by URI handlers. I cannot imagine you run one iframe in one WebProcess and another iframe of the same WebView in another process. That will surely break all the expectations Evolution has on WebKitGTK+.

[Michael Catanzaro]

(In reply to Milan Crha from comment #25)
> Regarding comment #22: how would be a "security origin" distinguished?
> 
> Evolution uses iframe-s, these (and the main body as well) can reference
> different URIs, like mail://, evo-http://, evo-https://, gtk-stock://, ...
> all of them being handled by URI handlers. I cannot imagine you run one
> iframe in one WebProcess and another iframe of the same WebView in another
> process. That will surely break all the expectations Evolution has on
> WebKitGTK+.

Security origin is <protocol, host, port>, so yes those would all be different -security origins (often abbreviated as "origins").

This is, in fact, the eventual desired behavior afaik. I think it's what Chrome is doing also (or at least planning to do). It is nowadays a security vulnerability to have separate origins running in the same process. However, iframes can script each other, so it's not trivial to do and might not be implemented in WebKit yet (I'm not sure).

[Carlos Garcia Campos]

Created attachment 377875 [details]
Patch

[Carlos Garcia Campos]

Created attachment 377876 [details]
Patch

[Carlos Garcia Campos]

Committed r249419: <https://trac.webkit.org/changeset/249419>