ChildProcessSecurityPolicy::CanAccessDataForOrigin is the main API for
performing site isolation enforcements, deciding whether a renderer
process is allowed to access data for a particular origin. The
implementation of this check (in CanAccessMaybeOpaqueOrigin) is
currently shared for three different kinds of checks and has gotten
very complicated over the years, trying to compute an expected process
lock from the provided URL and compare it to the actual process lock,
while accounting for things like whether the URL's origin should use
origin vs site isolation.
This CL is a first step towards simplifying this implementation by
providing an alternate mechanism for implementing
CanAccessDataForOrigin (CADFO). Namely, we can track all origins ever
committed for a particular process, and then only allow access to data
for origins that have been committed, because they have already been
validated at commit time. This subsumes the current process lock
checks, since CADFO only ever used the origin comparison and ignored
the other ProcessLock/SiteInfo bits, artificially making them match in
the expected and actual locks (see
https://source.chromium.org/chromium/chromium/src/+/main:content/browser/child_process_security_policy_impl.cc;l=2050-2065;drc=f522344e45882da4c7f7cb1b3a0a7bd747d654bb).
This new check is only useful for kHostsOrigin and
kCanAccessDataForCommittedOrigin checks (i.e., whether an origin was
previously committed in a process, and whether the process can access
data belonging to an already committed origin). It cannot be used for
kCanCommitNewOrigin checks that decide whether a new origin should be
allowed to commit in a particular process. These will still use the
process lock comparisons for now.
For implementation, SecurityState now tracks a set of committed
origins. Committed origins for navigations are added in
RFHI::UpdatePermissionsForNavigation() at ready-to-commit time and in
Navigator::DidNavigate() at DidCommit time. The former is sufficient
in vast majority of cases, but the latter is still needed in cases
like adding a sandboxed about:blank iframe, where a new opaque origin
is introduced into the parent's process (OOPSIFs aren't used in this
case), but this doesn't go through the full navigation flow and
doesn't hit UpdatePermissionsForNavigation(). Committed origins are
also added when creating ServiceWorkers. For now, other workers are
assumed to stay in the origin of their creator document; there is a
corner case where this isn't true (workers created via a data: URL),
but this will be fixed in a followup.
The set of committed origins can only grow; for simplicity, there is
currently no provision to revoke a committed origin. The hope is that
this set won't grow overly large; but this may need to be
revisited. Metrics to track the size of this set will be added in a
followup CL.
This does cause a slight behavior change where a process that no
longer has any documents or workers could be denied access with the
older checks, but would still be allowed access for the older origins
with the new checks. This is covered in
DynamicIsolatedOriginTest.NewBrowsingInstanceInOldProcess, where an
origin becomes dynamically isolated, and a process previously lost
access to that origin as soon as the last BrowsingInstance where the
origin was not isolated is removed from the process, but now that
access is maintained. For now, we don't consider this to be a
meaningful decrease in security, as it's hard for the attacker to take
advantage of, but this could be revisited in the future, e.g., by
marking newly isolated origins as needing revocation when their last
instance goes away from a process.
The new checks are also slightly stricter than the old checks because
they will compare the full origins, rather than the ProcessLocks
computed from them. So, for example, if https://foo.com/ was committed
and isolated at a site granularity, requests for data from
https://sub.foo.com would now be blocked. Also, port mismatches in
origins are no longer ignored. So, if https://foo.com:1234 has
committed, requests for https://foo.com:5678 would now be denied,
whereas they would be allowed by jail/citadel checks previously (since
the ProcessLock computation ignores ports).
The enforcements of the new checks (where they replace jail and
citadel checks) are behind a feature which is off by default.
Nonetheless, all affected existing tests have been fixed to pass with
the checks on. ChildProcessSecurityPolicy tests have been
parameterized to run both with and without the new checks. When the
new checks disagree with the jail/citadel checks, we generate a
DumpWithoutCrashing report to collect more information. (Some of these
cases, like port mismatches, will be expected, as explained above.)
Bug: 40148776
Change-Id: Id31c4130ef1f8b9936b27bddb4658641fca7ffde
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5410347
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1378730}
NOTREACHED() and NOTREACHED_IN_MIGRATION() are both CHECK-fatal now.
The former is [[noreturn]] so this CL also performs dead-code removal
after the NOTREACHED().
This CL does not attempt to do additional rewrites of any surrounding
code, like:
if (!foo) {
NOTREACHED();
}
to CHECK(foo);
Those transforms take a non-trivial amount of time (and there are
thousands of instances). Cleanup can be left as an exercise for the
reader.
This does clean up kCrashOnDanglingBrowserContext as both paths of the
kill switch are currently fatal. This has been rolled out for a long
time.
Bug: 40580068, 40062641
Change-Id: Ib88e710d003e2e48df3fc502ca54d2341d157a0e
Cq-Include-Trybots: luci.chromium.try:linux-dcheck-off-rel
Low-Coverage-Reason: OTHER Should-be-unreachable code
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5974816
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1376522}
When a file is dropped on the toolbar, or I think also on any page
without a drop handler, the renderer loads the file as a navigation to a
file: URL.
For android content-URIs, we will use the existing content-URI rather
than wrap it inside a file: URL. This requires changes in a few places
where the file is handled and registered to allow navigation to it.
CompositorViewHolder.java: do not release the permissions to read a file
when a tab changes since a new tab is created for the navigation when a
file is dropped and we need to keep permission for the new tab.
browser_file_system_helper, child_process_security_policy_impl,
file_metadata: add support for files represented as content: URLs rather
than only file: URLs.
file_data_source: android content-URIs do not support seek() or pread(),
so use read() when possible, which is always the case for reading
a file after a drop.
Bug: 40101963
Bug: 363217587
Change-Id: Ia59ee6679512d1bf1a45eb412ecaeaa0675b6923
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5832588
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1363924}
This CL refactors the jail and citadel checks in
ChildProcessSecurityPolicy::CanAccessMaybeOpaqueOrigin into a separate
helper function. This improves readability of
CanAccessMaybeOpaqueOrigin, which has gotten very complicated, and is
a prerequisite to adding committed origin enforcements to that
function, which will require juggling the old and new styles of checks
and seeing if they agree. A detailed description of how these checks
work is also added. No behavior changes.
Bug: 40148776
Change-Id: Ic09e1c373d1057fc1ba554c1e4705b7f1aa03f75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5905758
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1363364}
In particular, lock the various lists of schemes with a separate lock,
to avoid a potential lock re-entrance. Otherwise,
CanAccessMaybeOpaqueOrigin() can call back out to the embedder while
already holding ChildProcessSecurityPolicyImpl's lock, which makes it
unsafe for the embedder to call back into IsWebSafeScheme() or similar.
Thanks to creis@ and alexmos@ for helping figure this one out :)
Bug: 338434846
Change-Id: I137df79366ba6d313dec5d08ae77bd7b1905a9b1
Fixed: 355013083
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5788238
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Elly FJ <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1342294}
This is a followup to r1338897, which implemented a check in Blink to
disable access to local/session storage in PDF renderers to prevent a
browser-side renderer kill. It just moves the check to be behind the
PDF enforcements feature flag (which is moved to content/public), so
that this access check also has a kill switch, just in case.
Bug: 357014503
Change-Id: I3c53d4d88f32f9534def838bdf2a2519890b5bbc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5783756
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1340488}
Accessing an invalid iterator can sometimes be a security
issue and these checks are cheap, so upgrade to CHECKs.
Generally these DCHECKS precede a use or erase of the
checked iterator, which if the check is invalid (ie. the
iterator == .end()) is UB.
Added checks are NotFatalUntil::M130.
`base/not_fatal_until.h` is added using tools/add_header.py,
this may result in some main-file (foo.h for foo.cc) headers
being re-sorted to be first as part `git cl format` of this CL.
For this CL instances were located with a weggli query:
```
weggli --verbose=1 --cpp \
'DCHECK(_ != _.end());' \
-p 'DCHECK(_.end() != _);' \
-p 'DCHECK_NE(_, _.end());' \
-p 'DCHECK_NE(_.end(), _);'
```
which should avoid any potentially expensive calculations
of the thing to be matched against .end().
This CL was uploaded by git cl split.
R=alexmos@chromium.org, dom@chromium.org, jinsukkim@chromium.org, peter@chromium.org, wanderview@chromium.org
Bug: 351745839
Change-Id: Ic4b66209052fde03394b9f34241ae2bd9173ed7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5706540
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Auto-Submit: Alex Gough <ajgo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1327876}
This CL implements process isolation of documents that set
Document-Isolation-Policy. It introduces an AgentClusterKey passed to SiteInfo,
which is used to isolate pages with DIP from pages without DIP. In this CL, the
AgentClusterKey is only computed for pages with DIP. Pages without DIP only get
an AgentClusterKey with an empty URL. Follow-up work will properly compute the
AgentClusterKey for all navigations.
Bug: 333047378
Change-Id: I86f1fa637f68dfe0932be7b2373323472c19ac7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5588626
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1323878}
This CL modifies some ChildProcessSecurityPolicyTests to explicitly
disable kOriginKeyedProcessesByDefault for tests that make no sense
to have it enabled.
Bug: 40259221
Change-Id: I17d54f7eeb0cf281fe53a9088dcf3619d3c4a077
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5660816
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1320029}
After a recent refactoring that exposes the access type being checked
in ChildProcessSecurityPolicy::CanAccessOrigin(), it becomes
straightforward to address issue 40205612 and block PDF renderer
processes from being able to access cookies, storage, or other data,
while still allowing them to commit new URLs and validating
initiator/source origins for things like postMessage, which PDF
documents may still use (e.g., see issue 40141902). The approach taken
is similar to what was done for sandboxed frame processes in
https://crrev.com/c/5282423. A new kill switch, kPdfEnforcements, is
added to guard these new enforcements.
These enforcements necessitated another change in
RenderFrameHostImpl::SendCommitNavigation(). Namely, this function has
code that sets up storage and cookie interfaces for documents that are
about to commit. It was skipping some cases that shouldn't have access
to storage, like opaque origins, but it was actually performing the
setup for PDF renderer processes, which is wrong from least-privilege
point of view since, as mentioned above, PDF renderers should never
need storage access in the first place. The storage setup code was
eventually calling CanAccessDataForOrigin (see
DOMStorageContextWrapper::IsRequestValid()), which started to fail for
PDF renderers. This CL adds a condition to not set up storage
interfaces for PDF renderers (similarly to how opaque origins are
skipped), guarded by the same kill switch as the main
ChildProcessSecurityPolicy PDF enforcements.
Bug: 40205612
Change-Id: Ica809f5e1c513a4cc8934c374c79b28fdb641185
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5549118
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1319795}
Failed navigations commit an error page with the failed URL from the
browser process's perspective, but the renderer process uses
kUnreachableWebDataURL instead. This chrome-error://chromewebdata URL
can end up being inherited if document.open() is later used. This does
not normally occur on error pages, but it is possible in Android WebView
apps, leading to CanCommitURL failures because the chrome-error: scheme
has not been granted to the process.
This CL avoids a renderer kill in this scenario by granting access to
the kUnreachableWebDataURL when a process is legitimately committing a
failed navigation.
This CL also avoids checking CanAccessMaybeOpaqueOrigin for the
kUnreachableWebDataURL, since that function cannot compute the expected
process lock when error page isolation is not in effect. The rest of the
checks (including the CPSP::State::CanCommitURL check) still apply.
Bug: 326250356
Change-Id: I2ac56c14ce4dda242d0dd37e8087cd934228d9d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5601582
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1310986}
This CL modifies GetMatchingProcessIsolatedOrigin to make sure that,
if it's called with an IsolationContext that has no BrowsingInstanceId
(which commonly happens in tests), that it gives the correct isolation
when OriginKeyedProcessesByDefault is enabled.
This bug doesn't exist without OriginKeyedProcessesByDefault enabled,
since all that's left is opt-in/opt-out isolation, and neither of
those can occur without a BrowsingInstanceId.
The original CL missed an IsolationContext created with a null
BrowsingInstanceId in SiteInfo::GetNonOriginKeyedEquivalentForMetrics.
For re-landing the CHECK_IS_TEST is removed from this CL, along with
the associated changes to the IsolationContext constructors and the
related change in RenderFrameHostManager. These will be added in a
separate CL, along with fixing the null-BrowsingInstanceId in
SiteInfo::GetNonOriginKeyedEquivalentForMetrics().
Originally landed as
https://chromium-review.googlesource.com/c/chromium/src/+/5572325
Bug: 40259221
Change-Id: I6973d635a8fe5868f7a091a28cd0865701b7c6cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5593233
Commit-Queue: 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@{#1310852}
This reverts commit eb0a64dbb3.
Reason for revert: Causes a substantial increase in browser crashes on Canary.
Original change's description:
> Don't skip isolation check for empty BrowsingInstanceId in tests.
>
> This CL modifies GetMatchingProcessIsolatedOrigin to make sure that,
> if it's called with an IsolationContext that has no BrowsingInstanceId
> (which commonly happens in tests), that it gives the correct isolation
> when OriginKeyedProcessesByDefault is enabled.
>
> This bug doesn't exist without OriginKeyedProcessesByDefault enabled,
> since all that's left is opt-in/opt-out isolation, and neither of those can occur without a BrowsingInstanceId.
>
> Bug: 40259221
> Change-Id: Ib810c80f9710f8f270a7fad36abb2cbf5332d7a2
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5572325
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Commit-Queue: W. James Maclean <wjmaclean@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1308848}
Bug: 40259221, 338792924
Change-Id: Ie0bba451babfb732b143d8bbdaaf9fe6d2b1fc61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5592571
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Auto-Submit: Wez <wez@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1309339}
This CL modifies GetMatchingProcessIsolatedOrigin to make sure that,
if it's called with an IsolationContext that has no BrowsingInstanceId
(which commonly happens in tests), that it gives the correct isolation
when OriginKeyedProcessesByDefault is enabled.
This bug doesn't exist without OriginKeyedProcessesByDefault enabled,
since all that's left is opt-in/opt-out isolation, and neither of those can occur without a BrowsingInstanceId.
Bug: 40259221
Change-Id: Ib810c80f9710f8f270a7fad36abb2cbf5332d7a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5572325
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: W. James Maclean <wjmaclean@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1308848}
Report more information about why CanCommitURL is returning false,
including two more crash keys that are included on WebView:
* cpspi_can_commit_url_failure_reason: New diagnostic crash key.
* can_access_data_failure_reason: Existing key useful on WebView as well.
Bug: 326250356
Change-Id: Ia0362fbe659688b688d526bb630d4b5e6a56e03a
Low-Coverage-Reason: HARD_TO_TEST There are no known repro steps.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5581396
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Auto-Submit: Charlie Reis <creis@chromium.org>
Reviewed-by: Richard (Torne) Coles <torne@chromium.org>
Commit-Queue: Richard (Torne) Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1307657}
If an Android WebView app sets allow_universal_access_from_file_urls to
true, it can commit an otherwise illegal URL in a document, because some
of the navigation commit-time checks are disabled.
However, apps can then disable this setting, re-enabling the normal
checks. This is a problem if another frame's same-origin document
inherits the illegal URL via document.open, causing a renderer kill.
This CL avoids the renderer kill by allowing further exceptions in the
same process and origin if the setting was used earlier, even after the
setting is later disabled.
Bug: 326250356, 324934416
Change-Id: I23c6e2b48abf77cd3c8d3ffe92367c316209c486
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5531868
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Richard (Torne) Coles <torne@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1302627}
This was generated by replacing " NOTREACHED()" with
" NOTREACHED_IN_MIGRATION()" and running git cl format.
This prepares for making NOTREACHED() [[noreturn]] alongside
NotReachedIsFatal migration of existing inventory.
Bug: 40580068
Change-Id: I3b48b89911ac5e9ffcb211622992f917f8f9e8d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5539619
Auto-Submit: Peter Boström <pbos@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Owners-Override: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301096}
ChildProcessSecurityPolicy::HostsOrigin() provides a way to validate
that a particular renderer process hosts a particular origin. It
recently added stronger checks for processes that only host sandboxed
frames (see https://crrev.com/c/5282423), ensuring that only opaque
origins are hosted in those processes. Unfortunately, this led to
renderer kills, so the enforcement was turned off in
https://crrev.com/c/5421915.
The reason for renderer kills was in extensions code. Most callers
pass a real origin obtained via a renderer IPC, but extensions code
has a couple of HostsOrigin() calls which manually synthesize a
url::Origin of the form chrome-extension://id/. This leads to a
problem if an extension frame is sandboxed (e.g., if its parent used
sandbox flags on its <iframe> attribute or, probably more commonly, by
being embedded in a sandboxed web frame and inheriting the sandbox
flags, which was the scenario seen in renderer kill reports). With
sandboxed frame isolation (issue 40082497), this now ends up in a
sandboxed extension process, and HostsOrigin() thinks that only opaque
origins can ever be hosted in such a process, but the passed-in
chrome-extension://id/ origin is not opaque.
This CL fixes this by checking if the source extension document in
IPCs that are validated by HostsOrigin() is actually coming from a
sandboxed frame, and if so, deriving a new opaque origin with
chrome-extension://id/ as the precursor, to pass into
HostsOrigin(). This better matches what the actual origin would be in
those cases, and allows HostsOrigin() to still internally check the
precursor against the ProcessLock and ensure that only a matching
extension ID is allowed to be hosted in a particular sandboxed
process. (Note that a ProcessLock for a sandboxed
chrome-extension://id/ frame is still represented by
"chrome-extension://id/" with an additional "is_sandboxed" bit.) This
approach has a bit of precedent in https://crbug.com/40887633 and
https://crrev.com/c/4220512, which also ran into renderer kills with
sandboxed extension frames.
This only ever matters for IPCs coming from extension *frames*.
ServiceWorkers cannot be associated with an opaque origin and hence
should never end up in a sandboxed process from the process model
perspective (for more info about this, see
https://chromium-review.googlesource.com/c/chromium/src/+/5466063/comment/12922bce_3c5335a5/
and description of https://crrev.com/c/5421915).
This CL also adds a test for using sendMessage from a sandboxed
extension frame. This test triggered renderer kills prior to
https://crrev.com/c/5421915 which disarmed them.
Bug: 325410297
Change-Id: I7652f534751010b91f516dd8ab7f919a1d115864
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5430657
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: W. James Maclean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1296302}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ibc66b8c440e4bcdef414e77fef4d9874d2ea9951
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5493800
Auto-Submit: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293330}
This was causing new restrictions in CanCommitURL to fail on Android
WebView, when a pseudoscheme (with an opaque origin) tried to call
document.open or document.write on a same-origin about:blank frame,
causing it to inherit the illegal URL.
This CL fixes the problem by bypassing the relevant checks for all
documents in the origin of a LoadDataWithBaseURL document, as long as
that origin is opaque. Non-opaque origins have not been causing similar
problems, and we do not want actual content loaded from such origins to
bypass the same checks.
Bug: 326250356, 324934416
Change-Id: I1b01cc3972a43bcdafe0501bd82c17bf97ed82f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5410353
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1292708}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Iee14d10d544e9f0ec046117cc4ec8a55c427adc0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5469947
Reviewed-by: Darryl James <dljames@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1290838}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ifd155bbeff882ea939f74cf8b8f847f42847940b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5468156
Reviewed-by: Darryl James <dljames@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1290297}
These were found to cause renderer kill on canary, due to extensions
code providing a non-opaque origin derived from the frame's URL when
called from sandboxed extension frames. See
https://issues.chromium.org/issues/325410297#comment7 for more
info. To avoid renderer kills, temporarily relax the new sandboxing
restrictions for all HostsOrigin() calls. Note that they would still
be subject to normal CanAccessMaybeOpaqueOrigin checks that would
verify that the URL at least matches the ProcessLock, etc. Also, the
new data access checks (kCanAccessDataForCommittedOrigin) for
sandboxed frames remain in place, as those haven't produced any
renderer kills so far.
Bug: 325410297
Change-Id: Icfc756cf8833951d9cb8544904bd11e34120f84c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5421915
Auto-Submit: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1282515}
The changes of this CL are made using the following script.
```
target_directory="content/browser"
replace_string_in_files() {
old_string="$1"
new_string="$2"
find "$target_directory" -type f \( -name "*.cc" -o -name "*.h" \) \
-exec sed -i '' "s/$old_string/$new_string/g" {} +
}
delete_include() {
find "$target_directory" \( -name "*.h" -o -name "*.cc" \) -print0 | while IFS= read -r -d '' file; do
grep -v '#include "base/strings/string_piece.h"' "$file" > "$file.tmp" && mv "$file.tmp" "$file"
done
}
add_include() {
find "$target_directory" \( -name "*.h" -o -name "*.cc" \) -print0 | while IFS= read -r -d '' file; do
local include_added=false
local tempfile=$(mktemp)
if grep -qE 'std::(string|u16string)_view' "$file"; then
while IFS= read -r line; do
echo "$line" >> "$tempfile"
if [[ $line =~ ^\s*#include ]]; then
if ! $include_added; then
echo "#include <string_view>" >> "$tempfile"
include_added=true
fi
fi
done < "$file"
mv "$tempfile" "$file"
if $include_added; then
echo "Added #include <string_view> after the first include line in $file"
else
echo "No include line found in $file"
fi
else
echo "std::string_view not found in $file"
fi
done
}
replace_string_in_files "base::StringPiece16" "std::u16string_view"
replace_string_in_files "base::StringPiece" "std::string_view"
delete_include
add_include
```
Replaced base::StringPiece16 with std::u16string_view
Replaced base::StringPiece with std::string_view
Removed header "base/strings/string_piece.h"
Added header "<string_view>" where applicable
Bug: 40506050
Change-Id: I2bc22c79dd9a0c839745afe065123f7a53c4a5ca
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5401117
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1281746}
With OOPSIFs, it becomes possible to tighten security checks for
processes that host sandboxed frames. Namely, sandboxed frames have an
opaque origin and cannot access storage, passwords, cookies and
various other forms of data. This CL implements this restriction with
an additional check in CanAccessMaybeOpaqueOrigin() that's specific to
sandboxed frame processes, blocking all access to an existing origin's
data coming from a sandboxed process. Two other types of access are
still permitted:
- new URLs are still allowed to commit in sandboxed frames. Ideally,
we would also enforce that this requires the URL to commit with an
opaque origin, but currently this knowledge is not plumbed from
all the callers (namely, CanCommitURL and CanCommitOriginAndUrl).
This will be fixed in subsequent CLs.
- using a particular origin as an initiator origin (for example, in
postMessage) is still allowed, as long as that origin is
opaque. This is because sandboxed frames are still allowed to
communicate with other frames and send/receive messages. Note that
there are still some workarounds [1] in place that exclude opaque
origins from performing ChildProcessSecurityPolicy security checks
in the first place, which also will be removed in future CLs.
Here is a summary of current uses of CanAccessDataForOrigin(), as well
as why these are all safe to block in sandboxed frames, which is what
this CL does:
- DOM storage, such as localStorage, sessionStorage, or web
databases. These are not allowed in opaque origins.
- Passwords. Blink already enforces that opaque origins already cannot
access passwords [2], so this is just adding a browser-side
enforcement.
- cookies (the common path does not use CanAccessOrigin() and protects
cookie access by construction, but one Android-specific media path
currently still uses CanAccessOrigin() to protect cookie
access). Cookies can't be accessed by opaque origins.
- blob URL access. Blob URLs *can* be created in sandboxed frames (and
will have an opaque origin as a result), but after a recent blob URL
refactor (see net::features::kSupportPartitionedBlobUrl), that
doesn't go through CanAccessOrigin() checks. Only the old mode
(already disabled by default) still uses CanAccessOrigin(), and
specifically skips it for opaque origins [3]. Hence, it should be
safe to just treat blob URLs the same as any other type of storage
from CPSP's perspective.
- Push messaging access. These require ServiceWorkers to run in the
corresponding origin, but ServiceWorkers are blocked for sandboxed
frames: see code [4] and spec [5].
- Plugins. Plugins can't be loaded in sandboxed frames, and there's no
directive to bypass that restriction [6].
[1] https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_proxy_host.cc;l=549;drc=79fd5d71c46d0e6ecd842867bc1c787fae68e218
[2] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/weborigin/security_origin.h;l=240;drc=79fd5d71c46d0e6ecd842867bc1c787fae68e218
[3] https://source.chromium.org/chromium/chromium/src/+/main:storage/browser/blob/blob_registry_impl.cc;l=639;drc=b5b5329172a1607685db895653aa928560848ed3
[4] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/service_worker/navigator_service_worker.cc;l=36;drc=a5a3f3a8599e454645674a6de1a54660c34f8faf
[5] https://www.w3.org/TR/service-workers/#control-and-use-window-client
[6] https://web.dev/articles/sandboxed-iframes.
Change-Id: I4ae23c9eef75540ac96d0cfac699272a49152cf9
Bug: 325410297
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5282423
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1280332}
The features::kRequestFileSetCheckedInCanRequestURL kill switch was
introduced in https://crrev.com/c/5005884 to guard the move of
request_file_set_ checks from CanCommitURL() to CanRequestURL(). The
feature has been enabled since M121, and with stable releases already
being past that milestone, it should be safe to clean this up and
remove the old path.
Bug: 40539942
Change-Id: I194183d641a01e2c4ef8485e8ff24e5c07fd528c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5401455
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1280003}
Currently, ChildProcessSecurityPolicy::CanAccessDataForOrigin() is used
for performing a few different kinds of checks:
1. Whether a new URL or origin is allowed to commit in a particular
process.
2. Whether a particular process already has an instance of a
particular origin (e.g., from a previously committed document). For
example, this can validate source/initiator origins in IPCs from
the renderer, e.g. when processing postMessage.
3. Whether a particular process can access data (e.g., storage or
passwords) owned by a particular origin.
There are now a few types of processes that are more locked down,
including sandboxed frame processes and PDF processes. These can
benefit from stricter security checks: for example, (3) should be
blocked in both sandboxed and PDF processes, since they should never
access data for any origin, while (1)/(2) can enforce that the origin
must be opaque for sandboxed processes. However, there's currently no
context available within CanAccessDataForOrigin to distinguish these
kinds of checks. As a result, the current checks have to assume the
most lenient scenario, which is (1), and can't be refined further.
This CL makes it possible to distinguish (1), (2) and (3) in the
implementation of CanAccessDataForOrigin. To do this, it renames its
internal implementation to CanAccessOrigin (and its internal helper to
CanAccessMaybeOpaqueOrigin), which now takes in an additional
AccessType enum that maps to (1-3). Call sites that needs to check (1)
are only found within //content, so they are refactored to call
CanAccessOrigin with that access type
directly. ChildProcessSecurityPolicy::CanAccessDataForOrigin() becomes
solely responsible for (3), which aligns with how it's named. Callers
that used CanAccessDataForOrigin for validating source or initiator
origins (2) are refactored to use a new public API,
ChildProcessSecurityPolicy::HostsOrigin(), which maps to calling
CanAccessOrigin with an access type that corresponds to (2).
This CL is a pure refactor and does not change any functionality. No
new enforcements are introduced here. A followup CL (CL:5282423) will
use the new AccessType to implement stricter enforcements for
isolated sandboxed frame processes.
Bug: 325410297
Change-Id: I80f4219876441856db38426fc10a4f7566dc615a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5278426
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1279977}
We have changed our approach for gating all MIDI access behind a
permission prompt. Now, the existing SysEx permission and prompt will
be used to control all access to the Web MIDI API.
This CL does the following:
- Roll back registration of the basic MIDI content setting and
request type
- Move the feature flag location from content/public/common to
thrid_party/blink/public/common, since the feature is no longer
related to content
- Update test_driver tests (including WPTs) to request the
permission with the SysEx flag set to true
- Always show the SysEx version of the prompt by modifying
midi_access_initializer.cc
Note that one external WPT is also being updated. Although other
browsers may not need the sysex flag set to true for the idlharness
tests to pass, setting it to true should not cause the tests to fail
since it is stronger than the basic midi permission as per spec:
https://webaudio.github.io/web-midi-api/#permissions-integration
Bug: 1420307
Change-Id: I5a6c45641c440f34bfdba0fb2076ae030528c634
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5154368
Reviewed-by: Ravjit Uppal <ravjit@chromium.org>
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Sina Firoozabadi <sinafirooz@chromium.org>
Commit-Queue: Michael Wilson <mjwilson@chromium.org>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1270342}
It seems that a site that had previously been placed into an unlocked
process is subsequently failing the citadel enforcements at DidCommit
time, which implies that the site is now requiring a dedicated
process. Add some logging to determine what exactly might be
triggering the dedicated process requirement.
Bug: 326251583
Change-Id: I3c434f0abff42ced104b79386f3db6dac9826a7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5323696
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1265577}
There are reports of renderer kills on Android WebView due to new
CanCommitURL restrictions added in r1260537. Disable the new checks
on Android WebView until they are diagnosed and fixed.
Bug: 326250356, 324934416
Low-Coverage-Reason: OTHER Repro steps unknown.
Change-Id: I5449875c0cf06f93f69d38f2a4c9cc41e7036d68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5314892
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1263648}
Some commit-time checks are not performed on the URL in certain
edge cases, depending on the origin passed to
url::Origin::Resolve(url, origin). This change ensures both the
URL and origin are always checked.
The AdditionalNavigationCommitChecks feature flag can be used
to disable these extra checks if problems are encountered.
Bug: 324934416, 1380576
Change-Id: I6241cfbaed709f0402925a86ff88223494a29638
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5132388
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1260537}
An important part of SecurityState::browsing_instance_info_map_ is that
it stores the default isolation state for each BrowsingInstance. Add
default to the name of the class member to better reflect that.
Test: No behaviour change
Bug: 1506082
Change-Id: I11fd61945fee00e01f7051d8d64afb54685224d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5071838
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1232066}
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}
