[sql] Allow restricting database to user read access.

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index ac8c0cd095b00ea666cb4c66a0fbce9b0e475984..ac898ea0818e048f877eb7be8f0a22f28b0485ca 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),
@@ -728,6 +729,30 @@ bool Connection::OpenInternal(const std::string& file_name) {
     return false;
   }
 
+  // TODO(shess): OS_WIN support?
+#if defined(OS_POSIX)

[Greg Billock, 2013/07/15 23:26:06]

Should this be treated as a DB version upgrade? Thinking worst-case, if it
results in not being able to access the file, would we be able to roll back?

[Scott Hess - ex-Googler, 2013/07/16 18:08:13]

Not sure what you mean.  If it results in not being able to access the file,
we're going to have to roll custom code to fix it no matter what.  But older
versions shouldn't have any problems reading this (if someone rolled to a new
version of Chromium and then back), just as if it would be fine if they changed
their umask to 0077 then back to 0022 (umask is things to mask out, apparently).

+  if (restrict_to_user_) {
+    DCHECK_NE(file_name, std::string(":memory"));

[Jorge Lucangeli Obes, 2013/07/11 23:00:43]

Does ":memory" mean a memory-backed database?

[Scott Hess - ex-Googler, 2013/07/15 20:50:08]

Yeah, sql::Connection has separate Open() and OpenMemory() functions which both
call through to this one, with ":memory:" signaling memory-backed to SQLite. 
This code could, probably, have been up in Open(), but I somewhat prefer to have
it as close as possible to the file's creation (sqlite3_open above).

Since you're good with the approach, I'm going to pass it by another sql/OWNERS
member to see if they prefer a different placement.

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

[Greg Billock, 2013/07/15 23:26:06]

do we need IXUSR?

How about just mode = ... ? We're trying to tighten the permission, no? I see
for symlinks, this sets the permissions on the pointed-to file. Is that OK?

Thinking here... could there be configurations either with shared profiles for a
workgroup, or situations with VMs or something where you can't change the file
permissions that will end up getting broken by this?

[Scott Hess - ex-Googler, 2013/07/16 18:08:13]

I'm aiming for "Adjust what SQLite has done after accounting for umask".  Since
the goal is to restrict access to the data, applying to the file rather than the
symlink is desirable.

If we decided to trim the executable bits, to me that seems like something we
should trim on ALL files, not just the login database.  Likewise WRT fixing
broken permissions, if someone's database is read-only, that's not specific to
the login database.

I suppose it would make sense to push a histogram at some point logging
access(2) results for R_OK|W_OK for each database.
This is POSIX-only.  Justin indicated that Windows didn't have this class of
problem (I have no idea how to _really_ determine that independently).  As far
as I'm aware, we don't support anything like that for OSX or Linux - and even if
we did, whatever the system is would necessarily have to have consistent
ownership of the files, because otherwise all sorts of things would be busted.

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

[Greg Billock, 2013/07/15 23:26:06]

Do we need the same "if Get { Set }" formulation here?

[Scott Hess - ex-Googler, 2013/07/16 18:08:13]

The SQLite code uses the main database permissions for the |mode| parameter to
open(), so I think this version is sufficient.  By the same reasoning, if(Get)
{Set} on the individual files should provide identical permissions to |mode|. 
The latter approach would allow for permissions to not be identical, but I just
went with this because it has less code.

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