On 12/09/2016 05:58 AM, Michael Paquier wrote:
One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?
While hacking on this, I came up with the attached refactoring, against
current master. I think it makes the current code more readable, anyway,
and it provides a get_role_password() function that SCRAM can use, to
look up the stored password. (This is essentially the same refactoring
that was included in the SCRAM patch set, that introduced the
get_role_details() function.)
Barring objections, I'll go ahead and commit this first.
- Heikki
>From 30be98cf09e8807d477827257a1e55c979dbe877 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Fri, 9 Dec 2016 11:49:36 +0200
Subject: [PATCH 1/1] Refactor the code for verifying user's password.
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.
---
src/backend/libpq/auth.c | 18 +++-
src/backend/libpq/crypt.c | 214 ++++++++++++++++++++++++++++------------------
src/include/libpq/crypt.h | 9 +-
3 files changed, 151 insertions(+), 90 deletions(-)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index f8bffe3..5c9ee06 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -707,6 +707,7 @@ CheckMD5Auth(Port *port, char **logdetail)
{
char md5Salt[4]; /* Password salt */
char *passwd;
+ char *shadow_pass;
int result;
if (Db_user_namespace)
@@ -720,12 +721,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;
@@ -741,16 +746,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
index b4ca174..fb6d1af 100644
--- 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);
@@ -83,100 +83,146 @@ md5_crypt_verify(const char *role, char *client_pass,
{
*logdetail = psprintf(_("User \"%s\" has an empty password."),
role);
+ *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
index 4ca8a75..229ce76 100644
--- 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
--
2.10.2
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
