Date:   Tue, 7 Jul 2020 10:36:12 +0200
From:   Florian Schmaus <flo@...kplace.eu>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/3] e4crypt: if salt is explicitly provided to add_key,
 then use it

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@...kplace.eu>
> 
> 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



Download attachment "signature.asc" of type "application/pgp-signature" (619 bytes)

Powered by blists - more mailing lists