On Thu, Dec 15, 2016 at 9:48 PM, Heikki Linnakangas <hlinn...@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