From: Michael Paquier <michael.paquier@gmail.com>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Magnus Hagander <magnus@hagander.net>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: David Steele <david@pgmasters.net>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Fetter <david@fetter.org>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Julian Markwort <julian.markwort@uni-muenster.de>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Cc: Valery Popov <v.popov@postgrespro.ru>
Subject: Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
Date: Fri, 16 Dec 2016 10:31:14 +0900
Message-ID: <20161216013114.GA1400@paquier.xyz> (raw)
In-Reply-To: <a150e7c7-7d57-bf1c-7f73-cbbfedf99243@iki.fi>
References: <21eecdb8-365b-8347-1ae2-5278d1fe798c@iki.fi>
<CAB7nPqT2UGWm0ukdomC+mG-co6nmKer62vuSGRxwtc+jS8uQPA@mail.gmail.com>
<CAB7nPqQtD7jOpYCsB4Ga=Cz+tgh4Ngac6NAV-WLs9zxaw0Uu7w@mail.gmail.com>
<762f4875-a87e-cf0b-c06b-b357294d8be1@iki.fi>
<CAB7nPqTi5jC_AbVMOGidrVy1z6t3v==vWFvdNMxnQEN-WsutKg@mail.gmail.com>
<2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi>
<CABUevEyi7uw41nzyFRh2xUzB6uOh-QmavcwDMhW0jCTTe4KavQ@mail.gmail.com>
<9a7263eb-0980-2072-4424-440bb2513dc7@iki.fi>
<CAB7nPqR4RDjr1PYKm4D+aV4gxgQduq8GWb7SmEOnvoctnvVzsw@mail.gmail.com>
<a150e7c7-7d57-bf1c-7f73-cbbfedf99243@iki.fi>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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. The new code is being careful about trying to pass
down a plain password, but it is possible to load MD5 hashes directly as
well, aka pg_dumpall.
A simple ALTER USER role PASSWORD 'foo' causes a crash:
#0 0x00000000004764d7 in heap_compute_data_size (tupleDesc=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106
106 VARATT_CAN_MAKE_SHORT(DatumGetPointer(val)))
(gdb) bt
#0 0x00000000004764d7 in heap_compute_data_size (tupleDesc=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:106
#1 0x00000000004781e9 in heap_form_tuple (tupleDescriptor=0x277f090, values=0x27504b8, isnull=0x2750550 "") at heaptuple.c:736
#2 0x00000000004784d0 in heap_modify_tuple (tuple=0x277adc8, tupleDesc=0x277f090, replValues=0x7fff1369d030, replIsnull=0x7fff1369d020 "", doReplace=0x7fff1369d010 "")
at heaptuple.c:833
#3 0x0000000000673788 in AlterRole (stmt=0x27a4f78) at user.c:845
#4 0x000000000082aa49 in standard_ProcessUtility (parsetree=0x27a4f78, queryString=0x27a43e8 "alter role ioltas password 'toto';", context=PROCESS_UTILITY_TOPLEVEL,
params=0x0, dest=0x27a5300, completionTag=0x7fff1369d5b0 "") at utility.c:711
+ 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.
> Alternatively, you could argue that we should forbid storing passwords in
> plaintext altogether. I'm OK with that, too, if that's what people prefer.
> Then you cannot have a user that can log in with both MD5 and SCRAM
> authentication, but it's certainly more secure, and it's easier to document.
At the end this may prove to be a bad idea for some developers. In local
deployments when working on a backend application with Postgres as backend,
it is actually useful to have plain passwords. At least I have found that
useful in some stuff I did many years ago.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael.paquier@gmail.com, hlinnaka@iki.fi, magnus@hagander.net, robertmhaas@gmail.com, andres@anarazel.de, peter.eisentraut@2ndquadrant.com, david@pgmasters.net, tgl@sss.pgh.pa.us, david@fetter.org, alvherre@2ndquadrant.com, julian.markwort@uni-muenster.de, sfrost@snowman.net, v.popov@postgrespro.ru
Subject: Re: pg_authid.rolpassword format (was Re: Password identifiers, protocol aging and SCRAM protocol)
In-Reply-To: <20161216013114.GA1400@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox