Allow argon2id (instead of PBKDF2) for native encryption?

[trentbuck]

Describe the feature would like to see added to OpenZFS

I was just reading https://mjg59.dreamwidth.org/66429.html which is about LUKS.
Apparently if your passphrase KDF is PBKDF2 or argon2i, Bad Guys can brute-force it using GPGPUs.
But if your KDF is argon2id, that's harder, because argon2id is expensive for both CPU and RAM.
After five minutes reading "man zfsprops", and looking for existing issues, it seems like right now PBKDF2 is the only option for ZFS.
How viable is it to

1. offer argon2id as a KDF; and
2. eventually make it the default KDF?

How will this feature improve OpenZFS?

Make it harder for Bad Guys to brute-force decrypt a dataset encrypted with keyformat=passphrase

Additional context

This is really outside my personal skillset.
I expect the initial response will be

1. if the Bad Guys attacking you can afford $1M of GPGPUs, you're probably screwed no matter what
2. if you really care, use keyformat=raw and do your own argon2id KDF by hand using a custom script

Maybe that's a reasonable answer.
I think it's useful to at least have the discussion visible somewhere, and I couldn't find an existing ticket.

[KorewaKiyo]

As I understand it, PBKDF2 and Balloon are the only KDFs approved by NIST (5.1.1.2 in SP.800.63b), if that's maybe why one was chosen and no others implemented?

However, memory is cheap these days, as are relatively high-memory GPUs, and this presents a problem for use of a low-memory KDF such as PBKDF2, so this issue really should be addressed, either by changing the default KDF or by allowing a choice at least between PBKDF2 and Balloon if not Argon2id as well.

As a side note: this was raised over 2 years ago, and has had no movement, #10764

[mcmilk]

I will dig into this and provide some pull request for it then... but this will need some time.
Of cause, there is rfc9106 ... and it will become standards track I think ;)

We will need also some equivalent for this: cryptsetup luksConvertKey /dev/disk --pbkdf argon2id ...
And of cause: I would like to have passwort slots like it's done within LUKS.

Current TODO list:

• add argon2id with needed cost parameter for ram/cpu
• add keyslots for the real masterkey
• is there something I forgot?

@trentbuck - when you have some ideas how the parameter should look like, I am open for this.
I will implement it afterwards, when we have the documentation for it.

[nabijaczleweli]

If you just want argon2id, here's a patch that works; you'd need to parameterise it deeper for posting, but I've dealt with enough ZFS property code to not care.

The correct fix, rather than chasing the high of stronger KDFs, is to just put the 32-byte wrapping key in the TPM.

Don't CC me here, don't CC me if you end up using this patch.

commit 0216d14018a3d814f462411d317a82d15d913448
Author: наб <[email protected]>
Date:   Tue Apr 18 14:19:02 2023 +0200

    argon2id

diff --git a/config/user-libcrypto.m4 b/config/user-libcrypto.m4
index 7293e1b0b..fd1a3d0a2 100644
--- a/config/user-libcrypto.m4
+++ b/config/user-libcrypto.m4
@@ -1,8 +1,15 @@
 dnl #
-dnl # Check for libcrypto. Used for userspace password derivation via PBKDF2.
+dnl # Check for libcrypto and libargon. Used for userspace password derivation via PBKDF2 and argon2id.
 dnl #
 AC_DEFUN([ZFS_AC_CONFIG_USER_LIBCRYPTO], [
-	ZFS_AC_FIND_SYSTEM_LIBRARY(LIBCRYPTO, [libcrypto], [openssl/evp.h], [], [crypto], [PKCS5_PBKDF2_HMAC_SHA1], [], [
-		AC_MSG_FAILURE([
-		*** evp.h missing, libssl-devel package required])])
+	ZFS_AC_FIND_SYSTEM_LIBRARY(LIBCRYPTO_SSL, [libcrypto], [openssl/evp.h], [], [crypto], [PKCS5_PBKDF2_HMAC_SHA1], [], [
+		AC_MSG_FAILURE([*** evp.h missing, libssl-devel package required])])
+
+	ZFS_AC_FIND_SYSTEM_LIBRARY(LIBCRYPTO_ARGON2, [libargon2], [argon2.h], [], [argon2], [argon2id_hash_raw], [], [
+		AC_MSG_FAILURE([*** libargon2-dev package required])])
+
+	LIBCRYPTO_CFLAGS="$LIBCRYPTO_SSL_CFLAGS $LIBCRYPTO_ARGON2_CFLAGS"
+	LIBCRYPTO_LIBS="$LIBCRYPTO_SSL_LIBS $LIBCRYPTO_ARGON2_LIBS"
+	AC_SUBST(LIBCRYPTO_CFLAGS, [])
+	AC_SUBST(LIBCRYPTO_LIBS, [])
 ])
diff --git a/lib/libzfs/libzfs_crypto.c b/lib/libzfs/libzfs_crypto.c
index 8f2a50d55..f1f07d1e4 100644
--- a/lib/libzfs/libzfs_crypto.c
+++ b/lib/libzfs/libzfs_crypto.c
@@ -26,6 +26,7 @@
 #include <signal.h>
 #include <errno.h>
 #include <openssl/evp.h>
+#include <argon2.h>
 #if LIBFETCH_DYNAMIC
 #include <dlfcn.h>
 #endif
@@ -779,6 +780,7 @@ derive_key(libzfs_handle_t *hdl, zfs_keyformat_t format, uint64_t iters,
 {
 	int ret;
 	uint8_t *key;
+	boolean_t err;
 
 	*key_out = NULL;
 
@@ -800,10 +802,19 @@ derive_key(libzfs_handle_t *hdl, zfs_keyformat_t format, uint64_t iters,
 	case ZFS_KEYFORMAT_PASSPHRASE:
 		salt = LE_64(salt);
 
-		ret = PKCS5_PBKDF2_HMAC_SHA1((char *)key_material,
-		    strlen((char *)key_material), ((uint8_t *)&salt),
-		    sizeof (uint64_t), iters, WRAPPING_KEY_LEN, key);
-		if (ret != 1) {
+		ret = 0;
+		if (0)
+			err = PKCS5_PBKDF2_HMAC_SHA1((char *)key_material,
+			    strlen((char *)key_material), ((uint8_t *)&salt),
+			    sizeof (uint64_t), iters, WRAPPING_KEY_LEN, key)
+			    != 1;
+		else
+			err = argon2_hash(iters / 10, 18, 1,
+			    key_material, strlen((char *)key_material),
+			    &salt, sizeof (uint64_t), key, WRAPPING_KEY_LEN,
+			    NULL, 0, Argon2_id, ARGON2_VERSION_13)
+			    != ARGON2_OK;
+		if (err) {
 			ret = EIO;
 			zfs_error_aux(hdl, dgettext(TEXT_DOMAIN,
 			    "Failed to generate key from passphrase."));

[Rudd-O]

This is super important. I came here to open this ticket and I'm glad it's actually opened! Thank you.

[Rudd-O]

An important issue here is that it's probably necessary to upgrade existing keys to the new KDF when the KDF is changed. I think PBKDF2 should be considered broken out of an abundance of caution.

Maybe a feature flag could be made which, when upgraded to, upgrades all encrypted file systems to use the new, memory-hard KDF? A big part of making security easier is to make these sorts of processes as friction-free as possible — and a zpool upgrade seems like the right hook for that friction-free process.

[rincebrain]

I don't think adding another KDF option is a bad idea, but I do think concluding it's broken everywhere is a bit much without more data - certainly to the point of forcing a backward-incompatible change on every encrypted dataset on upgrade.

Also, if you're concerned, it's only passphrase-based keys that use PBKDF2 here - if you just hand load-key raw or hex keys, you can use whatever KDF you would like, and it's just using the bytes that result as the key.

[KorewaKiyo]

I agree that wildly concluding that the KDF is broken and ripping it out willy-nilly is probably an overstep, PBKDF2 is still reasonably secure, just not as secure as other KDFs against brute forcing with GPU/ASICs

[scineram]

Nothing will ever be ripped out, since the user needs to rekey the dataset.

[Rudd-O]

No one is proposing PBKDF2 be ripped out! We are proposing to move off of it as default. The title of the ticket clearly states Allow as the first word.

[PoisonFrog]

Great idea. The default really should be Argon2id. I hope this change gets implemented.

[oromenahar]

Right now I'm working to implement this with configuration params and so on. I think I can publish a preview off the work later this week. Right now I can choose between a PBKDF2 and an Argon2 derivation function and store the decision on disk using the ZFS-routines. Some configuration options (hidden and visible) are missing. I'm not sure which Argon2id paramters should be configuriable, there are several options:

• iteration
• threads
• memory consumption
• (argon version) I guess it is better to leave this option to the programmer
• I guess I'm missing some options, which I allready implemented in my work

Didn't test to change from PBKDF2 to Argon2, but I think it should work without any problem?!

I am not working on several key slots.

@mcmilk if you have some more Information or wanna share some work, I am open for for this.

I will dig into this and provide some pull request for it then... but this will need some time. Of cause, there is rfc9106 ... and it will become standards track I think ;)

We will need also some equivalent for this: cryptsetup luksConvertKey /dev/disk --pbkdf argon2id ... And of cause: I would like to have passwort slots like it's done within LUKS.

Current TODO list:

* add argon2id with needed cost parameter for ram/cpu

* add keyslots for the real masterkey

* is there something I forgot?

@trentbuck - when you have some ideas how the parameter should look like, I am open for this. I will implement it afterwards, when we have the documentation for it.

[oromenahar]

I added some configuration options for Argon2 and Argon2 itself. I used the code from @nabijaczleweli and added the necessary config options for Argon2. I published a kind of working code -> kdfargon2. The keys can be change to Argon2 but the config options are not stored on the disk after changing from PBKDF2 to Argon2. If you like you can test the code, but I didn't test much until now, just the basic functions like create a pool, add a dataset with Argon2 and/or PBKDF2, reboot and import pool, load key and change key.

There are several things to discuss I think.

[KorewaKiyo]

No one is proposing PBKDF2 be ripped out! We are proposing to move off of it as default. The title of the ticket clearly states Allow as the first word.

It sounded like it, my bad.

PBKDF2 should be considered broken out of an abundance of caution.

Maybe a feature flag could be made which, when upgraded to, upgrades all encrypted file systems to use the new, memory-hard KDF?

[digitalsignalperson]

Another consideration about PBKDF2 might be to allow for use of SHA256 or SHA512.

The implementation right now uses PKCS5_PBKDF2_HMAC_SHA1 and DEFAULT_PBKDF2_ITERATIONS=50000

According to https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pbkdf2
these are current recommendations if PBKDF2 must be used

• PBKDF2-HMAC-SHA1: 1,300,000 iterations
• PBKDF2-HMAC-SHA256: 600,000 iterations
• PBKDF2-HMAC-SHA512: 210,000 iterations

Or from https://tobtu.com/minimum-password-settings/ if "1 gpu = RTX4090", PBKDF2 increases to

• 2,000,000 (SHA1)
• 890,000 (SHA256)
• 320,000 (SHA512)

though I'm not sure what the model is to determine these numbers

[rincebrain]

Even just swapping that would have the same difficulties of introducing a new feature and incompatibility with older implementations if you do it.

I'm not saying it's not worthwhile, just that it is a caveat nonetheless, so I'd probably suggest overkilling the problem by adding several additional ones at once so we have one feature cliff, not three or four, and recommending people use raw or hex mode with whatever encoding they like if they're worried about the computational hardness of the bruteforce in the interim or without breaking compat.