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

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

[Florian Schmaus]

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>
---
 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.
 .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);
 	clear_secrets();
 	exit(0);
 }
-- 
2.26.2

[PATCH 2/3] e4crypt: refactor set_policy a little

[Florian Schmaus]

Remove the superfluous 'salt' variable and simply use the functions
parameter instead.

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
---
 misc/e4crypt.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/misc/e4crypt.c b/misc/e4crypt.c
index c82c6f8f..23980073 100644
--- a/misc/e4crypt.c
+++ b/misc/e4crypt.c
@@ -344,10 +344,9 @@ static void parse_salt(char *salt_str, int flags)
 	add_salt(salt_buf, salt_len);
 }
 
-static void set_policy(struct salt *set_salt, int pad,
+static void set_policy(struct salt *salt, int pad,
 		       int argc, char *argv[], int path_start_index)
 {
-	struct salt *salt;
 	struct ext4_encryption_policy policy;
 	uuid_t	uu;
 	int fd;
@@ -366,9 +365,7 @@ static void set_policy(struct salt *set_salt, int pad,
 			perror(argv[x]);
 			exit(1);
 		}
-		if (set_salt)
-			salt = set_salt;
-		else {
+		if (!salt) {
 			if (ioctl(fd, EXT4_IOC_GET_ENCRYPTION_PWSALT,
 				  &uu) < 0) {
 				perror("EXT4_IOC_GET_ENCRYPTION_PWSALT");
-- 
2.26.2

Re: [PATCH 2/3] e4crypt: refactor set_policy a little

[Eric Biggers]

On Mon, Jul 06, 2020 at 09:47:26PM +0200, Florian Schmaus wrote:
> Remove the superfluous 'salt' variable and simply use the functions
> parameter instead.
> 
> Signed-off-by: Florian Schmaus <flo@geekplace.eu>
> ---
>  misc/e4crypt.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/misc/e4crypt.c b/misc/e4crypt.c
> index c82c6f8f..23980073 100644
> --- a/misc/e4crypt.c
> +++ b/misc/e4crypt.c
> @@ -344,10 +344,9 @@ static void parse_salt(char *salt_str, int flags)
>  	add_salt(salt_buf, salt_len);
>  }
>  
> -static void set_policy(struct salt *set_salt, int pad,
> +static void set_policy(struct salt *salt, int pad,
>  		       int argc, char *argv[], int path_start_index)
>  {
> -	struct salt *salt;
>  	struct ext4_encryption_policy policy;
>  	uuid_t	uu;
>  	int fd;
> @@ -366,9 +365,7 @@ static void set_policy(struct salt *set_salt, int pad,
>  			perror(argv[x]);
>  			exit(1);
>  		}
> -		if (set_salt)
> -			salt = set_salt;
> -		else {
> +		if (!salt) {
>  			if (ioctl(fd, EXT4_IOC_GET_ENCRYPTION_PWSALT,
>  				  &uu) < 0) {
>  				perror("EXT4_IOC_GET_ENCRYPTION_PWSALT");

This is wrong.  If no salt was explicitly specified, then the salt returned by
EXT4_IOC_GET_ENCRYPTION_PWSALT for the directory should be used.  There can be
multiple directories being processed.  Your patch changes the behavior so that
the default salt of the first directory is also used for all later directories.

- Eric

[PATCH 3/3] Clarify in e4crypt man page that -S is an optional argument

[Florian Schmaus]

The Backus-Naur Form (BNF) of the synopsis of 'add_key' does currently
not state that the -S parameter takes an *mandatory* argument, while
the BNF found in the commands section of the man page does. The square
brackets around the optional parameter are also missing.

synopsis:
e4crypt add_key -S [ -k keyring ] [-v] [-q] [ -p pad ] [ path ... ]

commands:
e4crypt add_key [-vq] [-S salt ] [-k keyring ] [ -p pad ] [ path ... ]

This simply copies the add_key BNF from he commands section into the
synopsis.

Signed-off-by: Florian Schmaus <flo@geekplace.eu>
---
 misc/e4crypt.8.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/e4crypt.8.in b/misc/e4crypt.8.in
index 32fbd444..d2d6c872 100644
--- a/misc/e4crypt.8.in
+++ b/misc/e4crypt.8.in
@@ -2,7 +2,7 @@
 .SH NAME
 e4crypt \- ext4 filesystem encryption utility
 .SH SYNOPSIS
-.B e4crypt add_key -S \fR[\fB -k \fIkeyring\fR ] [\fB-v\fR] [\fB-q\fR] \fR[\fB -p \fIpad\fR ] [ \fIpath\fR ... ]
+.B e4crypt add_key \fR[\fB-vq\fR] [\fB-S\fI salt\fR ] [\fB-k \fIkeyring\fR ] [\fB -p \fIpad\fR ] [ \fIpath\fR ... ]
 .br
 .B e4crypt new_session
 .br
-- 
2.26.2

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

[Eric Biggers]

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

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

[Florian Schmaus]

[-- Attachment #1.1: Type: text/plain, Size: 5222 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 618 bytes --]

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

[Eric Biggers]

On Tue, Jul 07, 2020 at 10:36:12AM +0200, Florian Schmaus wrote:
> 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@geekplace.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.
> 

'fscrypt' comes with a PAM module pam_fscrypt which auto-unlocks directories at
login as well.

See the README (https://github.com/google/fscrypt/blob/master/README.md) and
also the Arch Linux Wiki article (https://wiki.archlinux.org/index.php/Fscrypt).

Can you get it packaged for Gentoo?

I don't have time to work on multiple redundant tools, so I've been focusing on
improving '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.
> 

I'm guessing the developer of pam_e4crypt did that because pam_e4crypt doesn't
know on what filesystem(s) the encrypted directories are located when it unlocks
them.  Note that that design only works with the deprecated encryption API, not
the new one in Linux v5.4+ that's recommended and 'fscrypt' uses by default.

'fscrypt' works a bit differently; it stores metadata files (policies and
protectors) in the ".fscrypt" directory at the root of each filesystem.
Passphrase protectors have a random salt generated and stored along with them.
So there's no need for users to explicitly specify a salt.

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

/etc/mtab is supposed to list all mounted filesystems.  Traditionally it could
become outdated, but now it's usually linked to /proc/mounts which is populated
by the kernel and thus is never outdated.

- Eric

[PATCH v2] e4crypt: if salt is explicitly provided to add_key, then use it

[Florian Schmaus]

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

Notes:
    - Clarify -S description in man page.
    - Do not store a reference to salt_list entry, as it
      could be reallocated causing a use-after-free.
    - Only parse the salts of the path arguments if no
      salt was explicitly specified.

 misc/e4crypt.8.in |  4 +++-
 misc/e4crypt.c    | 18 ++++++++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/misc/e4crypt.8.in b/misc/e4crypt.8.in
index 75b968a0..fe9372cf 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
+to derive the encryption key of those directories.  Otherwise a
+directory-specific default salt will be used.
 .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..67d25d88 100644
--- a/misc/e4crypt.c
+++ b/misc/e4crypt.c
@@ -26,6 +26,7 @@
 #include <getopt.h>
 #include <dirent.h>
 #include <errno.h>
+#include <stdbool.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -652,6 +653,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;
+	bool explicit_salt = false;
 	char *keyring = NULL;
 	int i, opt, pad = 4;
 	unsigned j;
@@ -666,8 +668,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 = true;
 			break;
 		case 'v':
 			options |= OPT_VERBOSE;
@@ -692,8 +699,9 @@ static void do_add_key(int argc, char **argv, const struct cmd_desc *cmd)
 		exit(1);
 	}
 	validate_paths(argc, argv, optind);
-	for (i = optind; i < argc; i++)
-		parse_salt(argv[i], PARSE_FLAGS_FORCE_FN);
+	if (!explicit_salt)
+		for (i = optind; i < argc; i++)
+			parse_salt(argv[i], PARSE_FLAGS_FORCE_FN);
 	printf("Enter passphrase (echo disabled): ");
 	get_passphrase(in_passphrase, sizeof(in_passphrase));
 	for (j = 0, salt = salt_list; j < num_salt; j++, salt++) {
@@ -702,8 +710,10 @@ static void do_add_key(int argc, char **argv, const struct cmd_desc *cmd)
 		generate_key_ref_str(salt);
 		insert_key_into_keyring(keyring, salt);
 	}
-	if (optind != argc)
-		set_policy(NULL, pad, argc, argv, optind);
+	if (optind != argc) {
+		salt = explicit_salt ? salt_list : NULL;
+		set_policy(salt, pad, argc, argv, optind);
+	}
 	clear_secrets();
 	exit(0);
 }
-- 
2.26.2

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

[Eric Biggers]

On Tue, Jul 07, 2020 at 10:27:30AM +0200, Florian Schmaus wrote:
> Providing -S and a path to 'add_key' previously exhibit an unintuitive

exhibit => exhibited

> 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>
> ---
> 
> Notes:
>     - Clarify -S description in man page.
>     - Do not store a reference to salt_list entry, as it
>       could be reallocated causing a use-after-free.
>     - Only parse the salts of the path arguments if no
>       salt was explicitly specified.
> 
>  misc/e4crypt.8.in |  4 +++-
>  misc/e4crypt.c    | 18 ++++++++++++++----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/misc/e4crypt.8.in b/misc/e4crypt.8.in
> index 75b968a0..fe9372cf 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
> +to derive the encryption key of those directories.  Otherwise a
> +directory-specific default salt will be used.
>  .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..67d25d88 100644
> --- a/misc/e4crypt.c
> +++ b/misc/e4crypt.c
> @@ -26,6 +26,7 @@
>  #include <getopt.h>
>  #include <dirent.h>
>  #include <errno.h>
> +#include <stdbool.h>

I'd like to use <stdbool.h> too, but I'm not sure if it's allowed in e2fsprogs;
this would be the first use.  Everywhere else seems to just use int, 0, and 1.
Ted, is stdbool.h allowed in e2fsprogs?

> +	if (!explicit_salt)
> +		for (i = optind; i < argc; i++)
> +			parse_salt(argv[i], PARSE_FLAGS_FORCE_FN);

There should be braces at the outer level (following Linux kernel coding style):

	if (!explicit_salt) {
		for (i = optind; i < argc; i++)
			parse_salt(argv[i], PARSE_FLAGS_FORCE_FN);
	}


Otherwise this patch looks fine.

Hopefully people aren't depending on this bug being present.

- Eric

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

[Theodore Y. Ts'o]

On Tue, Jul 07, 2020 at 10:27:30AM +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>

Applied, with the spell correction Eric pointed out.

	      	  		   	- Ted