Bug 182518 - [login.conf] Better Password Hashes

[482254ac_razorfever.net]

Summary

This patch contains:

• updated libcrypt, which includes new api: crypt_makesalt
• revised/rewritten crypt(3) manpage, detailing the uses of Modular Crypt Formats, and new crypt_makesalt api
• refactored pam_unix to use crypt_makesalt, instead of its own format
• refactored pw to support Modular Crypt Formats in login.conf

These things combined allow using Modular Crypt Formats in login.conf to be recognized by the system. Ultimately, this will allow selecting any supported crypt algorithm with whatever parameters that it supports, e.g. $2b$11$.

Contains modifications from the patch included in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=182518, rebased on -CURRENT.
git: https://github.com/derekmarcotte/freebsd/tree/dm-crypt-patch-current

Test Plan

Not sure what's appropriate.

Suggestions:

Update passwd_format to $2b$11$:

• add a user
• verify hash in master.passwd begins with $2b$11$
• verify login with pam_unix works

Change passwd_format to $6$rounds=10000$:

• change user's password
• verify hash in master.passwd begins with $6$rounds=10000$
• verify login with pam_unix works

[imp]

lib/libcrypt/crypt.3

167–197

This line renders wrong for David's name.

[482254ac_razorfever.net]

Fix accent on David's name, per imp feedback.

[482254ac_razorfever.net]

Fix loop always executing twice (in pw+pam_unix), with leaky memory.

[bcr]

There are a bunch of style issues with crypt.3. Can you run "mandoc -Tlint" and textproc/igor (from ports/packages) on it? It should give you some feedback where the problems are. For example, man pages need a line break after a sentence stop.

Thanks for the work!

lib/libutil/login_cap.3

575

s/favour/favor/
(we use american english in the project)

[delphij]

Could you please split the proposed change to smaller pieces, so it would be easier to review?

(I've commented some issues with the proposed change inline, by the way).

lib/libcrypt/crypt.3

328

Please use C99 or newer standard for definition instead of K&R main().

330

This is self-inflicted wound. Use crypt_r instead and drop all the locking (crypt is a non-thread safe wrapper of crypt_r).

338

See above.

lib/libcrypt/crypt.c

83

Why?

98

Why is this API exposed to user? ("Do not add new functionality unless an implementor cannot complete a real application without it.")

There are multiple style and coding issues, by the way, please fix.

109

You are using only 3 bytes, why 4 here?

150

Please refactor the code, use strl* instead of strn*.

157

Could you please change the code to use a separate base64 buffer and use strlcat instead of operating on the output buffer directly?

164

Just use strlcat, the trailing nul is not actually written, by the way.

165

Why?

[482254ac_razorfever.net]

This update covers the linting, and simplified example code in the libcrypt manpage.

[delphij]

lib/libcrypt/crypt.c

195

This introduced a security vulnerability.

[482254ac_razorfever.net]

lib/libcrypt/crypt.3

330

Thank you for taking the time to review. This has been updated. In fairness, this example pre-dates the crypt_r in FreeBSD by 4+ years.

[482254ac_razorfever.net]

lib/libcrypt/crypt.c

98

I will take care of the style (and strn*) items, please excuse.

With respect to the API, I think there's a good discussion to be had here.

It would seem that having the salt generation (and knowledge of what an appropriate salt is for the entire suite of algorithms supported by crypt) pushed into the consumer would be needlessly complex. It ossifies algorithm support all over the place.

This is not the only approach however, and perhaps we can take a page with the work done by the OpenBSD project in this space since the original patch was submitted:

https://www.tedunangst.com/flak/post/retiring-crypt
^ gives cert error, by intent

i.e. crypt_newhash and crypt_checkpass specifically.

Also, there's some old old discussion available on the mailing lists as well, if that interests you. I've linked into the thread here:

https://lists.freebsd.org/pipermail/freebsd-current/2014-March/048848.html

[emaste]

In fairness, this example pre-dates the crypt_r in FreeBSD by 4+ years.

Indeed - apologies that this issue has remained open for so long.

One note, for the next update you submit to Phabricator can you please include full context - i.e., with git diff -U99999 or svn diff --diff-cmd=diff -x -U999999 (see https://wiki.freebsd.org/Phabricator for more info).

[delphij]

lib/libcrypt/crypt.c

98

Thanks for the context. Is there some reason not to have the crypt functions to generate a salt on demand instead (e.g. when the passed salt is too short, which is something that we already will check, and the individual crypt function actually knows better)?

It worried me because with your new API, the callers are now required to do more work (allocating memory, check for the result, etc.).

[482254ac_razorfever.net]

lib/libcrypt/crypt.c

98

I'm not sure, it sounds like a good idea. I feel like having crypt do that automatically would be pretty surprising though, there's no precedent? (There's also no precedent for crypt_makesalt, but there are a few crypt_gensalt implementations.)

With regard to the more work, perhaps that's also a benefit:

• For the memory allocation, the example code and the updated user-land could be easily simplified by allocating a static buffer of _PASSWORD_LEN, which would be no worse than the current crypt_data constraint. The salt will never be longer than the crypt_data buffer, by definition. The retry logic could be removed, and just fail in this case. This isn't great, but it should only hit this code path if the the ABI changes under the consumer, and should be a simple if. If clients wish to protect against ABI changes (what does that even mean?) with retry logic, they could - but perhaps it's wrong-headed to have that as I've provided it.
• For the format case, I think having feedback that a format isn't supported by the crypt implementation is valuable. I imagine most people (i.e. sysadmins) specifying the crypt format in a configuration file, and the consumer utility being agnostic. How would we otherwise signal an unsupported or invalid crypt request? I don't think returning a "default" hash is a good idea (if we simply extend crypt), and a DES crypt would be particularly damaging - but would be required for backward compatibility.

I think the format validation is a strong case for this. The crypt_newhash approach also allows to signal an invalid format, but doesn't differentiate a short buffer with an invalid format. What do you think?

[482254ac_razorfever.net]

Updates remaining man styles.

[482254ac_razorfever.net]

I'm hopeful that this fixes style, and other suggestions from delphij.

[482254ac_razorfever.net]

lib/libcrypt/crypt.c

98

Any thoughts on a sensible way to move this portion forward?

I would submit a crypt_newhash approach similar to that of OpenBSD, if that was agreed to be a sensible way to proceed.

[ler]

Any chance of this being moved forward in time for the 12 branch?

[jmg]

All comments are minor.

lib/libcrypt/crypt.3

48

why was previous char * enclosed in quotes, but not this one?

62

I'd combine these two statements, into:
Password hashing is different than a standard one way hash function in that it tries to make it difficult to recover a low-entropy

128

What should be used instead?

140

concerted that it's not listed as $digit$ or $digit[variant]$, because this means that we cannot have more than 10 variants.. as $11$ will match md5-crypt per this spec. We are already up to 6...

169

this should probably be removed, as the next paragraph's It is unclear.

172

again, next paragraph It isn't clear.

233

space between 3DES and comma.

lib/libcrypt/crypt.c

102

was the addition of the semicolon intentional? this is usually left off and it looks like it may cause a compile error.

usr.sbin/pw/pw_user.c

512

might want to make this a define, like SALTBUFFER or something to replace the SALTSIZE define. There was another location this size was used.

[royce_techsolvency.com]

Minor comment - results of real-world testing of cracking resistance, both for the 11.x defaults and for those proposed by D15713.

Test platform: Ubuntu 16.04, 6x GTX 1080 GPUs, NVIDIA driver 418.43, hashcat 6.x pre-release (GitHub master, v5.1.0-820-g319bf801)

Summary (H/s = hashes per second, one complete cracking attempt):

sha512crypt, 11.x default:           Speed.#*.........:   912.9 kH/s
sha512crypt, rounds=10000 (D15713):  Speed.#*.........:   457.5 kH/s
bcrypt, cost 4 (11.x default):       Speed.#*.........:   248.1 kH/s
bcrypt, cost 11 (D15713):            Speed.#*.........:     2026 H/s
bcrypt, cost 12:                     Speed.#*.........:     1012 H/s

Full testing output here: https://gist.github.com/roycewilliams/0be79b4f531f13ae397589e7b15b6a81

As an attacker, bcrypt cost 12 is the most frustrating. :)

For future work, debdrup suggested tying work factor to local testing, similar to the 1-second GELI autotuning.

[tommi.pernila_iki.fi]

Any chance that this patch would make it into 13.0 RELEASE?