Support crypt parameters in login.conf
Diff Detail
Diff Detail
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 24465 Build 23272: arc lint + arc unit
Event Timeline
| lib/libcrypt/crypt.c | ||
|---|---|---|
| 105 | I realized this line shouldn't exist because the formats of the magic. I'll revert the diff to: | |
Comment Actions
(also see D15713)
| lib/libcrypt/crypt.3 | ||
|---|---|---|
| 224 | This is "traditional" crypt and caller might be expecting DES (see bug 192277). | |
| lib/libcrypt/crypt.c | ||
| 73 | What's the reasoning behind using Blowfish? (It's author, Bruce Schneier have advised to migrate to Twofish in ~2007 by the way) | |
| 80 | style: avoid hardcoding. | |
| 129 | Avoid strn*, use strl* instead. | |
Comment Actions
In the Kitchener/Waterloo hackathon we discussed the plan as three parts
- Allan is working to port the improvements from OpenBSD bcrypt back to FreeBSD.
- (This patch) allow users to overwrite the default parameters through login.conf
- Work on importing Argon2 as an alternative (and maybe scrypt if there's interest)
| lib/libcrypt/crypt.3 | ||
|---|---|---|
| 224 | Fair point do we need to default to MD5, because that isn't what the code was doing? | |
| usr.bin/login/login.conf | ||
| 26 ↗ | (On Diff #57737) | I'll restore this for this diff |
Comment Actions
But I think you are just changing the default here (which we can't do without breaking existing applications out there).
When DES is eventually removed, crypt() should return NULL to indicate an error (when salt was invalid), or use the then-default algorithm (as it's doing right now) should the salt was acceptable for the new algorithm, and with that, I think most of the change was unnecessary?
| lib/libcrypt/crypt.3 | ||
|---|---|---|
| 224 | I think it's fine to change the fallback (when DES is not available) default to anything. We only need the default be DES for legacy applications. POSIX says "The algorithm is implementation-defined." ( https://pubs.opengroup.org/onlinepubs/9699919799/functions/crypt.html ). | |
| lib/libcrypt/crypt.c | ||
| 37 | Is this needed? | |
| 82 | Please keep the blank line per style(9). | |
| 148 | Previously the switch was performed by modifying a pointer, which is typically atomic, and now it's being compared against a string. This made crypt_r no longer thread safe and we can't do this. | |
Comment Actions
But can't one already use e.g. $6$rounds=100000$xxxxx$ as salt? Or are you talking about something different?
| lib/libcrypt/crypt.3 | ||
|---|---|---|
| 265 | Good question (note that we do not set errno as POSIX suggested, and the crypt_* should probably be modified to support these). By the way I think the current lib/libcrypt/crypt.3 can land independently because it matches the current behavior. | |
Comment Actions
There is no way through the login_ functions in libutil nor in OpenPAM to select the parameters to crypt_*(). Arguably it doesn't make sense to expose the parameters in OpenPAM and the algorithm in login.conf.
Regarding the other stuff:
crypt_r() when the algorithm is set to DES is not re-entrant. The DES code is structured like the setkey/encrypt functions in the POSIX specification.
http://src.rcs.uwaterloo.ca/xref/freebsd-src/secure/lib/libcrypt/crypt-des.c#180
https://pubs.opengroup.org/onlinepubs/9699919799/functions/setkey.html
https://pubs.opengroup.org/onlinepubs/9699919799/functions/encrypt.html
We should probably set the errno variable (OpenBSD does) to be inline with the POSIX spec.