ibm-s390-linux / s390-tools

Tools for use with the s390 Linux kernel and device drivers
MIT License
62 stars 58 forks source link

zkey: use default benchmarked Argon2i with LUKS2 #138

Closed frank-heimes closed 2 years ago

frank-heimes commented 2 years ago

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 https://github.com/systemd/systemd/pull/14168 https://gitlab.com/cryptsetup/cryptsetup/-/issues/446 https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=924560

ifranzki commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/systemd/systemd/pull/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 commented 2 years ago

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 commented 2 years ago

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.

ifranzki commented 2 years ago

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 commented 2 years ago

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

frank-heimes commented 2 years ago

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

hoeppnerj commented 2 years ago

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