Validate file uploads using unguessable tokens [chromium/src : main]
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Hi Bashi-san, could you help review this? Some of the reasoning for this change is in the linked bug. There's still a few failing tests, but I think they are largely unrelated to the change, so I wanted to get your thoughts on the overall approach. Thank you!
Related details
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 11 comments![]( "Open in Gerrit")
Took a first round look. This approach was discussed in https://crbug.com/497428001.
toyoshim@: Does this approach look reasonable? I want to hear your thoughts since you have worked on IPC related stuff especially for the network service.
horoshige@: You've been working on URLLoaderFactory so I want to hear your thoughts too.
alexmos@: What do you think about this approach from the perspective of //content/browser/CHILD_PROCESS_SECURITY_POLICY_OWNERS?
base::flat_map<base::UnguessableToken, base::FilePath> browser_upload_tokens_Please fix this WARNING reported by autoreview issue finding:
---
Entries added to `browser_upload_tokens_` are never removed. This causes a memory leak that will grow for the lifetime of the browser process for every browser-initiated file upload.
Should there be a mechanism to revoke these tokens when the upload finishes, or perhaps an expiration mechanism for this map?
---
Using an LRU cache might be an option, but I'm not sure how we can guarantee that all necessary entries are kept alive.
base::flat_map<base::UnguessableToken, base::FilePath> browser_upload_tokens_Please add a comment explaining what this field is used for.
void RegisterUploadToken(ChildProcessId child_id,
const base::UnguessableToken& token,
const base::FilePath& file_path);
bool ValidateUploadToken(ChildProcessId child_id,
const base::UnguessableToken& token,
const base::FilePath& file_path);These are not ChildProcessSecurityPolicy implementations so we should move somewhere else.
Also, please add comments explaining what these methods do.
bool ValidateUploadToken(const base::UnguessableToken& token,Please fix this WARNING reported by autoreview issue finding: This method should be marked `const` so it can be called via `GetSecurityStateForQuery()` in `ChildProcessSecurityPolicyImpl::ValidateUploadToken()`.
base::flat_map<base::UnguessableToken, base::FilePath> upload_tokens_;Probably the same thing can be (memory leak) said for this?
auto* state = security_states_.GetSecurityStateForMutation(child_id);Please fix this WARNING reported by autoreview issue finding: For query operations, it's safer to use `GetSecurityStateForQuery(child_id)`.
`GetSecurityStateForMutation` intentionally returns null if the process is in `pending_remove_state_` (e.g., if the process is tearing down but an IO task is still resolving). Using it here could cause spurious validation failures during process shutdown.
You will also need to mark `SecurityState::ValidateUploadToken` as `const`.
*mutable_request.request_body->elements_mutable()) {Please fix this WARNING reported by autoreview issue finding:
`mutable_request.request_body` is a `scoped_refptr<ResourceRequestBody>`. Because it's `RefCountedThreadSafe`, mutating it in place via `elements_mutable()` modifies the underlying object which might be shared across multiple requests (e.g., during retries or multiple fetch calls).
You should create a new `ResourceRequestBody`, clone the elements into it, inject the tokens on the cloned file elements, copy over properties like `identifier_` and `contains_sensitive_info_`, and finally replace `mutable_request.request_body` with the new instance.
process_id.is_browser()Please fix this WARNING reported by autoreview issue finding: If `process_id.is_browser()` is true and there is no token, this will call `cpsp->CanReadFile(ChildProcessId(), file_path)`. Since there is no security state for a null `ChildProcessId`, this will always return `false`.
If the intention is to fail browser requests without a token, it might be clearer to explicitly write `validation_success = !process_id.is_browser() && cpsp->CanReadFile(...)`.
If the intention is to allow browser requests without a token (as the original code did), use `validation_success = process_id.is_browser() || cpsp->CanReadFile(...)`.
*out_factory = WrapBrowserProcessURLLoaderFactory(std::move(target_factory));I felt that creating a wrapped URLLoaderFactory every time might be a bit excessive. I don't have any specific ideas, but I'm wondering if there are other approaches.
toyoshim@, hiroshige@: Can I ask your opinion on this?
struct FileUploadRequest {Please add a doc comment (struct itself, each field).
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Nidhi Jaju
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 10 comments![]( "Open in Gerrit")
base::flat_map<base::UnguessableToken, base::FilePath> browser_upload_tokens_Please fix this WARNING reported by autoreview issue finding:
---
Entries added to `browser_upload_tokens_` are never removed. This causes a memory leak that will grow for the lifetime of the browser process for every browser-initiated file upload.
Should there be a mechanism to revoke these tokens when the upload finishes, or perhaps an expiration mechanism for this map?
---
Using an LRU cache might be an option, but I'm not sure how we can guarantee that all necessary entries are kept alive.
Done
base::flat_map<base::UnguessableToken, base::FilePath> browser_upload_tokens_Please add a comment explaining what this field is used for.
Done
void RegisterUploadToken(ChildProcessId child_id,
const base::UnguessableToken& token,
const base::FilePath& file_path);
bool ValidateUploadToken(ChildProcessId child_id,
const base::UnguessableToken& token,
const base::FilePath& file_path);These are not ChildProcessSecurityPolicy implementations so we should move somewhere else.
Also, please add comments explaining what these methods do.
Done
bool ValidateUploadToken(const base::UnguessableToken& token,Please fix this WARNING reported by autoreview issue finding: This method should be marked `const` so it can be called via `GetSecurityStateForQuery()` in `ChildProcessSecurityPolicyImpl::ValidateUploadToken()`.
Done
base::flat_map<base::UnguessableToken, base::FilePath> upload_tokens_;Probably the same thing can be (memory leak) said for this?
Added a comment about why this is tied to the lifetime of the renderer process.
auto* state = security_states_.GetSecurityStateForMutation(child_id);Please fix this WARNING reported by autoreview issue finding: For query operations, it's safer to use `GetSecurityStateForQuery(child_id)`.
`GetSecurityStateForMutation` intentionally returns null if the process is in `pending_remove_state_` (e.g., if the process is tearing down but an IO task is still resolving). Using it here could cause spurious validation failures during process shutdown.
You will also need to mark `SecurityState::ValidateUploadToken` as `const`.
Done
*mutable_request.request_body->elements_mutable()) {Please fix this WARNING reported by autoreview issue finding:
`mutable_request.request_body` is a `scoped_refptr<ResourceRequestBody>`. Because it's `RefCountedThreadSafe`, mutating it in place via `elements_mutable()` modifies the underlying object which might be shared across multiple requests (e.g., during retries or multiple fetch calls).
You should create a new `ResourceRequestBody`, clone the elements into it, inject the tokens on the cloned file elements, copy over properties like `identifier_` and `contains_sensitive_info_`, and finally replace `mutable_request.request_body` with the new instance.
Done
Please fix this WARNING reported by autoreview issue finding: If `process_id.is_browser()` is true and there is no token, this will call `cpsp->CanReadFile(ChildProcessId(), file_path)`. Since there is no security state for a null `ChildProcessId`, this will always return `false`.
If the intention is to fail browser requests without a token, it might be clearer to explicitly write `validation_success = !process_id.is_browser() && cpsp->CanReadFile(...)`.
If the intention is to allow browser requests without a token (as the original code did), use `validation_success = process_id.is_browser() || cpsp->CanReadFile(...)`.
Done
*out_factory = WrapBrowserProcessURLLoaderFactory(std::move(target_factory));I felt that creating a wrapped URLLoaderFactory every time might be a bit excessive. I don't have any specific ideas, but I'm wondering if there are other approaches.
toyoshim@, hiroshige@: Can I ask your opinion on this?
This design follows a common pattern for intercepting or auditing requests at the browser boundary. Other examples of URLLoaderFactory wrappers in the browser process include:
- Extensions WebRequest API: extensions::WebRequestProxyingURLLoaderFactory intercepts and rewrites headers.
- Safe Browsing Interception: safe_browsing::ProxyURLLoaderFactory scans requests at the factory level.
- DevTools Network Emulation: content::DevToolsURLLoaderFactory injects latency or mocks failures.
- Offline Pages: offline_pages::OfflinePageURLLoaderFactory loads cached pages.
- Wrapping the factory here allows us to securely bind upload tokens to browser-initiated requests at the API boundary, without modifying each individual client.
Alternatives considered:
- Modifying Network Service URLLoaderFactory directly: Not viable because the Network Service is sandboxed and cannot be trusted to self-register tokens.
- Modifying individual browser call sites: Not preferred as it would be fragile to update every browser-process component that uploads files.
Please add a doc comment (struct itself, each field).
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
*out_factory = WrapBrowserProcessURLLoaderFactory(std::move(target_factory));Nidhi JajuI felt that creating a wrapped URLLoaderFactory every time might be a bit excessive. I don't have any specific ideas, but I'm wondering if there are other approaches.
toyoshim@, hiroshige@: Can I ask your opinion on this?
This design follows a common pattern for intercepting or auditing requests at the browser boundary. Other examples of URLLoaderFactory wrappers in the browser process include:
- Extensions WebRequest API: extensions::WebRequestProxyingURLLoaderFactory intercepts and rewrites headers.
- Safe Browsing Interception: safe_browsing::ProxyURLLoaderFactory scans requests at the factory level.
- DevTools Network Emulation: content::DevToolsURLLoaderFactory injects latency or mocks failures.
- Offline Pages: offline_pages::OfflinePageURLLoaderFactory loads cached pages.
- Wrapping the factory here allows us to securely bind upload tokens to browser-initiated requests at the API boundary, without modifying each individual client.
Alternatives considered:
- Modifying Network Service URLLoaderFactory directly: Not viable because the Network Service is sandboxed and cannot be trusted to self-register tokens.
- Modifying individual browser call sites: Not preferred as it would be fragile to update every browser-process component that uploads files.
My gut feeling is: we should modify the body BEFORE sending a `network::ResourceRequest` at the call site of the request initiator, instead of intercepting requests in the middle of network loading (i.e. preferring your "Modifying individual browser call sites" option), because
- This will provide clearer view for who provides the unguessable token and for what purpose.
- This will avoid adding a new interceptor for all requests.
- I assume there are very limited number of call sites that actually need modifying (one or two?), while I don't know anything specific yet.
So my next question is: if we would proceed to this way, which call sites would be modified?
As for interception point (if we would proceed to the interceptor approach):
This (`StoragePartitionImpl::CreateURLLoaderFactoryForBrowserProcessInternal()`) is NOT the catch-all point to apply an interceptor for all browser-initiated requests. For example, `NavigationURLLoaderImpl::CreateURLLoaderFactoryWithHeaderClient()` doesn't go through this.
You mentioned `WebRequestProxyingURLLoaderFactory` etc., but they go through `ContentBrowserClient::WillCreateURLLoaderFactory()`, not here.
`ContentBrowserClient::WillCreateURLLoaderFactory()` have broader coverage (e.g.
`NavigationURLLoaderImpl::CreateURLLoaderFactoryWithHeaderClient()` still goes through `ContentBrowserClient::WillCreateURLLoaderFactory()`, while still not ALL browser-process-initiated requests go though `WillCreateURLLoaderFactory()` -- see the figure in https://docs.google.com/document/d/1oN3vkIgs76-KUO00rLTgYfploKz7MpDPBqgUUzxWupE/edit?tab=t.0#bookmark=id.c1ihcvsx6bkn ).
So, aligning with the reasoning above, I feel we should add the new interceptor through `WillCreateURLLoaderFactory()`, not here, while I'm not a big fan of `WillCreateURLLoaderFactory()`.
This would also remove the duplicated calls to `WrapBrowserProcessURLLoaderFactory()`.
Related details
- Alex Moshchuk
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Good point about the navigation requests with a `header_client` (e.g., when DevTools is open), they indeed bypass the wrapped browser factory. We can handle the navigations by registering upload tokens directly at the call sites (see below).
More broadly, there are four main ways content is uploaded from the browser process:
1. Google APIs (Drive Uploads): Uses `AttachFileForUpload()` with a file path in `UrlFetchRequestBase::StartAfterPrepare()`. Directly modifying this initiator is complicated because `google_apis` is prohibited by `google_apis/DEPS` from depending on `content/`, so it cannot call `ChildProcessSecurityPolicy` directly. Plumbing a token-registration callback from `//chrome/browser` through `RequestSender` down to `UrlFetchRequestBase` would touch dozens of files and add significant complexity.
2. Web Share Target (Navigation): Constructs file ranges in `//chrome/browser` for POST navigations. We can register tokens directly at these call sites (`share_target_utils.cc` and `webapk_post_share_target_navigator.cc`) since they are already in `//chrome/browser`.
3. Safe Browsing Deep Scanning (Enterprise Connectors): Maps files in the browser and streams them over a Mojo data pipe (`ConnectorDataPipeGetter`) instead of using raw `URLLoader` file elements, so it doesn't need upload tokens.
4. System / Feedback / Telemetry: Uploads string/bytes directly from memory using `AttachStringForUpload()`, so no file upload tokens are needed.
To reduce duplicate calls to `WrapBrowserProcessURLLoaderFactory`, we can choose to leave out wrapping for the Safe Browsing and System contexts. Since they do not perform raw file uploads today, covering them is optional for this CL. If omitted, future developers attempting uploads on those contexts will encounter `ERR_ACCESS_DENIED` until they manually wrap the factory or register tokens.
Ultimately, automatically registering tokens at the factory boundary is safe because the browser process is fully trusted. If the browser sets up a request with a file path, it has explicitly chosen to upload it. The token simply acts as cryptographic proof to `NetworkContextClient` that the upload was authorized by the trusted browser process.
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 4 comments![]( "Open in Gerrit")
Just leaving some comments. I'd like to hear other reviewers' opinion about the overall approach.
upload_tokens_[token] = {file_path, base::TimeTicks::Now()};Can still grow unconditionally?
base::TimeDelta timeout = base::Minutes(10);So does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
bool has_files = false;
for (const network::DataElement& element :
*mutable_request.request_body->elements()) {
if (element.type() == network::mojom::DataElementDataView::Tag::kFile) {
has_files = true;
break;
}
}How about:
```c++
bool has_files = std::ranges::any_of(
*mutable_request.request_body->elements(), [](const network::DataElement& element) {
return element.type() ==
network::mojom::DataElementDataView::Tag::kFile;
});
```
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Nidhi Jaju
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 3 comments![]( "Open in Gerrit")
upload_tokens_[token] = {file_path, base::TimeTicks::Now()};Can still grow unconditionally?
Entries in `upload_tokens_` have the same lifetime as the existing `file_permissions_`, they are tied to the `SecurityState` associated with the renderer process. This ensures that they last for the lifetime of the renderer process and are cleaned up when no longer needed.
base::TimeDelta timeout = base::Minutes(10);So does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
Memory usage here should scale with the number of file uploads that are occurring, so doing more uploads will take more memory, but that has always been the case (there will be a number of other objects that scale with the number of uploads, and this isn't meaningfully adding to that bloat).
- it's possible that the token might be lost if the upload takes a long time?
If you request an upload, but then don't start actually performing that upload for 10 minutes, then it's technically possible for the permission to expire. However, I did an audit of all of the relevant codepaths and did not find any places where we'll significantly delay starting the upload. Even if the network is slow, it doesn't matter if the upload takes more than 10 minutes, the time that matters is how long after token creation you start the upload.
bool has_files = false;
for (const network::DataElement& element :
*mutable_request.request_body->elements()) {
if (element.type() == network::mojom::DataElementDataView::Tag::kFile) {
has_files = true;
break;
}
}How about:
```c++
bool has_files = std::ranges::any_of(
*mutable_request.request_body->elements(), [](const network::DataElement& element) {
return element.type() ==
network::mojom::DataElementDataView::Tag::kFile;
});
```
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
*out_factory = WrapBrowserProcessURLLoaderFactory(std::move(target_factory));I've removed the wrapped URLLoaderFactory in favor of setting a token registration callback in the latest patchset, please take a look.
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 9 comments![]( "Open in Gerrit")
latter files are not yet reviewed, but let me publish tentatively.
request_body->AppendFileRange(file_path, 0, -1, base::Time(), upload_token);`/*expected_modification_time=*/base=::Time()`
request_body->AppendFileRange(file_path, 0, -1, base::Time(), upload_token);`/*offset=*/0`
request_body->AppendFileRange(file_path, 0, -1, base::Time(), upload_token);`/*length=*/-1`
base::TimeTicks creation_time;registration_time? `creation_time` for file_path is a bit confusing.
upload_tokens_[token] = {file_path, base::TimeTicks::Now()};Nidhi JajuCan still grow unconditionally?
Entries in `upload_tokens_` have the same lifetime as the existing `file_permissions_`, they are tied to the `SecurityState` associated with the renderer process. This ensures that they last for the lifetime of the renderer process and are cleaned up when no longer needed.
NavigationURLLoader case can be unregistered explicitly, but subresurce request cases are considerable here, but as we did the similar thing for file_path before, it would be ok.
base::flat_map<base::UnguessableToken, UploadTokenInfo> upload_tokens_;just file_path is enough for the child process initiator cases?
// Set the callback for SimpleURLLoader to automatically register unguessable
// upload tokens for browser-initiated requests. The
// ChildProcessSecurityPolicyImpl constructor runs exactly once very early on
// the UI thread during startup, ensuring thread-safe registration before any
// network requests can be processed.
network::SimpleURLLoader::SetUploadTokenRegisterCallback(base::BindRepeating(
[](const base::UnguessableToken& token, const base::FilePath& path) {
ChildProcessSecurityPolicyImpl::GetInstance()->RegisterUploadToken(
ChildProcessId(), token, path);
}));Well... technically said, this correctly handles the required layering dependency. But as the Rust version of this component is under experiments, this dependency might matter, and it might be better to have this callback registration outside this constructor, besides the ctor's call site.
creis@ is the best to ask?
base::TimeDelta timeout = base::Minutes(10);Nidhi JajuSo does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
Memory usage here should scale with the number of file uploads that are occurring, so doing more uploads will take more memory, but that has always been the case (there will be a number of other objects that scale with the number of uploads, and this isn't meaningfully adding to that bloat).
- it's possible that the token might be lost if the upload takes a long time?
If you request an upload, but then don't start actually performing that upload for 10 minutes, then it's technically possible for the permission to expire. However, I did an audit of all of the relevant codepaths and did not find any places where we'll significantly delay starting the upload. Even if the network is slow, it doesn't matter if the upload takes more than 10 minutes, the time that matters is how long after token creation you start the upload.
Is it possible to unregister explicitly?
Maybe called from the destructor, callback, or something like that of NavigationURLLoader and SimpleURLLoader?
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 10 comments![]( "Open in Gerrit")
reviewed on non-testing files.
so, my first interaction is done.
I will take another look after another patch set is uploaded.
validation_success =I think what you want here is
```
!base::FeatureList::IsEnabled(...) && (
process_id.is_browser() ||
(!process_id.is_browser() && cpsp->CanReadFile())
)
```
As we always want the token in the new mode?
#include "base/unguessable_token.h"wrong include place.
#include "base/unguessable_token.h"dup, already included in the header side.
using UploadTokenRegisterCallback =
base::RepeatingCallback<void(const base::UnguessableToken&,
const base::FilePath&)>;SimpleURLLoader::~SimpleURLLoader() {}
Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
SimpleURLLoader::~SimpleURLLoader() {}
Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
// and renderer-initiated requests.Can you add a TODO that says this will be non-nullable once the feature is fully enabled.
mojo_base.mojom.FilePath path;also the same TODO here.
std::vector<mojom::FileUploadRequestPtr> files_to_upload,why not const any more?
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Nidhi Jaju
Nidhi Jaju (Gerrit)
Nidhi Jaju added 16 comments![]( "Open in Gerrit")
request_body->AppendFileRange(file_path, 0, -1, base::Time(), upload_token);Nidhi Jaju`/*offset=*/0`
Done
request_body->AppendFileRange(file_path, 0, -1, base::Time(), upload_token);Nidhi Jaju`/*length=*/-1`
Done
request_body->AppendFileRange(file_path, 0, -1, base::Time(), upload_token);Nidhi Jaju`/*expected_modification_time=*/base=::Time()`
Done
registration_time? `creation_time` for file_path is a bit confusing.
Done
upload_tokens_[token] = {file_path, base::TimeTicks::Now()};Nidhi JajuCan still grow unconditionally?
Takashi ToyoshimaEntries in `upload_tokens_` have the same lifetime as the existing `file_permissions_`, they are tied to the `SecurityState` associated with the renderer process. This ensures that they last for the lifetime of the renderer process and are cleaned up when no longer needed.
NavigationURLLoader case can be unregistered explicitly, but subresurce request cases are considerable here, but as we did the similar thing for file_path before, it would be ok.
Acknowledged
base::flat_map<base::UnguessableToken, UploadTokenInfo> upload_tokens_;just file_path is enough for the child process initiator cases?
Done
base::TimeDelta timeout = base::Minutes(10);Nidhi JajuSo does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
Takashi Toyoshima
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
Memory usage here should scale with the number of file uploads that are occurring, so doing more uploads will take more memory, but that has always been the case (there will be a number of other objects that scale with the number of uploads, and this isn't meaningfully adding to that bloat).
- it's possible that the token might be lost if the upload takes a long time?
If you request an upload, but then don't start actually performing that upload for 10 minutes, then it's technically possible for the permission to expire. However, I did an audit of all of the relevant codepaths and did not find any places where we'll significantly delay starting the upload. Even if the network is slow, it doesn't matter if the upload takes more than 10 minutes, the time that matters is how long after token creation you start the upload.
Is it possible to unregister explicitly?
Maybe called from the destructor, callback, or something like that of NavigationURLLoader and SimpleURLLoader?
This turned out to be non-trivial so I've uploaded a separate CL(https://crrev.com/c/7895739) for you to take a look at, please let me know what you think. If we think it's worth it, I'll rebase the rest of the CLs on top.
validation_success =I think what you want here is
```
!base::FeatureList::IsEnabled(...) && (
process_id.is_browser() ||
(!process_id.is_browser() && cpsp->CanReadFile())
)
```
As we always want the token in the new mode?
This would break renderer-initiated uploads when the feature is enabled. Note that I've added the token logic for renderer-initiated uploads in the next CL in the chain. For these cases, they will not have an `upload_token` and must continue to be validated via `CanReadFile()` when no token is present.
#include "base/unguessable_token.h"Nidhi Jajuwrong include place.
Done
dup, already included in the header side.
Done
using UploadTokenRegisterCallback =
base::RepeatingCallback<void(const base::UnguessableToken&,
const base::FilePath&)>;Done
Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Done
Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Done
Can you add a TODO that says this will be non-nullable once the feature is fully enabled.
Done
also the same TODO here.
Done
std::vector<mojom::FileUploadRequestPtr> files_to_upload,why not const any more?
`mojom::FileUploadRequestPtr` is a Mojo struct pointer type which is move-only. This means that the C++ generated interface for Mojo methods passing arrays of move-only types uses `std::vector<mojom::FileUploadRequestPtr>` by value, allowing the elements to be moved. Passing it by `const &` is not supported by Mojo's generated C++ signature.
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 6 comments![]( "Open in Gerrit")
My major concerns are mostly resolved. Thanks! I want to hear alexmos@'s thoughts on the overall approach.
ChildProcessSecurityPolicyImpl::GetInstance()->RegisterUploadToken(Also curious about the life cycle of SimpleURLLoaders and ChildProcessSecurityPolicyImpl. Is it always guaranteed that the instance of CPSP outlives SimpleURLLoaders (especially shutdown/tear-down cases).
base::TimeDelta timeout = base::Minutes(10);Nidhi JajuSo does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
Takashi Toyoshima
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
Memory usage here should scale with the number of file uploads that are occurring, so doing more uploads will take more memory, but that has always been the case (there will be a number of other objects that scale with the number of uploads, and this isn't meaningfully adding to that bloat).
- it's possible that the token might be lost if the upload takes a long time?
If you request an upload, but then don't start actually performing that upload for 10 minutes, then it's technically possible for the permission to expire. However, I did an audit of all of the relevant codepaths and did not find any places where we'll significantly delay starting the upload. Even if the network is slow, it doesn't matter if the upload takes more than 10 minutes, the time that matters is how long after token creation you start the upload.
Nidhi JajuIs it possible to unregister explicitly?
Maybe called from the destructor, callback, or something like that of NavigationURLLoader and SimpleURLLoader?
This turned out to be non-trivial so I've uploaded a separate CL(https://crrev.com/c/7895739) for you to take a look at, please let me know what you think. If we think it's worth it, I'll rebase the rest of the CLs on top.
I want to hear toyoshim@'s opinion, but the separate CL looks a reasonable approach to me. If we go that direction, please add a TODO for the follow-up.
const base::UnguessableToken& upload_token = base::UnguessableToken());Could you add method description to explain what's this?
ASSERT_EQ(orig_elements[0].type(), cloned_elements[0].type());
EXPECT_EQ(orig_elements[0].As<DataElementBytes>().bytes(),
cloned_elements[0].As<DataElementBytes>().bytes());
ASSERT_EQ(orig_elements[1].type(), cloned_elements[1].type());
const auto& orig_file = orig_elements[1].As<DataElementFile>();
const auto& cloned_file = cloned_elements[1].As<DataElementFile>();
EXPECT_EQ(orig_file.path(), cloned_file.path());
EXPECT_EQ(orig_file.offset(), cloned_file.offset());
EXPECT_EQ(orig_file.length(), cloned_file.length());
EXPECT_EQ(orig_file.expected_modification_time(),
cloned_file.expected_modification_time());
EXPECT_EQ(orig_file.upload_token(), cloned_file.upload_token());These checks are useful, but these may not be robust enough to accommodate future field additions. May consider adding `operator==()` or `operator<=>()`?
// Sets callback for registering upload tokens.Could you make the comment a bit more detailed? For example, why is this necessary and how the callback will be used, who (embedder) sets this callback?
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Nidhi Jaju
- Takashi Toyoshima
Nidhi Jaju (Gerrit)
Nidhi Jaju added 4 comments![]( "Open in Gerrit")
ChildProcessSecurityPolicyImpl::GetInstance()->RegisterUploadToken(Also curious about the life cycle of SimpleURLLoaders and ChildProcessSecurityPolicyImpl. Is it always guaranteed that the instance of CPSP outlives SimpleURLLoaders (especially shutdown/tear-down cases).
Yes, it is always guaranteed. `ChildProcessSecurityPolicyImpl` is a singleton (whose destructor is never called), so it lives for the entire duration of the browser process, including through the shutdown and tear-down phases. Because of this, it will safely outlive all instances of `SimpleURLLoader`.
const base::UnguessableToken& upload_token = base::UnguessableToken());Could you add method description to explain what's this?
Done
ASSERT_EQ(orig_elements[0].type(), cloned_elements[0].type());
EXPECT_EQ(orig_elements[0].As<DataElementBytes>().bytes(),
cloned_elements[0].As<DataElementBytes>().bytes());
ASSERT_EQ(orig_elements[1].type(), cloned_elements[1].type());
const auto& orig_file = orig_elements[1].As<DataElementFile>();
const auto& cloned_file = cloned_elements[1].As<DataElementFile>();
EXPECT_EQ(orig_file.path(), cloned_file.path());
EXPECT_EQ(orig_file.offset(), cloned_file.offset());
EXPECT_EQ(orig_file.length(), cloned_file.length());
EXPECT_EQ(orig_file.expected_modification_time(),
cloned_file.expected_modification_time());
EXPECT_EQ(orig_file.upload_token(), cloned_file.upload_token());These checks are useful, but these may not be robust enough to accommodate future field additions. May consider adding `operator==()` or `operator<=>()`?
Done
Could you make the comment a bit more detailed? For example, why is this necessary and how the callback will be used, who (embedder) sets this callback?
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
Random drive-by. I haven't properly reviewed anything.
base::flat_map<base::UnguessableToken, UploadTokenInfo> browser_upload_tokens_IIUC, this could have thousands of entries. I think `flat_map` is a bad choice for this, because insertion takes O(N) time. `absl::flat_hash_map` might be better.
Related details
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Nidhi Jaju
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 3 comments![]( "Open in Gerrit")
Thanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
mutable base::Lock lock_;We probably should avoid this for now (as well as avoid marking the function that needed it as const). For the purposes of our Rust conversion, it will make it more difficult to reason about CPSP functions claiming to be const but still actually being able to mutate some of its members (e.g. when called from Rust). Also, the precedent in the rest of the file so far has been not to do this in similar functions, so even if we wanted to do it, it's better to split it into a separate CL.
// Set the callback for SimpleURLLoader to automatically register unguessable
// upload tokens for browser-initiated requests. The
// ChildProcessSecurityPolicyImpl constructor runs exactly once very early on
// the UI thread during startup, ensuring thread-safe registration before any
// network requests can be processed.
network::SimpleURLLoader::SetUploadTokenRegisterCallback(base::BindRepeating(
[](const base::UnguessableToken& token, const base::FilePath& path) {
ChildProcessSecurityPolicyImpl::GetInstance()->RegisterUploadToken(
ChildProcessId(), token, path);
}));Well... technically said, this correctly handles the required layering dependency. But as the Rust version of this component is under experiments, this dependency might matter, and it might be better to have this callback registration outside this constructor, besides the ctor's call site.
creis@ is the best to ask?
Thanks for asking!
Currently, this will get initialized when there's a very first check involving ChildProcessSecurityPolicy, and this is currently different on different platforms and can change over time. We've already seen a bug because of this: https://crbug.com/507737405, where it happened before the Finch flags were even initialized. I wonder if this might be problematic if the network service code gets invoked too early, though it looks like this particular call just does dependency injection and saves a callback, and doesn't actually need to send an IPC to the network service process. Is that right?
Overall, though, because of this non-determinism we feel that it might be better to move this registration to a more well-defined place somewhere on the browser startup path in content/. Maybe something like [this](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/browser_main_loop.cc;l=828-836;drc=46b1094602f60b37f23a55d04d9d2fefcd1c0a14), where we also initialize other site isolation properties that need to be in place before any navigation can happen?
This will also make it easier to reason about the Rust conversion, as there will be less of a risk of forgetting to do an equivalent registration in the Rust CPSP initialization code. And we can also consider similarly moving the `RegisterDefaultSchemes()` call outside of the CPSP constructor in a followup.
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Nidhi Jaju
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
base::TimeDelta timeout = base::Minutes(10);Nidhi JajuSo does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
Takashi Toyoshima
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
Memory usage here should scale with the number of file uploads that are occurring, so doing more uploads will take more memory, but that has always been the case (there will be a number of other objects that scale with the number of uploads, and this isn't meaningfully adding to that bloat).
- it's possible that the token might be lost if the upload takes a long time?
If you request an upload, but then don't start actually performing that upload for 10 minutes, then it's technically possible for the permission to expire. However, I did an audit of all of the relevant codepaths and did not find any places where we'll significantly delay starting the upload. Even if the network is slow, it doesn't matter if the upload takes more than 10 minutes, the time that matters is how long after token creation you start the upload.
Nidhi JajuIs it possible to unregister explicitly?
Maybe called from the destructor, callback, or something like that of NavigationURLLoader and SimpleURLLoader?
Kenichi IshibashiThis turned out to be non-trivial so I've uploaded a separate CL(https://crrev.com/c/7895739) for you to take a look at, please let me know what you think. If we think it's worth it, I'll rebase the rest of the CLs on top.
I want to hear toyoshim@'s opinion, but the separate CL looks a reasonable approach to me. If we go that direction, please add a TODO for the follow-up.
Yep. It looks reasonable to me too.
validation_success =Nidhi JajuI think what you want here is
```
!base::FeatureList::IsEnabled(...) && (
process_id.is_browser() ||
(!process_id.is_browser() && cpsp->CanReadFile())
)
```
As we always want the token in the new mode?
This would break renderer-initiated uploads when the feature is enabled. Note that I've added the token logic for renderer-initiated uploads in the next CL in the chain. For these cases, they will not have an `upload_token` and must continue to be validated via `CanReadFile()` when no token is present.
Ah, I see. So, we would want a TODO.
Also, it might be better to have clearly separated logics for each mode even if it is redundant. It helps to avoid mistakes.
I meant something like this;
```
if (!base::FeatureList::IsEnabled(...)) {
...
} else ]
// TODO...
...
}
```
std::vector<mojom::FileUploadRequestPtr> files_to_upload,Nidhi Jajuwhy not const any more?
`mojom::FileUploadRequestPtr` is a Mojo struct pointer type which is move-only. This means that the C++ generated interface for Mojo methods passing arrays of move-only types uses `std::vector<mojom::FileUploadRequestPtr>` by value, allowing the elements to be moved. Passing it by `const &` is not supported by Mojo's generated C++ signature.
Acknowledged
Related details
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Nidhi Jaju
Nidhi Jaju (Gerrit)
Nidhi Jaju added 7 comments![]( "Open in Gerrit")
Thanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Thank you for the detailed context! I'm working on a doc that I'll share with you early next week.
We probably should avoid this for now (as well as avoid marking the function that needed it as const). For the purposes of our Rust conversion, it will make it more difficult to reason about CPSP functions claiming to be const but still actually being able to mutate some of its members (e.g. when called from Rust). Also, the precedent in the rest of the file so far has been not to do this in similar functions, so even if we wanted to do it, it's better to split it into a separate CL.
Done
base::flat_map<base::UnguessableToken, UploadTokenInfo> browser_upload_tokens_IIUC, this could have thousands of entries. I think `flat_map` is a bad choice for this, because insertion takes O(N) time. `absl::flat_hash_map` might be better.
Good point, I've updated both `browser_upload_tokens_` and `upload_tokens_` to be `absl::flat_hash_map`s.
ChildProcessSecurityPolicyImpl::GetInstance()->RegisterUploadToken(Nidhi JajuAlso curious about the life cycle of SimpleURLLoaders and ChildProcessSecurityPolicyImpl. Is it always guaranteed that the instance of CPSP outlives SimpleURLLoaders (especially shutdown/tear-down cases).
Yes, it is always guaranteed. `ChildProcessSecurityPolicyImpl` is a singleton (whose destructor is never called), so it lives for the entire duration of the browser process, including through the shutdown and tear-down phases. Because of this, it will safely outlive all instances of `SimpleURLLoader`.
Resolving since I moved it to BrowserMainLoop based on Alex's most recent feedback.
// Set the callback for SimpleURLLoader to automatically register unguessable
// upload tokens for browser-initiated requests. The
// ChildProcessSecurityPolicyImpl constructor runs exactly once very early on
// the UI thread during startup, ensuring thread-safe registration before any
// network requests can be processed.
network::SimpleURLLoader::SetUploadTokenRegisterCallback(base::BindRepeating(
[](const base::UnguessableToken& token, const base::FilePath& path) {
ChildProcessSecurityPolicyImpl::GetInstance()->RegisterUploadToken(
ChildProcessId(), token, path);
}));Alex MoshchukWell... technically said, this correctly handles the required layering dependency. But as the Rust version of this component is under experiments, this dependency might matter, and it might be better to have this callback registration outside this constructor, besides the ctor's call site.
creis@ is the best to ask?
Thanks for asking!
Currently, this will get initialized when there's a very first check involving ChildProcessSecurityPolicy, and this is currently different on different platforms and can change over time. We've already seen a bug because of this: https://crbug.com/507737405, where it happened before the Finch flags were even initialized. I wonder if this might be problematic if the network service code gets invoked too early, though it looks like this particular call just does dependency injection and saves a callback, and doesn't actually need to send an IPC to the network service process. Is that right?
Overall, though, because of this non-determinism we feel that it might be better to move this registration to a more well-defined place somewhere on the browser startup path in content/. Maybe something like [this](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/browser_main_loop.cc;l=828-836;drc=46b1094602f60b37f23a55d04d9d2fefcd1c0a14), where we also initialize other site isolation properties that need to be in place before any navigation can happen?
This will also make it easier to reason about the Rust conversion, as there will be less of a risk of forgetting to do an equivalent registration in the Rust CPSP initialization code. And we can also consider similarly moving the `RegisterDefaultSchemes()` call outside of the CPSP constructor in a followup.
Thank you! Yes I was considering putting it in the `BrowserMainLoop::CreateStartupTasks()`, but wasn't sure it was a good idea (e.g. if it would block other critical startup tasks). Moved to `BrowserMainLoop::PreCreateThreads()` as you suggested.
base::TimeDelta timeout = base::Minutes(10);Nidhi JajuSo does this mean:
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
- it's possible that the token might be lost if the upload takes a long time?
(I'm not very familiar with file uploads so I might be saying something strange)
Takashi Toyoshima
- there's still a possibility of memory bloat when processing a large number of requests in a short period of time?
Memory usage here should scale with the number of file uploads that are occurring, so doing more uploads will take more memory, but that has always been the case (there will be a number of other objects that scale with the number of uploads, and this isn't meaningfully adding to that bloat).
- it's possible that the token might be lost if the upload takes a long time?
If you request an upload, but then don't start actually performing that upload for 10 minutes, then it's technically possible for the permission to expire. However, I did an audit of all of the relevant codepaths and did not find any places where we'll significantly delay starting the upload. Even if the network is slow, it doesn't matter if the upload takes more than 10 minutes, the time that matters is how long after token creation you start the upload.
Nidhi JajuIs it possible to unregister explicitly?
Maybe called from the destructor, callback, or something like that of NavigationURLLoader and SimpleURLLoader?
Kenichi IshibashiThis turned out to be non-trivial so I've uploaded a separate CL(https://crrev.com/c/7895739) for you to take a look at, please let me know what you think. If we think it's worth it, I'll rebase the rest of the CLs on top.
Takashi ToyoshimaI want to hear toyoshim@'s opinion, but the separate CL looks a reasonable approach to me. If we go that direction, please add a TODO for the follow-up.
Yep. It looks reasonable to me too.
Done
validation_success =Nidhi JajuI think what you want here is
```
!base::FeatureList::IsEnabled(...) && (
process_id.is_browser() ||
(!process_id.is_browser() && cpsp->CanReadFile())
)
```
As we always want the token in the new mode?
Takashi ToyoshimaThis would break renderer-initiated uploads when the feature is enabled. Note that I've added the token logic for renderer-initiated uploads in the next CL in the chain. For these cases, they will not have an `upload_token` and must continue to be validated via `CanReadFile()` when no token is present.
Ah, I see. So, we would want a TODO.
Also, it might be better to have clearly separated logics for each mode even if it is redundant. It helps to avoid mistakes.
I meant something like this;
```
if (!base::FeatureList::IsEnabled(...)) {
...
} else ]
// TODO...
...
}
```
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 2 comments![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
2 comments
if (child_id.is_null()) {Can you have an explicit TODO with a link to the follow-up CL?
DCHECK_EQ(paths.size(), tokens.size());Prefer EXPECT_EQ over DCHECK_EQ in tests
Related details
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Nidhi JajuThanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Thank you for the detailed context! I'm working on a doc that I'll share with you early next week.
Related details
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Nidhi Jaju (Gerrit)
Nidhi Jaju added 2 comments![]( "Open in Gerrit")
Can you have an explicit TODO with a link to the follow-up CL?
Done
Prefer EXPECT_EQ over DCHECK_EQ in tests
Done
Related details
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Hi Glenn, could you take a look at the web_share_target/ files? Thank you!
Related details
- Adam Rice
- Alex Moshchuk
- Glenn Hartmann
- Hiroshige Hayashizaki
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Glenn Hartmann (Gerrit)
Glenn Hartmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Glenn Hartmann
| Code-Review | +1 |
1 comment
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Kenichi Ishibashi
- Nidhi Jaju
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
3 comments
//services/network lgtm.
struct FileUploadInfo {nit: Please add struct comment for FileUploadInfo.
if (!(elements_[i] == other.elements_[i])) {nit: Simply `elements_[i] != other.elements_[i]`?
Related details
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Nidhi Jaju
Nidhi Jaju (Gerrit)
Nidhi Jaju added 2 comments![]( "Open in Gerrit")
nit: Please add struct comment for FileUploadInfo.
Done
nit: Simply `elements_[i] != other.elements_[i]`?
Done
Related details
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
Nidhi JajuThanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Nidhi JajuThank you for the detailed context! I'm working on a doc that I'll share with you early next week.
Here's the design doc: https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?usp=sharing&resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ
Thanks for writing the doc!
creis@, lbrady@, and I discussed it in our CSA sync today, and we have a high-level concern that this approach might be overly complicated. I've asked some clarifying questions on the doc, but the high-level points are:
- It's not clear why the solution focuses on both browser- and renderer-initiated file upload paths, since the bug seems to be purely about the browser-initiated cases, and we already have CPSP::CanReadFile validation for the renderer-initiated cases. What benefits do we get out of the additional validation for renderer-initiated file upload cases (which add a lot of complexity)? Note that a compromised network service already effectively has access to all files requested by any renderer (since those file upload requests go through the NS), and using tokens doesn't change that as the NS process can choose whatever token+path it wants to pass to the browser process.
- Even for the browser-initiated case, it's not clear what additional security value we're getting from tracking files with UnguessableTokens instead of a simple set of FilePaths. Could this be solved simply by tracking FilePaths for browser-initiated file uploads in CPSP, to complement the existing per-renderer FilePaths in SecurityState? Or maybe option 1 becomes simpler to implement without all the renderer-side changes, and then we don't need to modify CPSP at all, and just pass a PendingRemote for opening a file from browser-initiated upload entrypoints (which might also simplify reasoning about lifetimes in that case).
Related details
- Adam Rice
- Hiroshige Hayashizaki
- Nidhi Jaju
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
Nidhi JajuThanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Nidhi JajuThank you for the detailed context! I'm working on a doc that I'll share with you early next week.
Alex MoshchukHere's the design doc: https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?usp=sharing&resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ
Thanks for writing the doc!
creis@, lbrady@, and I discussed it in our CSA sync today, and we have a high-level concern that this approach might be overly complicated. I've asked some clarifying questions on the doc, but the high-level points are:
- It's not clear why the solution focuses on both browser- and renderer-initiated file upload paths, since the bug seems to be purely about the browser-initiated cases, and we already have CPSP::CanReadFile validation for the renderer-initiated cases. What benefits do we get out of the additional validation for renderer-initiated file upload cases (which add a lot of complexity)? Note that a compromised network service already effectively has access to all files requested by any renderer (since those file upload requests go through the NS), and using tokens doesn't change that as the NS process can choose whatever token+path it wants to pass to the browser process.
- Even for the browser-initiated case, it's not clear what additional security value we're getting from tracking files with UnguessableTokens instead of a simple set of FilePaths. Could this be solved simply by tracking FilePaths for browser-initiated file uploads in CPSP, to complement the existing per-renderer FilePaths in SecurityState? Or maybe option 1 becomes simpler to implement without all the renderer-side changes, and then we don't need to modify CPSP at all, and just pass a PendingRemote for opening a file from browser-initiated upload entrypoints (which might also simplify reasoning about lifetimes in that case).
Thank you for the discussion. Both proposals sound much simpler than current approach.
Nidhi,
What do you think? I'm fine with both of Alex's proposed approaches,
Nidhi Jaju (Gerrit)
Nidhi Jaju added 1 comment![]( "Open in Gerrit")
Nidhi JajuThanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Nidhi JajuThank you for the detailed context! I'm working on a doc that I'll share with you early next week.
Alex MoshchukHere's the design doc: https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?usp=sharing&resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ
Takashi ToyoshimaThanks for writing the doc!
creis@, lbrady@, and I discussed it in our CSA sync today, and we have a high-level concern that this approach might be overly complicated. I've asked some clarifying questions on the doc, but the high-level points are:
- It's not clear why the solution focuses on both browser- and renderer-initiated file upload paths, since the bug seems to be purely about the browser-initiated cases, and we already have CPSP::CanReadFile validation for the renderer-initiated cases. What benefits do we get out of the additional validation for renderer-initiated file upload cases (which add a lot of complexity)? Note that a compromised network service already effectively has access to all files requested by any renderer (since those file upload requests go through the NS), and using tokens doesn't change that as the NS process can choose whatever token+path it wants to pass to the browser process.
- Even for the browser-initiated case, it's not clear what additional security value we're getting from tracking files with UnguessableTokens instead of a simple set of FilePaths. Could this be solved simply by tracking FilePaths for browser-initiated file uploads in CPSP, to complement the existing per-renderer FilePaths in SecurityState? Or maybe option 1 becomes simpler to implement without all the renderer-side changes, and then we don't need to modify CPSP at all, and just pass a PendingRemote for opening a file from browser-initiated upload entrypoints (which might also simplify reasoning about lifetimes in that case).
Thank you for the discussion. Both proposals sound much simpler than current approach.
Nidhi,
What do you think? I'm fine with both of Alex's proposed approaches,
Thanks for reviewing the document and discussing it further! I've responded to the comments on the document with additional details, so will keep discussion there, but if we're okay with the less strict enforcement, then I'm happy to avoid some of the complexity wherever possible by using an alternate approach.
Related details
- Adam Rice
- Alex Moshchuk
- Hiroshige Hayashizaki
- Takashi Toyoshima
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
Nidhi JajuThanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Nidhi JajuThank you for the detailed context! I'm working on a doc that I'll share with you early next week.
Alex MoshchukHere's the design doc: https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?usp=sharing&resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ
Takashi ToyoshimaThanks for writing the doc!
creis@, lbrady@, and I discussed it in our CSA sync today, and we have a high-level concern that this approach might be overly complicated. I've asked some clarifying questions on the doc, but the high-level points are:
- It's not clear why the solution focuses on both browser- and renderer-initiated file upload paths, since the bug seems to be purely about the browser-initiated cases, and we already have CPSP::CanReadFile validation for the renderer-initiated cases. What benefits do we get out of the additional validation for renderer-initiated file upload cases (which add a lot of complexity)? Note that a compromised network service already effectively has access to all files requested by any renderer (since those file upload requests go through the NS), and using tokens doesn't change that as the NS process can choose whatever token+path it wants to pass to the browser process.
- Even for the browser-initiated case, it's not clear what additional security value we're getting from tracking files with UnguessableTokens instead of a simple set of FilePaths. Could this be solved simply by tracking FilePaths for browser-initiated file uploads in CPSP, to complement the existing per-renderer FilePaths in SecurityState? Or maybe option 1 becomes simpler to implement without all the renderer-side changes, and then we don't need to modify CPSP at all, and just pass a PendingRemote for opening a file from browser-initiated upload entrypoints (which might also simplify reasoning about lifetimes in that case).
Nidhi JajuThank you for the discussion. Both proposals sound much simpler than current approach.
Nidhi,
What do you think? I'm fine with both of Alex's proposed approaches,
Thanks for reviewing the document and discussing it further! I've responded to the comments on the document with additional details, so will keep discussion there, but if we're okay with the less strict enforcement, then I'm happy to avoid some of the complexity wherever possible by using an alternate approach.
Thanks, responded on the doc. I do still think that it's worth simplifying this to one of those two options ((1) passing something like a `mojo::PendingRemote<FileOpener>` to the network service for files to be uploaded, which can be used to open a particular file, or (2) tracking FilePaths for browser-initiated uploads in CPSP).
I'm realizing based on comments on the doc that (1) would kind of align with what we already do for downloading files, and it also avoids storing a second copy of all the FilePaths for form submissions (per [this discussion](https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ&disco=AAAB9DYsNe0)). But if (2) is simpler to do, it would work just as well.
Related details
- Adam Rice
- Hiroshige Hayashizaki
- Nidhi Jaju
- Takashi Toyoshima
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Nidhi JajuThanks for including me in the reviewers list (and thanks to toyoshim@ for mentioning it offline)! creis@, lbrady@ and I discussed this CL today - overall we feel this problem is definitely important to fix, and on a high-level the approach of validating file paths with tokens to protect against a compromised NS process should work. However, this is a very large CL with many moving parts (with more left to future CLs), and we feel it's worth having a more formal design doc for this where we could read about and discuss some of the aspects of this solution. For example, how the handling for renderer-initiated vs browser-initiated file uploads differs, why we need the timeouts in the browser-initiated case, how session restore gets handled, where the tokens should be generated (outside vs inside CPSP), how multiple network requests for the same file would get handled, etc. Would it be possible to write such a doc?
Also, it's worth mentioning that CSA has plans for a project to introduce a stronger type to represent files actually chosen by the user (UserChosenFilePath); the browser process and network service could then limit file uploads to instances of that type instead of any constructed FilePath, making it impossible for compromised renderers to invent a new path to upload without going through the validation that UserChosenFilePath would require. It's worth thinking about how that would align with this CL; it seems like the validation tokens proposed here would be complementary and could actually be stored within a UserChosenFilePath. More information here: https://docs.google.com/document/d/11n60HcDDLhgWOxicR0OTk0CXy0L2TgUYX7eRJdcp5G0/edit?tab=t.0#bookmark=id.gj7o4ss8b2l4.
Nidhi JajuThank you for the detailed context! I'm working on a doc that I'll share with you early next week.
Alex MoshchukHere's the design doc: https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?usp=sharing&resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ
Takashi ToyoshimaThanks for writing the doc!
creis@, lbrady@, and I discussed it in our CSA sync today, and we have a high-level concern that this approach might be overly complicated. I've asked some clarifying questions on the doc, but the high-level points are:
- It's not clear why the solution focuses on both browser- and renderer-initiated file upload paths, since the bug seems to be purely about the browser-initiated cases, and we already have CPSP::CanReadFile validation for the renderer-initiated cases. What benefits do we get out of the additional validation for renderer-initiated file upload cases (which add a lot of complexity)? Note that a compromised network service already effectively has access to all files requested by any renderer (since those file upload requests go through the NS), and using tokens doesn't change that as the NS process can choose whatever token+path it wants to pass to the browser process.
- Even for the browser-initiated case, it's not clear what additional security value we're getting from tracking files with UnguessableTokens instead of a simple set of FilePaths. Could this be solved simply by tracking FilePaths for browser-initiated file uploads in CPSP, to complement the existing per-renderer FilePaths in SecurityState? Or maybe option 1 becomes simpler to implement without all the renderer-side changes, and then we don't need to modify CPSP at all, and just pass a PendingRemote for opening a file from browser-initiated upload entrypoints (which might also simplify reasoning about lifetimes in that case).
Nidhi JajuThank you for the discussion. Both proposals sound much simpler than current approach.
Nidhi,
What do you think? I'm fine with both of Alex's proposed approaches,
Alex MoshchukThanks for reviewing the document and discussing it further! I've responded to the comments on the document with additional details, so will keep discussion there, but if we're okay with the less strict enforcement, then I'm happy to avoid some of the complexity wherever possible by using an alternate approach.
Thanks, responded on the doc. I do still think that it's worth simplifying this to one of those two options ((1) passing something like a `mojo::PendingRemote<FileOpener>` to the network service for files to be uploaded, which can be used to open a particular file, or (2) tracking FilePaths for browser-initiated uploads in CPSP).
I'm realizing based on comments on the doc that (1) would kind of align with what we already do for downloading files, and it also avoids storing a second copy of all the FilePaths for form submissions (per [this discussion](https://docs.google.com/document/d/1PnU2PGfuXcYEV8lC1ElCw_tiRPu2yl4zDDNukBlV_Bo/edit?resourcekey=0-tVjMQ_o3i0A60j8D8NVlrQ&disco=AAAB9DYsNe0)). But if (2) is simpler to do, it would work just as well.
I have a preference for UnguessableTokens over PendingRemotes because UnguessableTokens are copyable. Unfortunately we need to copy network::ResourceRequests in a few places, and any time we copy a PendingRemote we have to send a Clone() IPC which adds complexity and overhead.
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +0 |
Adam, if you want to avoid the PendingRemote of 1, how about the FilePath with CPSP of 2? We share the same network service process for all profiles including system context. So, UnguessableToken does not make it more secure as it can access it after reading anyway.
Related details
- Adam Rice
- Hiroshige Hayashizaki
- Nidhi Jaju