>From 25670580136d6d0cc5f7b519be2176870bca71a1 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 15 Dec 2016 14:25:54 +0200 Subject: [PATCH 1/1] Use "plain:" prefix for plaintext passwords stored in pg_authid. So far, 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. That kind of pattern matching gets more awkward when we add new kinds of verifiers, like the upcoming SCRAM verifiers. To be able to distinguish different kinds of password hashes, use a "plain:" prefix for plaintext passwords. With that, every password stored in pg_authid has either "plain:" or "md5" prefix, and future password schemes can likewise use different prefixes. Note that this doesn't change the format of MD5 hashed passwords, which is what almost everyone uses. This changes check_password_hook in an incompatible way. The 'password_type' argument is removed, and the hook is expected to call get_password_type() on the password verifier to determine its type. Moreover, when a plaintext password is passed to the hook, it will have the "plain:" prefix. That is a more subtle change! 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 | 133 +++++++++++------------ doc/src/sgml/catalogs.sgml | 4 +- doc/src/sgml/ref/create_role.sgml | 6 -- src/backend/commands/user.c | 66 ++++++------ src/backend/libpq/crypt.c | 192 +++++++++++++++++++++++++++------- src/backend/utils/misc/guc.c | 1 + src/include/commands/user.h | 13 +-- src/include/common/md5.h | 4 - src/include/libpq/crypt.h | 20 +++- 9 files changed, 273 insertions(+), 166 deletions(-) diff --git a/contrib/passwordcheck/passwordcheck.c b/contrib/passwordcheck/passwordcheck.c index a0db89b..3d06964c 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,78 @@ extern void _PG_init(void); */ static void check_password(const char *username, - const char *password, - int password_type, + const char *shadow_pass, 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 (get_password_type(shadow_pass) != 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 + */ + char *password = get_plain_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; + pfree(password); } /* all checks passed, password is ok */ diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 11c2019..abf98af 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -1320,8 +1320,8 @@ will be of the user's password concatenated to their user name. For example, if user joe has password xyzzy, PostgreSQL will store the md5 hash of - xyzzyjoe. A password that does not follow that - format is assumed to be unencrypted. + xyzzyjoe. If a password is stored unencrypted, the + column will begin with the string plain:. diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml index 38cd4c8..a3b8ed9 100644 --- a/doc/src/sgml/ref/create_role.sgml +++ b/doc/src/sgml/ref/create_role.sgml @@ -235,12 +235,6 @@ CREATE ROLE name [ [ WITH ] - - - Note that older clients might lack support for the MD5 - authentication mechanism that is needed to work with passwords - that are stored encrypted. - diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index adc6b99..e9061f8 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? */ @@ -368,11 +367,23 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt) * Call the password checking hook if there is one defined */ if (check_password_hook && password) + { + char *plain_password; + + /* + * We prefer to pass a plaintext verifier to the password hook, if + * possible, even if it's eventually stored in a hashed format, so + * that the hook can perform more extensive tests on it. + */ + plain_password = encode_password(PASSWORD_TYPE_PLAINTEXT, stmt->role, + password); + (*check_password_hook) (stmt->role, - password, - isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT, + plain_password, validUntil_datum, validUntil_null); + pfree(plain_password); + } /* * Build a tuple to insert @@ -393,17 +404,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); - } + /* Encode the password to the requested format. */ + password = encode_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 +510,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? */ @@ -743,11 +746,23 @@ AlterRole(AlterRoleStmt *stmt) * Call the password checking hook if there is one defined */ if (check_password_hook && password) + { + char *plain_password; + + /* + * We prefer to pass a plaintext verifier to the password hook, if + * possible, even if it's eventually stored in a hashed format, so + * that the hook can perform more extensive tests on it. + */ + plain_password = encode_password(PASSWORD_TYPE_PLAINTEXT, rolename, + password); + (*check_password_hook) (rolename, - password, - isMD5(password) ? PASSWORD_TYPE_MD5 : PASSWORD_TYPE_PLAINTEXT, + plain_password, validUntil_datum, validUntil_null); + pfree(plain_password); + } /* * Build an updated tuple, perusing the information just obtained @@ -804,17 +819,8 @@ 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); - } + /* Encode the password to the requested format. */ + password = encode_password(password_type, rolename, password); new_record_repl[Anum_pg_authid_rolpassword - 1] = true; } @@ -1232,7 +1238,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 7e9124f..5df049f 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,98 @@ 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, "plain:", 6) == 0 && strlen(shadow_pass) >= 6) + return PASSWORD_TYPE_PLAINTEXT; + if (strncmp(shadow_pass, "md5", 3) == 0 && strlen(shadow_pass) == MD5_PASSWD_LEN) + return PASSWORD_TYPE_MD5; + return PASSWORD_TYPE_UNKNOWN; +} + +/* + * Given a plaintext password verifier, "plain:", return the + * actual plaintext password without the prefix. The returned password is + * palloc'd. + */ +char * +get_plain_password(const char *shadow_pass) +{ + if (strlen(shadow_pass) < 6 || memcmp(shadow_pass, "plain:", 6) != 0) + elog(ERROR, "password is not a plaintext password verifier"); + + return pstrdup(&shadow_pass[strlen("plain:")]); +} + +/* + * 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. + * + * If the password follows the "plain:" format, it is interpreted + * as a plaintext password. If an MD5 hash was requested, it is hashed, + * otherwise it is returned as it is. + */ +char * +encode_password(PasswordType target_type, const char *role, + const char *password) +{ + PasswordType guessed_type = get_password_type(password); + const char *plain_password; + char *encrypted_password; + + switch (target_type) + { + case PASSWORD_TYPE_UNKNOWN: + elog(ERROR, "unknown target password type"); + + case PASSWORD_TYPE_PLAINTEXT: + switch (guessed_type) + { + case PASSWORD_TYPE_UNKNOWN: + return psprintf("plain:%s", password); + + case PASSWORD_TYPE_PLAINTEXT: + return pstrdup(password); + + case PASSWORD_TYPE_MD5: + /* We cannot convert an MD5 hash back to plaintext. Store the hash. */ + return pstrdup(password); + } + break; + + case PASSWORD_TYPE_MD5: + switch (guessed_type) + { + case PASSWORD_TYPE_UNKNOWN: + case PASSWORD_TYPE_PLAINTEXT: + encrypted_password = palloc(MD5_PASSWD_LEN + 1); + + /* strip the possible "plain:" prefix, and hash */ + if (guessed_type == PASSWORD_TYPE_PLAINTEXT) + plain_password = &password[strlen("plain:")]; + else + plain_password = password; + if (!pg_md5_encrypt(plain_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 +225,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 +296,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/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index a025117..ab71f47 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -40,6 +40,7 @@ #include "commands/trigger.h" #include "funcapi.h" #include "libpq/auth.h" +#include "libpq/crypt.h" #include "libpq/be-fsstubs.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" diff --git a/src/include/commands/user.h b/src/include/commands/user.h index 102c2a5..9cb3629 100644 --- a/src/include/commands/user.h +++ b/src/include/commands/user.h @@ -15,21 +15,10 @@ #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 */ /* 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, 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 4a04320..a29731b 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 229ce76..ed47c05 100644 --- a/src/include/libpq/crypt.h +++ b/src/include/libpq/crypt.h @@ -15,7 +15,25 @@ #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_UNKNOWN = -1, + PASSWORD_TYPE_PLAINTEXT = 0, + PASSWORD_TYPE_MD5 +} PasswordType; + +extern PasswordType get_password_type(const char *shadow_pass); +extern char *get_plain_password(const char *shadow_pass); +extern char *encode_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.10.2