On 7/6/20 11:57 PM, Eric Biggers wrote: > On Mon, Jul 06, 2020 at 09:47:25PM +0200, Florian Schmaus wrote: >> Providing -S and a path to 'add_key' previously exhibit an unintuitive >> behavior: instead of using the salt explicitly provided by the user, >> e4crypt would use the salt obtained via EXT4_IOC_GET_ENCRYPTION_PWSALT >> on the path. This was because set_policy() was still called with NULL >> as salt. >> >> With this change we now remember the explicitly provided salt (if any) >> and use it as argument for set_policy(). >> >> Eventually >> >> e4crypt add_key -S s:my-spicy-salt /foo >> >> will now actually use 'my-spicy-salt' and not something else as salt >> for the policy set on /foo. >> >> Signed-off-by: Florian Schmaus <flo@xxxxxxxxxxxx> > > Thanks for these patches for e4crypt. Thanks for your feedback. > Note that e4crypt is in maintenance mode, and it hasn't been updated to follow > recommended security practices (e.g. using Argon2), to support the new > encryption API which fixes a lot of problems with the original one, or to > support the other filesystems that share the same encryption API. > > Instead you should use the 'fscrypt' tool: https://github.com/google/fscrypt > > What is your use case for still using e4crypt? This sounds like 'fsscrypt' is an alternative to e4crypt. If so, then I guess I have no use case for e4crypt, but simply use it because it is available. Sadly there is no fscrypt package for my distribution (Gentoo) available. Guess I have to look into that. :) Besides that, my use case is to have a e4crytped directory accessible after PAM authentication. For that I recently looked into pam_e4crypt [1]. In fact, pam_e4crypt's README mentions fscrypt. But the small size of pam_e4crypt made it look more appealing to me than fscrypt. > And why do you want to explicitly specify a salt? For some reason pam_e4crypt removed support for the EXT4_IOC_GET_ENCRYPTION_PWSALT ioctl and only supports a file as source for the salt. It took me a while to figure out that e4crypt add_key -S s:my-spicy-salt /foo would not use 'my-spicy-salt' for /foo. This is an attempt to fix that. >> --- >> misc/e4crypt.8.in | 4 +++- >> misc/e4crypt.c | 8 +++++++- >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/misc/e4crypt.8.in b/misc/e4crypt.8.in >> index 75b968a0..32fbd444 100644 >> --- a/misc/e4crypt.8.in >> +++ b/misc/e4crypt.8.in >> @@ -48,7 +48,9 @@ values are 4, 8, 16, and 32. >> If one or more directory paths are specified, e4crypt will try to >> set the policy of those directories to use the key just added by the >> .B add_key >> -command. >> +command. If a salt was explicitly specified, then it will be used >> +by the policy of those directories. Otherwise a directory-specific >> +default salt will be used. > > This description isn't quite correct. The salt is a value used to derive the > encryption key; it's not part of the encryption policy itself. Noted. >> .TP >> .B e4crypt get_policy \fIpath\fR ... >> Print the policy for the directories specified on the command line. >> diff --git a/misc/e4crypt.c b/misc/e4crypt.c >> index 2ae6254a..c82c6f8f 100644 >> --- a/misc/e4crypt.c >> +++ b/misc/e4crypt.c >> @@ -652,6 +652,7 @@ static void do_help(int argc, char **argv, const struct cmd_desc *cmd); >> static void do_add_key(int argc, char **argv, const struct cmd_desc *cmd) >> { >> struct salt *salt; >> + struct salt *explicit_salt = NULL; >> char *keyring = NULL; >> int i, opt, pad = 4; >> unsigned j; >> @@ -666,8 +667,13 @@ static void do_add_key(int argc, char **argv, const struct cmd_desc *cmd) >> pad = atoi(optarg); >> break; >> case 'S': >> + if (explicit_salt) { >> + fputs("May only provide -S once\n", stderr); >> + exit(1); >> + } >> /* Salt value for passphrase. */ >> parse_salt(optarg, 0); >> + explicit_salt = salt_list; >> break; >> case 'v': >> options |= OPT_VERBOSE; >> @@ -703,7 +709,7 @@ static void do_add_key(int argc, char **argv, const struct cmd_desc *cmd) >> insert_key_into_keyring(keyring, salt); >> } >> if (optind != argc) >> - set_policy(NULL, pad, argc, argv, optind); >> + set_policy(explicit_salt, pad, argc, argv, optind); > > This causes a use-after-free because the memory pointed to by 'explicit_salt' > can be reallocated by add_salt(), called from parse_salt(): > > for (i = optind; i < argc; i++) > parse_salt(argv[i], PARSE_FLAGS_FORCE_FN); > > I think we shouldn't add these extra salts when a salt was explicitly specified. I actually considered this, but then decided to go for smallest possible change. The next iteration of the patch skips this if a salt was explicitly specified. > Moreover it appears the above code should just be removed, since > get_default_salts() already handles adding salts for all ext4 filesystems. I think only for the ones declared in /etc/mtab? Hence for filesystems that are not in mtab it appears sensible to keep the code. - Florian 1: https://github.com/neithernut/pam_e4crypt
Attachment:
signature.asc
Description: OpenPGP digital signature
