Repository navigation
crypt UPDATE replace libcrypt with internal password hashing based on OpenSSL/mbedTLS - #646
HanzlikPetr wants to merge 2 commits into
Conversation
fff806b to
2743f46
Compare
| /** | ||
| * @brief Parsed setting string. | ||
| */ | ||
| struct nc_crypt_setting { | ||
| enum nc_crypt_alg alg; /**< digest algorithm */ | ||
| unsigned long rounds; /**< SHA-crypt rounds */ | ||
| int rounds_custom; /**< "rounds=" was present in the setting */ | ||
| char salt[NC_CRYPT_SHA_SALT_MAX + 1]; /**< salt, truncated and NUL-terminated */ | ||
| size_t salt_len; /**< salt length */ | ||
| const char *stored; /**< stored digest part of the setting, NULL if absent */ | ||
| }; |
There was a problem hiding this comment.
Just to clarify, is there a specific reason to move nc_crypt_setting to crypt_p.h? It's only used locally within crypt.c, and moving it would also require moving the NC_CRYPT_SHA_SALT_MAX macro. Should I move both of them to crypt_p.h?
| * @return 0 on success. | ||
| * @return -2 on unsupported hash id. | ||
| * @return -3 on malformed setting. | ||
| */ |
There was a problem hiding this comment.
Pointless to have two different retvals when there is only 1 caller of this that doesn't differentiate them anyways, noticed it just because I found the -1 missing quite odd. See comment in crypt_p.h
| /** | ||
| * @brief One operand of a stretching round. | ||
| */ | ||
| struct nc_crypt_feed { | ||
| const void *data; /**< operand bytes */ | ||
| size_t len; /**< operand length */ | ||
| }; |
There was a problem hiding this comment.
Possibly also move to crypt_p.h
There was a problem hiding this comment.
Same question as above.
| * @return 0 on success. | ||
| * @return -1 on invalid arguments. | ||
| * @return -2 on unsupported hash id. | ||
| * @return -3 on malformed setting (including invalid salt/digest characters). | ||
| * @return -4 if the output buffer is too small. | ||
| * @return -5 on internal (digest backend) failure. |
There was a problem hiding this comment.
Just return -1 on error and don't deal with the other errors, just make sure to log them as errors properly - I don't think the callers care about the specific return values anyways.
| ret = nc_crypt(received_pw, stored_pw, &cdata); | ||
| if (ret) { | ||
| ERR(NULL, "Verifying the password failed (%s).", nc_crypt_strerr(ret)); |
There was a problem hiding this comment.
Probably the biggest problem with the current state of the PR:
This function also verifies hashes from /etc/shadow, when local-users-supported is disabled. Those aren't iana-crypt-hash values. Their format comes from the system libcrypt.
I don't think that adding more algorithms to nc_crypt is realistic, because OpenSSL/MbedTLS may not implement them at all.
I see two options:
- Split by source: config passwords use
nc_crypt, and/etc/shadowkeepscrypt_r()underHAVE_SHADOWonly. Easy, but libcrypt stays for those builds. - PAM only for system users: drop direct shadow verification and run password and kbdint through PAM. This removes libcrypt completely, but there's no system password auth without PAM.
Need to discuss and decide which way to go.
3 participants
Motivation
Password hashing for SSH authentication (storing clear-text passwords and verifying
stored hashes) was dependency on the system
crypt_r(3)implementation.This caused several problems:
libcrypt(with special-casing for QNX),complicating the build on platforms without it (e.g. musl, Windows);
crypt_r()fallback had to be maintained incompat, which serializedcrypt(3)behind a global mutex;Since the library already depends on OpenSSL or mbedTLS for SSH/TLS, the hashing
is now implemented internally on top of these crypto backends.
Implementation
New internal
nc_crypt()API:src/crypt.c- algorithm core implementing the md5-crypt ("$1$"),SHA-256-crypt ("$5$") and SHA-512-crypt ("$6$") password hashing schemes,
including "rounds=" support and crypt base64 encoding;
src/crypt_openssl.c/src/crypt_mbedtls.c- digest backends selected bythe same CMake option as the SSH/TLS backend;
src/crypt_p.h- internal API with full documentation of the supportedsetting format and the deliberate deviations from crypt(3).
The algorithm output is byte-identical to the reference glibc implementation,
so hashes generated by crypt(3) (in /etc/shadow, etc.) verify unchanged.
Improvements over
crypt_r():Behavior changes
type ("$1$", "$5$", "$6$"). Hashes in other system-specific formats (e.g.
bcrypt "$2b$", yescrypt "$y$", DES) now fail to verify with a logged error.
"rounds=" outside the format range [1000, 999999999]) are rejected instead of
being interpreted leniently. glibc-generated hashes are not affected.
Build system
libcryptdetection and the QNX login-library special case removed fromCMakeLists.txtandUseCompat.cmake;crypt_r()compat fallback andcrypt.hcompat handling removed;crypt(3)removed from the README requirements.Tests
tests/test_crypt.ccmocka test with reference vectors for all threeschemes, generated by the reference glibc
crypt_r(3). Covers salttruncation, "rounds=", empty salt, '$' in the password, non-ASCII passwords,
wrong-password verification, corrupted stored hashes and invalid settings;