password authenticaton broken?
Posted: Thu Jul 17, 2014 12:54 pm
In commit 4b92354057 the code to cut off the entered password's newline character was removed. (It was probably not obvious that that's its purpose.)
This task is now performed in a somewhat unsettling way by the generate_hash function like this:
This is however not exactly the same (albeit equivalent when you type the password on the commandline).
The problem though arises when trying to authenticate, because the -a operation only reads the passowrd, but doesn't generate a hash. That means a password is stored without a newline in the pw file, but keeps its newline when trying to authenticate, making that impossible.
I think it would be better if generate_hash() would take a `const` input, and not mess with it, and the newline-removal needs to be added back to read_user_passwd() in gradm_pw.c
gradm_sha256.c: generate_hash()
gradm_pw.c: read_user_passwd()
Commit url: https://cvsweb.grsecurity.net/?p=gradm. ... aa9c962775
(And maybe add a comment as it has nothing to do with what's suggested in the commit: making sure the password is null terminated. Any code of a form similar `x[strlen(x)] = 0` should raise a red flag.)
This task is now performed in a somewhat unsettling way by the generate_hash function like this:
- Code: Select all
pos = (char *)memchr(entry->passwd, '\n', strlen((char *)entry->passwd));
if (pos)
*pos = '\0';
This is however not exactly the same (albeit equivalent when you type the password on the commandline).
The problem though arises when trying to authenticate, because the -a operation only reads the passowrd, but doesn't generate a hash. That means a password is stored without a newline in the pw file, but keeps its newline when trying to authenticate, making that impossible.
I think it would be better if generate_hash() would take a `const` input, and not mess with it, and the newline-removal needs to be added back to read_user_passwd() in gradm_pw.c
gradm_sha256.c: generate_hash()
gradm_pw.c: read_user_passwd()
Commit url: https://cvsweb.grsecurity.net/?p=gradm. ... aa9c962775
(And maybe add a comment as it has nothing to do with what's suggested in the commit: making sure the password is null terminated. Any code of a form similar `x[strlen(x)] = 0` should raise a red flag.)