[sql] Allow restricting database to user read access.

[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

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212106

[Scott Hess - ex-Googler, 2013-07-11 20:59:16]

Posting this to you first in case I missed a trick.

[Scott Hess - ex-Googler, 2013-07-11 21:24:11]

Jorge, Justin is apparently too platform-specific to understand complicated
things.

[Scott Hess - ex-Googler, 2013-07-11 21:28:23]

On 2013/07/11 21:24:11, shess wrote:
> Jorge, Justin is apparently too platform-specific to understand complicated
> things.

Um, and I could have gbillock (in OWNERS for login_database.cc) review, too, I
just wanted someone on security to +1 because of Type-Bug-Security, so I mostly
wanted to get +1 from that side before +1 from someone else and then having to
rewrite it.

[Jorge Lucangeli Obes, 2013-07-11 23:00:42]

lgtm

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
File sql/connection.cc (right):

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:735: DCHECK_NE(file_name, std::string(":memory"));
Does ":memory" mean a memory-backed database?

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

Greg, could you give this a sql/OWNERS type review?  Per my comment to Jorge, I
could see some debate as to where this should specifically live.

For background, AFAICT there is no PRAGMA or anything to see per-connection
permissions.  So it can only be set manually like this.  The code could either
be here or in Open().  I suppose another option would be to have Open() test for
existence, and create an zero-length file with the right mode _before_
sqlite3_open() is called.

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
File sql/connection.cc (right):

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:735: DCHECK_NE(file_name, std::string(":memory"));
On 2013/07/11 23:00:43, Jorge Lucangeli Obes wrote:
> Does ":memory" mean a memory-backed database?

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.

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

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
File sql/connection.cc (right):

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:733: #if defined(OS_POSIX)
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?

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:741: mode &= file_util::FILE_PERMISSION_USER_MASK;
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?

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:750: file_util::SetPosixFilePermissions(journal_path, mode);
Do we need the same "if Get { Set }" formulation here?

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection_unittest.cc:574: // woudln't test anything real.
wouldn't

But I'm not sure I understood this comment anyway. I think you mean that the
user as which the test executes could have a umask which is already  0x700 or
something, in which case tests aren't doing anything. Is it all the tests? Just
after some point? I think that's where I'm confused.

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection_unittest.cc:579: ASSERT_NE((mode &
file_util::FILE_PERMISSION_USER_MASK), mode);
Will this pass for such a umask? Looks like not, correct? Perhaps bail out of
the test completely here...

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

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
File sql/connection.cc (right):

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:733: #if defined(OS_POSIX)
On 2013/07/15 23:26:06, Greg Billock wrote:
> 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?

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

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:741: mode &= file_util::FILE_PERMISSION_USER_MASK;
On 2013/07/15 23:26:06, Greg Billock wrote:
> 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?

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.

> 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?

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.

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection.cc:750: file_util::SetPosixFilePermissions(journal_path, mode);
On 2013/07/15 23:26:06, Greg Billock wrote:
> Do we need the same "if Get { Set }" formulation here?

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.

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection_unittest.cc:574: // woudln't test anything real.
On 2013/07/15 23:26:06, Greg Billock wrote:
> wouldn't
> 
> But I'm not sure I understood this comment anyway. I think you mean that the
> user as which the test executes could have a umask which is already  0x700 or
> something, in which case tests aren't doing anything. Is it all the tests?
Just
> after some point? I think that's where I'm confused.

Usually, I would expect the default umask for OSX or Linux to be 022, with some
people setting a more-restrictive umask like 077.  Unfortunately, some of our
bots are that kind of people, and it's entirely possible that people running
bots would consider more-restrictive umask to be a good thing, and if all of the
bots were running with 077, then none of them would actually be testing that the
code works.

Maybe a scoper is in order.  Indeed, base/memory/shared_memory_unittest.cc has
just such a scoper for just such a reason.  I'm going to go with that.

https://codereview.chromium.org/5125579611308032/diff/5785905063264256/sql/co...
sql/connection_unittest.cc:579: ASSERT_NE((mode &
file_util::FILE_PERMISSION_USER_MASK), mode);
On 2013/07/15 23:26:06, Greg Billock wrote:
> Will this pass for such a umask? Looks like not, correct? Perhaps bail out of
> the test completely here...

Changed so this won't happen.

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

Can't think of any more wacky edge case situations to ask about. :-)

lgtm

https://codereview.chromium.org/5125579611308032/diff/17001/sql/connection_un...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/5125579611308032/diff/17001/sql/connection_un...
sql/connection_unittest.cc:240: // Inserting something other than a number into
the primary key
obsolete comment? Looks like you're aiming at the "UNIQUE" constraint here.

https://codereview.chromium.org/5125579611308032/diff/17001/sql/connection_un...
sql/connection_unittest.cc:256: // base::Bind() can curry arguments to be passed
by const reference
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:08]

Crap.  I see that I need another OWNER for login_database.  I recognize Ilya's
name, so there we go!

https://codereview.chromium.org/5125579611308032/diff/17001/sql/connection_un...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/5125579611308032/diff/17001/sql/connection_un...
sql/connection_unittest.cc:240: // Inserting something other than a number into
the primary key
On 2013/07/16 20:44:05, Greg Billock wrote:
> obsolete comment? Looks like you're aiming at the "UNIQUE" constraint here.

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

https://codereview.chromium.org/5125579611308032/diff/17001/sql/connection_un...
sql/connection_unittest.cc:256: // base::Bind() can curry arguments to be passed
by const reference
On 2013/07/16 20:44:05, Greg Billock wrote:
> 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.

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.

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

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.

[Ilya Sherman, 2013-07-16 22:35:52]

password_manager changes LGTM % nits

https://codereview.chromium.org/5125579611308032/diff/33001/chrome/browser/pa...
File chrome/browser/password_manager/login_database_unittest.cc (right):

https://codereview.chromium.org/5125579611308032/diff/33001/chrome/browser/pa...
chrome/browser/password_manager/login_database_unittest.cc:572: #if
defined(OS_POSIX)
nit: Mebbe add a comment for why this is restricted just to POSIX?

https://codereview.chromium.org/5125579611308032/diff/33001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/5125579611308032/diff/33001/sql/connection.h#...
sql/connection.h:121: // TODO(shess): Currently only supported on OS_POSIX.
nit: Mebbe clarify that this is a no-op on other platforms?

[Scott Hess - ex-Googler, 2013-07-17 00:25:36]

Apologies for the sadness of my comment additions, but my brain feels like it's
trying to prove a negative or something.

https://codereview.chromium.org/5125579611308032/diff/33001/chrome/browser/pa...
File chrome/browser/password_manager/login_database_unittest.cc (right):

https://codereview.chromium.org/5125579611308032/diff/33001/chrome/browser/pa...
chrome/browser/password_manager/login_database_unittest.cc:572: #if
defined(OS_POSIX)
On 2013/07/16 22:35:52, Ilya Sherman wrote:
> nit: Mebbe add a comment for why this is restricted just to POSIX?

Done.  Comment is also kind posix-specific :-).

https://codereview.chromium.org/5125579611308032/diff/33001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/5125579611308032/diff/33001/sql/connection.h#...
sql/connection.h:121: // TODO(shess): Currently only supported on OS_POSIX.
On 2013/07/16 22:35:52, Ilya Sherman wrote:
> nit: Mebbe clarify that this is a no-op on other platforms?

Done.

[commit-bot: I haz the power, 2013-07-17 00:33:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/5125579611308032/45001

[commit-bot: I haz the power, 2013-07-17 18:08:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/5125579611308032/45001

[Scott Hess - ex-Googler, 2013-07-17 19:11:07]

Committed patchset #6 manually as r212106 (presubmit successful).

[Scott Hess - ex-Googler, 2013-07-17 19:11:47]

Sheriff note: For some reason, this doesn't seem to be progressing through the
commit-queue, even though trybots and the like seem fine with it.  So landing
manually, I'll be around monitoring.