diff --git a/chrome/browser/net/network_context_configuration_browsertest.cc b/chrome/browser/net/network_context_configuration_browsertest.cc index 4049636013e08..62e6b41650d82 100644 --- a/chrome/browser/net/network_context_configuration_browsertest.cc +++ b/chrome/browser/net/network_context_configuration_browsertest.cc @@ -53,6 +53,7 @@ #include "components/proxy_config/proxy_config_pref_names.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/browser_thread.h" +#include "content/public/browser/child_process_security_policy.h" #include "content/public/browser/network_service_instance.h" #include "content/public/browser/network_service_util.h" #include "content/public/browser/storage_partition.h" @@ -1659,6 +1660,8 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, UploadFile) { base::PathService::Get(chrome::DIR_TEST_DATA, &dir_test_data); base::FilePath path = dir_test_data.Append(base::FilePath(FILE_PATH_LITERAL("simple.html"))); + content::ChildProcessSecurityPolicy::GetInstance() + ->GrantReadFileForBrowserUpload(path); simple_loader->AttachFileForUpload(path, "text/html"); simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie( diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc index be5fcf5f725ee..6eb9695ab531d 100644 --- a/content/browser/child_process_security_policy_impl.cc +++ b/content/browser/child_process_security_policy_impl.cc @@ -1409,6 +1409,12 @@ void ChildProcessSecurityPolicyImpl::GrantReadFile(ChildProcessId child_id, GrantPermissionsForFile(child_id, file, READ_FILE_GRANT); } +void ChildProcessSecurityPolicyImpl::GrantReadFileForBrowserUpload( + const base::FilePath& file) { + base::AutoLock lock(lock_); + browser_upload_files_.insert(file.StripTrailingSeparators()); +} + void ChildProcessSecurityPolicyImpl::GrantCreateReadWriteFile( int child_id, const base::FilePath& file) { @@ -1840,6 +1846,25 @@ bool ChildProcessSecurityPolicyImpl::CanReadAllFiles( }); } +bool ChildProcessSecurityPolicyImpl::CanReadFileForBrowserUpload( + const base::FilePath& file) { + base::AutoLock lock(lock_); + return browser_upload_files_.count(file.StripTrailingSeparators()) > 0; +} + +void ChildProcessSecurityPolicyImpl::GrantRequestBodyForBrowserUpload( + const scoped_refptr& body) { + if (!body) { + return; + } + for (const network::DataElement& element : *body->elements()) { + if (element.type() == network::DataElement::Tag::kFile) { + GrantReadFileForBrowserUpload( + element.As().path()); + } + } +} + bool ChildProcessSecurityPolicyImpl::CanReadRequestBody( ChildProcessId child_id, const storage::FileSystemContext* file_system_context, diff --git a/content/browser/child_process_security_policy_impl.h b/content/browser/child_process_security_policy_impl.h index 578457088b0cf..67841243a5627 100644 --- a/content/browser/child_process_security_policy_impl.h +++ b/content/browser/child_process_security_policy_impl.h @@ -179,6 +179,7 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl bool IsWebSafeScheme_Cpp(const std::string& scheme); void GrantReadFile(ChildProcessId child_id, const base::FilePath& file) override; + void GrantReadFileForBrowserUpload(const base::FilePath& file) override; void GrantCreateReadWriteFile(int child_id, const base::FilePath& file) override; void GrantCopyInto(int child_id, const base::FilePath& dir) override; @@ -425,6 +426,14 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl bool CanReadAllFiles(ChildProcessId child_id, const std::vector& files); + // Returns true if `file` was previously passed to + // GrantReadFileForBrowserUpload(). Can be called on any thread. + bool CanReadFileForBrowserUpload(const base::FilePath& file); + + // Calls GrantReadFileForBrowserUpload() for each file element in `body`. + void GrantRequestBodyForBrowserUpload( + const scoped_refptr& body); + // Validate that |child_id| in |file_system_context| is allowed to access // data in the POST body specified by |body|. Can be called on any thread. bool CanReadRequestBody( @@ -1167,6 +1176,11 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl FileSystemPermissionPolicyMap file_system_policy_map_ GUARDED_BY(lock_); + // Files that the browser process has attached to request bodies sent through + // URLLoaderFactories created for the browser process, and that the network + // process may therefore ask the browser to open for upload. + std::set browser_upload_files_ GUARDED_BY(lock_); + // You must acquire this lock before reading or writing isolated_origins_. // You must not block while holding this lock. // diff --git a/content/browser/loader/navigation_url_loader_impl.cc b/content/browser/loader/navigation_url_loader_impl.cc index 84330ab13462e..fb888d4eb675c 100644 --- a/content/browser/loader/navigation_url_loader_impl.cc +++ b/content/browser/loader/navigation_url_loader_impl.cc @@ -30,6 +30,7 @@ #include "content/browser/about_url_loader_factory.h" #include "content/browser/attribution_reporting/attribution_manager.h" #include "content/browser/blob_storage/chrome_blob_storage_context.h" +#include "content/browser/child_process_security_policy_impl.h" #include "content/browser/client_hints/client_hints.h" #include "content/browser/data_url_loader_factory.h" #include "content/browser/devtools/devtools_instrumentation.h" @@ -307,6 +308,14 @@ std::unique_ptr CreateResourceRequest( new_request->load_flags = load_flags; new_request->request_body = request_info.common_params->post_data.get(); + // Navigation requests are sent through a URLLoaderFactory created on behalf + // of the browser process, so the network process will use the browser + // process id when asking the browser to open any files referenced by + // `request_body`. Files in the body have already been validated against the + // initiating process where applicable, so allow the browser to open them on + // behalf of the network process for this navigation. + ChildProcessSecurityPolicyImpl::GetInstance() + ->GrantRequestBodyForBrowserUpload(new_request->request_body); new_request->has_user_gesture = request_info.common_params->has_possibly_filtered_user_gesture; diff --git a/content/browser/network_context_client_base_impl.cc b/content/browser/network_context_client_base_impl.cc index 14aa0658b40e2..36906e7b66814 100644 --- a/content/browser/network_context_client_base_impl.cc +++ b/content/browser/network_context_client_base_impl.cc @@ -36,11 +36,15 @@ void HandleFileUploadRequest( std::vector files; uint32_t file_flags = base::File::FLAG_OPEN | base::File::FLAG_READ | (async ? base::File::FLAG_ASYNC : 0); - ChildProcessSecurityPolicy* cpsp = ChildProcessSecurityPolicy::GetInstance(); + auto* cpsp = ChildProcessSecurityPolicyImpl::GetInstance(); for (const auto& file_path : file_paths) { - if (!process_id.is_browser() && - !cpsp->CanReadFile(ToChildProcessId(process_id.renderer_process_id()), - file_path)) { + bool allowed = + process_id.is_browser() + ? cpsp->CanReadFileForBrowserUpload(file_path) + : cpsp->CanReadFile( + ToChildProcessId(process_id.renderer_process_id()), + file_path); + if (!allowed) { task_runner->PostTask( FROM_HERE, base::BindOnce(std::move(callback), net::ERR_ACCESS_DENIED, std::vector())); diff --git a/content/browser/network_context_client_base_impl_unittest.cc b/content/browser/network_context_client_base_impl_unittest.cc index 194b0920fb5c5..04cf281146bb2 100644 --- a/content/browser/network_context_client_base_impl_unittest.cc +++ b/content/browser/network_context_client_base_impl_unittest.cc @@ -226,7 +226,8 @@ TEST_F(NetworkContextClientBaseTest, UploadOneValidFileAndOneNotFound) { TEST_F(NetworkContextClientBaseTest, UploadFromBrowserProcess) { base::FilePath path = temp_dir_.GetPath().AppendASCII("filename"); CreateFile(path, kFileContent1); - // No grant necessary for browser process. + ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFileForBrowserUpload( + path); UploadResponse response; client_.OnFileUploadRequested( @@ -238,4 +239,35 @@ TEST_F(NetworkContextClientBaseTest, UploadFromBrowserProcess) { ValidateFileContents(response.opened_files[0], kFileContent1); } +TEST_F(NetworkContextClientBaseTest, UploadFromBrowserProcessUnauthorized) { + base::FilePath path = temp_dir_.GetPath().AppendASCII("filename"); + CreateFile(path, kFileContent1); + + UploadResponse response; + client_.OnFileUploadRequested( + network::OriginatingProcessId::browser(), false, {path}, + /*destination_url=*/GURL(), std::move(response.callback)); + task_environment_.RunUntilIdle(); + EXPECT_EQ(net::ERR_ACCESS_DENIED, response.error_code); + EXPECT_EQ(0U, response.opened_files.size()); +} + +TEST_F(NetworkContextClientBaseTest, + UploadFromBrowserProcessOneValidFileAndOneUnauthorized) { + base::FilePath path1 = temp_dir_.GetPath().AppendASCII("filename1"); + base::FilePath path2 = temp_dir_.GetPath().AppendASCII("filename2"); + CreateFile(path1, kFileContent1); + CreateFile(path2, kFileContent2); + ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFileForBrowserUpload( + path1); + + UploadResponse response; + client_.OnFileUploadRequested( + network::OriginatingProcessId::browser(), false, {path1, path2}, + /*destination_url=*/GURL(), std::move(response.callback)); + task_environment_.RunUntilIdle(); + EXPECT_EQ(net::ERR_ACCESS_DENIED, response.error_code); + EXPECT_EQ(0U, response.opened_files.size()); +} + } // namespace content diff --git a/content/public/browser/child_process_security_policy.h b/content/public/browser/child_process_security_policy.h index 23af7d9cd2706..b706e574fe52b 100644 --- a/content/public/browser/child_process_security_policy.h +++ b/content/public/browser/child_process_security_policy.h @@ -70,6 +70,13 @@ class ChildProcessSecurityPolicy { virtual void GrantReadFile(ChildProcessId child_id, const base::FilePath& file) = 0; + // Allows the network process to ask the browser to open `file` for upload on + // behalf of a request that originated in the browser process. Browser code + // that attaches local file paths to a request body and sends the request + // through a URLLoaderFactory created for the browser process must call this + // for each such path before starting the load. + virtual void GrantReadFileForBrowserUpload(const base::FilePath& file) = 0; + // This permission grants creation, read, and full write access to a file, // including attributes. virtual void GrantCreateReadWriteFile(int child_id,