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 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}
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 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 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}
Code outside //content shouldn't really be asking whether a particular
URL is allowed to commit in a given process, since the actual
navigation commit logic is inside //content. CanCommitURL() was
already not used by non-test code outside of //content, so this CL
moves it out of the content/public ChildProcessSecurityPolicy API. It
remains accessible via ChildProcessSecurityPolicyImpl, and a test-only
helper is added to maintain coverage in an extension-related
blob/filesystem test.
Bug: 325410297
Change-Id: I158786a41a26e88c506b0c0543fd6760387111c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5398459
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1280021}
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}
This CL fixes the use of incorrect namespaces detected by
`bugprone-forward-declaration-namespace` clang-tidy check in //content.
Bug: 1519029
Change-Id: I4cfe6c9a1c44eb8d9d4fa7da4e700dc0fa2a12dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5210693
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Miyoung Shin <myid.shin@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1250628}
This CL should not cause any behavior changes as long as the flag
kBlockMidiByDefault is disabled.
As part of gating the entire WebMIDI API behind a permissions prompt,
implement logic to actually prevent sending and receiving all MIDI
messages when the permission is blocked.
This CL follows the existing pattern used for SysEx MIDI messages.
This is a sub-change of crrev.com/c/4410060
Low-Coverage-Reason: We expect the new code to be covered by unittests in crrev.com/c/4410060
Bug: 1420307
Change-Id: I9bdb0a93e8545550651dbf89f35ea96c17009817
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4569305
Commit-Queue: Michael Wilson <mjwilson@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1151593}
This cl adds a boolean to SiteInfo to allow it to track cases where
`SiteInfo::requires_origin_keyed_process_` was set due to default
isolation, and not by an explicit opt-in (e.g. receiving the header
"Origin-Agent-Cluster: ?1"). In these cases we do not want to track
the isolation state of the origin in ChildProcessSecurityPolicyImpl
since we expect the vast majority of origins to be isolated.
The new boolean is `SiteInfo::requires_origin_keyed_process_by_default_`
and will only be set when the feature kOriginKeyedProcessesByDefault
is enabled alongside kOriginAgentClusterDefaultEnable.
Bug: 1421329
Change-Id: I554c1949c2a6e7dbcaff8adb117d2e3549850d3d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4439195
Commit-Queue: James Maclean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1143406}
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}
As part of SiteInstanceGroup, we want to transition RenderFrameProxyHost
from all uses of SiteInstance to SiteInstanceGroup. One such use is in
VerifyOpenURLParams, so update it to take a RenderProcessHost instead.
The parameter is used to access the process and StoragePartition.
Test: No behaviour change
Bug: 1261963
Change-Id: I6ee0084e30a579fae6cafe4b6b88f69e0676578f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4368187
Commit-Queue: Sharon Yang <yangsharon@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1121788}
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 patch must be a no-op with all the common build flags.
Add the "DanglingUntriaged" raw_ptr annotation. It indicates a
raw_ptr becomes dangling, and it should be triaged/fixed.
This annotates the one firing during content_browsertests.
Multiple follow-up will complete the list, up until being able to
enable dangling pointer detection on a bot.
Stats:
- 175 DanglingUntriaged in code
- 69 DanglingUntriaged in tests
Bug=1291138
Change-Id: I00771bd46403f90297aa5b972fdd1ddc23b18e07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3687960
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Owners-Override: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1013525}
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 is a reland of ef8b0e4751
This CL changes the reverted CL by adding nullptr checks before
dereferences of FileSystemOperationRunner, which can be become null if
the corresponding renderer process terminates before the file system
operation is scheduled. After consultation with FileSystemManagerImpl
OWNERS, it is safe to simply discard the orphaned file system op in such
cases where the requesting renderer is being terminated.
Original change's description:
> Refactors all FileSystemManagerImpl security checks that directly or
> indirectly call ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin.
>
> Only the Browser UI thread has the necessary data to soundly evaluate
> CanAccessDataForOrigin decisions. Therefore, the end goal is to ensure
> CanAccessDataForOrigin always executes on the browser UI thread.
>
> Although all direct & indirect callers of CanAccessDataForOrigin will
> eventually need similar refactoring, this CL is an incremental landing;
> FileSystemManagerImpl was selected due to its prevalence in test results,
> and refactoring will subsequently extend to other subsystems.
>
> Change-Id: I4eda1355cc879e6731ead32caaad585e899701db
> Bug: 1266451
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3238053
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Chris Bookholt <bookholt@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#947355}
Bug: 1266451
Change-Id: I37b9573d2fd0357561619695426219535095b22b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3318774
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Charles Reis <creis@chromium.org>
Commit-Queue: Chris Bookholt <bookholt@chromium.org>
Cr-Commit-Position: refs/heads/main@{#950670}
This reverts commit ef8b0e4751.
Reason for revert: Suspected to break FileSystemApiTest.FileSystemApiRetainDirectoryEntry on Windows builders
See crbug/1275902
Original change's description:
> Refactors all FileSystemManagerImpl security checks that directly or
> indirectly call ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin.
>
> Only the Browser UI thread has the necessary data to soundly evaluate
> CanAccessDataForOrigin decisions. Therefore, the end goal is to ensure
> CanAccessDataForOrigin always executes on the browser UI thread.
>
> Although all direct & indirect callers of CanAccessDataForOrigin will
> eventually need similar refactoring, this CL is an incremental landing;
> FileSystemManagerImpl was selected due to its prevalence in test results,
> and refactoring will subsequently extend to other subsystems.
>
> Change-Id: I4eda1355cc879e6731ead32caaad585e899701db
> Bug: 1266451
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3238053
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Chris Bookholt <bookholt@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#947355}
Bug: 1266451,1275902
Change-Id: I1f8da353b2f3451542447441f8fba7b8ec421f72
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3311285
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Owners-Override: Tim Schumann <tschumann@chromium.org>
Auto-Submit: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/main@{#947413}
indirectly call ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin.
Only the Browser UI thread has the necessary data to soundly evaluate
CanAccessDataForOrigin decisions. Therefore, the end goal is to ensure
CanAccessDataForOrigin always executes on the browser UI thread.
Although all direct & indirect callers of CanAccessDataForOrigin will
eventually need similar refactoring, this CL is an incremental landing;
FileSystemManagerImpl was selected due to its prevalence in test results,
and refactoring will subsequently extend to other subsystems.
Change-Id: I4eda1355cc879e6731ead32caaad585e899701db
Bug: 1266451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3238053
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Chris Bookholt <bookholt@chromium.org>
Cr-Commit-Position: refs/heads/main@{#947355}
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 combines `origin_isolation_by_browsing_instance_` and
`origin_isolation_non_isolated_by_browsing_instance_` since recent
changes to `origin_isolation_by_browsing_instance_` mean that we can
track non-isolated cases there too.
This CL also adds a helper function LookupOriginIsolationState to
simplify lookups of specific origins within the entries for a given
BrowsingInstance.
Finally, it renames ShouldOriginGetOptInIsolation to
DetermineOriginAgentClusterIsolation.
Bug: 1269748
Change-Id: I782d9e5add01c752efa9c60d2b2bd57f07a831e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3289527
Commit-Queue: W. James MacLean <wjmaclean@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#943306}
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}
This inlines all remaining DISALLOW_* macros in content/. This is done
manually (vim regex + manually finding insertion position).
IWYU cleanup is left as a separate pass that is easier when these macros
go away.
Bug: 1010217
Change-Id: I8b5ea6dd9f8a3f584cf3eef82634017a38b15be8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3193883
Commit-Queue: Peter Boström <pbos@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Owners-Override: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#936160}
This reverts commit 55e139344d.
Reason for revert: This was a diagnostic CL, and after 3 weeks has not yielded any hits, so it's time to take it out.
Original change's description:
> Add DwoC when NoBIIDs but RFH > 0.
>
> When the number of BrowsingInstanceIDs in the SecurityState goes to
> zero, the associated process should either no longer exist, or at least
> not have any associated RenderFrameHosts. This CL adds a DwoC to collect
> some state if this condition is violated. It is a potential avenue of
> investigation for the associated issue.
>
> Bug: 1148542
> Change-Id: Ia27e8a113c53c1047dca52d93e371e9b862186ff
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3188170
> 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@{#925889}
Bug: 1148542
Change-Id: I055aefc08b91870431bc6dc9fbcee7f451f27af3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3233360
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#933781}
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}
When the number of BrowsingInstanceIDs in the SecurityState goes to
zero, the associated process should either no longer exist, or at least
not have any associated RenderFrameHosts. This CL adds a DwoC to collect
some state if this condition is violated. It is a potential avenue of
investigation for the associated issue.
Bug: 1148542
Change-Id: Ia27e8a113c53c1047dca52d93e371e9b862186ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3188170
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@{#925889}
Add a new field to SiteInfo to distinguish PDF content from HTML
content. Use that new field in SiteInfo and ProcessLock comparisons to
separate PDF content and HTML in different processes.
Bug: 1231763
Change-Id: Ic38df1d2efef51fc0575831325b2df0541b52786
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3116987
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#919057}