Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zkey: use default benchmarked Argon2i with LUKS2 #138

Closed
wants to merge 1 commit into from

Conversation

frank-heimes
Copy link
Contributor

Argon2i is not only the winner of Password Hashing Competition,
it's meanwhile also officially recommended by RFC 9106.
So for Ubuntu we moved back from '--pbkdf pbkdf2' to the default (argon2i) a while ago.

The PR is to raise the discussion if this should not be generally the case?

More details can be found the rational can be found here:
https://bugs.launchpad.net/bugs/1820049
systemd/systemd#14168
https://gitlab.com/cryptsetup/cryptsetup/-/issues/446
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=924560

@ifranzki
Copy link
Contributor

ifranzki commented Jun 13, 2022

The reason why we (still) use PBKDF2 here is documented within the comments you also remove with this PR:

Use PBKDF2 as key derivation function for LUKS2 volumes. LUKS2 uses Argon2i as default, but this might cause out-of-memory errors when multiple LUKS2 volumes are opened automatically via /etc/crypttab

Please also see here:
https://www.ibm.com/docs/en/linux-on-systems?topic=recovery-troubleshooting-problems
section "Out-of-memory errors when opening a LUKS2 volume"

@ifranzki
Copy link
Contributor

At the moment I would tend to not change this. The Out-of-memory errors mentioned above is 'real' , we have seen this in many situations. So I would like to keep the default as is, not forcing users to run into the memory failures by default.

For zkey use with secure keys, the password based key derivation function is of no or only very minimal relevance for security anyway. Secure keys are wrapped by the HSM master key, wich provides an much better protection that the wrapping by a key derived from a passphrase. So using a weaker password based key derivation function is no no security risk.

@frank-heimes
Copy link
Contributor Author

Hi, there is no doubt that there were real issues with "out-of-memory errors" - and we saw the reason and how it got faced in the code.

I think it's just worth discussing if these reasons still exist, or if were were meanwhile addressed, like esp. in:
systemd/systemd#14168
https://gitlab.com/cryptsetup/cryptsetup/-/issues/446

With older (unpatched) versions, there is of course still a danger (and I assume that the latest code was not yet picked up by everyone), but we think if going with the latest, thinks should be fine.

(Btw, I haven't expected that this PR is accepted right away - it was opened as discussion item.
And "At the moment I would tend to not change this." is of course a fair statement ...)

@ifranzki
Copy link
Contributor

Well, if the changes in cryptsetup and systemd really hinder this OOM from happening, then we might in deed consider changing zkey to no longer use PBKDF2, at least for upstream.

We then need to make sure that all distros that get this change (i.e. the s3909-tool version that includes the change), also have the changes for cryptsetup and systemd included. Can you tell which version of cryptsetup and systemd got the changes?

@xnox
Copy link
Contributor

xnox commented Jun 13, 2022

However LUKS2 allows to configure the memory requirements of Argon2i algorithm. In Ubuntu Core encryption project that also seals the key to hardware TPM, in a similar fashion, and uses Argon2i with reduced memory requirements, because pure argon2i protection does not add any additional security benefit.

For example, we often force memory requirement down to 32kib with 4 iterations, which avoids running Argon2i benchmark, and ensures low memory requirements to unlock, which is similar in resource reuirements to the legacy key derivation function. https://github.com/snapcore/snapd/blob/master/secboot/encrypt_sb.go#L49

For other contexts, which are not sealed to hardware, we pick sensible Argon2i benchmark parameters to ensure that recovery key slots are appropriately sized to the hardware in question https://github.com/snapcore/snapd/blob/master/secboot/keymgr/keymgr_luks2.go#L78

For zkey, where encryption keys are sealed to hardware module it makes sense to switch to argon2i with 4 interations and 32KiB memory requirement. This has the benefit of making the encrypted volumes future proof, when cryptsetup/kernel eventually will deprecate even more and remove the old kdf algorith. As at the moment argon2i seems to be the one that will be supported for longer into the future.

zkey/keystore.c Outdated Show resolved Hide resolved
@ifranzki
Copy link
Contributor

Could you please also update the man page and explicitly mention why the low memory options are used? I would like to make sure users understand why we don't use the default. Maybe also re-add the comment in zkey/keystore.c to explain it there also.

Suggested man page update:

For LUKS2 volumes, the generated \fBcryptsetup luksFormat\fP contains
options \fB-\-pbkdf argon2i \-\-pbkdf\-memory 32 \-\-pbkdf\-force\-iterations 4
\fP for low memory and time requirements. Using the default \fBArgon2i\fP
options might cause out-of-memory errors when multiple encrypted volumes are
unlocked automatically at boot through /etc/crypttab. Because PAES uses secure
AES keys as volume keys, the security of the key derivation function used to encrypt
the volume key in the LUKS key slots is of less relevance. 
.P

Suggestion for the comment in zkey7keystore.c:

			/*
			 * Use Argon2i as key derivation function for LUKS2
			 * volumes, but with options for low memory and time requirements. 
			 * Using the default Argon2i options might cause out-of-memory
			 * errors when multiple LUKS2 volumes are opened automatically 
			 * via /etc/crypttab
			 */

@ifranzki
Copy link
Contributor

Also, please squash all commits together into just one single commit, and sign-off the commit properly (see https://github.com/ibm-s390-linux/s390-tools/blob/master/CONTRIBUTING.md).

cryptsetup 2.1.0 requires excessive amount of RAM (1GB) to luksOpen encrypted
drives (LP: #1820049).
LUKS2 introduced support for Argon2i and Argon2id as a Password-Based Key
Derivation Function (PBKDF).
Argon2 is the winner of Password Hashing Competition and is now officially
recommended by RFC 9106.
PBKDF2 is currently used in zkey to mitigate out-of-memory errors when
multiple LUKS2 volumes are opened automatically via /etc/crypttab.

This patch is to use Argon2i (the deflaut algorithm) as key derivation function
for LUKS2 volumes, but with options for low memory and time requirements.
Using the default Argon2i options might still cause out-of-memory errors.

Signed-off-by: Frank Heimes <frank.heimes@canonical.com>
@frank-heimes
Copy link
Contributor Author

Hi Ingo, I've just incorporated the outcome of the discussions and all the changes and force pushed.
Please have a look:
dd6dd3f
https://github.com/ibm-s390-linux/s390-tools/compare/master...frank-heimes:argon2i?expand=1
Thx, Frank

@hoeppnerj
Copy link
Contributor

Since @ifranzki approved, I've pulled the changes. Thanks for the contribution @frank-heimes.

@hoeppnerj hoeppnerj closed this in 51b9504 Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants