[sql] Allow restricting database to user read access.

sql/connection_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "sql/connection.h"
 #include "sql/meta_table.h"
@@ skipping 53 matching lines @@
 void ErrorCallbackResetHelper(sql::Connection* db,
                               size_t* counter,
                               const RefCounter& r,
                               int error, sql::Statement* stmt) {
   // The ref count should not go to zero when clearing the callback.
   EXPECT_GT(*counter, 0u);
   db->reset_error_callback();
   EXPECT_GT(*counter, 0u);
 }
 
+// Set a umask and restore the old mask on destruction.
+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);
+};
+
 class SQLConnectionTest : public testing::Test {
  public:
   SQLConnectionTest() {}
 
   virtual void SetUp() {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(db_.Open(db_path()));
   }
 
   virtual void TearDown() {
@@ skipping 134 matching lines @@
 TEST_F(SQLConnectionTest, ErrorCallback) {
   const char* kCreateSql = "CREATE TABLE foo (id INTEGER UNIQUE)";
   ASSERT_TRUE(db().Execute(kCreateSql));
   ASSERT_TRUE(db().Execute("INSERT INTO foo (id) VALUES (12)"));
 
   int error = SQLITE_OK;
   {
     sql::ScopedErrorCallback sec(
         &db(), base::Bind(&sql::CaptureErrorCallback, &error));
 
     // Inserting something other than a number into the primary key

[Greg Billock, 2013/07/16 20:44:05]

obsolete comment? Looks like you're aiming at the "UNIQUE" constraint here.

[Scott Hess - ex-Googler, 2013/07/16 21:03:09]

Ah, I think this is copied forward from a different implementation.

     // should result in the callback seeing SQLITE_MISMATCH.
     EXPECT_FALSE(db().Execute("INSERT INTO foo (id) VALUES (12)"));
     EXPECT_EQ(SQLITE_CONSTRAINT, error);
   }
 
   // Callback is no longer in force due to reset.
   {
     error = SQLITE_OK;
     sql::ScopedErrorIgnorer ignore_errors;
     ignore_errors.IgnoreError(SQLITE_CONSTRAINT);
     ASSERT_FALSE(db().Execute("INSERT INTO foo (id) VALUES (12)"));
     ASSERT_TRUE(ignore_errors.CheckIgnoredErrors());
     EXPECT_EQ(SQLITE_OK, error);
   }
 
   // base::Bind() can curry arguments to be passed by const reference

[Greg Billock, 2013/07/16 20:44:05]

I think these are good tests, but I think after a few weeks it'll be hard for me
to remember what they're doing and why. I'm not sure I have a suggestion to make
this easier to recollect, though.

Perhaps lead off the comment talking about that the error callback could set the
error callback, thus causing any parameters to that same call to go out of
scope, therefore we have to copy it in Connection before executing.

Weird re-entrant sort of case, should we just disallow it? Or does it turn out
to be useful for single-shot error functions?

[Scott Hess - ex-Googler, 2013/07/16 21:03:09]

The comment already mentioned the callback setting the callback, I can add a
little text to indicate that the delete could happen while the callback is
executing.  Hopefully this will never break and nobody will have to think it
through again.
The problem with disallowing it entirely is that if the callback knows that
there is a persistent error of some sort and sets out to fix things, then it's
highly likely that fixing things will fire the callback.  I think the most
natural option would be for the callback function to simply reset the error
callback for the database.  Each callback could handle the reentrancy problem
locally, of course, but that seems likely to result in a half-dozen unique ways
to handle it, each incorrect in a different way.

From that point, it wouldn't take much more for me to convince myself that the
error callback should be automatically disabled while in the error callback. 
Don't really want to re-create all of the joys of POSIX signal handling without
some concrete justification, though.

The previous error-handler object had similar issues WRT ref-counting, they just
manifested somewhere else.

   // to the callback function.  If the callback function causes
   // re/set_error_callback() to be called, the storage for those
   // arguments can be deleted.
   //
   // RefCounter() counts how many objects are live using an external
   // count.  The same counter is passed to the callback, so that it
   // can check directly even if the RefCounter object is no longer
   // live.
   {
     size_t count = 0;
@@ skipping 400 matching lines @@
   // of journal_mode PERSIST.
   base::FilePath journal(db_path().value() + FILE_PATH_LITERAL("-journal"));
   ASSERT_TRUE(base::PathExists(db_path()));
   ASSERT_TRUE(base::PathExists(journal));
 
   sql::Connection::Delete(db_path());
   EXPECT_FALSE(base::PathExists(db_path()));
   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