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 <reillyg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1125803}

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

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);

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<bool> {
+ 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.

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);
 

content/browser/network/cookie_store_factory.cc

@@ -68,7 +68,8 @@ std::unique_ptr<net::CookieStore> CreateCookieStore(
     scoped_refptr<net::SQLitePersistentCookieStore> 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<net::CookieMonster>(std::move(sqlite_store), net_log);

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`.

net/extras/sqlite/sqlite_persistent_cookie_store.cc

@@ -257,13 +257,15 @@ class SQLitePersistentCookieStore::Backend
           scoped_refptr<base::SequencedTaskRunner> client_task_runner,
           scoped_refptr<base::SequencedTaskRunner> 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<base::SequencedTaskRunner>& client_task_runner,
     const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
     bool restore_old_session_cookies,
-    CookieCryptoDelegate* crypto_delegate)
+    CookieCryptoDelegate* crypto_delegate,
+    bool enable_exclusive_access)
     : backend_(base::MakeRefCounted<Backend>(path,
                                              client_task_runner,
                                              background_task_runner,
                                              restore_old_session_cookies,
-                                             crypto_delegate)) {}
+                                             crypto_delegate,
+                                             enable_exclusive_access)) {}
 
 void SQLitePersistentCookieStore::DeleteAllInList(
     const std::list<CookieOrigin>& cookies) {

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<base::SequencedTaskRunner>& client_task_runner,
       const scoped_refptr<base::SequencedTaskRunner>& 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&) =

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<SQLitePersistentCookieStore>(
         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<CanonicalCookie*> cookies;
     Load();
     ASSERT_EQ(0u, cookies_.size());
@@ -115,7 +116,8 @@ class SQLitePersistentCookieStorePerfTest : public testing::Test {
 
     store_ = base::MakeRefCounted<SQLitePersistentCookieStore>(
         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.

net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc

@@ -12,6 +12,7 @@
 #include <utility>
 
 #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<CookieCryptor>();
 
@@ -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<SQLitePersistentCookieStore>(
       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<SQLitePersistentCookieStore>(
       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<CookieMonster>(store_.get(), /*net_log=*/nullptr);
   ResultSavingCookieCallback<bool> 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<CookieMonster> cookie_monster =
       std::make_unique<CookieMonster>(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<bool> {
+ 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<CanonicalCookie> 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,

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;