lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <16765dd5-3686-6083-7f9b-261b51953d32@geekplace.eu>
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