[cryptohome] make guest-mode homedir mount with -rwx------

[cryptohome] make guest-mode homedir mount with -rwx------

BUG=chromium-os:13173
TEST=unit tests, manual: ls -ald /home/chronos/user -> drwx------ 10 chronos chronos 500 Apr 21 14:26 /home/chronos/user/

Change-Id: Ifd60b6c31e773d55d334a6f6c58565c2c64365b8

R=jimhebert@chromium.org

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=ad07f0a

[Will Drewry, 2011-04-21 21:42:09]

With the error cleanup, this would be good to go (with some optional nits).
Thanks!

http://codereview.chromium.org/6895001/diff/1/mount.cc
File mount.cc (right):

http://codereview.chromium.org/6895001/diff/1/mount.cc#newcode852
mount.cc:852: if (!platform_->SetPermissions(home_dir_, S_IRUSR | S_IWUSR |
S_IXUSR))
Doesn't this need to call platform_->Unmount if it fails?

(nit: it'd be nice to have it as a constant like kDefaultMask, but since it is
only used here, I think it's fine as is.)

http://codereview.chromium.org/6895001/diff/1/platform.h
File platform.h (right):

http://codereview.chromium.org/6895001/diff/1/platform.h#newcode163
platform.h:163: virtual bool SetPermissions(const std::string& path, mode_t
new_perms);
nit: mind adding it to mock_platform.h?

[jimhebert, 2011-04-21 21:51:59]

In theory this leaves a racey window of time between time-of-mount and
time-of-chmod where the directory is still world writable. 

From a quick glance at http://en.wikipedia.org/wiki/Tmpfs#Linux it looks like
tmpfs accepts a mount-time option like -o mode=0700 where we could get these
permissions without the race?

If that was considered-and-decided-against for some reason, then the alternative
as implemented LGTM. But I just wanted to raise the question at least...

Thanks for working on this!

On 2011/04/21 21:42:09, Will Drewry wrote:
> With the error cleanup, this would be good to go (with some optional nits).
> Thanks!
> 
> http://codereview.chromium.org/6895001/diff/1/mount.cc
> File mount.cc (right):
> 
> http://codereview.chromium.org/6895001/diff/1/mount.cc#newcode852
> mount.cc:852: if (!platform_->SetPermissions(home_dir_, S_IRUSR | S_IWUSR |
> S_IXUSR))
> Doesn't this need to call platform_->Unmount if it fails?
> 
> (nit: it'd be nice to have it as a constant like kDefaultMask, but since it is
> only used here, I think it's fine as is.)
> 
> http://codereview.chromium.org/6895001/diff/1/platform.h
> File platform.h (right):
> 
> http://codereview.chromium.org/6895001/diff/1/platform.h#newcode163
> platform.h:163: virtual bool SetPermissions(const std::string& path, mode_t
> new_perms);
> nit: mind adding it to mock_platform.h?

[Chris Masone, 2011-04-21 22:00:59]

Jim, that seems like a way better idea.  Testing that change now.

On 2011/04/21 21:51:59, jimhebert wrote:
> In theory this leaves a racey window of time between time-of-mount and
> time-of-chmod where the directory is still world writable. 
> 
> From a quick glance at http://en.wikipedia.org/wiki/Tmpfs#Linux it looks like
> tmpfs accepts a mount-time option like -o mode=0700 where we could get these
> permissions without the race?
> 
> If that was considered-and-decided-against for some reason, then the
alternative
> as implemented LGTM. But I just wanted to raise the question at least...
> 
> Thanks for working on this!
> 
> On 2011/04/21 21:42:09, Will Drewry wrote:
> > With the error cleanup, this would be good to go (with some optional nits).
> > Thanks!
> > 
> > http://codereview.chromium.org/6895001/diff/1/mount.cc
> > File mount.cc (right):
> > 
> > http://codereview.chromium.org/6895001/diff/1/mount.cc#newcode852
> > mount.cc:852: if (!platform_->SetPermissions(home_dir_, S_IRUSR | S_IWUSR |
> > S_IXUSR))
> > Doesn't this need to call platform_->Unmount if it fails?
> > 
> > (nit: it'd be nice to have it as a constant like kDefaultMask, but since it
is
> > only used here, I think it's fine as is.)
> > 
> > http://codereview.chromium.org/6895001/diff/1/platform.h
> > File platform.h (right):
> > 
> > http://codereview.chromium.org/6895001/diff/1/platform.h#newcode163
> > platform.h:163: virtual bool SetPermissions(const std::string& path, mode_t
> > new_perms);
> > nit: mind adding it to mock_platform.h?

[jimhebert, 2011-04-21 22:16:29]

LGTM :)

[Will Drewry, 2011-04-21 22:16:53]

LGTM