From 4e37d8b3fb5edac4e61bc568a26212b299fe2676 Mon Sep 17 00:00:00 2001 From: Will Harris Date: Tue, 4 Apr 2023 04:17:52 +0000 Subject: [PATCH] Add ability to open cookie database file with exclusive flag. This CL adds the ability to specify that the sqlite file containing cookies be opened with the `exclusive` uri parameter. On Windows, this instructs sqlite to open it without the FILE_SHARE_READ and FILE_SHARE_WRITE sharing dispositions, meaning that handles can no longer be obtained to the file while the database is open. The flag is wired up through the net/extras/sqlite layer and into the network service, where it is exposed as an optional parameter on network_context. In //chrome, the flag is set on Windows for profile based cookie stores, based on the feature LockProfileCookieDatabase, which is disabled by default. Tests are added at each layer to verify the behavior: - sql_unittests -> that the new parameter enable_exclusive_database_file_lock is respected. - net_unittests -> that the new parameter to SQLitePersistentCookieStore is respected. - browser_tests -> that the feature LockProfileCookieDatabase successfully toggles the feature for the profile cookie store. BUG=1429117 Change-Id: I2beaa0eab330535eecf733ecc8fd6a93f748227a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4378424 Reviewed-by: Reilly Grant Reviewed-by: Scott Violet Reviewed-by: Austin Sullivan Reviewed-by: Rohit Rao Commit-Queue: Will Harris Cr-Commit-Position: refs/heads/main@{#1125803} --- chrome/browser/browser_features.cc | 6 ++ chrome/browser/browser_features.h | 1 + .../net/chrome_network_service_browsertest.cc | 56 +++++++++++++ .../net/profile_network_context_service.cc | 8 ++ .../browser/network/cookie_store_factory.cc | 3 +- ios/components/cookie_util/cookie_util.mm | 3 +- .../sqlite/sqlite_persistent_cookie_store.cc | 12 ++- .../sqlite/sqlite_persistent_cookie_store.h | 7 +- ...sqlite_persistent_cookie_store_perftest.cc | 6 +- ...sqlite_persistent_cookie_store_unittest.cc | 81 ++++++++++++++++--- ...lite_persistent_reporting_and_nel_store.cc | 3 +- .../sqlite_persistent_store_backend_base.cc | 20 ++++- .../sqlite_persistent_store_backend_base.h | 10 ++- services/network/cookie_manager_unittest.cc | 3 +- services/network/network_context.cc | 8 +- .../public/mojom/network_context.mojom | 7 ++ .../session_cleanup_cookie_store_unittest.cc | 3 +- sql/database.cc | 22 ++++- sql/database.h | 25 +++++- sql/database_unittest.cc | 73 +++++++++++++++++ 20 files changed, 323 insertions(+), 34 deletions(-) diff --git a/chrome/browser/browser_features.cc b/chrome/browser/browser_features.cc index ab39c257bacada..5fe540c2d081e3 100644 --- a/chrome/browser/browser_features.cc +++ b/chrome/browser/browser_features.cc @@ -209,6 +209,12 @@ BASE_FEATURE(kRestartNetworkServiceUnsandboxedForFailedLaunch, BASE_FEATURE(kAppBoundEncryptionMetrics, "AppBoundEncryptionMetrics", base::FEATURE_ENABLED_BY_DEFAULT); + +// Enables locking the cookie database for profiles. +// TODO(crbug.com/1430226): Remove after fully launched. +BASE_FEATURE(kLockProfileCookieDatabase, + "LockProfileCookieDatabase", + base::FEATURE_DISABLED_BY_DEFAULT); #endif // Enables showing the email of the flex org admin that setup CBCM in the diff --git a/chrome/browser/browser_features.h b/chrome/browser/browser_features.h index d43ecb7138202f..45afe53ce925a0 100644 --- a/chrome/browser/browser_features.h +++ b/chrome/browser/browser_features.h @@ -74,6 +74,7 @@ BASE_DECLARE_FEATURE(kKeyPinningComponentUpdater); #if BUILDFLAG(IS_WIN) BASE_DECLARE_FEATURE(kAppBoundEncryptionMetrics); +BASE_DECLARE_FEATURE(kLockProfileCookieDatabase); #endif BASE_DECLARE_FEATURE(kFlexOrgManagementDisclosure); diff --git a/chrome/browser/net/chrome_network_service_browsertest.cc b/chrome/browser/net/chrome_network_service_browsertest.cc index a03cd775c702f4..724bdde76c494d 100644 --- a/chrome/browser/net/chrome_network_service_browsertest.cc +++ b/chrome/browser/net/chrome_network_service_browsertest.cc @@ -173,6 +173,62 @@ INSTANTIATE_TEST_SUITE_P(OutOfProcess, ChromeNetworkServiceBrowserTest, ::testing::Values(false)); +#if BUILDFLAG(IS_WIN) +class ChromeNetworkServiceBrowserCookieLockTest + : public InProcessBrowserTest, + public ::testing::WithParamInterface { + public: + void SetUp() override { + scoped_feature_list_.InitWithFeatureState( + features::kLockProfileCookieDatabase, ShouldBeLocked()); + + InProcessBrowserTest::SetUp(); + } + + void SetUpOnMainThread() override { + host_resolver()->AddRule("*", "127.0.0.1"); + ASSERT_TRUE(embedded_test_server()->Start()); + } + + protected: + const bool& ShouldBeLocked() { return GetParam(); } + + private: + base::test::ScopedFeatureList scoped_feature_list_; +}; + +// This test verifies that if the kLockProfileCookieDatabase feature is enabled, +// then the cookie store cannot be opened once sqlite has an exclusive lock on +// the file. +IN_PROC_BROWSER_TEST_P(ChromeNetworkServiceBrowserCookieLockTest, + CookiesAreLocked) { + ASSERT_TRUE(ui_test_utils::NavigateToURL( + browser(), embedded_test_server()->GetURL("/title1.html"))); + base::FilePath cookie_filename = browser() + ->profile() + ->GetPath() + .Append(chrome::kNetworkDataDirname) + .Append(chrome::kCookieFilename); + { + base::ScopedAllowBlockingForTesting allow_blocking; + + ASSERT_TRUE(base::PathExists(cookie_filename)); + base::File cookie_file( + cookie_filename, + base::File::Flags::FLAG_OPEN_ALWAYS | base::File::Flags::FLAG_READ); + EXPECT_EQ(ShouldBeLocked(), !cookie_file.IsValid()); + } +} + +INSTANTIATE_TEST_SUITE_P(All, + ChromeNetworkServiceBrowserCookieLockTest, + ::testing::Bool(), + [](const auto& info) { + return info.param ? "Locked" : "NotLocked"; + }); + +#endif // BUILDFLAG(IS_WIN) + // See `NetworkServiceBrowserTest` for content's version of tests. This test // merely tests that chrome's feature is wired up correctly to the migration // code that exists in content. diff --git a/chrome/browser/net/profile_network_context_service.cc b/chrome/browser/net/profile_network_context_service.cc index db593e5f07ea8b..bf8ff00e18cf29 100644 --- a/chrome/browser/net/profile_network_context_service.cc +++ b/chrome/browser/net/profile_network_context_service.cc @@ -849,6 +849,14 @@ void ProfileNetworkContextService::ConfigureNetworkContextParamsInternal( base::FilePath(chrome::kNetworkPersistentStateFilename); network_context_params->file_paths->cookie_database_name = base::FilePath(chrome::kCookieFilename); + +#if BUILDFLAG(IS_WIN) + // If this feature is enabled, then the cookie database used by this profile + // will be locked for exclusive access by sqlite3 implementation in the + // network service. + network_context_params->enable_locking_cookie_database = + base::FeatureList::IsEnabled(features::kLockProfileCookieDatabase); +#endif // BUILDFLAG(IS_WIN) network_context_params->file_paths->trust_token_database_name = base::FilePath(chrome::kTrustTokenFilename); diff --git a/content/browser/network/cookie_store_factory.cc b/content/browser/network/cookie_store_factory.cc index dcce4ec23a30d8..a57c74f04752ba 100644 --- a/content/browser/network/cookie_store_factory.cc +++ b/content/browser/network/cookie_store_factory.cc @@ -68,7 +68,8 @@ std::unique_ptr CreateCookieStore( scoped_refptr sqlite_store( new net::SQLitePersistentCookieStore( config.path, client_task_runner, background_task_runner, - config.restore_old_session_cookies, config.crypto_delegate)); + config.restore_old_session_cookies, config.crypto_delegate, + /*enable_exclusive_access=*/false)); cookie_monster = std::make_unique(std::move(sqlite_store), net_log); diff --git a/ios/components/cookie_util/cookie_util.mm b/ios/components/cookie_util/cookie_util.mm index 16a75f9205daf4..51dcefccbc8f98 100644 --- a/ios/components/cookie_util/cookie_util.mm +++ b/ios/components/cookie_util/cookie_util.mm @@ -49,7 +49,8 @@ base::ThreadPool::CreateSequencedTaskRunner( {base::MayBlock(), base::TaskPriority::BEST_EFFORT, base::TaskShutdownBehavior::BLOCK_SHUTDOWN}), - restore_old_session_cookies, crypto_delegate)); + restore_old_session_cookies, crypto_delegate, + /*enable_exclusive_access=*/false)); } // Creates a CookieMonster configured by `config`. diff --git a/net/extras/sqlite/sqlite_persistent_cookie_store.cc b/net/extras/sqlite/sqlite_persistent_cookie_store.cc index 2ac7a2f4df4e63..f35a95c7dd7361 100644 --- a/net/extras/sqlite/sqlite_persistent_cookie_store.cc +++ b/net/extras/sqlite/sqlite_persistent_cookie_store.cc @@ -257,13 +257,15 @@ class SQLitePersistentCookieStore::Backend scoped_refptr client_task_runner, scoped_refptr background_task_runner, bool restore_old_session_cookies, - CookieCryptoDelegate* crypto_delegate) + CookieCryptoDelegate* crypto_delegate, + bool enable_exclusive_access) : SQLitePersistentStoreBackendBase(path, /* histogram_tag = */ "Cookie", kCurrentVersionNumber, kCompatibleVersionNumber, std::move(background_task_runner), - std::move(client_task_runner)), + std::move(client_task_runner), + enable_exclusive_access), restore_old_session_cookies_(restore_old_session_cookies), crypto_(crypto_delegate) {} @@ -1719,12 +1721,14 @@ SQLitePersistentCookieStore::SQLitePersistentCookieStore( const scoped_refptr& client_task_runner, const scoped_refptr& background_task_runner, bool restore_old_session_cookies, - CookieCryptoDelegate* crypto_delegate) + CookieCryptoDelegate* crypto_delegate, + bool enable_exclusive_access) : backend_(base::MakeRefCounted(path, client_task_runner, background_task_runner, restore_old_session_cookies, - crypto_delegate)) {} + crypto_delegate, + enable_exclusive_access)) {} void SQLitePersistentCookieStore::DeleteAllInList( const std::list& cookies) { diff --git a/net/extras/sqlite/sqlite_persistent_cookie_store.h b/net/extras/sqlite/sqlite_persistent_cookie_store.h index bb026cb9f7b128..e68b3a930918b8 100644 --- a/net/extras/sqlite/sqlite_persistent_cookie_store.h +++ b/net/extras/sqlite/sqlite_persistent_cookie_store.h @@ -46,13 +46,16 @@ class COMPONENT_EXPORT(NET_EXTRAS) SQLitePersistentCookieStore // All blocking database accesses will be performed on // |background_task_runner|, while |client_task_runner| is used to invoke - // callbacks. + // callbacks. If |enable_exclusive_access| is set to true then sqlite will + // be asked to open the database with flag `exclusive=1`. In practice, this is + // only respected on Windows. SQLitePersistentCookieStore( const base::FilePath& path, const scoped_refptr& client_task_runner, const scoped_refptr& background_task_runner, bool restore_old_session_cookies, - CookieCryptoDelegate* crypto_delegate); + CookieCryptoDelegate* crypto_delegate, + bool enable_exclusive_access); SQLitePersistentCookieStore(const SQLitePersistentCookieStore&) = delete; SQLitePersistentCookieStore& operator=(const SQLitePersistentCookieStore&) = diff --git a/net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc b/net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc index 5bd15e5ac93b81..2a50c6ba9de633 100644 --- a/net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc +++ b/net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc @@ -96,7 +96,8 @@ class SQLitePersistentCookieStorePerfTest : public testing::Test { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); store_ = base::MakeRefCounted( temp_dir_.GetPath().Append(cookie_filename), client_task_runner_, - background_task_runner_, false, nullptr); + background_task_runner_, /*restore_old_session_cookies=*/true, + /*crypto_delegate=*/nullptr, /*enable_exclusive_access=*/false); std::vector cookies; Load(); ASSERT_EQ(0u, cookies_.size()); @@ -115,7 +116,8 @@ class SQLitePersistentCookieStorePerfTest : public testing::Test { store_ = base::MakeRefCounted( temp_dir_.GetPath().Append(cookie_filename), client_task_runner_, - background_task_runner_, false, nullptr); + background_task_runner_, /*restore_old_session_cookies=*/true, + /*crypto_delegate=*/nullptr, /*enable_exclusive_access=*/false); } // Pick a random cookie out of the 15000 in the store and return it. diff --git a/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc b/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc index 530e953997492c..d084942b22393f 100644 --- a/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc +++ b/net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc @@ -12,6 +12,7 @@ #include #include "base/containers/span.h" +#include "base/files/file.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" #include "base/functional/bind.h" @@ -145,7 +146,8 @@ class SQLitePersistentCookieStoreTest : public TestWithTaskEnvironment { void Create(bool crypt_cookies, bool restore_old_session_cookies, - bool use_current_thread) { + bool use_current_thread, + bool enable_exclusive_access) { if (crypt_cookies) cookie_crypto_delegate_ = std::make_unique(); @@ -154,14 +156,14 @@ class SQLitePersistentCookieStoreTest : public TestWithTaskEnvironment { use_current_thread ? base::SingleThreadTaskRunner::GetCurrentDefault() : client_task_runner_, background_task_runner_, restore_old_session_cookies, - cookie_crypto_delegate_.get()); + cookie_crypto_delegate_.get(), enable_exclusive_access); } void CreateAndLoad(bool crypt_cookies, bool restore_old_session_cookies, CanonicalCookieVector* cookies) { Create(crypt_cookies, restore_old_session_cookies, - false /* use_current_thread */); + false /* use_current_thread */, /*enable_exclusive_access=*/false); Load(cookies); } @@ -323,7 +325,7 @@ TEST_F(SQLitePersistentCookieStoreTest, TestSessionCookiesDeletedOnStartup) { // transient cookie and flush it to disk. store_ = base::MakeRefCounted( temp_dir_.GetPath().Append(kCookieFilename), client_task_runner_, - background_task_runner_, false, nullptr); + background_task_runner_, false, nullptr, false); // Posting a blocking task to db_thread_ makes sure that the DB thread waits // until both Load and Flush have been posted to its task queue. @@ -358,7 +360,7 @@ TEST_F(SQLitePersistentCookieStoreTest, TestSessionCookiesDeletedOnStartup) { // which was added during the second cookie store load. store_ = base::MakeRefCounted( temp_dir_.GetPath().Append(kCookieFilename), client_task_runner_, - background_task_runner_, true, nullptr); + background_task_runner_, true, nullptr, false); store_->Load(base::BindOnce(&SQLitePersistentCookieStoreTest::OnLoaded, base::Unretained(this)), NetLogWithSource()); @@ -389,7 +391,7 @@ TEST_F(SQLitePersistentCookieStoreTest, TestLoadCookiesForKey) { // base::SingleThreadTaskRunner::GetCurrentDefault() instead of // |client_task_runner_| for this test. Create(false /* crypt_cookies */, false /* restore_old_session_cookies */, - true /* use_current_thread */); + true /* use_current_thread */, false /* enable_exclusive_access */); // Posting a blocking task to db_thread_ makes sure that the DB thread waits // until both Load and LoadCookiesForKey have been posted to its task queue. @@ -1385,7 +1387,7 @@ TEST_F(SQLitePersistentCookieStoreTest, KeyInconsistency) { // its PersistentCookieStore on the same thread as its methods are invoked on; // so to avoid needing to post every CookieMonster API call, this uses the // current thread for SQLitePersistentCookieStore's |client_task_runner|. - Create(false, false, true /* use_current_thread */); + Create(false, false, true /* use_current_thread */, false); // Create a cookie on a scheme that doesn't handle cookies by default, // and save it. @@ -1433,7 +1435,8 @@ TEST_F(SQLitePersistentCookieStoreTest, KeyInconsistency) { // destroyed store's ops will happen on same runners as the previous // instances, so they should complete before the new PersistentCookieStore // starts looking at the state on disk. - Create(false, false, true /* want current thread to invoke cookie monster */); + Create(false, false, true /* want current thread to invoke cookie monster */, + false); cookie_monster = std::make_unique(store_.get(), /*net_log=*/nullptr); ResultSavingCookieCallback cookie_scheme_callback2; @@ -1463,7 +1466,8 @@ TEST_F(SQLitePersistentCookieStoreTest, OpsIfInitFailed) { // (e.g. lsan) to actual catch thing. ASSERT_TRUE( base::CreateDirectory(temp_dir_.GetPath().Append(kCookieFilename))); - Create(false, false, true /* want current thread to invoke cookie monster */); + Create(false, false, true /* want current thread to invoke cookie monster */, + false); std::unique_ptr cookie_monster = std::make_unique(store_.get(), /*net_log=*/nullptr); @@ -1509,7 +1513,8 @@ TEST_F(SQLitePersistentCookieStoreTest, Coalescing) { absl::nullopt /* cookie_partition_key */); for (const TestCase& testcase : testcases) { - Create(false, false, true /* want current thread to invoke the store. */); + Create(false, false, true /* want current thread to invoke the store. */, + false); base::RunLoop run_loop; store_->Load(base::BindLambdaForTesting( @@ -1549,7 +1554,8 @@ TEST_F(SQLitePersistentCookieStoreTest, Coalescing) { } TEST_F(SQLitePersistentCookieStoreTest, NoCoalesceUnrelated) { - Create(false, false, true /* want current thread to invoke the store. */); + Create(false, false, true /* want current thread to invoke the store. */, + false); base::RunLoop run_loop; store_->Load(base::BindLambdaForTesting( @@ -1581,6 +1587,59 @@ TEST_F(SQLitePersistentCookieStoreTest, NoCoalesceUnrelated) { db_thread_event_.Signal(); } +// Locking is only supported on Windows. +#if BUILDFLAG(IS_WIN) + +class SQLitePersistentCookieStoreExclusiveAccessTest + : public SQLitePersistentCookieStoreTest, + public ::testing::WithParamInterface { + protected: + const bool& ShouldBeExclusive() { return GetParam(); } +}; + +TEST_P(SQLitePersistentCookieStoreExclusiveAccessTest, LockedStore) { + Create(false, false, true /* want current thread to invoke the store. */, + /* exclusive access */ ShouldBeExclusive()); + + base::RunLoop run_loop; + store_->Load(base::BindLambdaForTesting( + [&](CanonicalCookieVector cookies) { run_loop.Quit(); }), + NetLogWithSource()); + run_loop.Run(); + + std::unique_ptr cookie = CanonicalCookie::Create( + GURL("http://www.example.com/path"), "Tasty=Yes", base::Time::Now(), + absl::nullopt /* server_time */, + absl::nullopt /* cookie_partition_key */); + + // Wedge the background thread to make sure that it doesn't start consuming + // the queue. + background_task_runner_->PostTask( + FROM_HERE, base::BindOnce(&SQLitePersistentCookieStoreTest::WaitOnDBEvent, + base::Unretained(this))); + + store_->AddCookie(*cookie); + + { + base::File file( + temp_dir_.GetPath().Append(kCookieFilename), + base::File::Flags::FLAG_OPEN_ALWAYS | base::File::Flags::FLAG_READ); + // If locked, should not be able to open file even for read. + EXPECT_EQ(ShouldBeExclusive(), !file.IsValid()); + } + + db_thread_event_.Signal(); +} + +INSTANTIATE_TEST_SUITE_P(All, + SQLitePersistentCookieStoreExclusiveAccessTest, + ::testing::Bool(), + [](const auto& info) { + return info.param ? "Exclusive" : "NotExclusive"; + }); + +#endif // BUILDFLAG(IS_WIN) + bool CreateV10Schema(sql::Database* db) { sql::MetaTable meta_table; if (!meta_table.Init(db, /* version = */ 10, diff --git a/net/extras/sqlite/sqlite_persistent_reporting_and_nel_store.cc b/net/extras/sqlite/sqlite_persistent_reporting_and_nel_store.cc index 6d28aa404f971e..05b120b35f563b 100644 --- a/net/extras/sqlite/sqlite_persistent_reporting_and_nel_store.cc +++ b/net/extras/sqlite/sqlite_persistent_reporting_and_nel_store.cc @@ -115,7 +115,8 @@ class SQLitePersistentReportingAndNelStore::Backend kCurrentVersionNumber, kCompatibleVersionNumber, background_task_runner, - client_task_runner) {} + client_task_runner, + /*enable_exclusive_access=*/false) {} Backend(const Backend&) = delete; Backend& operator=(const Backend&) = delete; diff --git a/net/extras/sqlite/sqlite_persistent_store_backend_base.cc b/net/extras/sqlite/sqlite_persistent_store_backend_base.cc index 9bb63c70ae41b9..4fc5314c3b3507 100644 --- a/net/extras/sqlite/sqlite_persistent_store_backend_base.cc +++ b/net/extras/sqlite/sqlite_persistent_store_backend_base.cc @@ -25,13 +25,15 @@ SQLitePersistentStoreBackendBase::SQLitePersistentStoreBackendBase( const int current_version_number, const int compatible_version_number, scoped_refptr background_task_runner, - scoped_refptr client_task_runner) + scoped_refptr client_task_runner, + bool enable_exclusive_access) : path_(path), histogram_tag_(std::move(histogram_tag)), current_version_number_(current_version_number), compatible_version_number_(compatible_version_number), background_task_runner_(std::move(background_task_runner)), - client_task_runner_(std::move(client_task_runner)) {} + client_task_runner_(std::move(client_task_runner)), + enable_exclusive_access_(enable_exclusive_access) {} SQLitePersistentStoreBackendBase::~SQLitePersistentStoreBackendBase() { // If `db_` hasn't been reset by the time this destructor is called, @@ -83,7 +85,12 @@ bool SQLitePersistentStoreBackendBase::InitializeDatabase() { return false; } - db_ = std::make_unique(); + // TODO(crbug.com/1430231): Remove explicit_locking = false. This currently + // needs to be set to false because of several failing MigrationTests. + db_ = std::make_unique(sql::DatabaseOptions{ + .exclusive_locking = false, + .exclusive_database_file_lock = enable_exclusive_access_}); + db_->set_histogram_tag(histogram_tag_); // base::Unretained is safe because |this| owns (and therefore outlives) the @@ -98,7 +105,12 @@ bool SQLitePersistentStoreBackendBase::InitializeDatabase() { Reset(); return false; } - db_->Preload(); + + // It is not possible to preload a database opened with exclusive access, + // because the file cannot be re-opened by the preloader. + if (!enable_exclusive_access_) { + db_->Preload(); + } if (!MigrateDatabaseSchema() || !CreateDatabaseSchema()) { DLOG(ERROR) << "Unable to update or initialize " << histogram_tag_ diff --git a/net/extras/sqlite/sqlite_persistent_store_backend_base.h b/net/extras/sqlite/sqlite_persistent_store_backend_base.h index 9512c9dfda65c2..d8eb5431ea7a28 100644 --- a/net/extras/sqlite/sqlite_persistent_store_backend_base.h +++ b/net/extras/sqlite/sqlite_persistent_store_backend_base.h @@ -70,14 +70,16 @@ class SQLitePersistentStoreBackendBase // |current_version_number| and |compatible_version_number| must be greater // than 0, as per //sql/meta_table.h. |background_task_runner| should be - // non-null. + // non-null. If |enable_exclusive_access| is true then the sqlite3 database + // will be opened with exclusive flag. SQLitePersistentStoreBackendBase( const base::FilePath& path, std::string histogram_tag, const int current_version_number, const int compatible_version_number, scoped_refptr background_task_runner, - scoped_refptr client_task_runner); + scoped_refptr client_task_runner, + bool enable_exclusive_access); virtual ~SQLitePersistentStoreBackendBase(); @@ -186,6 +188,10 @@ class SQLitePersistentStoreBackendBase const scoped_refptr background_task_runner_; const scoped_refptr client_task_runner_; + // If true, then sqlite will be requested to open the file with exclusive + // access. + const bool enable_exclusive_access_; + // Callback to be run before Commit. base::RepeatingClosure before_commit_callback_ GUARDED_BY(before_commit_callback_lock_); diff --git a/services/network/cookie_manager_unittest.cc b/services/network/cookie_manager_unittest.cc index c329d293fd57ac..837dbb12507a05 100644 --- a/services/network/cookie_manager_unittest.cc +++ b/services/network/cookie_manager_unittest.cc @@ -2864,7 +2864,8 @@ class SessionCleanupCookieManagerTest : public CookieManagerTest { auto sqlite_store = base::MakeRefCounted( temp_dir_.GetPath().Append(kTestCookiesFilename), task_environment_.GetMainThreadTaskRunner(), background_task_runner_, - true, nullptr); + /*restore_old_session_cookies=*/true, /*crypto_delegate=*/nullptr, + /*enable_exclusive_access=*/false); return base::MakeRefCounted(sqlite_store.get()); } diff --git a/services/network/network_context.cc b/services/network/network_context.cc index 2800a880bbc12c..8eb64aed9e1538 100644 --- a/services/network/network_context.cc +++ b/services/network/network_context.cc @@ -2659,10 +2659,16 @@ NetworkContext::MakeSessionCleanupCookieStore() const { crypto_delegate = cookie_config::GetCookieCryptoDelegate(); } +#if BUILDFLAG(IS_WIN) + const bool enable_exclusive_access = params_->enable_locking_cookie_database; +#else + const bool enable_exclusive_access = false; +#endif // BUILDFLAG(IS_WIN) scoped_refptr sqlite_store( new net::SQLitePersistentCookieStore( cookie_path, client_task_runner, background_task_runner, - params_->restore_old_session_cookies, crypto_delegate)); + params_->restore_old_session_cookies, crypto_delegate, + enable_exclusive_access)); return base::MakeRefCounted(sqlite_store); } diff --git a/services/network/public/mojom/network_context.mojom b/services/network/public/mojom/network_context.mojom index f32ad0616a72ff..4a8acaee48cd0f 100644 --- a/services/network/public/mojom/network_context.mojom +++ b/services/network/public/mojom/network_context.mojom @@ -338,6 +338,13 @@ struct NetworkContextParams { // If true, cookies will be stored encrypted. bool enable_encrypted_cookies = true; + // On Windows, whether or not the cookie database should be opened with + // the sqlite3 flag `exclusive=true` which asks the sqlite VFS to open them + // without file sharing enabled. This has the consequence that other processes + // should not be able to open the cookie database file. + [EnableIf=is_win] + bool enable_locking_cookie_database = false; + // If the cookie file is given, this controls whether previously written // session cookies are restored. Otherwise it should be false. bool restore_old_session_cookies = false; diff --git a/services/network/session_cleanup_cookie_store_unittest.cc b/services/network/session_cleanup_cookie_store_unittest.cc index 22aa2d3d562469..583564661a5548 100644 --- a/services/network/session_cleanup_cookie_store_unittest.cc +++ b/services/network/session_cleanup_cookie_store_unittest.cc @@ -55,7 +55,8 @@ class SessionCleanupCookieStoreTest : public testing::Test { auto sqlite_store = base::MakeRefCounted( temp_dir_.GetPath().Append(kTestCookiesFilename), base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()}), - background_task_runner_, true, nullptr); + background_task_runner_, /*restore_old_session_cookies=*/true, + /*crypto_delegate=*/nullptr, /*enable_exclusive_access=*/false); store_ = base::MakeRefCounted(sqlite_store.get()); return Load(); diff --git a/sql/database.cc b/sql/database.cc index 44d9a784ef0c0e..1f2f2360c5fd4e 100644 --- a/sql/database.cc +++ b/sql/database.cc @@ -435,6 +435,9 @@ void Database::Preload() { return; } + CHECK(!options_.exclusive_database_file_lock) + << "Cannot preload an exclusively locked database."; + absl::optional scoped_blocking_call; InitScopedBlockingCall(FROM_HERE, &scoped_blocking_call); @@ -1818,8 +1821,25 @@ bool Database::OpenInternal(const std::string& db_file_path, // https://www.sqlite.org/rescode.html for details. int open_flags = SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_EXRESCODE | SQLITE_OPEN_PRIVATECACHE; + std::string uri_file_path = db_file_path; + if (options_.exclusive_database_file_lock) { +#if BUILDFLAG(IS_WIN) + if (mode == OpenMode::kNone || mode == OpenMode::kRetryOnPoision) { + // Do not allow query injection. + if (db_file_path.find('?') != std::string::npos) { + return false; + } + open_flags |= SQLITE_OPEN_URI; + uri_file_path = base::StrCat({"file:", db_file_path, "?exclusive=true"}); + } +#else + NOTREACHED_NORETURN() + << "exclusive_database_file_lock is only supported on Windows."; +#endif // BUILDFLAG(IS_WIN) + } + auto sqlite_result_code = ToSqliteResultCode( - sqlite3_open_v2(db_file_path.c_str(), &db_, open_flags, vfs_name)); + sqlite3_open_v2(uri_file_path.c_str(), &db_, open_flags, vfs_name)); if (sqlite_result_code != SqliteResultCode::kOk) { OnSqliteError(ToSqliteErrorCode(sqlite_result_code), nullptr, "-- sqlite3_open_v2()"); diff --git a/sql/database.h b/sql/database.h index f957787adc5978..e49e3af88120c1 100644 --- a/sql/database.h +++ b/sql/database.h @@ -74,8 +74,10 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions { // on every transaction, and comes with a small performance penalty. // // Setting this to true causes the locking protocol to be used once, when the - // database is opened. No other process will be able to access the database at - // the same time. + // database is opened. No other SQLite process will be able to access the + // database at the same time. Note that this uses OS-level + // advisory/cooperative locking, so this does not protect the database file + // from uncooperative processes. // // More details at https://www.sqlite.org/pragma.html#pragma_locking_mode // @@ -87,6 +89,25 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions { // due to lock contention. bool exclusive_locking = true; + // If true, enables exclusive=true vfs URI parameter on the database file. + // This is only supported on Windows. + // + // If this option is true then the database file cannot be opened by any + // processes on the system until the database has been closed. Note, this is + // not the same as `exclusive_locking` above, which refers to + // advisory/cooperative locks. This option sets file handle sharing attributes + // to prevent the database files from being opened from any process including + // being opened a second time by the hosting process. + // + // A side effect of setting this flag is that the database cannot be + // preloaded. If you would like to set this flag on a preloaded database, + // please reach out to a //sql owner. + // + // This option is experimental and will be merged into the `exclusive_locking` + // option above if proven to cause no OS compatibility issues. + // TODO(crbug.com/1429117): Merge into above option, if possible. + bool exclusive_database_file_lock = false; + // If true, enables SQLite's Write-Ahead Logging (WAL). // // WAL integration is under development, and should not be used in shipping diff --git a/sql/database_unittest.cc b/sql/database_unittest.cc index 0119f13ee3da50..70819a2faf0eb1 100644 --- a/sql/database_unittest.cc +++ b/sql/database_unittest.cc @@ -8,6 +8,7 @@ #include #include +#include "base/files/file.h" #include "base/files/file_util.h" #include "base/files/scoped_temp_dir.h" #include "base/functional/bind.h" @@ -15,6 +16,7 @@ #include "base/logging.h" #include "base/memory/raw_ptr.h" #include "base/sequence_checker.h" +#include "base/strings/strcat.h" #include "base/strings/string_number_conversions.h" #include "base/test/bind.h" #include "base/test/gtest_util.h" @@ -1913,6 +1915,77 @@ TEST_P(SQLDatabaseTest, TriggersDisabledByDefault) { EXPECT_TRUE(db_->Execute("DROP TRIGGER IF EXISTS trigger")); } +#if BUILDFLAG(IS_WIN) + +class SQLDatabaseTestExclusiveFileLockMode + : public testing::Test, + public testing::WithParamInterface<::testing::tuple> { + public: + ~SQLDatabaseTestExclusiveFileLockMode() override = default; + + void SetUp() override { + db_ = std::make_unique(GetDBOptions()); + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + db_path_ = temp_dir_.GetPath().AppendASCII("maybelocked.sqlite"); + ASSERT_TRUE(db_->Open(db_path_)); + } + + DatabaseOptions GetDBOptions() { + DatabaseOptions options; + options.wal_mode = IsWALEnabled(); + options.exclusive_locking = true; + options.exclusive_database_file_lock = IsExclusivelockEnabled(); + return options; + } + + bool IsWALEnabled() { return std::get<0>(GetParam()); } + bool IsExclusivelockEnabled() { return std::get<1>(GetParam()); } + + protected: + base::ScopedTempDir temp_dir_; + base::FilePath db_path_; + std::unique_ptr db_; +}; + +TEST_P(SQLDatabaseTestExclusiveFileLockMode, BasicStatement) { + ASSERT_TRUE(db_->Execute("CREATE TABLE data(contents TEXT)")); + EXPECT_EQ(SQLITE_OK, db_->GetErrorCode()); + + ASSERT_TRUE(base::PathExists(db_path_)); + base::File open_db(db_path_, base::File::Flags::FLAG_OPEN_ALWAYS | + base::File::Flags::FLAG_READ); + + // If exclusive lock is enabled, then the test should not be able to re-open + // the database file, on Windows only. + EXPECT_EQ(IsExclusivelockEnabled(), !open_db.IsValid()); +} + +INSTANTIATE_TEST_SUITE_P( + All, + SQLDatabaseTestExclusiveFileLockMode, + ::testing::Combine(::testing::Bool(), ::testing::Bool()), + [](const auto& info) { + return base::StrCat( + {std::get<0>(info.param) ? "WALEnabled" : "WALDisabled", + std::get<1>(info.param) ? "ExclusiveLock" : "NoExclusiveLock"}); + }); + +#else + +TEST(SQLInvalidDatabaseFlagsDeathTest, ExclusiveDatabaseLock) { + base::ScopedTempDir temp_dir; + ASSERT_TRUE(temp_dir.CreateUniqueTempDir()); + auto db_path = temp_dir.GetPath().AppendASCII("database_test_locked.sqlite"); + + Database db({.exclusive_database_file_lock = true}); + + EXPECT_DEATH_IF_SUPPORTED( + { std::ignore = db.Open(db_path); }, + "exclusive_database_file_lock is only supported on Windows"); +} + +#endif // BUILDFLAG(IS_WIN) + class SQLDatabaseTestExclusiveMode : public testing::Test, public testing::WithParamInterface { public: