Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)

On 12/16/2016 03:31 AM, Michael Paquier wrote:
> On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)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