Refactor the code for verifying user's password.

hlinnaka · hlinnaka · commit e7f051b8f9a6 · 2016-12-12T12:48:13.000+02:00
Split md5_crypt_verify() into three functions: * get_role_password() to fetch user's password from pg_authid, and check its expiration. * md5_crypt_verify() to check an MD5 authentication challenge * plain_crypt_verify() to check a plaintext password. get_role_password() will be needed as a separate function by the upcoming SCRAM authentication patch set. Most of the remaining functionality in md5_crypt_verify() was different for MD5 and plaintext authentication, so split that for readability. While we're at it, simplify the *_crypt_verify functions by using stack-allocated buffers to hold the temporary MD5 hashes, instead of pallocing. Reviewed by Michael Paquier. Discussion: https://www.postgresql.org/message-id/3029e460-d47c-710e-507e-d8ba759d7cbb@iki.fi
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
@@ -704,6 +704,7 @@ CheckMD5Auth(Port *port, char **logdetail)
 {
 	char		md5Salt[4];		/* Password salt */
 	char	   *passwd;
+	char	   *shadow_pass;
 	int			result;
 
 	if (Db_user_namespace)
@@ -722,12 +723,16 @@ CheckMD5Auth(Port *port, char **logdetail)
 	sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
 
 	passwd = recv_password_packet(port);
-
 	if (passwd == NULL)
 		return STATUS_EOF;		/* client wouldn't send password */
 
-	result = md5_crypt_verify(port->user_name, passwd, md5Salt, 4, logdetail);
+	result = get_role_password(port->user_name, &shadow_pass, logdetail);
+	if (result == STATUS_OK)
+		result = md5_crypt_verify(port->user_name, shadow_pass, passwd,
+								  md5Salt, 4, logdetail);
 
+	if (shadow_pass)
+		pfree(shadow_pass);
 	pfree(passwd);
 
 	return result;
@@ -743,16 +748,21 @@ CheckPasswordAuth(Port *port, char **logdetail)
 {
 	char	   *passwd;
 	int			result;
+	char	   *shadow_pass;
 
 	sendAuthRequest(port, AUTH_REQ_PASSWORD, NULL, 0);
 
 	passwd = recv_password_packet(port);
-
 	if (passwd == NULL)
 		return STATUS_EOF;		/* client wouldn't send password */
 
-	result = md5_crypt_verify(port->user_name, passwd, NULL, 0, logdetail);
+	result = get_role_password(port->user_name, &shadow_pass, logdetail);
+	if (result == STATUS_OK)
+		result = plain_crypt_verify(port->user_name, shadow_pass, passwd,
+									logdetail);
 
+	if (shadow_pass)
+		pfree(shadow_pass);
 	pfree(passwd);
 
 	return result;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
@@ -30,28 +30,28 @@
 
 
 /*
- * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ * Fetch stored password for a user, for authentication.
  *
- * 'client_pass' is the password response given by the remote user.  If
- * 'md5_salt' is not NULL, it is a response to an MD5 authentication
- * challenge, with the given salt.  Otherwise, it is a plaintext password.
+ * Returns STATUS_OK on success.  On error, returns STATUS_ERROR, and stores
+ * a palloc'd string describing the reason, for the postmaster log, in
+ * *logdetail.  The error reason should *not* be sent to the client, to avoid
+ * giving away user information!
  *
- * In the error case, optionally store a palloc'd string at *logdetail
- * that will be sent to the postmaster log (but not the client).
+ * If the password is expired, it is still returned in *shadow_pass, but the
+ * return code is STATUS_ERROR.  On other errors, *shadow_pass is set to
+ * NULL.
  */
 int
-md5_crypt_verify(const char *role, char *client_pass,
-				 char *md5_salt, int md5_salt_len, char **logdetail)
+get_role_password(const char *role, char **shadow_pass, char **logdetail)
 {
 	int			retval = STATUS_ERROR;
-	char	   *shadow_pass,
-			   *crypt_pwd;
 	TimestampTz vuntil = 0;
-	char	   *crypt_client_pass = client_pass;
 	HeapTuple	roleTup;
 	Datum		datum;
 	bool		isnull;
 
+	*shadow_pass = NULL;
+
 	/* Get role info from pg_authid */
 	roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(role));
 	if (!HeapTupleIsValid(roleTup))
@@ -70,7 +70,7 @@ md5_crypt_verify(const char *role, char *client_pass,
 							  role);
 		return STATUS_ERROR;	/* user has no password */
 	}
-	shadow_pass = TextDatumGetCString(datum);
+	*shadow_pass = TextDatumGetCString(datum);
 
 	datum = SysCacheGetAttr(AUTHNAME, roleTup,
 							Anum_pg_authid_rolvaliduntil, &isnull);
@@ -79,104 +79,151 @@ md5_crypt_verify(const char *role, char *client_pass,
 
 	ReleaseSysCache(roleTup);
 
-	if (*shadow_pass == '\0')
+	if (**shadow_pass == '\0')
 	{
 		*logdetail = psprintf(_("User \"%s\" has an empty password."),
 							  role);
+		pfree(*shadow_pass);
+		*shadow_pass = NULL;
 		return STATUS_ERROR;	/* empty password */
 	}
 
 	/*
-	 * Compare with the encrypted or plain password depending on the
-	 * authentication method being used for this connection.  (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.)
+	 * Password OK, now check to be sure we are not past rolvaliduntil
 	 */
-	if (md5_salt)
+	if (isnull)
+		retval = STATUS_OK;
+	else if (vuntil < GetCurrentTimestamp())
 	{
-		/* MD5 authentication */
-		Assert(md5_salt_len > 0);
-		crypt_pwd = palloc(MD5_PASSWD_LEN + 1);
-		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))
-			{
-				pfree(crypt_pwd);
-				return STATUS_ERROR;
-			}
-		}
-		else
+		*logdetail = psprintf(_("User \"%s\" has an expired password."),
+							  role);
+		retval = STATUS_ERROR;
+	}
+	else
+		retval = STATUS_OK;
+
+	return retval;
+}
+
+/*
+ * Check MD5 authentication response, and return STATUS_OK or STATUS_ERROR.
+ *
+ * 'shadow_pass' is the user's correct password or password hash, as stored
+ * in pg_authid.rolpassword.
+ * 'client_pass' is the response given by the remote user to the MD5 challenge.
+ * 'md5_salt' is the salt used in the MD5 authentication challenge.
+ *
+ * In the error case, optionally store a palloc'd string at *logdetail
+ * that will be sent to the postmaster log (but not the client).
+ */
+int
+md5_crypt_verify(const char *role, const char *shadow_pass,
+				 const char *client_pass,
+				 const char *md5_salt, int md5_salt_len,
+				 char **logdetail)
+{
+	int			retval;
+	char		crypt_pwd[MD5_PASSWD_LEN + 1];
+	char		crypt_pwd2[MD5_PASSWD_LEN + 1];
+
+	Assert(md5_salt_len > 0);
+
+	/*
+	 * Compute the correct answer for the MD5 challenge.
+	 *
+	 * We do not bother setting logdetail for any pg_md5_encrypt failure
+	 * 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))
 		{
-			/* stored password is plain, double-encrypt */
-			char	   *crypt_pwd2 = palloc(MD5_PASSWD_LEN + 1);
-
-			if (!pg_md5_encrypt(shadow_pass,
-								role,
-								strlen(role),
-								crypt_pwd2))
-			{
-				pfree(crypt_pwd);
-				pfree(crypt_pwd2);
-				return STATUS_ERROR;
-			}
-			if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
-								md5_salt, md5_salt_len,
-								crypt_pwd))
-			{
-				pfree(crypt_pwd);
-				pfree(crypt_pwd2);
-				return STATUS_ERROR;
-			}
-			pfree(crypt_pwd2);
+			return STATUS_ERROR;
 		}
 	}
 	else
 	{
-		/* Client sent password in plaintext */
-		if (isMD5(shadow_pass))
+		/* stored password is plain, double-encrypt */
+		if (!pg_md5_encrypt(shadow_pass,
+							role,
+							strlen(role),
+							crypt_pwd2))
 		{
-			/* Encrypt user-supplied password to match stored MD5 */
-			crypt_client_pass = palloc(MD5_PASSWD_LEN + 1);
-			if (!pg_md5_encrypt(client_pass,
-								role,
-								strlen(role),
-								crypt_client_pass))
-			{
-				pfree(crypt_client_pass);
-				return STATUS_ERROR;
-			}
+			return STATUS_ERROR;
+		}
+		if (!pg_md5_encrypt(crypt_pwd2 + strlen("md5"),
+							md5_salt, md5_salt_len,
+							crypt_pwd))
+		{
+			return STATUS_ERROR;
 		}
-		crypt_pwd = shadow_pass;
 	}
 
-	if (strcmp(crypt_client_pass, crypt_pwd) == 0)
+	if (strcmp(client_pass, crypt_pwd) == 0)
+		retval = STATUS_OK;
+	else
 	{
-		/*
-		 * Password OK, now check to be sure we are not past rolvaliduntil
-		 */
-		if (isnull)
-			retval = STATUS_OK;
-		else if (vuntil < GetCurrentTimestamp())
+		*logdetail = psprintf(_("Password does not match for user \"%s\"."),
+							  role);
+		retval = STATUS_ERROR;
+	}
+
+	return retval;
+}
+
+/*
+ * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ *
+ * 'shadow_pass' is the user's correct password or password hash, as stored
+ * in pg_authid.rolpassword.
+ * 'client_pass' is the password given by the remote user.
+ *
+ * In the error case, optionally store a palloc'd string at *logdetail
+ * that will be sent to the postmaster log (but not the client).
+ */
+int
+plain_crypt_verify(const char *role, const char *shadow_pass,
+				   const char *client_pass,
+				   char **logdetail)
+{
+	int			retval;
+	char		crypt_client_pass[MD5_PASSWD_LEN + 1];
+
+	/*
+	 * Client sent password in plaintext.  If we have an MD5 hash stored, hash
+	 * the password the client sent, and compare the hashes.  Otherwise
+	 * compare the plaintext passwords directly.
+	 */
+	if (isMD5(shadow_pass))
+	{
+		if (!pg_md5_encrypt(client_pass,
+							role,
+							strlen(role),
+							crypt_client_pass))
 		{
-			*logdetail = psprintf(_("User \"%s\" has an expired password."),
-								  role);
-			retval = STATUS_ERROR;
+			/*
+			 * 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;
 		}
-		else
-			retval = STATUS_OK;
+		client_pass = crypt_client_pass;
 	}
+
+	if (strcmp(client_pass, shadow_pass) == 0)
+		retval = STATUS_OK;
 	else
+	{
 		*logdetail = psprintf(_("Password does not match for user \"%s\"."),
 							  role);
-
-	if (crypt_pwd != shadow_pass)
-		pfree(crypt_pwd);
-	if (crypt_client_pass != client_pass)
-		pfree(crypt_client_pass);
+		retval = STATUS_ERROR;
+	}
 
 	return retval;
 }
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
@@ -15,7 +15,12 @@
 
 #include "datatype/timestamp.h"
 
-extern int md5_crypt_verify(const char *role, char *client_pass,
-				 char *md5_salt, int md5_salt_len, char **logdetail);
+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,
+				 int md5_salt_len, char **logdetail);
+extern int plain_crypt_verify(const char *role, const char *shadow_pass,
+				   const char *client_pass, char **logdetail);
 
 #endif
