Don't allow logging in with empty password.

Some authentication methods allowed it, others did not. In the client-side,
libpq does not even try to authenticate with an empty password, which makes
using empty passwords hazardous: an administrator might think that an
account with an empty password cannot be used to log in, because psql
doesn't allow it, and not realize that a different client would in fact
allow it. To clear that confusion and to be be consistent, disallow empty
passwords in all authentication methods.

All the authentication methods that used plaintext authentication over the
wire, except for BSD authentication, already checked that the password
received from the user was not empty. To avoid forgetting it in the future
again, move the check to the recv_password_packet function. That only
forbids using an empty password with plaintext authentication, however.
MD5 and SCRAM need a different fix:

* In stable branches, check that the MD5 hash stored for the user does not
not correspond to an empty string. This adds some overhead to MD5
authentication, because the server needs to compute an extra MD5 hash, but
it is not noticeable in practice.

* In HEAD, modify CREATE and ALTER ROLE to clear the password if an empty
string, or a password hash that corresponds to an empty string, is
specified. The user-visible behavior is the same as in the stable branches,
the user cannot log in, but it seems better to stop the empty password from
entering the system in the first place. Secondly, it is fairly expensive to
check that a SCRAM hash doesn't correspond to an empty string, because
computing a SCRAM hash is much more expensive than an MD5 hash by design,
so better avoid doing that on every authentication.

We could clear the password on CREATE/ALTER ROLE also in stable branches,
but we would still need to check at authentication time, because even if we
prevent empty passwords from being stored in pg_authid, there might be
existing ones there already.

Reported by Jeroen van der Ham, Ben de Graaff and Jelte Fennema.

Security: CVE-2017-7546

diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 43f2303b4817686ec761de016678b4f58f408ff6..4881e544391f9360c6ec71d3dc896ad63925e73e 100644 (file)
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -219,6 +219,17 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
         user.  A null password can optionally be written explicitly as
         <literal>PASSWORD NULL</literal>.
        </para>
+       <note>
+         <para>
+           Specifying an empty string will also set the password to null,
+           but that was not the case before <productname>PostgreSQL</>
+           version 10. In earlier versions, an empty string could be used,
+           or not, depending on the authentication method and the exact
+           version, and libpq would refuse to use it in any case.
+           To avoid the ambiguity, specifying an empty string should be
+           avoided.
+         </para>
+       </note>
        <para>
         The password is always stored encrypted in the system catalogs. The
         <literal>ENCRYPTED</> keyword has no effect, but is accepted for

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 0a72c2ecb3747ed6251829d91f2fc38b249046da..f2941352d7c3579bf4eb8f449406b8310a4f5a96 100644 (file)
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -384,13 +384,36 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
 
        if (password)
        {
-               /* Encrypt the password to the requested format. */
                char       *shadow_pass;
+               char       *logdetail;
 
-               shadow_pass = encrypt_password(Password_encryption, stmt->role,
-                                                                          password);
-               new_record[Anum_pg_authid_rolpassword - 1] =
-                       CStringGetTextDatum(shadow_pass);
+               /*
+                * Don't allow an empty password. Libpq treats an empty password the
+                * same as no password at all, and won't even try to authenticate. But
+                * other clients might, so allowing it would be confusing. By clearing
+                * the password when an empty string is specified, the account is
+                * consistently locked for all clients.
+                *
+                * Note that this only covers passwords stored in the database itself.
+                * There are also checks in the authentication code, to forbid an
+                * empty password from being used with authentication methods that
+                * fetch the password from an external system, like LDAP or PAM.
+                */
+               if (password[0] == '\0' ||
+                       plain_crypt_verify(stmt->role, password, "", &logdetail) == STATUS_OK)
+               {
+                       ereport(NOTICE,
+                                       (errmsg("empty string is not a valid password, clearing password")));
+                       new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
+               }
+               else
+               {
+                       /* Encrypt the password to the requested format. */
+                       shadow_pass = encrypt_password(Password_encryption, stmt->role,
+                                                                                  password);
+                       new_record[Anum_pg_authid_rolpassword - 1] =
+                               CStringGetTextDatum(shadow_pass);
+               }
        }
        else
                new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
@@ -782,13 +805,25 @@ AlterRole(AlterRoleStmt *stmt)
        /* password */
        if (password)
        {
-               /* Encrypt the password to the requested format. */
                char       *shadow_pass;
+               char       *logdetail;
 
-               shadow_pass = encrypt_password(Password_encryption, rolename,
-                                                                          password);
-               new_record[Anum_pg_authid_rolpassword - 1] =
-                       CStringGetTextDatum(shadow_pass);
+               /* Like in CREATE USER, don't allow an empty password. */
+               if (password[0] == '\0' ||
+                       plain_crypt_verify(rolename, password, "", &logdetail) == STATUS_OK)
+               {
+                       ereport(NOTICE,
+                                       (errmsg("empty string is not a valid password, clearing password")));
+                       new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
+               }
+               else
+               {
+                       /* Encrypt the password to the requested format. */
+                       shadow_pass = encrypt_password(Password_encryption, rolename,
+                                                                                  password);
+                       new_record[Anum_pg_authid_rolpassword - 1] =
+                               CStringGetTextDatum(shadow_pass);
+               }
                new_record_repl[Anum_pg_authid_rolpassword - 1] = true;
        }
 

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index dd7de7c3a4ef1a0ffbc246d63bbd4bf1ca9b806f..cb30fc7b7146f055b1115ba318f0fbb8d2a09180 100644 (file)
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -688,6 +688,24 @@ recv_password_packet(Port *port)
                                (errcode(ERRCODE_PROTOCOL_VIOLATION),
                                 errmsg("invalid password packet size")));
 
+       /*
+        * Don't allow an empty password. Libpq treats an empty password the same
+        * as no password at all, and won't even try to authenticate. But other
+        * clients might, so allowing it would be confusing.
+        *
+        * Note that this only catches an empty password sent by the client in
+        * plaintext. There's also a check in CREATE/ALTER USER that prevents an
+        * empty string from being stored as a user's password in the first place.
+        * We rely on that for MD5 and SCRAM authentication, but we still need
+        * this check here, to prevent an empty password from being used with
+        * authentication methods that check the password against an external
+        * system, like PAM, LDAP and RADIUS.
+        */
+       if (buf.len == 1)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PASSWORD),
+                                errmsg("empty password returned by client")));
+
        /* Do not echo password to logs, for security. */
        elog(DEBUG5, "received password packet");
 
@@ -2081,12 +2099,6 @@ pam_passwd_conv_proc(int num_msg, const struct pam_message **msg,
                                                 */
                                                goto fail;
                                        }
-                                       if (strlen(passwd) == 0)
-                                       {
-                                               ereport(LOG,
-                                                               (errmsg("empty password returned by client")));
-                                               goto fail;
-                                       }
                                }
                                if ((reply[i].resp = strdup(passwd)) == NULL)
                                        goto fail;
@@ -2277,6 +2289,8 @@ CheckBSDAuth(Port *port, char *user)
         */
        retval = auth_userokay(user, NULL, "auth-postgresql", passwd);
 
+       pfree(passwd);
+
        if (!retval)
                return STATUS_ERROR;
 
@@ -2407,16 +2421,12 @@ CheckLDAPAuth(Port *port)
        if (passwd == NULL)
                return STATUS_EOF;              /* client wouldn't send password */
 
-       if (strlen(passwd) == 0)
-       {
-               ereport(LOG,
-                               (errmsg("empty password returned by client")));
-               return STATUS_ERROR;
-       }
-
        if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
+       {
                /* Error message already sent */
+               pfree(passwd);
                return STATUS_ERROR;
+       }
 
        if (port->hba->ldapbasedn)
        {
@@ -2448,6 +2458,7 @@ CheckLDAPAuth(Port *port)
                        {
                                ereport(LOG,
                                                (errmsg("invalid character in user name for LDAP authentication")));
+                               pfree(passwd);
                                return STATUS_ERROR;
                        }
                }
@@ -2464,6 +2475,7 @@ CheckLDAPAuth(Port *port)
                        ereport(LOG,
                                        (errmsg("could not perform initial LDAP bind for ldapbinddn \"%s\" on server \"%s\": %s",
                                                        port->hba->ldapbinddn, port->hba->ldapserver, ldap_err2string(r))));
+                       pfree(passwd);
                        return STATUS_ERROR;
                }
 
@@ -2488,6 +2500,7 @@ CheckLDAPAuth(Port *port)
                        ereport(LOG,
                                        (errmsg("could not search LDAP for filter \"%s\" on server \"%s\": %s",
                                                        filter, port->hba->ldapserver, ldap_err2string(r))));
+                       pfree(passwd);
                        pfree(filter);
                        return STATUS_ERROR;
                }
@@ -2508,6 +2521,7 @@ CheckLDAPAuth(Port *port)
                                                                                  count,
                                                                                  filter, port->hba->ldapserver, count)));
 
+                       pfree(passwd);
                        pfree(filter);
                        ldap_msgfree(search_message);
                        return STATUS_ERROR;
@@ -2523,6 +2537,7 @@ CheckLDAPAuth(Port *port)
                        ereport(LOG,
                                        (errmsg("could not get dn for the first entry matching \"%s\" on server \"%s\": %s",
                                                        filter, port->hba->ldapserver, ldap_err2string(error))));
+                       pfree(passwd);
                        pfree(filter);
                        ldap_msgfree(search_message);
                        return STATUS_ERROR;
@@ -2543,6 +2558,7 @@ CheckLDAPAuth(Port *port)
                        ereport(LOG,
                                        (errmsg("could not unbind after searching for user \"%s\" on server \"%s\": %s",
                                                        fulluser, port->hba->ldapserver, ldap_err2string(error))));
+                       pfree(passwd);
                        pfree(fulluser);
                        return STATUS_ERROR;
                }
@@ -2553,6 +2569,7 @@ CheckLDAPAuth(Port *port)
                 */
                if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
                {
+                       pfree(passwd);
                        pfree(fulluser);
 
                        /* Error message already sent */
@@ -2573,10 +2590,12 @@ CheckLDAPAuth(Port *port)
                ereport(LOG,
                                (errmsg("LDAP login failed for user \"%s\" on server \"%s\": %s",
                                                fulluser, port->hba->ldapserver, ldap_err2string(r))));
+               pfree(passwd);
                pfree(fulluser);
                return STATUS_ERROR;
        }
 
+       pfree(passwd);
        pfree(fulluser);
 
        return STATUS_OK;
@@ -2720,17 +2739,11 @@ CheckRADIUSAuth(Port *port)
        if (passwd == NULL)
                return STATUS_EOF;              /* client wouldn't send password */
 
-       if (strlen(passwd) == 0)
-       {
-               ereport(LOG,
-                               (errmsg("empty password returned by client")));
-               return STATUS_ERROR;
-       }
-
        if (strlen(passwd) > RADIUS_MAX_PASSWORD_LENGTH)
        {
                ereport(LOG,
                                (errmsg("RADIUS authentication does not support passwords longer than %d characters", RADIUS_MAX_PASSWORD_LENGTH)));
+               pfree(passwd);
                return STATUS_ERROR;
        }
 
@@ -2756,9 +2769,15 @@ CheckRADIUSAuth(Port *port)
                 *------
                 */
                if (ret == STATUS_OK)
+               {
+                       pfree(passwd);
                        return STATUS_OK;
+               }
                else if (ret == STATUS_EOF)
+               {
+                       pfree(passwd);
                        return STATUS_ERROR;
+               }
 
                /*
                 * secret, port and identifiers either have length 0 (use default),
@@ -2775,6 +2794,7 @@ CheckRADIUSAuth(Port *port)
        }
 
        /* No servers left to try, so give up */
+       pfree(passwd);
        return STATUS_ERROR;
 }
 

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 0013ee38786d8928715bcdca9b64bbfe2f7db96b..1715c5246225662788f6dd19dbc40404ff3aee14 100644 (file)
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -71,14 +71,6 @@ get_role_password(const char *role, char **logdetail)
 
        ReleaseSysCache(roleTup);
 
-       if (*shadow_pass == '\0')
-       {
-               *logdetail = psprintf(_("User \"%s\" has an empty password."),
-                                                         role);
-               pfree(shadow_pass);
-               return NULL;                    /* empty password */
-       }
-
        /*
         * Password OK, but check to be sure we are not past rolvaliduntil
         */

diff --git a/src/test/regress/expected/password.out b/src/test/regress/expected/password.out
index bb25ad0c2cfb21508fc8920d60c5dc3fbc7d5966..393d836eada1cdf4b30892ef73ea2ab374a65b2e 100644 (file)
--- a/src/test/regress/expected/password.out
+++ b/src/test/regress/expected/password.out
@@ -75,11 +75,25 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
  regress_passwd5 | md5e73a4b11df52a6068f8b39f90be36023
 (5 rows)
 
+-- An empty password is not allowed, in any form
+CREATE ROLE regress_passwd_empty PASSWORD '';
+NOTICE:  empty string is not a valid password, clearing password
+ALTER ROLE regress_passwd_empty PASSWORD 'md585939a5ce845f1a1b620742e3c659e0a';
+NOTICE:  empty string is not a valid password, clearing password
+ALTER ROLE regress_passwd_empty PASSWORD 'SCRAM-SHA-256$4096:hpFyHTUsSWcR7O9P$LgZFIt6Oqdo27ZFKbZ2nV+vtnYM995pDh9ca6WSi120=:qVV5NeluNfUPkwm7Vqat25RjSPLkGeoZBQs6wVv+um4=';
+NOTICE:  empty string is not a valid password, clearing password
+SELECT rolpassword FROM pg_authid WHERE rolname='regress_passwd_empty';
+ rolpassword 
+-------------
+ 
+(1 row)
+
 DROP ROLE regress_passwd1;
 DROP ROLE regress_passwd2;
 DROP ROLE regress_passwd3;
 DROP ROLE regress_passwd4;
 DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd_empty;
 -- all entries should have been removed
 SELECT rolname, rolpassword
     FROM pg_authid

diff --git a/src/test/regress/sql/password.sql b/src/test/regress/sql/password.sql
index f16824372541642eeddb572debe608f149bdd918..8f8252d127f0bae3f063d0de5178cdedf873e22c 100644 (file)
--- a/src/test/regress/sql/password.sql
+++ b/src/test/regress/sql/password.sql
@@ -59,11 +59,18 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
     WHERE rolname LIKE 'regress_passwd%'
     ORDER BY rolname, rolpassword;
 
+-- An empty password is not allowed, in any form
+CREATE ROLE regress_passwd_empty PASSWORD '';
+ALTER ROLE regress_passwd_empty PASSWORD 'md585939a5ce845f1a1b620742e3c659e0a';
+ALTER ROLE regress_passwd_empty PASSWORD 'SCRAM-SHA-256$4096:hpFyHTUsSWcR7O9P$LgZFIt6Oqdo27ZFKbZ2nV+vtnYM995pDh9ca6WSi120=:qVV5NeluNfUPkwm7Vqat25RjSPLkGeoZBQs6wVv+um4=';
+SELECT rolpassword FROM pg_authid WHERE rolname='regress_passwd_empty';
+
 DROP ROLE regress_passwd1;
 DROP ROLE regress_passwd2;
 DROP ROLE regress_passwd3;
 DROP ROLE regress_passwd4;
 DROP ROLE regress_passwd5;
+DROP ROLE regress_passwd_empty;
 
 -- all entries should have been removed
 SELECT rolname, rolpassword