New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 791796 link

Starred by 1 user
Status: Fixed
Owner:
alex...@chromium.org
Closed: Aug 31
Cc:
rdevlin....@chromium.org
creis@chromium.org
lukasza@chromium.org
█ nasko@chromium.org
wfh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 718516



Sign in to add a comment

Hosted apps spanning multiple sites break --site-per-process guarantees

Project Member Reported by alex...@chromium.org, Dec 4 2017 
What steps will reproduce the problem?
(1) Install the following hosted app:
{
  "app": {
     "launch": {
        "web_url": "https://csreis.github.io/tests"
     },
     "urls": [ "*://csreis.github.io/tests" ]
  },
  "background": {
     "allow_js_access": false
  },
  "name": "csreis hosted app",
  "manifest_version": 2,
  "version": "1.0"
}

(2) Start Chrome with --site-per-process and navigate to http://csreis.github.io/tests/cross-site-iframe.html

(3) Click "Go cross-site (simple page)"

What is the expected result?
The HTTPS iframe is cross-site from its HTTP parent, and should go into its own process with --site-per-process.

What happens instead?
Both sites stay in the app's process.

This example uses the scheme wildcard to put two schemes with the csreis.github.io host into the same app.  This is actually an example we give on the hosted app documentation: https://developer.chrome.com/webstore/hosted_apps

This is a problem, as such a site is shooting itself in the foot with such an app -- an exploit delivered via a mitm to the http part of the site would be able to reach the https part of the site as well.  This is something that site isolation should guard against, but does not in this case.

It's also a problem if the app spans multiple hosts: e.g., the "urls" above could be      
  "urls": [ "*://csreis.github.io/tests", "*://alexmos17.github.io" ]
which is four different sites (note that github.io is a TLD, so each of these is a different site, not subdomain).  Frames from any of these sites would all stay in the same (app) process even under --site-per-process.  I've verified that this is what happens in practice.

The docs don't have much in terms of restrictions on "urls"; the only thing I see is:

"If the user downloads the app's .crx file from a server that's not the Chrome Web Store, only one domain is allowed, and it must be the same as the domain that serves the .crx file. For more information on hosting options, see the extensions documentation for Hosting."

But this doesn't seem apply for regular apps installed via CWS.

We should find a way to swap to a different process in these cases.  I found this while working on issue 718516 and can try to do the next round of investigation into what this entails.

Comment 1 by alex...@chromium.org, Dec 4 2017

Blocking: 718516

Comment 2 by alex...@chromium.org, Dec 5 2017

Labels: -Pri-2 Pri-1

Comment 3 by alex...@chromium.org, Dec 5 2017

Labels: Restrict-View-SecurityTeam
Restricting this for now, as it's a potential security issue with --site-per-process.

Comment 4 by creis@chromium.org, Mar 6 2018 

Would this be fixed by locking hosted app processes to an origin ( issue 794315 )?  Note that even multi-site hosted apps can't script across origins (including to a background page), so it might just be a question of updating how the extension logic keeps track of hosted app processes.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 15 

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

commit d4c4ca38ffda7852a98d0137d05dad69ebb0d9d3
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Aug 15 20:36:51 2018

Remove RenderprocessHost param from ShouldLockToOrigin.

This param was only being used to check for --single-process mode,
which can be done via a static function on RenderProcessHost instead.
This CL will simplify some of the plumbing in a later CL for locking
hosted apps to an origin (https://crrev.com/c/959346).

Bug:  791796 
Change-Id: I0ef1e8f3a4a00f80e74f1109a092b795f3c63722
Reviewed-on: https://chromium-review.googlesource.com/1176216
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583376}
[modify] https://crrev.com/d4c4ca38ffda7852a98d0137d05dad69ebb0d9d3/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/d4c4ca38ffda7852a98d0137d05dad69ebb0d9d3/content/browser/site_instance_impl.cc
[modify] https://crrev.com/d4c4ca38ffda7852a98d0137d05dad69ebb0d9d3/content/browser/site_instance_impl.h

Comment 6 by alex...@chromium.org, Aug 16

Status: Started (was: Assigned)
I've got a CL started for locking hosted apps to their underlying web origin (or rather, site): https://chromium-review.googlesource.com/c/chromium/src/+/959346.  Using LockToOrigin for hosted apps will also fix this issue, though we need to be careful to preserve process-per-site hosted app behavior.  For multi-site hosted apps that means that all same-site tabs for a particular hosted app should still share one process.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 17 

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

commit 71426a90ee24d449971c658cecbdb2fb2d713e2c
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Aug 17 00:14:43 2018

Lock hosted apps to their underlying web origin.

Previously, hosted apps were exempt from LockToOrigin() even in
--site-per-process mode.  That meant that hosted apps were not subject
to enforcements such as not allowing access to cookies, passwords, or
local storage of other sites.  Worse, it meant that hosted apps could
arbitrarily share a process (e.g., when over process limit), even if
they covered different web sites with --site-per-process.

This CL starts locking hosted apps to their underlying web origin.  If
a frame commits a navigation to URL http://foo.com, which is part of a
hosted app X's web extent, the process for that frame will be locked
to http://foo.com.  Note that the SiteInstance for this frame will
still use a site URL based on the effective URL (i.e.,
chrome-extension://<ext_id_for_X>/), but the origin lock will not be
based on effective URLs.  This requires plumbing to compute the origin
lock as a site URL that does not use an effective URL, and to plumb it
into various places that make process model decisions, such as
RPHI::IsSuitableHost().

Bug:  811939 ,  794315 ,  791796 
Change-Id: Icc9b3c0a04253e581ea35953f3c566308305db59
Reviewed-on: https://chromium-review.googlesource.com/959346
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583895}
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/browsing_instance.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/site_instance_impl.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/site_instance_impl.h
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/71426a90ee24d449971c658cecbdb2fb2d713e2c/content/public/browser/site_instance.h
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 17 

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

commit 218dcacf7e8891fcad111eaffcfe86794ce79b40
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Aug 17 23:12:44 2018

Revert "Lock hosted apps to their underlying web origin."

This reverts commit 71426a90ee24d449971c658cecbdb2fb2d713e2c.

Reason for revert: Causing cookies and localStorage renderer kills involving the Drive app. I found a local repro where an app covers both http and https versions of a site, and a renderer-initiated navigation from http to https does not trigger a process transfer as it should.

Original change's description:
> Lock hosted apps to their underlying web origin.
> 
> Previously, hosted apps were exempt from LockToOrigin() even in
> --site-per-process mode.  That meant that hosted apps were not subject
> to enforcements such as not allowing access to cookies, passwords, or
> local storage of other sites.  Worse, it meant that hosted apps could
> arbitrarily share a process (e.g., when over process limit), even if
> they covered different web sites with --site-per-process.
> 
> This CL starts locking hosted apps to their underlying web origin.  If
> a frame commits a navigation to URL http://foo.com, which is part of a
> hosted app X's web extent, the process for that frame will be locked
> to http://foo.com.  Note that the SiteInstance for this frame will
> still use a site URL based on the effective URL (i.e.,
> chrome-extension://<ext_id_for_X>/), but the origin lock will not be
> based on effective URLs.  This requires plumbing to compute the origin
> lock as a site URL that does not use an effective URL, and to plumb it
> into various places that make process model decisions, such as
> RPHI::IsSuitableHost().
> 
> Bug:  811939 ,  794315 ,  791796 
> Change-Id: Icc9b3c0a04253e581ea35953f3c566308305db59
> Reviewed-on: https://chromium-review.googlesource.com/959346
> Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#583895}

TBR=creis@chromium.org,rdevlin.cronin@chromium.org,alexmos@chromium.org

Change-Id: Ie459ef9b61eb78b6dad44e253768d8c4234e6abf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  811939 ,  794315 ,  791796 
Reviewed-on: https://chromium-review.googlesource.com/1180308
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584231}
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/browsing_instance.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/site_instance_impl.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/site_instance_impl.h
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/218dcacf7e8891fcad111eaffcfe86794ce79b40/content/public/browser/site_instance.h
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 29 

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

commit 5f926a551d7803b47517c7f928448ffc944d50d0
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Wed Aug 29 20:57:30 2018

Reland: lock hosted apps to their underlying web origin.

This is a reland of https://crrev.com/c/959346 with an additional fix
for cross-site navigations between sites covered by the same hosted
app.  The fix modifies site URLs for effective URLs to also include
the non-translated site in the hash.  This ensures that when multiple
sites translate to the same effective app URL, they still have
distinct site URLs.  This ensures proper process swaps when navigating
among these sites.

Original description:

Previously, hosted apps were exempt from LockToOrigin() even in
--site-per-process mode.  That meant that hosted apps were not subject
to enforcements such as not allowing access to cookies, passwords, or
local storage of other sites.  Worse, it meant that hosted apps could
arbitrarily share a process (e.g., when over process limit), even if
they covered different web sites with --site-per-process.

This CL starts locking hosted apps to their underlying web origin.  If
a frame commits a navigation to URL http://foo.com, which is part of a
hosted app X's web extent, the process for that frame will be locked
to http://foo.com.  Note that the SiteInstance for this frame will
still use a site URL based on the effective URL (i.e.,
chrome-extension://<ext_id_for_X>/), but the origin lock will not be
based on effective URLs.  This requires plumbing to compute the origin
lock as a site URL that does not use an effective URL, and to plumb it
into various places that make process model decisions, such as
RPHI::IsSuitableHost().

Bug:  811939 ,  794315 ,  791796 
Change-Id: I32cc54f3ee12ed5762272e5cfe43bd5eca94e2eb
Reviewed-on: https://chromium-review.googlesource.com/959346
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#583895}
Reviewed-on: https://chromium-review.googlesource.com/1194724
Cr-Commit-Position: refs/heads/master@{#587295}
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/browsing_instance.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/child_process_security_policy_impl.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/frame_host/render_frame_host_manager.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/isolated_origin_browsertest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/renderer_host/render_process_host_impl.h
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/renderer_host/render_process_host_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/site_instance_impl.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/site_instance_impl.h
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/browser/site_instance_impl_unittest.cc
[modify] https://crrev.com/5f926a551d7803b47517c7f928448ffc944d50d0/content/public/browser/site_instance.h

Comment 10 by alex...@chromium.org, Aug 31

Status: Fixed (was: Started)
This has been out for a couple of canaries, and I've checked for crashes at  https://crbug.com/811939#c11 , without signs of regressions so far, so I'll go ahead and close this.  Note that this fix should make it possible to clean up isolated origin / hosted app interaction, for which I've just filed  issue 879722 .
Project Member

Comment 11 by sheriffbot@chromium.org, Sep 1

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 12 by sheriffbot@chromium.org, Dec 8

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
►

Sign in to add a comment