-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unencrypted keys in user directory for single-password access (from #1399) #1585
base: master
Are you sure you want to change the base?
Conversation
Thanks for working on this! I don't think there needs to be a new parameter, though. When you go to encrypt the chroot, and only if you specify the |
@dnschneid I will get on that. It shouldn't take so long now that i'm familiar with the code. On an off topic git-related tangent, If I want to roll back my current commit, and start fresh (I think that would be easiest), should I Thanks |
If you want to start fresh and throw away the work, |
OK thanks for the tip. The question I have with the -k parameter is that if .ecryptfs is a pointer for the moved keyfile, then the only location for the keys is in the user directory, which is lost if the user account is deleted. I would prefer to have the keys in 2 places, both encrypted in the folder of the chroot and decrypted in the user home (this is especially true with separate-partition). Any suggestions for this? |
That's kind of the point of moving the key around. You can manually back up your key somewhere else (and then point to it if need-be by passing -k to enter-chroot), but for security, crouton shouldn't be silently duplicating your keys in multiple places. |
host-bin/mount-chroot
Outdated
# Detect the key file | ||
# Detect unencrypted keys | ||
if [ -z "$UKEYFILE" ]; then | ||
UKEYFILE="/home/chronos/user/crouton/$NAME.chkey" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why make ~/crouton
visible? If it is for storing keys, I think ~/.crouton
will be more suitale. Just like ~/.ssh
And maybe it is better to store the path in an variable? The path is used several times in the code.
Is that the sort of change you were looking for? I guess that makes more sense. |
@@ -64,11 +66,6 @@ promptNewPassphrase() { | |||
[ -t 0 ] && stty -echo | |||
while [ -z "$passphrase" ]; do | |||
read -r passphrase | |||
if [ -z "$passphrase" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think this should still be enforced if keyfile wasn't set by the user.
But yes, this was pretty much how I was expecting it to behave (kudos for the very clean way of handling the unwrapped keys). |
I still have a problem. They 63-char keys are coming back to haunt me. They are not properly handled. But It should be good otherwise. |
read -r passphrase | ||
if [ -z "$passphrase" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this... I wasn't sure if the warning about secure location was enough, or the blank passphrase should be linked to the -k flag. Right now they are independent features. I get the secure location thing, but seeing as the crouton default is no encryption at all, it feels like overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of the -e
flag is to make your chroot install more secure. If we allow blank passwords without further configuration, then people will assume that even with a blank passphrase, things are fine (they will ignore the later warnings). Unless you see actual value in allowing co-located unencrypted passphrases, I'd rather disallow it to avoid any chance of a false sense of security.
host-bin/mount-chroot
Outdated
echo | cat - "$wrappedkey" "$wrappedfnek" > "$KEYFILE" | ||
else | ||
echo -n " | ||
................$key................$fnek" >> "$KEYFILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truncate, not append
host-bin/mount-chroot
Outdated
@@ -216,32 +228,78 @@ $passphrase" | ecryptfs-wrap-passphrase "$wrappedfnek" - | |||
tail -c 160 "$KEYFILE" | head -c 80 > "$wrappedkey" | |||
tail -c 80 "$KEYFILE" > "$wrappedfnek" | |||
|
|||
PAD1="`head -c 16 "$wrappedkey"`" | |||
PAD2="`tail -c 16 "$wrappedfnek"`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now needs to be head
...I would look at the rest of the key handling (especially below) to make sure it's up-to-date with the new ...xxx...yyy format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey in my defense I caught this before you pointed it out :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serves me right for checking things as you're pushing new commits :)
Is hexdump still giving you a bad byte count, even with the |
The issue with the hexdump is that old keys (made before the fix) are still too short. I will have to do line-by-line separation. |
Good point. I don't think you need to do line-by-line though. Something like: If first 16 characters of key is ....., it's unencrypted (don't check fnek to confirm). Then you can easily extract the keys (which are just hex):
|
OK I finished testing and worked out a few bugs. It all works for the most parts.
|
Well this is going to sound lame, but I just lost the completed above TODO. Somebody pressed the spacebar..... grr. I will have it done again in a few days. |
Whoops! No rush :) |
Haha, well it looks like I got side-tracked for a long time.... ** Rebased and dealt with echo_tty ** |
mount-chroot now accepts blank passwords (after prompt), and will save padded unencrypted keys instead of wrapped keys. requires -k flag to specify custom location for keys.
host-bin/mount-chroot
Outdated
echo_tty '' | ||
echo_tty -n 'Please confirm your passphrase: ' | ||
echo -n 'Please confirm your passphrase: ' 1>&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be echo_tty
This is shaping up very well! |
The last thing I'd like to see is additions to the 17-encryption tests to handle no passwords (both with -k and without -k specified). |
host-bin/mount-chroot
Outdated
echo_tty -n "Choose an encryption passphrase for $NAME: " | ||
[ -t 0 ] && stty -echo | ||
while [ -z "$passphrase" ]; do | ||
NOPASS='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it's a local variable, so add local
Considering how critical this is to users' data, I can't merge this unless there are tests in place to prove it works. Could you add tests to 17-encryption to stress the new code paths? |
OK, I'm still figuring out the testing architecture, But I will figure it out eventually. |
@Timvrakas shouldn't be too complex (you can run the encryption tests on your device by dropping the repo outside your chroot and running |
Much simpler to just store some long random password in the home folder, then modify mount-chroot to read it out. Additionally I recommend moving all the scripts from /usr/local/bin to within the eCryptfs home folder (i.e. /home/chronos/user/crouton/) and making a symlink to the chroot storage (i.e. /home/chronos/user/chroot)
|
The password would have to have as many bits of entropy as the key itself, assuming the wrapping function actually can use that much data. Seems like one way or another this would weaken the chroot protection somewhat, compared to storing the key directly in encrypted storage. Of course, it's still better (and more convenient) than the current approach.
This is also very prudent. To be clear, you have to run the scripts directly from the encrypted storage. |
455c029
to
cebf84f
Compare
NOTE: Continuation of #1399
OK here it is!
Adds new encryption system that uses unencrypted keys the the encrypted user partition, to allow for easier encrypted login.
edit-chroot -u
ormount-chroot -u
will create the keys, and they will be detected from then on.I think edit-chroot and mount-chroot are good to go. I tested them on my C720
I am unsure if (or how) I should edit main.sh and add an option to set this up on creation of a chroot, for now you have to run edit-chroot -u or mount-chroot -u to create the keys.
I'm still new to all this (crouton, git, bash, life) so feel free to offer comments, questions, concerns, criticisms, critiques, complaints.
Thanks!