Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Timvrakas
Copy link

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 or mount-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!

@dnschneid
Copy link
Owner

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 -k parameter, you should be allowed to leave the passphrase empty. crouton should confirm that you want to leave the passphrase in the clear, and note that this should only be done if the keyfile is located in encrypted storage (such as your user profile) or on removable media. When you go to mount the chroot, the chroot should automatically find the unwrapped key (as it already does for -k).

@Timvrakas
Copy link
Author

@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 git revert or git reset --hard and then push --force? I looked it up, but I'm unsure which is correct.

Thanks

@dnschneid
Copy link
Owner

If you want to start fresh and throw away the work, git reset --hard origin/master followed by git push origin encryption --force-with-lease will safely update github.

@Timvrakas
Copy link
Author

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?

@dnschneid
Copy link
Owner

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.

# Detect the key file
# Detect unencrypted keys
if [ -z "$UKEYFILE" ]; then
UKEYFILE="/home/chronos/user/crouton/$NAME.chkey"
Copy link
Contributor

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.

@Timvrakas
Copy link
Author

Is that the sort of change you were looking for? I guess that makes more sense.
FYI the previous commit was moved to timvrakas/old-encryption
WARNING: The above not tested, I will not have access to my chrome book until later tomorrow.

@@ -64,11 +66,6 @@ promptNewPassphrase() {
[ -t 0 ] && stty -echo
while [ -z "$passphrase" ]; do
read -r passphrase
if [ -z "$passphrase" ]; then
Copy link
Owner

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.

@dnschneid
Copy link
Owner

But yes, this was pretty much how I was expecting it to behave (kudos for the very clean way of handling the unwrapped keys).

@Timvrakas
Copy link
Author

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
Copy link
Author

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.

Copy link
Owner

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.

echo | cat - "$wrappedkey" "$wrappedfnek" > "$KEYFILE"
else
echo -n "
................$key................$fnek" >> "$KEYFILE"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truncate, not append

@@ -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"`"
Copy link
Owner

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.

Copy link
Author

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

Copy link
Owner

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

@dnschneid
Copy link
Owner

Is hexdump still giving you a bad byte count, even with the 32/1 update? It seemed fixed to me.

@Timvrakas
Copy link
Author

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.

@dnschneid
Copy link
Owner

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

contents="`tail -n+1 keyfile`"
key="${contents#${DOTS}}"
key="${key%%.*}"
fnek="${contents##*.}"

@Timvrakas
Copy link
Author

OK I finished testing and worked out a few bugs. It all works for the most parts.
TODO:

  1. Deal with the malformed (short) keys (as mentioned above)
  2. The -ee change password should check for request password.
  3. Link empty-password to -k flag (as mentioned above)

@Timvrakas
Copy link
Author

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.

@dnschneid
Copy link
Owner

Whoops! No rush :)

@Timvrakas
Copy link
Author

Haha, well it looks like I got side-tracked for a long time....
Anyway now I have to deal with the new echo_tty functions

** 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.
echo_tty ''
echo_tty -n 'Please confirm your passphrase: '
echo -n 'Please confirm your passphrase: ' 1>&2
Copy link
Owner

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

@dnschneid
Copy link
Owner

This is shaping up very well!

@dnschneid
Copy link
Owner

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

echo_tty -n "Choose an encryption passphrase for $NAME: "
[ -t 0 ] && stty -echo
while [ -z "$passphrase" ]; do
NOPASS=''
Copy link
Owner

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

@dnschneid
Copy link
Owner

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?

@Timvrakas
Copy link
Author

OK, I'm still figuring out the testing architecture, But I will figure it out eventually.

@dnschneid
Copy link
Owner

@Timvrakas shouldn't be too complex (you can run the encryption tests on your device by dropping the repo outside your chroot and running ./test/run.sh 17). If you need help, swing by #crouton-devel for advice. It'd be awesome to get this feature merged, but we have to be careful.

@dnschneid dnschneid added this to the Fixit Oct 2016 milestone Oct 11, 2016
@dnschneid dnschneid added the P2 label Oct 11, 2016
@dnschneid dnschneid removed this from the Fixit Oct 2016 milestone Oct 14, 2016
@gbrld
Copy link

gbrld commented Dec 5, 2016

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)

mount-chroot:
...
PWFILE="/home/chronos/user/crouton/pwfile"
...
            if [ -e "$PWFILE" ]; then
                passphrase="`cat $PWFILE`"
            else
                echo_tty -n "Enter encryption passphrase for $NAME: "
                [ -t 0 ] && stty -echo
                if [ -z "$passphrase" ]; then
                    read -r passphrase
                fi
                [ -t 0 ] && stty echo
                echo_tty ''
            fi
...

@dnschneid
Copy link
Owner

Much simpler to just store some long random password in the home folder, then modify mount-chroot to read it out.

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.

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)

This is also very prudent. To be clear, you have to run the scripts directly from the encrypted storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants