Re: [PATCH 1/3] e4crypt: if salt is explicitly provided to add_key, then use it

[Eric Biggers <ebiggers@kernel.org>]

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

Thanks for these patches for e4crypt.

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?  And why do you want to
explicitly specify a salt?

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

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

Moreover it appears the above code should just be removed, since
get_default_salts() already handles adding salts for all ext4 filesystems.

- Eric