sqlite: Create all databases with restrictive permissions.

Chrome's SQLite currently uses permissive (0644) POSIX access controls
for newly created databases, and //sql offers a method for opting into
restrictive (0600) permissions. This method is only used by the login
database. However, all Chrome databases are likely to have private data.
For example, the cookies database may contain long-lived OAuth tokens,
and can be just as valuable as the login database. The same argument
applies for the DOMStorage database.

This CL configures SQLite to use restrictive permissions by default, and
removes the method for opting into the restrictive permissions.

Change-Id: I5f0ce9e7f038081fad515cfc30c45ccccf7ff1b6
Reviewed-on: https://chromium-review.googlesource.com/1146295Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Chris Mumford <cmumford@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577476}

components/password_manager/core/browser/login_database.cc

@@ -533,7 +533,6 @@ bool LoginDatabase::Init() {
   db_.set_page_size(2048);
   db_.set_cache_size(32);
   db_.set_exclusive_locking();
-  db_.set_restrict_to_user();
   db_.set_histogram_tag("Passwords");
 
   if (!db_.Open(db_path_)) {

sql/connection.cc

@@ -203,6 +203,21 @@ void Connection::ResetErrorExpecter() {
   current_expecter_cb_ = nullptr;
 }
 
+// static
+base::FilePath Connection::JournalPath(const base::FilePath& db_path) {
+  return base::FilePath(db_path.value() + FILE_PATH_LITERAL("-journal"));
+}
+
+// static
+base::FilePath Connection::WriteAheadLogPath(const base::FilePath& db_path) {
+  return base::FilePath(db_path.value() + FILE_PATH_LITERAL("-wal"));
+}
+
+// static
+base::FilePath Connection::SharedMemoryFilePath(const base::FilePath& db_path) {
+  return base::FilePath(db_path.value() + FILE_PATH_LITERAL("-shm"));
+}
+
 Connection::StatementRef::StatementRef(Connection* connection,
                                        sqlite3_stmt* stmt,
                                        bool was_valid)
@@ -245,7 +260,6 @@ Connection::Connection()
       page_size_(0),
       cache_size_(0),
       exclusive_locking_(false),
-      restrict_to_user_(false),
       transaction_nesting_(0),
       needs_rollback_(false),
       in_memory_(false),
@@ -562,8 +576,7 @@ bool Connection::RegisterIntentToUpload() const {
   // Put the collection of diagnostic data next to the databases.  In most
   // cases, this is the profile directory, but safe-browsing stores a Cookies
   // file in the directory above the profile directory.
-  base::FilePath breadcrumb_path(
-      db_path.DirName().Append(FILE_PATH_LITERAL("sqlite-diag")));
+  base::FilePath breadcrumb_path = db_path.DirName().AppendASCII("sqlite-diag");
 
   // Lock against multiple updates to the diagnostics file.  This code should
   // seldom be called in the first place, and when called it should seldom be
@@ -1137,8 +1150,8 @@ void Connection::Poison() {
 bool Connection::Delete(const base::FilePath& path) {
   base::AssertBlockingAllowed();
 
-  base::FilePath journal_path(path.value() + FILE_PATH_LITERAL("-journal"));
-  base::FilePath wal_path(path.value() + FILE_PATH_LITERAL("-wal"));
+  base::FilePath journal_path = Connection::JournalPath(path);
+  base::FilePath wal_path = Connection::WriteAheadLogPath(path);
 
   std::string journal_str = AsUTF8ForSQL(journal_path);
   std::string wal_str = AsUTF8ForSQL(wal_path);
@@ -1162,16 +1175,13 @@ bool Connection::Delete(const base::FilePath& path) {
   vfs->xDelete(vfs, path_str.c_str(), 0);
 
   int journal_exists = 0;
-  vfs->xAccess(vfs, journal_str.c_str(), SQLITE_ACCESS_EXISTS,
-               &journal_exists);
+  vfs->xAccess(vfs, journal_str.c_str(), SQLITE_ACCESS_EXISTS, &journal_exists);
 
   int wal_exists = 0;
-  vfs->xAccess(vfs, wal_str.c_str(), SQLITE_ACCESS_EXISTS,
-               &wal_exists);
+  vfs->xAccess(vfs, wal_str.c_str(), SQLITE_ACCESS_EXISTS, &wal_exists);
 
   int path_exists = 0;
-  vfs->xAccess(vfs, path_str.c_str(), SQLITE_ACCESS_EXISTS,
-               &path_exists);
+  vfs->xAccess(vfs, path_str.c_str(), SQLITE_ACCESS_EXISTS, &path_exists);
 
   return !journal_exists && !wal_exists && !path_exists;
 }
@@ -1631,30 +1641,6 @@ bool Connection::OpenInternal(const std::string& file_name,
     return false;
   }
 
-  // TODO(shess): OS_WIN support?
-#if defined(OS_POSIX)
-  if (restrict_to_user_) {
-    DCHECK_NE(file_name, std::string(":memory"));
-    base::FilePath file_path(file_name);
-    int mode = 0;
-    // TODO(shess): Arguably, failure to retrieve and change
-    // permissions should be fatal if the file exists.
-    if (base::GetPosixFilePermissions(file_path, &mode)) {
-      mode &= base::FILE_PERMISSION_USER_MASK;
-      base::SetPosixFilePermissions(file_path, mode);
-
-      // SQLite sets the permissions on these files from the main
-      // database on create.  Set them here in case they already exist
-      // at this point.  Failure to set these permissions should not
-      // be fatal unless the file doesn't exist.
-      base::FilePath journal_path(file_name + FILE_PATH_LITERAL("-journal"));
-      base::FilePath wal_path(file_name + FILE_PATH_LITERAL("-wal"));
-      base::SetPosixFilePermissions(journal_path, mode);
-      base::SetPosixFilePermissions(wal_path, mode);
-    }
-  }
-#endif  // defined(OS_POSIX)
-
   // Enable extended result codes to provide more color on I/O errors.
   // Not having extended result codes is not a fatal problem, as
   // Chromium code does not attempt to handle I/O errors anyhow.  The

sql/connection.h

@@ -108,12 +108,6 @@ class SQL_EXPORT Connection {
   // This must be called before Open() to have an effect.
   void set_exclusive_locking() { exclusive_locking_ = true; }
 
-  // Call to cause Open() to restrict access permissions of the
-  // database file to only the owner.
-  //
-  // This is only supported on OS_POSIX and is a noop on other platforms.
-  void set_restrict_to_user() { restrict_to_user_ = true; }
-
   // Call to use alternative status-tracking for mmap.  Usually this is tracked
   // in the meta table, but some databases have no meta table.
   // TODO(shess): Maybe just have all databases use the alt option?
@@ -469,6 +463,39 @@ class SQL_EXPORT Connection {
     clock_ = std::move(clock);
   }
 
+  // Computes the path of a database's rollback journal.
+  //
+  // The journal file is created at the beginning of the database's first
+  // transaction. The file may be removed and re-created between transactions,
+  // depending on whether the database is opened in exclusive mode, and on
+  // configuration options. The journal file does not exist when the database
+  // operates in WAL mode.
+  //
+  // This is intended for internal use and tests. To preserve our ability to
+  // iterate on our SQLite configuration, features must avoid relying on
+  // the existence of specific files.
+  static base::FilePath JournalPath(const base::FilePath& db_path);
+
+  // Computes the path of a database's write-ahead log (WAL).
+  //
+  // The WAL file exists while a database is opened in WAL mode.
+  //
+  // This is intended for internal use and tests. To preserve our ability to
+  // iterate on our SQLite configuration, features must avoid relying on
+  // the existence of specific files.
+  static base::FilePath WriteAheadLogPath(const base::FilePath& db_path);
+
+  // Computes the path of a database's shared memory (SHM) file.
+  //
+  // The SHM file is used to coordinate between multiple processes using the
+  // same database in WAL mode. Thus, this file only exists for databases using
+  // WAL and not opened in exclusive mode.
+  //
+  // This is intended for internal use and tests. To preserve our ability to
+  // iterate on our SQLite configuration, features must avoid relying on
+  // the existence of specific files.
+  static base::FilePath SharedMemoryFilePath(const base::FilePath& db_path);
+
  private:
   // For recovery module.
   friend class Recovery;
@@ -718,7 +745,6 @@ class SQL_EXPORT Connection {
   int page_size_;
   int cache_size_;
   bool exclusive_locking_;
-  bool restrict_to_user_;
 
   // Holds references to all cached statements so they remain active.
   //

sql/connection_unittest.cc

@@ -867,85 +867,74 @@ TEST_F(SQLConnectionTest, SetTempDirForSQL) {
 }
 #endif  // defined(OS_ANDROID)
 
-TEST_F(SQLConnectionTest, Delete) {
+TEST_F(SQLConnectionTest, DeleteNonWal) {
   EXPECT_TRUE(db().Execute("CREATE TABLE x (x)"));
   db().Close();
 
   // Should have both a main database file and a journal file because
   // of journal_mode TRUNCATE.
-  base::FilePath journal(db_path().value() + FILE_PATH_LITERAL("-journal"));
+  base::FilePath journal_path = sql::Connection::JournalPath(db_path());
   ASSERT_TRUE(GetPathExists(db_path()));
-  ASSERT_TRUE(GetPathExists(journal));
+  ASSERT_TRUE(GetPathExists(journal_path));
 
   sql::Connection::Delete(db_path());
   EXPECT_FALSE(GetPathExists(db_path()));
-  EXPECT_FALSE(GetPathExists(journal));
+  EXPECT_FALSE(GetPathExists(journal_path));
 }
 
-// This test manually sets on disk permissions, these don't exist on Fuchsia.
-#if defined(OS_POSIX)
-// Test that set_restrict_to_user() trims database permissions so that
-// only the owner (and root) can read.
-TEST_F(SQLConnectionTest, UserPermission) {
-  // If the bots all had a restrictive umask setting such that
-  // databases are always created with only the owner able to read
-  // them, then the code could break without breaking the tests.
-  // Temporarily provide a more permissive umask.
+#if defined(OS_POSIX)  // This test operates on POSIX file permissions.
+TEST_F(SQLConnectionTest, PosixFilePermissions) {
   db().Close();
   sql::Connection::Delete(db_path());
   ASSERT_FALSE(GetPathExists(db_path()));
+
+  // If the bots all had a restrictive umask setting such that databases are
+  // always created with only the owner able to read them, then the code could
+  // break without breaking the tests. Temporarily provide a more permissive
+  // umask.
   ScopedUmaskSetter permissive_umask(S_IWGRP | S_IWOTH);
+
   ASSERT_TRUE(db().Open(db_path()));
 
-  // Cause the journal file to be created.  If the default
-  // journal_mode is changed back to DELETE, then parts of this test
-  // will need to be updated.
+  // Cause the journal file to be created. If the default journal_mode is
+  // changed back to DELETE, this test will need to be updated.
   EXPECT_TRUE(db().Execute("CREATE TABLE x (x)"));
 
-  base::FilePath journal(db_path().value() + FILE_PATH_LITERAL("-journal"));
   int mode;
-
-  // Given a permissive umask, the database is created with permissive
-  // read access for the database and journal.
   ASSERT_TRUE(GetPathExists(db_path()));
-  ASSERT_TRUE(GetPathExists(journal));
-  mode = base::FILE_PERMISSION_MASK;
   EXPECT_TRUE(base::GetPosixFilePermissions(db_path(), &mode));
-  ASSERT_NE((mode & base::FILE_PERMISSION_USER_MASK), mode);
-  mode = base::FILE_PERMISSION_MASK;
-  EXPECT_TRUE(base::GetPosixFilePermissions(journal, &mode));
-  ASSERT_NE((mode & base::FILE_PERMISSION_USER_MASK), mode);
+  ASSERT_EQ(mode, 0600);
 
-  // Re-open with restricted permissions and verify that the modes
-  // changed for both the main database and the journal.
-  db().Close();
-  db().set_restrict_to_user();
-  ASSERT_TRUE(db().Open(db_path()));
-  ASSERT_TRUE(GetPathExists(db_path()));
-  ASSERT_TRUE(GetPathExists(journal));
-  mode = base::FILE_PERMISSION_MASK;
-  EXPECT_TRUE(base::GetPosixFilePermissions(db_path(), &mode));
-  ASSERT_EQ((mode & base::FILE_PERMISSION_USER_MASK), mode);
-  mode = base::FILE_PERMISSION_MASK;
-  EXPECT_TRUE(base::GetPosixFilePermissions(journal, &mode));
-  ASSERT_EQ((mode & base::FILE_PERMISSION_USER_MASK), mode);
+  {
+    base::FilePath journal_path = sql::Connection::JournalPath(db_path());
+    DLOG(ERROR) << "journal_path: " << journal_path;
+    ASSERT_TRUE(GetPathExists(journal_path));
+    EXPECT_TRUE(base::GetPosixFilePermissions(journal_path, &mode));
+    ASSERT_EQ(mode, 0600);
+  }
 
-  // Delete and re-create the database, the restriction should still apply.
+  // Reopen the database and turn on WAL mode.
   db().Close();
   sql::Connection::Delete(db_path());
+  ASSERT_FALSE(GetPathExists(db_path()));
   ASSERT_TRUE(db().Open(db_path()));
-  ASSERT_TRUE(GetPathExists(db_path()));
-  ASSERT_FALSE(GetPathExists(journal));
-  mode = base::FILE_PERMISSION_MASK;
-  EXPECT_TRUE(base::GetPosixFilePermissions(db_path(), &mode));
-  ASSERT_EQ((mode & base::FILE_PERMISSION_USER_MASK), mode);
+  ASSERT_TRUE(db().Execute("PRAGMA journal_mode = WAL"));
+  ASSERT_EQ("wal", ExecuteWithResult(&db(), "PRAGMA journal_mode"));
 
-  // Verify that journal creation inherits the restriction.
-  EXPECT_TRUE(db().Execute("CREATE TABLE x (x)"));
-  ASSERT_TRUE(GetPathExists(journal));
-  mode = base::FILE_PERMISSION_MASK;
-  EXPECT_TRUE(base::GetPosixFilePermissions(journal, &mode));
-  ASSERT_EQ((mode & base::FILE_PERMISSION_USER_MASK), mode);
+  // The WAL file is created lazily on first change.
+  ASSERT_TRUE(db().Execute("CREATE TABLE foo (a, b)"));
+
+  {
+    base::FilePath wal_path = sql::Connection::WriteAheadLogPath(db_path());
+    ASSERT_TRUE(GetPathExists(wal_path));
+    EXPECT_TRUE(base::GetPosixFilePermissions(wal_path, &mode));
+    ASSERT_EQ(mode, 0600);
+
+    base::FilePath shm_path = sql::Connection::SharedMemoryFilePath(db_path());
+    ASSERT_TRUE(GetPathExists(shm_path));
+    EXPECT_TRUE(base::GetPosixFilePermissions(shm_path, &mode));
+    ASSERT_EQ(mode, 0600);
+  }
 }
 #endif  // defined(OS_POSIX)
 
@@ -1439,8 +1428,8 @@ TEST_F(SQLConnectionTest, CollectDiagnosticInfo) {
 }
 
 TEST_F(SQLConnectionTest, RegisterIntentToUpload) {
-  base::FilePath breadcrumb_path(
-      db_path().DirName().Append(FILE_PATH_LITERAL("sqlite-diag")));
+  base::FilePath breadcrumb_path =
+      db_path().DirName().AppendASCII("sqlite-diag");
 
   // No stale diagnostic store.
   ASSERT_TRUE(!base::PathExists(breadcrumb_path));

sql/sqlite_features_unittest.cc

@@ -321,7 +321,7 @@ TEST_F(SQLiteFeaturesTest, TimeMachine) {
   ASSERT_TRUE(db().Execute("CREATE TABLE t (id INTEGER PRIMARY KEY)"));
   db().Close();
 
-  base::FilePath journal(db_path().value() + FILE_PATH_LITERAL("-journal"));
+  base::FilePath journal = sql::Connection::JournalPath(db_path());
   ASSERT_TRUE(GetPathExists(db_path()));
   ASSERT_TRUE(GetPathExists(journal));
 
@@ -450,7 +450,7 @@ TEST_F(SQLiteFeaturesTest, SmartAutoVacuum) {
 // additional work into Chromium shutdown.  Verify that SQLite supports a config
 // option to not checkpoint on close.
 TEST_F(SQLiteFeaturesTest, WALNoClose) {
-  base::FilePath wal_path(db_path().value() + FILE_PATH_LITERAL("-wal"));
+  base::FilePath wal_path = sql::Connection::WriteAheadLogPath(db_path());
 
   // Turn on WAL mode, then verify that the mode changed (WAL is supported).
   ASSERT_TRUE(db().Execute("PRAGMA journal_mode = WAL"));

third_party/sqlite/BUILD.gn

@@ -66,6 +66,12 @@ config("chromium_sqlite3_compile_options") {
     # TODO(pwnall): Upstream the ability to use this define.
     "SQLITE_MMAP_READ_ONLY=1",
 
+    # The default POSIX permissions for a newly created SQLite database.
+    #
+    # If unspecified, this defaults to 0644. All the data stored by Chrome is
+    # private, so our databases use stricter settings.
+    "SQLITE_DEFAULT_FILE_PERMISSIONS=0600",
+
     # SQLite uses a lookaside buffer to improve performance of small mallocs.
     # Chrome already depends on small mallocs being efficient, so we disable
     # this to avoid the extra memory overhead.
@@ -137,9 +143,6 @@ config("chromium_sqlite3_compile_options") {
 
     # Uses isnan() in the C99 standard library.
     "SQLITE_HAVE_ISNAN",
-
-    # TODO(pwnall): Add SQLITE_OMIT_COMPLETE when we import a SQLite release
-    #               including https://www.sqlite.org/src/info/c3e816cca4ddf096
   ]
 
   # On OSX, SQLite has extra logic for detecting the use of network