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

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Magnus Hagander <magnus(at)hagander(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Fetter <david(at)fetter(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Julian Markwort <julian(dot)markwort(at)uni-muenster(dot)de>, Stephen Frost <sfrost(at)snowman(dot)net>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Valery Popov <v(dot)popov(at)postgrespro(dot)ru>
Subject: Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
Date: 2016-12-20 12:23:51
Message-ID: a356e0d2-5330-2dd8-7faa-af8758db1be4@iki.fi
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Use-plain-prefix-for-plaintext-passwords-stored-in-p-2.patch invalid/octet-stream 22.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2016-12-20 12:55:08 Re: too low cost of Bitmap index scan
Previous Message Magnus Hagander 2016-12-20 12:22:32 Re: invalid combination of options "-D - -F t -X stream" in pg_basebackup