Page MenuHomeFreeBSD

Bug 182518: Support crypt parameters in login.conf
Needs RevisionPublic

Authored by ali_mashtizadeh.com on May 22 2019, 10:52 PM.

Details

Summary

Support crypt parameters in login.conf

Test Plan

Tested with different login.conf parameters

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24465
Build 23272: arc lint + arc unit

Event Timeline

  • Update default rounds for bcrypt
ali_mashtizadeh.com retitled this revision from Change default hash function and support crypt parameters in login.conf to Bug 182518: Change default hash function and support crypt parameters in login.conf.May 22 2019, 11:06 PM
lib/libcrypt/crypt.c
105

I realized this line shouldn't exist because the formats of the magic. I'll revert the diff to:
crypt_default_magic[0] = '\0';

delphij requested changes to this revision.May 23 2019, 5:24 PM

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

This revision now requires changes to proceed.May 23 2019, 5:24 PM

In the Kitchener/Waterloo hackathon we discussed the plan as three parts

  1. Allan is working to port the improvements from OpenBSD bcrypt back to FreeBSD.
  2. (This patch) allow users to overwrite the default parameters through login.conf
  3. 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

  • Making requested changes
ali_mashtizadeh.com retitled this revision from Bug 182518: Change default hash function and support crypt parameters in login.conf to Bug 182518: Support crypt parameters in login.conf.May 24 2019, 3:43 AM
ali_mashtizadeh.com edited the summary of this revision. (Show Details)
ali_mashtizadeh.com edited the test plan for this revision. (Show Details)
ali_mashtizadeh.com marked an inline comment as done and an inline comment as not done.May 24 2019, 3:59 AM
ali_mashtizadeh.com updated this revision to Diff 57828.
  • strn -> strl
delphij requested changes to this revision.May 24 2019, 4:33 PM

In the Kitchener/Waterloo hackathon we discussed the plan as three parts

  1. (This patch) allow users to overwrite the default parameters through login.conf

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.

This revision now requires changes to proceed.May 24 2019, 4:33 PM

This version does not change the default, but allows users to change the parameters.

lib/libcrypt/crypt.3
265

This section of the man page should also reflect that this is compatible with posix semantics?

lib/libcrypt/crypt.c
37

I'll remove this

82

Will do

This version does not change the default, but allows users to change the parameters.

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.

This version does not change the default, but allows users to change the parameters.

But can't one already use e.g. $6$rounds=100000$xxxxx$ as salt? Or are you talking about something different?

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.