Initial patch from Will.

Initial patch from Will.

[Will Drewry, 2010-05-26 20:13:02]

I reviewed the diff on the plane and attached the vim quickfix err notes from
it.

On the whole, looks great.  Only a few issues of concern (erase v empty, skel on
incognito?, ...) along with some style stuff - CHECK v BAIL_, etc.

Thanks!!
will


ecryptfs_final.diff:1199:24:!= NULL: old_password_out should be enough
ecryptfs_final.diff:1179:24:user_out->length()? 
ecryptfs_final.diff:1170:18:should this be erase()?
ecryptfs_final.diff:2397:10:add a TODO somewhere to add a Platform ro Env class
that abstracts calls to mount,umount,chown,etc so that we can mock them later
ecryptfs_final.diff:2444:1:I just lost an hour's worth of notes. Awesome. I'll
try to reconstruct.
ecryptfs_final.diff:1207:11:not sure if we use cin style-wise, but for this
cmdline tool, I think it is the best choice
ecryptfs_final.diff:2051:13:is this salt_file var used?  Or is this for side
effects of FIlePath
ecryptfs_final.diff:2387:12:we should stir in at least one round from the TPM
(TODO)
ecryptfs_final.diff:2120:7:note, lazy unmount with ecryptfs may not be safe for
good and bad reasons. See below
ecryptfs_final.diff:2130:7:On keyctl clear, ecryptfs goes dead in the water. 
This is _really_ awesome, but it means that on lazy unmount, if there is data
flushing from the VFS, if it hasn't hit ecryptfs yet or ecreyptfs needs the key,
the data flush will fail.  We will need to test this to ensure this doesn't
introduce flakiness with Chrome flushing its data.  IDeally, logout will be
trigger from Chrome and it will already be on its way out when you send the
SIGINTs
ecryptfs_final.diff:2127:7:yes this needs ot happen, but if it is done in
ui.conf, that's fine.  Basically, we have to kill every chronos-owned process
otherwise an attacker could persist a compromise across logins (but not reboots)
ecryptfs_final.diff:2184:7:these vars use?
ecryptfs_final.diff:2311:1:Not sure if this is done elsewhere, but you may still
need to copy skel in.  It brings up another issue.  pkcs11.conf (pending change)
may freak out setting up a tpm keychain for the incognito user on login.  Is
there a good way to detect incognito or is it sane to grep mtab for incognitofs?
ecryptfs_final.diff:2143:38:migth be worth noting that mtab is symlinked to
/proc/mounts on CrOS
ecryptfs_final.diff:2156:38:Overloading based on parameters is forbidden :/ Is
it possible to easily separate these?
ecryptfs_final.diff:2137:4:is this worth making CHECK() or is it only likely a
problem internally?
ecryptfs_final.diff:2201:14:fwiw, we may want to consider making this a call out
to a priv'd helper but that's irrelevant for right now _and_ if you abstract
platform calls to a mock-able class, we could always change it out from
underneath this code :)
ecryptfs_final.diff:2408:9:a todo here to use either openssl with tpm_engine or
calls to the opencryptoki will be used to wrap/unwrap int he next month or so
ecryptfs_final.diff:2565:55:SHA_DIGEST_LENGTH is for SHA-1?
ecryptfs_final.diff:2569:36:fwiw, we could use scrypt or something else here to
get memory bound key strengthening
ecryptfs_final.diff:2574:46:if we can't get the tpm support integrated in the
next few weeks, we should pull in scrypt and stop getting weakened passhashes, I
guess
ecryptfs_final.diff:2645:12:do the callers know to check if it is not nul
terminated?
ecryptfs_final.diff:2742:17:these are allocated on the stack - is std::vector
resilient to being passed up the stack like that?
ecryptfs_final.diff:2742:17:nevermind. not passing by reference
ecryptfs_final.diff:2750:47:s/staring/starting
ecryptfs_final.diff:2750:45:why not just check for /mount/point/ with the
trailing slash since all files open on the mntpoint should be under it or is
that a mistaken assumption?
ecryptfs_final.diff:2757:18:might want to ignore pid 1 too or at least throw a
big error if pid == 1 is seen touching the mntpoint.
ecryptfs_final.diff:2775:18:I see you already have the comment I mentioned
earlier. nm 
ecryptfs_final.diff:2752:8:scoped_array<char> linkbuf?
ecryptfs_final.diff:2808:8:Add a TODO - this is yet-another-function that would
be useful as a standalone helper to other code
ecryptfs_final.diff:2902:29:isn't this now master.%d.salt?
ecryptfs_final.diff:2964:29:explicit?
ecryptfs_final.diff:2964:29:Generally this stuff is pushed to Init() to avoid
overloading but I think constructor overloading is okay. I'd need to check the
style guide.
ecryptfs_final.diff:3040:35:not sure if = NULL is okay here
ecryptfs_final.diff:1813:43:Is it worth changing all the lowercase out_ to OUT_
too?
ecryptfs_final.diff:3999:29:man I hate that Blob typedef hehe.
ecryptfs_final.diff:4022:29:style: we tend to avoid operate overloading in lieu
of descriptive calls like .assign(x)
ecryptfs_final.diff:4052:43:ah cool - now I see how passkey is just out partial
pw.  also good that we can clean that up in the same code later!
ecryptfs_final.diff:4059:43:this should totally be a shared helper in the
namespace to avoid the code dupe
ecryptfs_final.diff:3985:1:hrm style sez no on using a whole namespace.
ecryptfs_final.diff:4286:8:LOG_IF(FATAL, (X) > (Y)) << message
ecryptfs_final.diff:4286:16:or CHECK()
ecryptfs_final.diff:5064:1:a useful constant I see :/
ecryptfs_final.diff:4406:4:I love that our ecryptfs signatures are random.
thanks for being extra diligent on that!!

http://codereview.chromium.org/2051003/diff/13001/14003
File src/platform/cryptohome/SConstruct (right):

http://codereview.chromium.org/2051003/diff/13001/14003#newcode18
src/platform/cryptohome/SConstruct:18: secure_blob.cc
tabs v spaces? [ may just be codereview though]

http://codereview.chromium.org/2051003/diff/13001/14007
File src/platform/cryptohome/bin/mount (right):

http://codereview.chromium.org/2051003/diff/13001/14007#newcode105
src/platform/cryptohome/bin/mount:105: echo -n "$wrapper" | openssl aes-256-ecb
-in /dev/fd/9 -out "$keyfile" -pass fd:0 \
line over 80 chars

someone else should maybe review this for correctness since I was vaguely
involved :)

http://codereview.chromium.org/2051003/diff/13001/14011
File src/platform/cryptohome/cryptohome.cc (right):

http://codereview.chromium.org/2051003/diff/13001/14011#newcode6
src/platform/cryptohome/cryptohome.cc:6: #include <iostream>
duped below

[Will Drewry, 2010-05-27 00:09:51]

LGTM with style nits


Awesome.

http://codereview.chromium.org/2051003/diff/44001/45023
File src/platform/cryptohome/mount.cc (right):

http://codereview.chromium.org/2051003/diff/44001/45023#newcode21
src/platform/cryptohome/mount.cc:21: #include <dirent.h>
style nit: these should come before C++ includes

http://codereview.chromium.org/2051003/diff/44001/45023#newcode128
src/platform/cryptohome/mount.cc:128: if (mount_error != NULL) {
style nit: these can just be (mount_error) right?

http://codereview.chromium.org/2051003/diff/44001/45023#newcode187
src/platform/cryptohome/mount.cc:187: "ecryptfs", MS_NOEXEC | MS_NOSUID |
MS_NODEV,
style nit: it'd be nice to see this as a constant since it is used for
incognitofs too

[mschilder, 2010-05-27 04:20:18]

http://codereview.chromium.org/2051003/diff/47001/39052
File src/common/chromeos/dbus/service_constants.cc (right):

http://codereview.chromium.org/2051003/diff/47001/39052#newcode17
src/common/chromeos/dbus/service_constants.cc:17: const char
*kCryptohomeMigrateKey = "MigrateKey";
Should these be const char * const? Now you allocate a malleable .data ptr for
each. The compiler might optimize const * const away entirely, e.g. equiv to
const char foo[] = ""

http://codereview.chromium.org/2051003/diff/47001/39064
File src/platform/cryptohome/cryptohome_common.h (right):

http://codereview.chromium.org/2051003/diff/47001/39064#newcode22
src/platform/cryptohome/cryptohome_common.h:22: #define CRYPTOHOME_MIN(X, Y)
(((X) < (Y)) ? (X) : (Y))
These are scary of course, double eval of arguments;
CRYPTOHOME_MIN(x++, y--) type stuff.

gcc-ism like a >? b or typeof can help?

http://codereview.chromium.org/2051003/diff/47001/39074
File src/platform/cryptohome/mount.cc (right):

http://codereview.chromium.org/2051003/diff/47001/39074#newcode84
src/platform/cryptohome/mount.cc:84: struct passwd* user_info =
getpwnam(default_username_.c_str());
getpwnam_r() instead?

http://codereview.chromium.org/2051003/diff/47001/39074#newcode899
src/platform/cryptohome/mount.cc:899: while (struct dirent* fd_dirent =
readdir(fd_dir)) {
readdir_r()?

http://codereview.chromium.org/2051003/diff/47001/39084
File src/platform/cryptohome/username_passkey.cc (right):

http://codereview.chromium.org/2051003/diff/47001/39084#newcode80
src/platform/cryptohome/username_passkey.cc:80:
reinterpret_cast<char*>(&password_hash[0]),
Add a void* data() method to SecureBlob? This casting is not very future proof
in case the implementation details of SecureBlob change. From this context it's
not clear what SecureBlob is. The [0] suggests it's a typedef, not a class?

Avoid reinterpret_cast like the plague.