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

Attachment: 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

Reply via email to