Would it make sense to make sure crosh is always isolated? It seems powerful enough that it'd make sense that it never shares...

Summarizing some discussion between dcheng@, meacer@ and me: We agree better isolation rules are probably the way forward, but we're not sure what is a good signal to base isolation policy on. Ideally, we'd run every extension in its own renderer, but that may not scale for people with many extensions and little memory. Potential ideas for isolation signals include:
* Always run component extensions in their own renderer. Note that this catches the built-in hterm / crosh extension, but misses the hterm dev/prod extensions from the webstore which are used for crosh if installed.
* Isolate extensions that have access to private APIs, motivated by the observation that private APIs typically allow more and deeper access to Chrome and the system.

My instinct is to treat this link in the exploit chain as a security feature request for Site Isolation team. Does that make sense to other people?

I absolutely think that extensions which have access to private APIs should not be allowed to share with normal extensions.  If component extensions generally fall into this category, we could use that as a signal.

And yes, the Site Isolation team is interested in this, and it relates to some of the ideas in issue 760757 for generalizing how extension process sharing is handled (using a groups mechanism).

I wonder if there's something we can do in the short term, though, to block the attack at this level in the short term.  Maybe we should just add another PRIV bucket.

(Sorry for delayed replies; I'm at BlinkOn.)

Re comment #8: Makes sense to me. This link in the chain is definitely a weakness and it would be good to improve, however the fact that extensions share processes is WAI - we'll not be able to achieve full isolation in the general case.

OK, setting Feature flags.

creis: Assigning to you just so that it stays on someone's radar.

if it helps, we can provide a (short) list of extension ids that should be kept isolated ... 3 to be exact.  Google corp also wants to make sure process isolation is enforced for Secure Shell processes which is good because the intersection of crosh & Secure Shell is 1-to-1 :).

Unblocking, per rationale in comments #9, #10, #11.

Comment 12: Thanks-- I'll follow up with you when I get back to the office to figure out how we should do the grouping.

vapier@: Can you share the 3 extensions you mention in comment 12 that deserve isolated processes?  Is there some property they have that we can use to identify them (e.g., use of private APIs or something in the manifest), rather than baking the IDs into Chrome?

mnissler@/rdevin.cronin@: If we want to create another PRIV bucket for the time being, let's figure out which signal to use to classify extensions into some kind of "normal" and "privileged" groups.  We would do this by updating GetPrivilegeRequiredByUrl in chrome_content_browser_client_extensions_part.cc.  At the moment, I'm not seeing anything on the Extension object that would be useful as a signal.

Thanks!

if you keyed off of the terminalPrivate manifest permission (which controls access to crosh), that'd cover everything.  all the Secure Shell ids are whitelisted for that.

Comment 16: Thanks!  Looks like that's one of many private permissions in api_permission.h.

mnissler@ / rdevlin.cronin@: Do we want to treat any other private API permissions as a reason to put an extension into a different PRIV bucket than most extensions?  Is there a classifier, or would we need to list them by hand?

As discussed offline, dropping view restrictions as this is hardening work, and the details are already public in  bug 766253 .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bf6264a5041160a66a1739a275007f35ea36b0b9

commit bf6264a5041160a66a1739a275007f35ea36b0b9
Author: Nick Carter <nick@chromium.org>
Date: Fri Apr 06 02:39:33 2018

Allow extension process reuse in --site-per-process;
make SiteIsolationPolicy public.

The problem was that --site-per-process disabled extension
process sharing, but the site-per-process base::Feature (which
we've been field trialing) did not. This was due to the
extensions code checking only for the flag, and not considering
the field trial state as well.

components/printing actually got the logic right, but only by
reproducing a lot of business logic. Thus, it seems
appropriate to move SiteIsolationPolicy to content/public,
so that we can centralize the "what kind of oopifs are there"
logic. For printing, this change adds a new getter function
specific to oopif compositor, since that's basically a
derived policy of the process model.

For extensions, we've decided to disable LockToOrigin in
--site-per-process (rather than to enable it in the feature),
since origin-locking extensions doesn't help with the spectre
threat, and --site-per-process is about spectre these days.
[Charlie suggests we develop some kind of "extension isolation v2"
proposal, maybe reviving the --isolate-extension flag for that
purpose!]

Bug:  824966 , 766267

Change-Id: Ibf7592c9d522fd0c99057358bcc34b5881780db8
Reviewed-on: https://chromium-review.googlesource.com/949966
Commit-Queue: Nick Carter <nick@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Wei Li <weili@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548645}
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/components/printing/browser/print_manager_utils.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/BUILD.gn
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/browser_main_loop.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/browsing_instance.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/browser/site_instance_impl.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/BUILD.gn
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/content_browser_client.h
[rename] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/site_isolation_policy.cc
[rename] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/site_isolation_policy.h
[rename] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/browser/site_isolation_policy_unittest.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/public/test/test_utils.cc
[modify] https://crrev.com/bf6264a5041160a66a1739a275007f35ea36b0b9/content/test/BUILD.gn

Setting defect without priority to Pri-2.