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

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