google / libpam-policycache

Password caching module with advanced policies for PAM.
Apache License 2.0
34 stars 18 forks source link

add support for yescrypt #26

Open haraldj opened 3 years ago

haraldj commented 3 years ago

Dear Nikki VonHollen,

how do you think about adding yescrypt support as the default encryption method?

Kind regards Harald Jenny

smu-ggl commented 3 years ago

Nikki isn't working on this anymore, TTBOMK.

Given that it is the default in Debian is yescrypt now (apart from some other players, we would certainly accept a pull request to do that change. We aren't likely to do the change ourselves though (at least not yet), since we are not using it internally in policycache.

haraldj commented 3 years ago

Hi Sven Mueller,

first thanks for your reply and I'm sorry that Nikky isn't working on this anymore. I'm thinking of doing the necessary changes but IMHO this would likely be more invasive as I would switch from libscrypt to libxcrypt. May I ask what hashing algorithm you use internally because I think of dropping both crypt and SHA256 as they are considered insecure.

Kind regards Harald Jenny

ericchiang commented 3 years ago

Can you point to where in the code you'd like to change?

I am not a cryptographer, but aren't yescrypt and scrypt are key derivation functions? If we were going to change to something, wouldn't something like bcrypt be better?

Why is SHA256 insecure? google.com uses a HTTPS certificate with a SHA256 signature, for example.

haraldj commented 3 years ago

Hi Eric,

first thanks for your reply - the change in the code would affect the codebase in src/entry.h and src/entry.c where the whole crypto coding is done, I guess I will first do a POC with recoding the current algorithms for xcrypt usage.

You're right considering the KDF functionality as according to https://www.openwall.com/yescrypt/: yescrypt is a password-based key derivation function (KDF) and password hashing scheme. It builds upon Colin Percival's scrypt. and https://en.wikipedia.org/wiki/Scrypt: In cryptography, scrypt (pronounced "ess crypt"[1]) is a password-based key derivation function created by Colin Percival, originally for the Tarsnap online backup service. while https://en.wikipedia.org/wiki/Bcrypt: bcrypt is a password-hashing function designed by Niels Provos and David Mazières, based on the Blowfish cipher and presented at USENIX in 1999. On the other hand xscypt as used by most distributions declares in https://manpages.debian.org/unstable/libcrypt-dev/crypt.5.en.html: yescrypt is a scalable passphrase hashing scheme designed by Solar Designer, which is based on Colin Percival's scrypt. Recommended for new hashes. So I guess staying with yescrypt as the default password hashing scheme would stay in line with current recommendations, also when considering the limitations of bcrypt described on this page.

Considering the problem with SHA256: While the normal signature is still considered secure (although I personally prefer SHA512 or SHA3-512) the following line in https://manpages.debian.org/unstable/libcrypt-dev/crypt.5.en.html make me a little bit worried: Acceptable for new hashes. The default CPU time cost parameter is 5000, which is too low for modern hardware. So I guess while using SHA256 in password hashing per default does not pose a security risk the CPU time cost parameter would need to be adapted to provide a better protection against attacks.

So my proposal would be to add yescrypt as default while keeping scrypt and maybe also SHA256 with an increased CPU time cost parameter (not sure how high we should go), removing standard crypt and therefore maybe adding SHA512 (with increased CPU time cost parameter) and brypt as available hashes. While I'm not very eager to add new configuration options I would maybe add a hash and CPU time cost parameter option to the module so users could finetune their defaults.

Hope this answers all your questions Eric if not please come back to me.

Wish you a nice day Harald

smu-ggl commented 3 years ago

So, I had a brief look at the source and realized that for most parts, this is actually using libcrypt1/xcrypt (via libglib, g_checksum_new, which calls g_checksum_reset here: https://gitlab.gnome.org/GNOME/glib/-/blob/main/glib/gchecksum.c line 1470ff and 1494ff) - which in turn doesn't support YesCrypt.

libpam_policycache would only use scrypt when scrypt is used for the caching entry (via CacheEntryPasswordSet) - which is the case, unconditionally, with the current code. The code has a TODO to make this algo configurable. (Which was never done unfortunately.)

So to achieve your goal, you might need to patch GNOME GLib before touching libpam-policycache. Or pretty heavily change how libpam-policycache handles these hashes. I'd hate it if we would add special code to directly use xcrypt/YesCrypt if we could possibly get GLib to support it and then leverage that support.

haraldj commented 3 years ago

Hi Sven,

sorry to say but that is just one branch of the code which is used for SHA256, the current relevant code with scrypt directly uses the scrypt libscrypt_scrypt function. The most important codebase for me is the CACHE_ENTRY_ALGORITHM_CRYPT code path which uses crypt_r which is also exported by libxcrypt. Doing some prelimiary coding I was able to make libpam-policycache use CRYPT, SHA256 and SCRYPT from libxcrypt as well as adding YESCRYPT support. So I guess the better solution would be to port libpam-policycache to use all the algos directly from libxcrypt than adding only YesCrypt support to GNOME GLib. Above all this would keep libpam-policycache in line with other pam auth modules which also use libxcrypt (like pam_unix and pam_pwdfile). And it would also make it easy to support new password hashing algorithms like for example Argon when they get implemented in libxcrypt.

Bye Harald