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@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