Re: pg_authid.rolpassword format (was Re: [HACKERS] Passwordidentifiers, protocol aging and SCRAM protocol)

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