From 30be98cf09e8807d477827257a1e55c979dbe877 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas 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