From 81a2a6094b498e6da0218892c8ccd776b660b829 Mon Sep 17 00:00:00 2001 From: "shess@chromium.org" Date: Wed, 17 Jul 2013 19:10:36 +0000 Subject: [PATCH] [sql] Allow restricting database to user read access. By default POSIX umask is generally 0644. For some databases, it makes sense to restrict access to 0600. Use new setting for password database. BUG=258771 R=gbillock@chromium.org, isherman@chromium.org, jorgelo@chromium.org Review URL: https://codereview.chromium.org/5125579611308032 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@212106 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/password_manager/login_database.cc | 1 + .../password_manager/login_database_unittest.cc | 14 ++++ sql/connection.cc | 25 ++++++ sql/connection.h | 7 ++ sql/connection_unittest.cc | 90 ++++++++++++++++++++-- 5 files changed, 131 insertions(+), 6 deletions(-) diff --git a/chrome/browser/password_manager/login_database.cc b/chrome/browser/password_manager/login_database.cc index be50c614ccd8..9b0ebdacbed8 100644 --- a/chrome/browser/password_manager/login_database.cc +++ b/chrome/browser/password_manager/login_database.cc @@ -96,6 +96,7 @@ bool LoginDatabase::Init(const base::FilePath& db_path) { db_.set_page_size(2048); db_.set_cache_size(32); db_.set_exclusive_locking(); + db_.set_restrict_to_user(); if (!db_.Open(db_path)) { LOG(WARNING) << "Unable to open the password store database."; diff --git a/chrome/browser/password_manager/login_database_unittest.cc b/chrome/browser/password_manager/login_database_unittest.cc index a0383da28664..4a44fc0920d4 100644 --- a/chrome/browser/password_manager/login_database_unittest.cc +++ b/chrome/browser/password_manager/login_database_unittest.cc @@ -5,6 +5,7 @@ #include "testing/gtest/include/gtest/gtest.h" #include "base/basictypes.h" +#include "base/file_util.h" #include "base/files/scoped_temp_dir.h" #include "base/path_service.h" #include "base/strings/string_number_conversions.h" @@ -567,3 +568,16 @@ TEST_F(LoginDatabaseTest, VectorSerialization) { output = DeserializeVector(temp); EXPECT_THAT(output, Eq(vec)); } + +#if defined(OS_POSIX) +// Only the current user has permission to read the database. +// +// Only POSIX because GetPosixFilePermissions() only exists on POSIX. +// This tests that sql::Connection::set_restrict_to_user() was called, +// and that function is a noop on non-POSIX platforms in any case. +TEST_F(LoginDatabaseTest, FilePermissions) { + int mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(file_, &mode)); + EXPECT_EQ((mode & file_util::FILE_PERMISSION_USER_MASK), mode); +} +#endif // defined(OS_POSIX) diff --git a/sql/connection.cc b/sql/connection.cc index 95d09c24ff53..d11b40cb24ff 100644 --- a/sql/connection.cc +++ b/sql/connection.cc @@ -168,6 +168,7 @@ Connection::Connection() page_size_(0), cache_size_(0), exclusive_locking_(false), + restrict_to_user_(false), transaction_nesting_(0), needs_rollback_(false), in_memory_(false), @@ -732,6 +733,30 @@ 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 (file_util::GetPosixFilePermissions(file_path, &mode)) { + mode &= file_util::FILE_PERMISSION_USER_MASK; + file_util::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")); + file_util::SetPosixFilePermissions(journal_path, mode); + file_util::SetPosixFilePermissions(wal_path, mode); + } + } +#endif // defined(OS_POSIX) + // SQLite uses a lookaside buffer to improve performance of small mallocs. // Chromium already depends on small mallocs being efficient, so we disable // this to avoid the extra memory overhead. diff --git a/sql/connection.h b/sql/connection.h index 1c4d72cfffe3..ab80a6c5d749 100644 --- a/sql/connection.h +++ b/sql/connection.h @@ -116,6 +116,12 @@ 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. + // TODO(shess): Currently only supported on OS_POSIX, is a noop on + // other platforms. + void set_restrict_to_user() { restrict_to_user_ = true; } + // Set an error-handling callback. On errors, the error number (and // statement, if available) will be passed to the callback. // @@ -488,6 +494,7 @@ class SQL_EXPORT Connection { int page_size_; int cache_size_; bool exclusive_locking_; + bool restrict_to_user_; // All cached statements. Keeping a reference to these statements means that // they'll remain active. diff --git a/sql/connection_unittest.cc b/sql/connection_unittest.cc index 8cf32fb3f867..f516215b133d 100644 --- a/sql/connection_unittest.cc +++ b/sql/connection_unittest.cc @@ -71,6 +71,21 @@ void ErrorCallbackResetHelper(sql::Connection* db, EXPECT_GT(*counter, 0u); } +#if defined(OS_POSIX) +// Set a umask and restore the old mask on destruction. Cribbed from +// shared_memory_unittest.cc. Used by POSIX-only UserPermission test. +class ScopedUmaskSetter { + public: + explicit ScopedUmaskSetter(mode_t target_mask) { + old_umask_ = umask(target_mask); + } + ~ScopedUmaskSetter() { umask(old_umask_); } + private: + mode_t old_umask_; + DISALLOW_IMPLICIT_CONSTRUCTORS(ScopedUmaskSetter); +}; +#endif + class SQLConnectionTest : public testing::Test { public: SQLConnectionTest() {} @@ -224,9 +239,6 @@ TEST_F(SQLConnectionTest, ErrorCallback) { { sql::ScopedErrorCallback sec( &db(), base::Bind(&sql::CaptureErrorCallback, &error)); - - // Inserting something other than a number into the primary key - // should result in the callback seeing SQLITE_MISMATCH. EXPECT_FALSE(db().Execute("INSERT INTO foo (id) VALUES (12)")); EXPECT_EQ(SQLITE_CONSTRAINT, error); } @@ -242,9 +254,9 @@ TEST_F(SQLConnectionTest, ErrorCallback) { } // base::Bind() can curry arguments to be passed by const reference - // to the callback function. If the callback function causes - // re/set_error_callback() to be called, the storage for those - // arguments can be deleted. + // to the callback function. If the callback function calls + // re/set_error_callback(), the storage for those arguments can be + // deleted while the callback function is still executing. // // RefCounter() counts how many objects are live using an external // count. The same counter is passed to the callback, so that it @@ -671,4 +683,70 @@ TEST_F(SQLConnectionTest, Delete) { EXPECT_FALSE(base::PathExists(journal)); } +#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. + db().Close(); + sql::Connection::Delete(db_path()); + ASSERT_FALSE(base::PathExists(db_path())); + 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. + 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(base::PathExists(db_path())); + ASSERT_TRUE(base::PathExists(journal)); + mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(db_path(), &mode)); + ASSERT_NE((mode & file_util::FILE_PERMISSION_USER_MASK), mode); + mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(journal, &mode)); + ASSERT_NE((mode & file_util::FILE_PERMISSION_USER_MASK), mode); + + // 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(base::PathExists(db_path())); + ASSERT_TRUE(base::PathExists(journal)); + mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(db_path(), &mode)); + ASSERT_EQ((mode & file_util::FILE_PERMISSION_USER_MASK), mode); + mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(journal, &mode)); + ASSERT_EQ((mode & file_util::FILE_PERMISSION_USER_MASK), mode); + + // Delete and re-create the database, the restriction should still apply. + db().Close(); + sql::Connection::Delete(db_path()); + ASSERT_TRUE(db().Open(db_path())); + ASSERT_TRUE(base::PathExists(db_path())); + ASSERT_FALSE(base::PathExists(journal)); + mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(db_path(), &mode)); + ASSERT_EQ((mode & file_util::FILE_PERMISSION_USER_MASK), mode); + + // Verify that journal creation inherits the restriction. + EXPECT_TRUE(db().Execute("CREATE TABLE x (x)")); + ASSERT_TRUE(base::PathExists(journal)); + mode = file_util::FILE_PERMISSION_MASK; + EXPECT_TRUE(file_util::GetPosixFilePermissions(journal, &mode)); + ASSERT_EQ((mode & file_util::FILE_PERMISSION_USER_MASK), mode); +} +#endif // defined(OS_POSIX) + } // namespace -- 2.11.4.GIT