Hi Stefan,
Thanks for r3esponding so quickly.
On 2013-12-04 11:56, Stefan Bühler wrote:
If the characters are not from this limited set, especially if the
first character is '$', then the Glibc crypt() expects '$' + id +
'$' + real-salt + '$'.
Glibc shouldn't require NUL-termination as long as there is a 3rd '$'.
I was concerned that if the stored password is malformed, crypt() could
scan arbitrarily far away beyond the end of the string. Granted, a
malicious user shouldn't ever be able to cause the stored password to be
malformed.
The Glibc API limits real-salt to 16 bytes, and id is so far at most 2
bytes long, so the NUL-terminated salt should fit into 22 bytes (making
it 32 shouldn't hurt).
For what it's worth, I used the following patch before I came across
your bug report. I guess the 256 is overkill considering your comments
about the maximum salt size of 22.
If you or the maintainer feel that making a copy of the string is not
worth it and would prefer to use your patch, I'm ok with that.
-kv
diff --git a/modules/pam_userdb/pam_userdb.c b/modules/pam_userdb/pam_userdb.c
index 37af682..04a17d6 100644
--- a/modules/pam_userdb/pam_userdb.c
+++ b/modules/pam_userdb/pam_userdb.c
@@ -214,15 +214,16 @@ user_lookup (pam_handle_t *pamh, const char *database, const char *cryptmode,
/* crypt(3) password storage */
char *cryptpw;
- char salt[2];
+ char salt[256];
+ int salt_len;
- if (data.dsize != 13) {
- compare = -2;
- } else if (ctrl & PAM_ICASE_ARG) {
+ if (ctrl & PAM_ICASE_ARG) {
compare = -2;
} else {
- salt[0] = *data.dptr;
- salt[1] = *(data.dptr + 1);
+ salt_len = sizeof(salt)-1;
+ if (data.dsize < salt_len) salt_len = data.dsize;
+ memcpy(salt, data.dptr, salt_len);
+ salt[salt_len] = 0;
cryptpw = crypt (pass, salt);
