Experimentally isolate OpenItemViaShell in a utility process.

Experimentally isolate OpenItemViaShell in a utility process.

Shell operations may cause 3rd-party shell extensions to be loaded into the calling process. Isolating them in a utility process protects the browser process from potential instability.

BUG=73098
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291128

[erikwright (departed), 2014-08-01 19:47:06]

sky: PTAL for chrome/
jochen: PTAL for content/
cpalmer: PTAL for chrome_utility_messages.h

[jochen (gone - plz use gerrit), 2014-08-04 09:13:41]

https://codereview.chromium.org/431343002/diff/20001/chrome/utility/sandboxed...
File chrome/utility/sandboxed_shell_handler_win.cc (right):

https://codereview.chromium.org/431343002/diff/20001/chrome/utility/sandboxed...
chrome/utility/sandboxed_shell_handler_win.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
why this file? Where's the header and the gyp/gn changes?

[erikwright (departed), 2014-08-12 18:34:31]

palmer: PTAL for chrome_utility_messages.h

[-cpalmer, +palmer]

https://codereview.chromium.org/431343002/diff/20001/chrome/utility/sandboxed...
File chrome/utility/sandboxed_shell_handler_win.cc (right):

https://codereview.chromium.org/431343002/diff/20001/chrome/utility/sandboxed...
chrome/utility/sandboxed_shell_handler_win.cc:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/08/04 09:13:41, jochen (slow - soon OOO) wrote:
> why this file? Where's the header and the gyp/gn changes?

Sorry, this was somehow left over from an earlier draft that was since
renamed/heavily modified.

[palmer, 2014-08-12 21:23:57]

https://codereview.chromium.org/431343002/diff/40001/chrome/utility/shell_han...
File chrome/utility/shell_handler_win.cc (right):

https://codereview.chromium.org/431343002/diff/40001/chrome/utility/shell_han...
chrome/utility/shell_handler_win.cc:32: ui::win::OpenItemViaShell(full_path);
Is |full_path| tested for sanity, quoted, or transformed in any way? It doesn't
seem to happen in |ui::win::OpenItemViaShell| or its callee, |OpenAnyViaShell|.
Are those functions expecting their caller to sanitize the arguments first? Or
is the utility process sandboxed such that it is guaranteed not to cause any
trouble?

[erikwright (departed), 2014-08-13 17:27:13]

https://codereview.chromium.org/431343002/diff/40001/chrome/utility/shell_han...
File chrome/utility/shell_handler_win.cc (right):

https://codereview.chromium.org/431343002/diff/40001/chrome/utility/shell_han...
chrome/utility/shell_handler_win.cc:32: ui::win::OpenItemViaShell(full_path);
On 2014/08/12 21:23:57, Chromium Palmer wrote:
> Is |full_path| tested for sanity, quoted, or transformed in any way? It
doesn't
> seem to happen in |ui::win::OpenItemViaShell| or its callee,
|OpenAnyViaShell|.
> Are those functions expecting their caller to sanitize the arguments first? Or
> is the utility process sandboxed such that it is guaranteed not to cause any
> trouble?

The utility process is not sandboxed. It is invoking a function that was
previously executed in the browser process. By putting it in a separate process
we protect the browser from 3rd-party modules that certain shell functions can
cause to be loaded into the client process. But this IPC handler is intended to
open whatever file is passed to it, in exactly the same way that its client (the
browser process) could have done.

I'm not sure what kind of sanitization would be required or possible. Presumably
a sandboxed process cannot invoke an unsandboxed utility process.

The FilePath IPC parameter is apparently serialized/deserialized as a string via
Pickle. If we are able to successfully read a string, there is not really any
other sanity check I can perform.

[erikwright (departed), 2014-08-15 17:58:30]

jschuh: PTAL. This is similar from a security POV to the other CL you looked at
for me. Unsandboxed->unsandboxed IPC message, moving a shell operation to a
utility process to reduce instability due to loading 3rd party modules.

palmer: Replaced you with jschuh per your request on the other CL, and because
this CL is similar in nature.

[jschuh, 2014-08-15 18:04:10]

Same deal as last time: unsandboxed browser <-> unsandboxed utility. It's weird,
but ipc security lgtm if it helps address things like crbug.com/403389

[erikwright (departed), 2014-08-15 18:15:49]

brettw: PTAL for content/browser/utility_process_host_impl.cc . This CL makes
the IPC client optional, which is useful for cases where there is no response
message expected.

[erikwright (departed), 2014-08-15 18:16:02]

sky: PTAL for chrome/browser

[sky, 2014-08-15 19:23:04]

When doing this, do we generally also provide a flag to explicitly turn it off?

[erikwright (departed), 2014-08-15 19:26:55]

On 2014/08/15 19:23:04, sky wrote:
> When doing this, do we generally also provide a flag to explicitly turn it
off?

I'm not quite sure what you mean. This is finch controlled. The default is the
existing behaviour. The experimental new behaviour can be enabled by Finch or by
a user from the command-line using --force-fieldtrials=...

The reason for making this experimental is to be able to A/B test the crash data
between the existing and new implementation. Once the data is collected, and
assuming the new implementation _is_ in fact worth its weight in complexity, the
experiment control and the old implementation will be removed.

[sky, 2014-08-15 19:31:17]

I was thinking of the case of someone getting in the experiment and having it
crash. Without a flag they have no way out. As I said, I'm not sure if we
generally provide a way for users to get out of the experiment.

I'll LGTM this assuming we don't.

[erikwright (departed), 2014-08-15 19:43:39]

On 2014/08/15 19:31:17, sky wrote:
> I was thinking of the case of someone getting in the experiment and having it
> crash. Without a flag they have no way out. As I said, I'm not sure if we
> generally provide a way for users to get out of the experiment.
> 
> I'll LGTM this assuming we don't.

You're correct that we don't, short of altering the command-line.

I think your question is motivated by the overloading of the word
"experimental". This is not really an experimental implementation that may or
may not work, in the sense of many features that are controlled by flags. The
experiment consists of testing whether third-party modules loaded via shell
extensions are a significant source of instability for our users. This CL is
considered as stable and reliable as any other CL that could be committed (the
vast majority of which are not, obviously, controlled by flags).

[erikwright (departed), 2014-08-19 19:08:22]

brettw: ping. Hopefully a very small review for
content/browser/utility_process_host_impl.cc and
content/public/browser/utility_process_host.h.

[brettw, 2014-08-21 16:45:53]

lgtm

[erikwright (departed), 2014-08-21 16:49:22]

The CQ bit was checked by erikwright@chromium.org

[commit-bot: I haz the power, 2014-08-21 16:51:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikwright@chromium.org/431343002/80001

[commit-bot: I haz the power, 2014-08-21 17:49:05]

Committed patchset #5 (80001) as 291128