New issue
Advanced search Search tips

Issue 921774 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 786673



Sign in to add a comment

Ensure renderer processes are locked before a navigation is committed

Project Member Reported by alex...@chromium.org, Jan 14

Issue description

Currently, we lock renderer processes to a "process lock URL" (typically a site URL) as part of SiteInstanceImpl::SetSite() [1]. This is currently called from the following places:

1. GetSiteInstanceForURL(), when creating a SiteInstance for a specific URL.

2. DetermineSiteInstanceForURL(), in a session restore corner case.

3. NavigationRequest::OnResponseStarted(), when we've determined the final URL for a navigation and know that it will probably commit.

4. NavigatorImpl::DidNavigate(), as part of processing DidCommitProvisionalLoad.

Cases 1-3 happen before a navigation commits, and before a document is sent to the renderer process.  In practice, I expect most cases to go through case 1 and 3.

However, case 4 happens after commit, and it's a potential problem for site isolation enforcements, because a compromised renderer could skip the DidCommit IPC to leave its process unlocked.  This would let it access data for other sites, at least until issue 764958 is fixed.  I can't remember when case 4 was used, and whether it's still needed - most of its usage was supposed to be replaced with case 3 by r508590.  We should try to remove it, and also introduce checks to guarantee that in --site-per-process mode, processes are locked at commit time (with the exception of ShouldLockToOrigin() cases, like chrome-guest:// and extensions).

A separate issue is that in cases 2-4, the calls to SiteInstanceImpl::SetSite() are gated on SiteInstanceImpl::ShouldAssignSiteForURL(), which returns false for about:blank (but not about:blank#foo, which should be at least made consistent), as well as for chrome-native: scheme.  This should only apply to browser-initiated about:blank navigations, which are not scriptable by anything, since renderer-initiated navigations should stay in their initiator frame's SiteInstance, which should already have a site assigned and be in a locked process.  However, creis@ points out even this may be dangerous, e.g., if content scripts inject into that process, or if the user pastes something into DevTools, since that could bypass site isolation.  I vaguely remember the browser-initiated about:blank case was relied on widely in tests.

Some older context for this might be in issue 773809.  In particular: "We've also considered setting sites for SiteInstances with dedicated processes right away, completely avoiding the lazily-initialized SiteInstance paths for them, in https://chromium-review.googlesource.com/c/chromium/src/+/636787.  We ended up not doing this on that CL, as it seemed unnecessarily risky and might have perf impact."  (perf impact, I think, referred to avoiding spinning up potentially unneeded processes during redirects, as in a redirect from foo.com to isolated.origin.com then back to foo.com.)

[1] https://cs.chromium.org/chromium/src/content/browser/site_instance_impl.cc?l=206&rcl=0abd626ef136c39711131a2ad9947cb61d6b4b7f
 

Sign in to add a comment