This new bit will make the storage partition config to preserved
across navigation (similar to the "is_guest" bit).
Also switch the isolated PWA to use this instead of guest SiteInstance.
Bug: b/304851565
Change-Id: I43755d6bce622749dbe6cca2ae5ac170e381bbba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5012192
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Jason Lin <lxj@google.com>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1227266}
ChildProcessSecurityPolicy (CPSP) maintains a set of files that a
renderer process is permitted to request in a request_file_set_ map
stored in SecurityState. This was originally introduced in 2012 in
https://chromiumcodereview-private.appspot.com/10517009 to fix
https://crbug.com/127525, where dragging a file onto a frame exposed
access to the entire file: scheme (i.e., all local files), rather than
the specific dragged file. At that time, request_file_set_
enforcement was done when CPSP::CanRequestURL() checked
SecurityState::CanRequestURL(). There was no CPSP::CanCommitURL() or
SecurityState::CanCommitURL() yet.
In 2015, https://codereview.chromium.org/1270663002/, which added
validation for the Origin header, renamed SecurityState::CanRequestURL
to CanCommitURL, so the request_file_set_ check was now in
SecurityState::CanCommitURL(). That CL also split
CPSPI::CanRequestURL() into CPSPI::CanRequestURL() and
CPSPI::CanCommitURL(), both of which used the renamed
SecurityState::CanCommitURL(). The request path added a comment to
explain this: "If the process can commit the URL, it can request it."
In 2018,
https://chromium-review.googlesource.com/c/chromium/src/+/1108485
(tightening of CPSP grants) re-introduced
SecurityState::CanRequestURL(), which was now different from
SecurityState::CanCommitURL(). CPSPI::CanRequestURL() was now
checking SecurityState::CanRequestURL(), and CPSPI::CanCommitURL()
checked SecurityState::CanCommitURL(). The "If the process can commit
the URL, it can request it" comment was removed. However, the
request_file_set_ check stayed in SS::CanCommitURL().
This CL moves the request_file_set_ check from
SecurityState::CanCommitURL() to SecurityState::CanRequestURL() - back
to where it originally started from. The rationale is that dragging a
file should give the renderer process ability to request that file -
which is for example necessary if the renderer process decides to
navigate to that file, but not to commit it, since that breaks site
isolation guarantees. For example, dragging file:///foo.txt onto a
frame in a process locked to https://foo.com does *not* allow the
foo.com process to commit a file: navigation, but it may start a
navigation to that file which will swap processes and commit in
another process that's locked to file:. Given the history of this
code, this seems to have been the intent all along, so this CL simply
cleans up some incorrect code evolution that inadvertently happened.
The drag-and-drop functionality seems to be unaffected by this - all
DnD tests still pass, and manual verification was done to ensure that
dragging a file to be uploaded or navigated to still works.
Nonetheless, given that there have been tricky bugs in this area
(e.g., https://crbug.com/705295, a kill switch is also added for this,
so the old path will be kept around until we can validate that the new
one doesn't cause problems.
There was existing test coverage in two unit tests, where the
expectations have been updated accordingly. One of these tests,
PrepareDropDataForChildProcess_LocalFiles, was blocking Citadel checks
from being turned on due to relying on CamCommitURL() still being true
after simulating dragging in a file (which normally requires a
dedicated file to commit) to an unlocked/untracked process, which
should really not be the case.
See also the discussion on
https://chromium-review.googlesource.com/c/chromium/src/+/4764617 for
more context and motivation behind this change.
Bug: 764958
Change-Id: Ic50880ddd33bfd16c3ff15e75f67f45f6bd947ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5005884
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1223814}
There are two headers to declare features in content.
- the public one: `content/public/common/content_features.h`
- the private one: `content/common/features.h`.
Unfortunately, most are declared in the public one, despite being used
privately exclusively. This violate the `content/public/` rules. This
patches provides a fix.
Parts of this patch was made programmatically using this script:
https://paste.googleplex.com/6699322946093056, with the following
output: https://paste.googleplex.com/5591288895242240
This patch:
1. Update `docs/how_to_add_your_feature_flag.md` to incentive
developers to the non public versions.
2. Move ~70 features back into the private version.
3. Programmatically update the includes to include the correct
#include header(s).
4. For consistency and minimizing the amount of files modified,
the two headers to use the `features::` namespace.
AX-Relnotes: n/a.
Change-Id: Id9126a95dfbc533d4778b188b659b5acc9b3d9e3
Bug: None
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4836057
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1194718}
1. Use base::Contains() instead of std::find or <container>::find()
where appropriate in //content/browser.
2. Refactor some code to enhance performance when executing the code.
3. The remaining code that needs to be modified in the //content/browser
directory will be modified in subsequent patches.
AX-Relnotes: n/a
Bug: 561800
Change-Id: Ibf6b4f7919256484826dfeb73859e23185579061
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4845751
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Ho Cheung <uioptt24@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1193668}
This CL renames kOriginAgentCluster, kRequiresOriginKeyedProcess and
related accessor functions on UrlInfo to clarify that they are specific
to the case where origin isolation requests derive from the explicit
presence of the Origin-Agent-Cluster header.
Bug: 1421329
Change-Id: I10ea6ec97cddd58edec3a164bb1a2890f4a332f2
Low-Coverage-Reason: refactor, no behavior change.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4518660
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1141585}
This CL is a refactor to store the default OriginAgentIsolationState
in the IsolationContext owned by a BrowsingInstance. By doing this at
the creation of the BrowsingInstance, we snapshot the default in case it
dynamically changes (e.g. via a change in the value of the enterprise
policy OriginAgentClusterDefaultEnabled).
This refactor also allows CanAccessDataForOrigin access to a
BrowsingInstance's default isolation state so that it can appropriately
construct the expected_process_lock. This is potentially a behavior
change, but only after we introduce process-isolated default OAC in a
follow-on CL.
Bug: 1421329
Change-Id: I7829c151365b685c724f79e8d40c56dd27ec5819
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4427512
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1136085}
Until the kill switch for citadel enforcements is removed, this will
provide some test coverage both with and without Citadel enforcements.
This also involves tweaking some tests to do proper cleanup, to allow
them to run twice in a row without breaking.
Bug: 764958
Change-Id: Icd4b4dee0d80ac4247b2badd40db5c3b82039d7f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4396425
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1127681}
At this point, we can expand the use of citadel protection from
Android to all platforms. This means that unlocked processes will not
be able to access data belonging to sites that require a
dedicated/locked process.
This CL also adds a default-enabled base::Feature for this
enforcement, so that there's a kill switch if anything goes wrong.
Note that this enforcement is only possible on the UI thread.
Currently, all CanAccessDataForOrigin() checks have migrated to the UI
thread, with one exception for kSupportPartitionedBlobUrl, which
should also be re-enabled at some point in https://crbug.com/1407944.
For now, though, we can already use citadel protection for all the
remaining checks on the UI thread, and skip them on the IO thread.
This has now become possible on desktop thanks to the following
prerequisite work:
- site isolation is now unconditionally enabled in <webview> tags
(https://crbug.com/1267977)
- third-party NTPs are locked to their underlying origin, and
a proper exception is made to allow most-visited tiles to live
in that locked process (CL:4111688)
- Only empty document schemes are allowed to have unassigned
SiteInstances (https://crbug.com/1296173). Previously, tests that
exercised non-empty schemes with unassigned SiteInstances
led to Citadel failures.
- Initiator origins for about:blank are honored for process and
SiteInstance assignment even when the about:blank's source
SiteInstance can't be used (https://crbug.com/1426928).
Bug: 764958
Change-Id: I7fadb4b841ca0e11c8dbcca41554d5efd35d24a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4210109
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1125697}
This CL introduces a new kRestrictCanAccessDataForOriginToUIThread
feature (disabled by default) that ensures
ChildProcessSecurityPolicy::CanAccessDataForOrigin can only be called
on the UI thread, since other threads do not have sufficient
information to perform the full set of checks. All current calls now
match this expectation, and once the feature is enabled, the checks
will prevent new code from breaking the invariant.
Note that disabling kSupportPartitionedBlobUrl also disables these
new checks, since the old blob URL code makes some
CanAccessDataForOrigin calls on the IO thread.
Bug: 1286501, 1286533
Change-Id: I7868f1fb77ec01c88da0b75af01e4ff1573e8365
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4107698
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084182}
Depending on the platform and compile flag, GTest's death test are
more or less reliable. This forced users to wrap their code with
macros. This header provide a better alternative. This patch makes
use of it to simplify code and increase the proportion of developer
using them (by the virtue of copy-pasting existing usages).
This patch uses base/test/gtest_util.h to simplify this file.
Bug: 1376132
Change-Id: I06bc662aa51b2d77e60521490e79f1f9d1ca71c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3966773
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1063526}
Depending on the platform and compile flag, GTest's death test are
more or less reliable. This forced users to wrap their code with
macros. This header provide a better alternative. This patch makes
use of it to simplify code and increase the proportion of developer
using them (by the virtue of copy-pasting existing usages).
Bug:None
Change-Id: Ic0fd4e0c0812ec672490ee83aad9995456f82c2a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3921761
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1060433}
This CL implements process isolation for fenced frames (isolating
fenced frames from their embedders) behind a flag. It adds an
'is_fenced' attribute to SiteInfo and IsolationContext to help enforce
this isolation. Having the is_fenced bit in IsolationContext is used to
maintain the same value of is_fenced for all related SiteInstances
created in a particular BrowsingInstance (e.g., for subframes of a
fenced frame).
The changes here currently only have an effect when
the flag is enabled and strict site isolation is enabled. Changes for
other site isolation modes will be in future CLs. See attached bug for
more details on design (design doc, explainer).
Bug: 1340662
Change-Id: I821943158286c9fa69639ad1156112bd74a4410e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3821704
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041755}
With the advent of OriginAgentCluster(OAC)-by-default, the number of
navigations getting OAC isolation is expected to increase dramatically. Prior to this CL, each time a navigation got OAC (either via header or
OAC-by-default) we had to check if the navigation's origin had ever
been isolated before, and if not do a potentially expensive search of
the frame tree and session history to verify that we hadn't previously
encountered the origin in the current BrowsingInstance.
To avoid OAC-by-default causing a performance regression due to many
additional global walks, this CL refactors the global walk logic so
that it is only invoked when the OAC header is explicitly present,
either for opt-in or opt-out, and not invoked when an origin gets
OAC-by-default. Since the number of sites explicitly opting-out is expected to be small, this should help keep the total number of global walks small.
When OAC-by-default is enabled, we will explicitly track all origins
that have sent the OAC header, along with the requested opt-in or
opt-out state. All untracked origins will be assumed to have the
default OAC-enabled state.
When OAC-by-default is not enabled, we continue to track only origins
that have explicitly requested opt-in via the header.
This CL also renames a number of functions used in the global walk
to make their behavior more easily understood.
More details about the global walk and this refactor/redesign can
be found at
https://docs.google.com/document/d/1zrMXDOXDhp4-qFZvkZkDVSuqsT718Y-kRVRPqestiMY/edit?usp=sharing
Bug: 1259920
Change-Id: I0a31641eebe187f3961ddfcb2a2d1977a7a3f1f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3763843
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1027872}
This CL fixes an issue when the <webview> site isolation feature is
combined with a flag to turn off strict site isolation. This results
in <webview> using the default SiteInstance paths, where we currently
assume that a default SiteInstance is never created for guests,
leading to a failed CHECK. This configuration can happen if someone
turns off strict site isolation via chrome://flags or
--disable-site-isolation-trials while also enabling
features::kSiteIsolationForGuests (e.g., via field trials).
While disabling strict site isolation is not a supported
configuration, we do not want kSiteIsolationForGuests to take a
dependency on strict site isolation, since we eventually want to
remove the old chrome-guest:// path completely. So, this CL makes
this configuration work properly by teaching default SiteInstances
about whether they are created for a guest. The proper
StoragePartition information is already passed in for that case. This
means that without strict site isolation, guest navigations will stay
in a default SiteInstance that carries the proper guest bit and
StoragePartition info.
Bug: 1337478
Change-Id: I5255545a15dbb02940fb072d1d46758b69bfbbb5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3715759
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1017356}
This is a reland of commit 81175d85f6
Original change's description:
> Do not allow ProcessLocks to be refined if process has been used.
>
> If ShouldAssignSiteForURL were to return false for URLs that commit
> actual content, then other sites that require dedicated processes
> might incorrectly share a process with them. Instead, prevent an
> allows_any_site() ProcessLock from being locked to a single site if
> that process has been used.
>
> Some of the test changes are adapted from ahemery's
> https://chromium-review.googlesource.com/c/chromium/src/+/3483667.
>
> Bug: 1324407
> Change-Id: I778d31c5e8b740179f0b6c9a40b010508ce7cfa2
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640283
> Commit-Queue: Charlie Reis <creis@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1004469}
Bug: 1324407
Change-Id: I9db2f24f66a112005664164ff19f33f1ebc098b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654240
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004979}
If ShouldAssignSiteForURL were to return false for URLs that commit
actual content, then other sites that require dedicated processes
might incorrectly share a process with them. Instead, prevent an
allows_any_site() ProcessLock from being locked to a single site if
that process has been used.
Some of the test changes are adapted from ahemery's
https://chromium-review.googlesource.com/c/chromium/src/+/3483667.
Bug: 1324407
Change-Id: I778d31c5e8b740179f0b6c9a40b010508ce7cfa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3640283
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004469}
This CL adds support for loadDataWithBaseUrl() <webview> API [1] when
<webview> is running in site isolation mode. The high-level idea is
to perform process selection and site URL computation using the base
URL's origin, rather than the data URL's opaque origin that would be
used normally. This matches up with the resulting document also
having the base URL's origin. For example,
webview.loadDataWithBaseUrl("data,foo", "https://www.example.com")
will load a document with contents "foo" with an origin of
https://www.example.com, using a guest process that's locked to
https://example.com.
To do this, this CL extends the mechanism originally introduced to
support an alternate origin for resources loaded from Web Bundles.
That mechanism used the UrlInfo::origin field, setting it to the
bundle's origin rather than the origin of UrlInfo::url, and then using
that origin for web bundle (urn:) URLs in
SiteInfo::GetSiteForURLInternal(). Now, UrlInfo::origin is also set
to the base URL origin when the navigation is done via
LoadDataWithBaseURL. UrlInfo::origin is converted to be an
absl::optional, and when it has a value,
SiteInfo::GetSiteForURLInternal() uses it in navigations to urn: and
data: URLs. Note that SiteInfo::GetSiteForURLInternal() does not know
precisely whether it's in the context of a LoadDataWithBaseURL
navigation, and adding a bit to convey that to UrlInfo seemed like
unneeded complexity, so we compromise by allowing UrlInfo::origin to
be used for all data: URLs, which is still safer than allowing origin
overrides for arbitrary navigations. NavigationRequest::GetUrlInfo()
is modified to populate the UrlInfo::origin for both Web Bundles and
LoadDataWithBaseURL.
Test coverage is provided through existing <webview>
LoadDataWithBaseURL tests, one of which is also slightly updated to
verify site isolation characteristics of the committed document. A
few tests are also updated to not pass .WithOrigin() unnecessarily
in cases where the origin is already the same as the URL.
One area of complexity here is the fact that UrlInfo is not only used
to describe information about the URL being navigated to, but it's
also used to implicitly pass around the renderer's *committed URL* and
*committed origin* from RenderFrameHostImpl::CanCommitOriginAndUrl()
(called as part of ValidateDidCommitParams()) into a few other places
that may end up referencing
RenderFrameHostImpl::CanCommitOriginAndUrl() later (e.g., to do a
ProcessLock comparison). This necessitated a few tweaks in
ChildProcessSecurityPolicyImpl::CanCommitOriginAndUrl() and
Navigator::CheckWebUIRendererDoesNotDisplayNormalURL() to maintain the
old assumptions, but this area of code should be investigated and
cleaned up further (separately from this CL). For more info, see
https://crbug.com/1320402.
Note that LoadDataWithBaseURL() has some special logic to skip
commit-time checks [2], but this is intended for Android Webview and
applies only when the process isn't locked. In the <webview> tag
case, the LoadDataWithBaseURL process will now be locked and subject
to regular checks. Android Webview's loadDataWithBaseURL behavior
remains unchanged by this CL, with no site isolation before or after.
[1] https://developer.chrome.com/docs/extensions/reference/webviewTag/#method-loadDataWithBaseUrl
[2] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=10728-10744;drc=edab26bf11e993e5069d433e8c2186ad2dee012e
Bug: 1267977
Change-Id: I9ebc0c5dc61d7b2dc8d12648b84e80f1423eab38
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3591660
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#998992}
This CL adds initial support for enabling site isolation inside
<webview> guests. This mode is behind a new feature called
SiteIsolationForGuests.
The overall approach is to stop using special chrome-guest: site URLs
when creating SiteInfos for guests. Instead, site-isolated guests use
real site URLs that are computed with the existing process model code,
and rely on SiteInfo::storage_partition_config to keep navigations
within guests in the same StoragePartition and in guest SiteInstances.
This CL supports cross-process navigations in guests, OOPIFs, and
cross-BrowsingInstance navigations in guests. A particular guest will
no longer be restricted to a single SiteInstance and BrowsingInstance,
though it will always stay in the same StoragePartition, and for all
SiteInstances in a guest, SiteInstance::IsGuest() will be true.
In the current SiteInstance assignment paths, there are various early
returns that make guests always in the same SiteInstance. Those are
now only used when the new feature is off. Since guests will now
descend much deeper into the SiteInstance assignment paths, we needed
an additional signal as to whether the new SiteInstance/SiteInfo would
be for a guest. To do this, instead of plumbing an additional flag to
places like SiteInfo::CreateInternal, is_guest is added to
IsolationContext which is already plumbed into all the needed code
paths (see SiteInfo::CreateInternal for how it's used). Conceptually,
IsolationContext::is_guest just says whether a particular
BrowsingInstance is for a guest or not, so that any related
SiteInstances within it stay in the guest.
To handle cross-BrowsingInstance navigations in guests,
SiteInstanceImpl::CreateForUrlInfo (which is normally used to create
a new SiteInstance in a new BrowsingInstance) is updated to be able to
handle guests, in which case the StoragePartition info needs to be
transferred into the new SiteInstance/BrowsingInstance.
The following are some things that are not part of this CL and will be
addressed in future CLs:
- converting <webview> tests to run in both the legacy and site
isolation modes. (This CL does add standalone tests to validate
site isolation properties.)
- embedder navigating <webview> to about:blank and siteless
SiteInstances.
- error page navigations, WebViewRendererState, loadDataWithBaseURL.
Bug: 1267977
Change-Id: I3b747640c083a302dc07ee4106af4f6d33928165
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3380343
Reviewed-by: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charles Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#965525}
The flags have been renamed to make it clear that they only
apply to Windows (see associated bug).
If you're reviewing this CL, please verify your code's usage
of the flag to ensure that the behavior is as intended. Thanks!
This CL was uploaded by git cl split.
R=alexmos@chromium.org
Bug: 1244149
Change-Id: I337a7aa817e66a13817f660baa193855bcba017c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3313719
Auto-Submit: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#947901}
ProcessLocks are used to represent what level of isolation a given
SiteInfo principal requires (e.g., whether it needs a dedicated process
for its site or origin), as well as what sites or origins are allowed
into a given process.
Both SiteInstance and SiteInstanceGroup simply use their SiteInfo to
represent the required lock, and do not need dedicated APIs for it.
Instead, callers should use ProcessLock::FromSiteInfo to do ProcessLock
comparisons. A ProcessLock constructor that takes a SiteInfo exists,
but has been made private in favour of using the static function.
Internal doc with more background
https://docs.google.com/document/d/19H19czBu8_FjOT8Cy8KdEVXxN-gZ93uNVM5rx1Qu9kw/edit?usp=sharing&resourcekey=0-bf4Im7qJWJDYnImt6OsAzw .
Test: CQ (no behaviour change)
Bug: 1261963
Change-Id: Idd0aefea850a795450b62bfc5dcff0b770739e5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3284127
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#946273}
A significant number of crashes for Issue 1148542 actually have request
urls that match the process' ProcessLock, but CanAccessDataForOrigin
(CADFO) fails solely on account of all the BrowsingInstanceIDs for that
process having been deleted. Since it seems possible that these requests
could be from lingering JS running in the renderer process, it seems
reasonable not to fail CADFO so long as the requested url matched the
ProcessLock.
This CL implements that strategy as a temporary approach to reduce
crashes until a better solution for the no BrowsingInstance ID case is
found.
Bug: 1148542
Change-Id: I38f63c5f2059b3da419b0ba2b893c22263a32c89
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3270640
Commit-Queue: W. James MacLean <wjmaclean@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944170}
This CL refactors the code that tracks OAC (OriginAgentCluster)
isolation opt-ins to allow for having both origin-keyed OAC processes
and non-origin-keyed OAC processes present at the same time.
The map in ChildProcessSecurityPolicyImpl that tracks OAC opt-ins is
|origin_isolation_by_browsing_instance_|. Prior to this CL it just
tracks a list of origins, with the assumption being that any origin
in the list is opted in for whatever OAC mechanism is currently being
used.
The two mechanisms are origin_keyed, in which each origin is assigned
its own process, and non-origin_keyed, in which each origin is logically
isolated in the renderer process, but may share a renderer process with
other origins. At present, only one of these mechanisms is active for
a given browser session.
In this CL we modify |origin_isolation_by_browsing_instance_| to track
which mechanism to use for each origin, thus allowing both mechanisms
to be active at once.
This CL also enhances the UrlInfo::OriginIsolationRequest flags to
allow us (in some future CL) to control which mechanism to register at
opt-in time.
Bug: 1259920
Change-Id: Id6a9c396f2cf94264aab171b80d72c7f4917a2f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3244802
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: W. James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#941698}
Chromium no longer supports FTP, delegating handling of FTP urls to an
external handler. As such, content sourced from FTP is no longer handled
by renderer processes.
We therefore no longer need to treat the FTP scheme as a "web safe"
scheme for the purposes of child process security isolation.
Similarly, FEED was never supported by Blink/Chromium, but the scheme
special-casing was carried over from WebKit and incompletely removed
back in 2015.
Bug: 333943
Change-Id: Icc106d89247034fc2593ade5b673d201568baa96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3269125
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: W. James MacLean <wjmaclean@chromium.org>
Commit-Queue: Eric Lawrence [MSFT] <ericlaw@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#940815}
This CL starts to pass the LocalFrameToken and verifies it in the
renderer, but won't ever fatal (just return) if the RenderFrameHost
can't be found. The fatal is the more dangerous part, and we'll save it
for the next CL.
The prior attempt at this change (https://crrev.com/c/3122751). Took a
quick and dirty approach which required a revert. This code is on a
critical path, has a lot of quick fixes layered, and is under-tested.
Let's take a slower approach as follows:
(1) Refactor session and local storage checks
(2) Add StorageKey to IPC
(3) Add LocalFrameToken to IPC and verify
Bug: 1212808
Change-Id: I50bc1bd6e49a7db43d5bf0a9b187467b68f80f66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173194
Commit-Queue: Ari Chivukula <arichiv@chromium.org>
Auto-Submit: Ari Chivukula <arichiv@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#927047}
We pass the WebExposedIsolationInfo all around the process model code,
but we already have a structure that is meant to package all the
information needed take these decisions: UrlInfo.
This patch bundles WebExposedIsolationInfo into UrlInfo, hopefully
simplifying and clearing up the code base.
The way we achieve this:
- Add WebExposedIsolationInfo into UrlInfo in site_instance_impl.h
- Move the computation of WebExposedIsolationInfo from
RenderFrameHostManager::GetWebExposedIsolationInfo to
NavigationRequest::ComputeWebExposedIsolationInfo, as a part of
NavigationRequest::GetUrlInfo, effectively embedding isolation status
into UrlInfo.
- Remove WebExposedIsolationInfo parameters in functions where we also
pass UrlInfo.
Change-Id: I4f808bf168e71da8d492a09be76a9c164fb4b867
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3101045
Commit-Queue: Arthur Hemery <ahemery@chromium.org>
Reviewed-by: W. James MacLean <wjmaclean@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#916398}
This adds a StoragePartitionConfig (SPC) to SiteInfo that is populated
via a new optional SPC parameter in UrlInfo, or
ContentBrowserClient::GetStoragePartitionConfig if no SPC is provided.
Because SiteInfo's SPC is required, its GetStoragePartitionConfig method
no longer takes a BrowserContext, and its default constructor is
replaced by one that takes a BrowserContext.
Bug: 1212266, 1229912
Change-Id: If92feddc27a05ef1ee1810f4af0ed77255a7e8d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3021578
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#905021}
This CL replaces the return type of SiteInstance::GetBrowsingInstanceId
(int32_t before this CL, BrowsingInstanceId/base::IdType32 after this
CL) and fixes the resulting fall out (switching to use
BrowsingInstanceId instead of int32_t in other places in non-test code).
Bug: 1229798
Change-Id: I7ba69edb38be618b79a16a679c15edddb96be48a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3039322
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904092}
Remove render_process_host_impl.h from headers that do not need it, and
then do IWYU for files that no longer build. This reduces the amount of
data necessary to build the chrome target by 788 MB.
Bug: 242216
Change-Id: Ia3777b5d538bcf2fd2c293a6f614c4fdb7860fb5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2852464
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#876759}
This better conveys the difference between that method and
AddIsolatedOriginsForBrowsingInstance, now that we have a growing
number of use cases for each of these, with COOP isolation using both.
Bug: 1018656
Change-Id: I79ffe77d4c83deb7efee9398951d935f06acae8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2753527
Reviewed-by: Charlie Reis <creis@chromium.org>
Owners-Override: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#862325}
Previously, isolated origins could only support isolation for future
BrowsingInstances. With COOP Isolation, we'll need an ability to
isolate a site only within the current BrowsingInstance until we get a
user gesture on that site. This CL updates the current implementation
in ChildProcessSecurityPolicy to support this new use case, exposed
via AddIsolatedOriginForBrowsingInstance() on CPSP.
Note that while OriginAgentCluster isolation also supports
per-BrowsingInstance isolation, it also forces origin-keyed isolation
that leaves subdomains of an isolated site as non-isolated. For COOP
Isolation, we want subdomains to be combined with the isolated site,
so we leverage the existing subdomain matching implementation in the
legacy isolated origins mechanism.
Future CLs will plumb COOP header status into SiteInstance selection
code and invoke AddIsolatedOriginForBrowsingInstance() once a
BrowsingInstance for a site with COOP headers is finalized.
Design doc: https://docs.google.com/document/d/122niZuMrub8vu4PJRGQrU_bG02tPPcjqWpsj3GJ1Uq0/edit?usp=sharing
Bug: 1018656
Change-Id: I520ec3f9748628f0665f495a9b591e92d8c9d4b9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2682739
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#861449}
This CL implements the behaviours of Origin-Keyed Agent Clusters for
same-process isolation. The same-process behaviour engages when
SiteIsolationPolicy::IsSiteIsolationDisabled() returns true, and is
assumed to be constant for the lifetime of the browser session. This
avoids (for now) the need to enforce consistent same-/different-process
behaviour for a BrowsingInstance, but this may change in future if
this mechanism is adapted for dynamic use gated on memory pressure
and/or process count.
Bug: 1148490
Change-Id: I1177a25b890aec43a8b2eedf7b2311fd161acd8a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2657930
Commit-Queue: James MacLean <wjmaclean@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#857906}
This change moves all site URL and lock URL generation out of
SiteInstanceImpl and into SiteInfo. This allows all details related
to the creation of these URLs to be hidden from other classes. Tests
were updated to use the new methods. ProcessLock creation was modified
slightly to make it easier to hide threading concerns related to
ProcessLocks and their related SiteInfos. A test was also added to verify
that ProcessLocks created on different threads match even though the
underlying SiteInfos do no. This should help prevent this behavior from
accidentally regressing in the future. There should not be any
user visible behavior changes introduced by this patch. It is primarily
just moving code around and renaming things.
Bug: 1085275
Change-Id: I46794960f2bb669c2e6e2750e139dc25cd03ae84
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2680068
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#853243}
