On 12/16/2016 03:31 AM, Michael Paquier wrote:
On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas <hlinn...@iki.fi> wrote:The only way to distinguish, is to know about every verifier kind there is, and check whether rolpassword looks valid as anything else than a plaintext password. And we already got tripped by a bug-of-omission on that once. If we add more verifier formats in the future, it's bound to happen again. Let's nip that source of bugs in the bud. Attached is a patch to implement what I have in mind.OK, I had a look at the patch proposed. - if (!pg_md5_encrypt(username, username, namelen, encrypted)) - elog(ERROR, "password encryption failed"); - if (strcmp(password, encrypted) == 0) - ereport(ERROR, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("password must not contain user name"))); This patch removes the only possible check for MD5 hashes that it has never been done in passwordcheck. It may be fine to remove it, but I would think that it is a good source of example regarding what could be done with MD5 hashes, though limited. So it seems to me that this check should involve as well pg_md5_encrypt on the username and compare if with the MD5 hash given by the caller.
Actually, it does still perform that check. There's a new function, plain_crypt_verify, that passwordcheck uses now. plain_crypt_verify() is intended to work with any future hash formats we might introduce in the future (including SCRAM), so that passwordcheck doesn't need to know about all the hash formats.
A simple ALTER USER role PASSWORD 'foo' causes a crash:
Ah, fixed.
+ case PASSWORD_TYPE_PLAINTEXT:
+ shadow_pass = &shadow_pass[strlen("plain:")];
+ break;
It would be a good idea to have a generic routine able to get the plain
password value. In short I think that we should reduce the amount of
locations where "plain:" prefix is hardcoded.
There is such a function included in the patch, get_plain_password(char *shadow_pass), actually. Contrib/passwordcheck uses it. I figured that in crypt.c itself, it's OK to do the above directly, but get_plain_password() is intended to be used elsewhere.
Thanks for having a look! Attached is a new version, with that bug fixed. - Heikki
0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p-2.patch
Description: invalid/octet-stream
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
