From 2abe24b64311cfcf22e3891a08f947507206b1e0 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 3 Jan 2017 13:57:52 +0200
Subject: [PATCH 1/2] Replace isMD5() with a more future-proof way to check if
 pw is encrypted.

The rule has been that if pg_authid.rolpassword begins with "md5" and has
the right length, it's an MD5 hash, otherwise it's a plaintext password.
The idiom has been to use isMD5() to check for that, but that gets awkward,
when we add new kinds of verifiers, like the verifiers for SCRAM
authentication, in the pending patch set. Replace isMD5() with a new
get_password_type() function, so that when new verifier types are added, we
don't need to remember to modify every place that currently calls isMD5(),
to also recognize the new kinds of verifiers.

This doesn't change the representation of passwords in the catalogs. There
was a long discussion on pgsql-hackers on adding a "plain:" prefix to
plaintext passwords, but this patch doesn't do that yet.

For the benefit of passwordcheck contrib module, and any 3rd party modules
using password_check_hook, add a new function called "plain_crypt_verify",
which can be used to check if a password matches. This allows conveniently
guessing or "brute-forcing" the password, whether it's stored as an MD5
hash, or in the future, as some other kind of a hash or verifier.

In the passing, remove the bogus notice from the documentation of CREATE
ROLE, claiming that older clients might not work with MD5 hashed passwords.
That was written when we still had "crypt" authentication, and it was
referring to the fact that an older client might support "crypt"
authentication but not "md5". But we haven't supported "crypt" for years.
(As soon as we add a new authentication mechanism that doesn't work with
MD5 hashes, we'll need a similar notice again, though!)

Discussion: https://www.postgresql.org/message-id/2d07165c-1793-e243-a2a9-e45b624c7580@iki.fi
---
 contrib/passwordcheck/passwordcheck.c | 134 ++++++++++++++---------------
 src/backend/commands/user.c           |  40 +++------
 src/backend/libpq/crypt.c             | 155 +++++++++++++++++++++++++---------
 src/include/commands/user.h           |  17 +---
 src/include/common/md5.h              |   4 -
 src/include/libpq/crypt.h             |  18 +++-
 6 files changed, 210 insertions(+), 158 deletions(-)

diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c
index a0db89bbbf..03dfdfd0e1 100644
--- a/contrib/passwordcheck/passwordcheck.c
+++ b/contrib/passwordcheck/passwordcheck.c
@@ -21,7 +21,7 @@
 #endif
 
 #include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
 #include "fmgr.h"
 
 PG_MODULE_MAGIC;
@@ -50,87 +50,77 @@ extern void _PG_init(void);
  */
 static void
 check_password(const char *username,
-			   const char *password,
-			   int password_type,
+			   const char *shadow_pass,
+			   PasswordType password_type,
 			   Datum validuntil_time,
 			   bool validuntil_null)
 {
-	int			namelen = strlen(username);
-	int			pwdlen = strlen(password);
-	char		encrypted[MD5_PASSWD_LEN + 1];
-	int			i;
-	bool		pwd_has_letter,
-				pwd_has_nonletter;
-
-	switch (password_type)
+	if (password_type != PASSWORD_TYPE_PLAINTEXT)
 	{
-		case PASSWORD_TYPE_MD5:
-
-			/*
-			 * Unfortunately we cannot perform exhaustive checks on encrypted
-			 * passwords - we are restricted to guessing. (Alternatively, we
-			 * could insist on the password being presented non-encrypted, but
-			 * that has its own security disadvantages.)
-			 *
-			 * We only check for username = password.
-			 */
-			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")));
-			break;
-
-		case PASSWORD_TYPE_PLAINTEXT:
-
+		/*
+		 * Unfortunately we cannot perform exhaustive checks on encrypted
+		 * passwords - we are restricted to guessing. (Alternatively, we
+		 * could insist on the password being presented non-encrypted, but
+		 * that has its own security disadvantages.)
+		 *
+		 * We only check for username = password.
+		 */
+		char	   *logdetail;
+
+		if (plain_crypt_verify(username, shadow_pass, username, &logdetail) == STATUS_OK)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("password must not contain user name")));
+	}
+	else
+	{
+		/*
+		 * For unencrypted passwords we can perform better checks
+		 */
+		const char *password = shadow_pass;
+		int			pwdlen = strlen(password);
+		int			i;
+		bool		pwd_has_letter,
+					pwd_has_nonletter;
+
+		/* enforce minimum length */
+		if (pwdlen < MIN_PWD_LENGTH)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("password is too short")));
+
+		/* check if the password contains the username */
+		if (strstr(password, username))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("password must not contain user name")));
+
+		/* check if the password contains both letters and non-letters */
+		pwd_has_letter = false;
+		pwd_has_nonletter = false;
+		for (i = 0; i < pwdlen; i++)
+		{
 			/*
-			 * For unencrypted passwords we can perform better checks
+			 * isalpha() does not work for multibyte encodings but let's
+			 * consider non-ASCII characters non-letters
 			 */
-
-			/* enforce minimum length */
-			if (pwdlen < MIN_PWD_LENGTH)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("password is too short")));
-
-			/* check if the password contains the username */
-			if (strstr(password, username))
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("password must not contain user name")));
-
-			/* check if the password contains both letters and non-letters */
-			pwd_has_letter = false;
-			pwd_has_nonletter = false;
-			for (i = 0; i < pwdlen; i++)
-			{
-				/*
-				 * isalpha() does not work for multibyte encodings but let's
-				 * consider non-ASCII characters non-letters
-				 */
-				if (isalpha((unsigned char) password[i]))
-					pwd_has_letter = true;
-				else
-					pwd_has_nonletter = true;
-			}
-			if (!pwd_has_letter || !pwd_has_nonletter)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+			if (isalpha((unsigned char) password[i]))
+				pwd_has_letter = true;
+			else
+				pwd_has_nonletter = true;
+		}
+		if (!pwd_has_letter || !pwd_has_nonletter)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 				errmsg("password must contain both letters and nonletters")));
 
 #ifdef USE_CRACKLIB
-			/* call cracklib to check password */
-			if (FascistCheck(password, CRACKLIB_DICTPATH))
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("password is easily cracked")));
+		/* call cracklib to check password */
+		if (FascistCheck(password, CRACKLIB_DICTPATH))
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("password is easily cracked")));
 #endif
-			break;
-
-		default:
-			elog(ERROR, "unrecognized password type: %d", password_type);
-			break;
 	}
 
 	/* all checks passed, password is ok */
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index a3521e7757..48969d3a47 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -29,7 +29,7 @@
 #include "commands/dbcommands.h"
 #include "commands/seclabel.h"
 #include "commands/user.h"
-#include "common/md5.h"
+#include "libpq/crypt.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
 #include "utils/acl.h"
@@ -81,7 +81,6 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	ListCell   *option;
 	char	   *password = NULL;	/* user password */
 	int			password_type = Password_encryption;
-	char		encrypted_password[MD5_PASSWD_LEN + 1];
 	bool		issuper = false;	/* Make the user a superuser? */
 	bool		inherit = true; /* Auto inherit privileges? */
 	bool		createrole = false;		/* Can this user create roles? */
@@ -370,7 +369,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 	if (check_password_hook && password)
 		(*check_password_hook) (stmt->role,
 								password,
-			   isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+								get_password_type(password),
 								validUntil_datum,
 								validUntil_null);
 
@@ -393,17 +392,10 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 
 	if (password)
 	{
-		if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
-			new_record[Anum_pg_authid_rolpassword - 1] =
-				CStringGetTextDatum(password);
-		else
-		{
-			if (!pg_md5_encrypt(password, stmt->role, strlen(stmt->role),
-								encrypted_password))
-				elog(ERROR, "password encryption failed");
-			new_record[Anum_pg_authid_rolpassword - 1] =
-				CStringGetTextDatum(encrypted_password);
-		}
+		/* Encrypt the password to the requested format. */
+		password = encrypt_password(password_type, stmt->role, password);
+		new_record[Anum_pg_authid_rolpassword - 1] =
+			CStringGetTextDatum(password);
 	}
 	else
 		new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
@@ -506,7 +498,6 @@ AlterRole(AlterRoleStmt *stmt)
 	char	   *rolename = NULL;
 	char	   *password = NULL;	/* user password */
 	int			password_type = Password_encryption;
-	char		encrypted_password[MD5_PASSWD_LEN + 1];
 	int			issuper = -1;	/* Make the user a superuser? */
 	int			inherit = -1;	/* Auto inherit privileges? */
 	int			createrole = -1;	/* Can this user create roles? */
@@ -745,7 +736,7 @@ AlterRole(AlterRoleStmt *stmt)
 	if (check_password_hook && password)
 		(*check_password_hook) (rolename,
 								password,
-			   isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT,
+								get_password_type(password),
 								validUntil_datum,
 								validUntil_null);
 
@@ -804,17 +795,10 @@ AlterRole(AlterRoleStmt *stmt)
 	/* password */
 	if (password)
 	{
-		if (password_type == PASSWORD_TYPE_PLAINTEXT || isMD5(password))
-			new_record[Anum_pg_authid_rolpassword - 1] =
-				CStringGetTextDatum(password);
-		else
-		{
-			if (!pg_md5_encrypt(password, rolename, strlen(rolename),
-								encrypted_password))
-				elog(ERROR, "password encryption failed");
-			new_record[Anum_pg_authid_rolpassword - 1] =
-				CStringGetTextDatum(encrypted_password);
-		}
+		/* Encrypt the password to the requested format. */
+		password = encrypt_password(password_type, rolename, password);
+		new_record[Anum_pg_authid_rolpassword - 1] =
+			CStringGetTextDatum(password);
 		new_record_repl[Anum_pg_authid_rolpassword - 1] = true;
 	}
 
@@ -1232,7 +1216,7 @@ RenameRole(const char *oldname, const char *newname)
 
 	datum = heap_getattr(oldtuple, Anum_pg_authid_rolpassword, dsc, &isnull);
 
-	if (!isnull && isMD5(TextDatumGetCString(datum)))
+	if (!isnull && get_password_type(TextDatumGetCString(datum)) == PASSWORD_TYPE_MD5)
 	{
 		/* MD5 uses the username as salt, so just clear it on a rename */
 		repl_repl[Anum_pg_authid_rolpassword - 1] = true;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 7e9124f39e..5ec5951ece 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -1,10 +1,8 @@
 /*-------------------------------------------------------------------------
  *
  * crypt.c
- *	  Look into the password file and check the encrypted password with
- *	  the one passed in from the frontend.
- *
- * Original coding by Todd A. Brandys
+ *	  Functions for dealing with encrypted passwords stored in
+ *	  pg_authid.rolpassword.
  *
  * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
@@ -106,6 +104,61 @@ get_role_password(const char *role, char **shadow_pass, char **logdetail)
 }
 
 /*
+ * What kind of a password verifier is 'shadow_pass'?
+ */
+PasswordType
+get_password_type(const char *shadow_pass)
+{
+	if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN)
+		return PASSWORD_TYPE_MD5;
+	return PASSWORD_TYPE_PLAINTEXT;
+}
+
+/*
+ * Given a user-supplied password, convert it into a verifier of
+ * 'target_type' kind.
+ *
+ * If the password looks like a valid MD5 hash, it is stored as it is.
+ * We cannot reverse the hash, so even if the caller requested a plaintext
+ * plaintext password, the MD5 hash is returned.
+ */
+char *
+encrypt_password(PasswordType target_type, const char *role,
+				const char *password)
+{
+	PasswordType guessed_type = get_password_type(password);
+	char	   *encrypted_password;
+
+	switch (target_type)
+	{
+		case PASSWORD_TYPE_PLAINTEXT:
+			/*
+			 * We cannot convert an MD5 hash back to plaintext,
+			 * so no need to what what the input was. Just store
+			 * it as it is.
+			 */
+			return pstrdup(password);
+
+		case PASSWORD_TYPE_MD5:
+			switch (guessed_type)
+			{
+				case PASSWORD_TYPE_PLAINTEXT:
+					encrypted_password = palloc(MD5_PASSWD_LEN + 1);
+
+					if (!pg_md5_encrypt(password, role, strlen(role),
+										encrypted_password))
+						elog(ERROR, "password encryption failed");
+					return encrypted_password;
+
+				case PASSWORD_TYPE_MD5:
+					return pstrdup(password);
+			}
+	}
+
+	elog(ERROR, "unrecognized password type conversion");
+}
+
+/*
  * Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.
  *
  * 'shadow_pass' is the user's correct password or password hash, as stored
@@ -135,32 +188,40 @@ md5_crypt_verify(const char *role, const char *shadow_pass,
 	 * below: the only possible error is out-of-memory, which is unlikely, and
 	 * if it did happen adding a psprintf call would only make things worse.
 	 */
-	if (isMD5(shadow_pass))
-	{
-		/* stored password already encrypted, only do salt */
-		if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
-							md5_salt, md5_salt_len,
-							crypt_pwd))
-		{
-			return STATUS_ERROR;
-		}
-	}
-	else
+	switch(get_password_type(shadow_pass))
 	{
-		/* stored password is plain, double-encrypt */
-		if (!pg_md5_encrypt(shadow_pass,
-							role,
-							strlen(role),
-							crypt_pwd2))
-		{
-			return STATUS_ERROR;
-		}
-		if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
-							md5_salt, md5_salt_len,
-							crypt_pwd))
-		{
+		case PASSWORD_TYPE_MD5:
+			/* stored password already encrypted, only do salt */
+			if (!pg_md5_encrypt(shadow_pass + strlen("md5"),
+								md5_salt, md5_salt_len,
+								crypt_pwd))
+			{
+				return STATUS_ERROR;
+			}
+			break;
+
+		case PASSWORD_TYPE_PLAINTEXT:
+			/* stored password is plain, double-encrypt */
+			if (!pg_md5_encrypt(&shadow_pass[strlen("plain:")],
+								role,
+								strlen(role),
+								crypt_pwd2))
+			{
+				return STATUS_ERROR;
+			}
+			if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
+								md5_salt, md5_salt_len,
+								crypt_pwd))
+			{
+				return STATUS_ERROR;
+			}
+			break;
+
+		default:
+			/* unknown password hash format. */
+			*logdetail = psprintf(_("User \"%s\" has a password that cannot be used with MD5 authentication."),
+								  role);
 			return STATUS_ERROR;
-		}
 	}
 
 	if (strcmp(client_pass, crypt_pwd) == 0)
@@ -198,22 +259,36 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
 	 * the password the client sent, and compare the hashes.  Otherwise
 	 * compare the plaintext passwords directly.
 	 */
-	if (isMD5(shadow_pass))
+	switch (get_password_type(shadow_pass))
 	{
-		if (!pg_md5_encrypt(client_pass,
-							role,
-							strlen(role),
-							crypt_client_pass))
-		{
+		case PASSWORD_TYPE_MD5:
+			if (!pg_md5_encrypt(client_pass,
+								role,
+								strlen(role),
+								crypt_client_pass))
+			{
+				/*
+				 * We do not bother setting logdetail for pg_md5_encrypt failure:
+				 * the only possible error is out-of-memory, which is unlikely,
+				 * and if it did happen adding a psprintf call would only make
+				 * things worse.
+				 */
+				return STATUS_ERROR;
+			}
+			client_pass = crypt_client_pass;
+			break;
+		case PASSWORD_TYPE_PLAINTEXT:
+			shadow_pass = &shadow_pass[strlen("plain:")];
+			break;
+
+		default:
 			/*
-			 * We do not bother setting logdetail for pg_md5_encrypt failure:
-			 * the only possible error is out-of-memory, which is unlikely,
-			 * and if it did happen adding a psprintf call would only make
-			 * things worse.
+			 * This shouldn't happen. Plain "password" authentication should be
+			 * possible with any kind of stored password hash.
 			 */
+			*logdetail = psprintf(_("Password of user \"%s\" is in unrecognized format."),
+								  role);
 			return STATUS_ERROR;
-		}
-		client_pass = crypt_client_pass;
 	}
 
 	if (strcmp(client_pass, shadow_pass) == 0)
diff --git a/src/include/commands/user.h b/src/include/commands/user.h
index 102c2a5861..08037e0f81 100644
--- a/src/include/commands/user.h
+++ b/src/include/commands/user.h
@@ -12,24 +12,15 @@
 #define USER_H
 
 #include "catalog/objectaddress.h"
+#include "libpq/crypt.h"
 #include "nodes/parsenodes.h"
 #include "parser/parse_node.h"
 
-
-/*
- * Types of password, for Password_encryption GUC and the password_type
- * argument of the check-password hook.
- */
-typedef enum PasswordType
-{
-	PASSWORD_TYPE_PLAINTEXT = 0,
-	PASSWORD_TYPE_MD5
-} PasswordType;
-
-extern int	Password_encryption;	/* GUC */
+/* GUC. Is actually of type PasswordType. */
+extern int	Password_encryption;
 
 /* Hook to check passwords in CreateRole() and AlterRole() */
-typedef void (*check_password_hook_type) (const char *username, const char *password, int password_type, Datum validuntil_time, bool validuntil_null);
+typedef void (*check_password_hook_type) (const char *username, const char *shadow_pass, PasswordType password_type, Datum validuntil_time, bool validuntil_null);
 
 extern PGDLLIMPORT check_password_hook_type check_password_hook;
 
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index 4a0432076a..a29731b9bc 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -18,10 +18,6 @@
 
 #define MD5_PASSWD_LEN	35
 
-#define isMD5(passwd)	(strncmp(passwd, "md5", 3) == 0 && \
-						 strlen(passwd) == MD5_PASSWD_LEN)
-
-
 extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum);
 extern bool pg_md5_binary(const void *buff, size_t len, void *outbuf);
 extern bool pg_md5_encrypt(const char *passwd, const char *salt,
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 229ce76b61..338be28b9d 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -15,7 +15,23 @@
 
 #include "datatype/timestamp.h"
 
-extern int	get_role_password(const char *role, char **shadow_pass, char **logdetail);
+/*
+ * Types of password.
+ *
+ * This is also used for the password_encryption GUC.
+ */
+typedef enum PasswordType
+{
+	PASSWORD_TYPE_PLAINTEXT = 0,
+	PASSWORD_TYPE_MD5
+} PasswordType;
+
+extern PasswordType get_password_type(const char *shadow_pass);
+extern char *encrypt_password(PasswordType target_type, const char *role,
+				const char *password);
+
+extern int get_role_password(const char *role, char **shadow_pass,
+				  char **logdetail);
 
 extern int md5_crypt_verify(const char *role, const char *shadow_pass,
 				 const char *client_pass, const char *md5_salt,
-- 
2.11.0

