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

Passphrase #28

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

Passphrase #28

wants to merge 7 commits into from

Conversation

jnvsor
Copy link
Contributor

@jnvsor jnvsor commented Dec 4, 2017

  • Has a script to set up all the files you'll need for wrapped decryption
  • Has a hook to rewrap when you change your password

Depends on #25 and #27

The entire wrapper system is brand new, but there are no breaking
changes besides the ones already in #25 and #27

All told this PR and #25, #27 were thrown together in a few days so it
needs a bit more thorough testing, but I feel confident it won't explode
unless you do something strange with it.

I might push a commit later rewriting a large portion of the readme to
give more baby-steps instructions. Perhaps the init script shoudl be part
of the install target in the makefile? (No idea how to do that in cmake)

@samuelsadok
Copy link

I'd like to mention 2 scenarios that I think would be useful to consider:

  • A single user might want to keep more than one encryption key (for different directories).
  • A single directory encryption key might need to be accessible for multiple wrap keys (for different users).

So I suggest the following directory structure:

[...]/wrap/1111111111111111/auth0
[...]/wrap/1111111111111111/salt0
[...]/wrap/1111111111111111/auth1
[...]/wrap/1111111111111111/salt1
[...]/wrap/2222222222222222/auth0
[...]/wrap/2222222222222222/salt0

where 1111111111111111 and 2222222222222222 are the wrap-key identifiers as shown by keyctl show. Optionally we could add a description[n] for each auth[n] and another description for each wrap key.

As an illustration, I wrote a small script (based off your init_wrapper_files.sh) to handle the directory structure I just described. I didn't integrate this with the pam_e4crypt.so module yet but if we want to go this path maybe we should do the utility in C too and share most of the code between the utility and the module.

@jnvsor
Copy link
Contributor Author

jnvsor commented Jan 10, 2018

A single user might want to keep more than one encryption key (for different directories).

If they're being unlocked by the same passphrase there's no practical security there. As a placebo it might even be worse.

maybe we should do the utility in C too and share most of the code between the utility and the module

Not much to share. You'd be replacing a 54 line shell script with an extra ~1000 lines of C

A single directory encryption key might need to be accessible for multiple wrap keys (for different users).

Possible. Perhaps the whole wrapped decryption thing should just be a separate utility loaded into the so as a library. But that's backwards compatible with this so I don't think it's worth worrying about until this is tested and approved.

@samuelsadok
Copy link

If they're being unlocked by the same passphrase there's no practical security there. As a placebo it might even be worse.

Key wrapping in general does not add security, but it improves usability. Consider Alice and Bob both encrypt their home directory but they also share a photos directory. Clearly all three directories should have a distinct encryption key, otherwise either Alice or Bob need to reveal their home directory's key.

This demonstrates both use cases:

  • Alice wants to wrap two keys with one password
  • the photos-directory needs to be wrapped by two separate passwords

Not much to share. You'd be replacing a 54 line shell script with an extra ~1000 lines of C

Well 1000 lines is overstated. Regarding code sharing, here's what comes to my mind:

  • deriving a key from a password or another source by matching it with the appropriate salt
  • enumerating all inner keys that are accessible from a given wrap keys
  • copy all inner keys from one wrap key to another
  • clearing out specific keys from memory (on session close)

If you share the code you can have a high confidence that both the module and the utility behave the same and you can easily debug stuff if the module fails. Besides shell code is fragile and very easy to get wrong.

But that's backwards compatible with this so I don't think it's worth worrying about until this is tested and approved.

To make future changes easy I just suggest to replace all occurrences of $WRAPPATH/$USER by $WRAPPATH/$outerkeyhash (and the corresponding tweak in the module). I can make a PR against your branch if you want. Also you might wanna put most variables in double quotes (e.g. $SALTPATH/$USER => "$SALTPATH/$USER") in case they contain spaces.

@jnvsor
Copy link
Contributor Author

jnvsor commented Jan 11, 2018

Well 1000 lines is overstated

Not really. All of the C code that's there right now does is read files and handle encryption. All the shell file does is write files and encrypt a folder (Using a pre-existing tool for that matter)

You'd need to write from scratch:

  • User input
  • Permission elevation (IE sudo)
  • Lots and lots of file/folder handling

1000 lines seems about right

@samuelsadok
Copy link

User input: scanf_s(). Permission elevation: setuid(). If you include the boilerplate code that's four lines each. I would have accepted the challenge, however now I first want to evaluate if fscrypt does what I need.

@neithernut
Copy link
Owner

@jnvsor It's been a while. Sorry for neglecting my duties.

I've finally decided to go with this PR. However, I do want to get myself a proper test-setup already. Hence, it may take a bit until I can give you a proper review.

@jnvsor
Copy link
Contributor Author

jnvsor commented Jun 11, 2018

Oh yeah I agree, encryption and PAM are two things you don't want to wing it with :)

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

Successfully merging this pull request may close these issues.

3 participants