8000 Don't allow logging in with empty password. · codeimmortal/postgres@bf6b9e9 · GitHub
[go: up one dir, main page]
Skip to content

Commit bf6b9e9

Browse files
hlinnakahlinnaka
committed
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
1 parent 86524f0 commit bf6b9e9

File tree

6 files changed

+117
-38
lines changed

6 files changed

+117
-38
lines changed

‎doc/src/sgml/ref/create_role.sgml

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,17 @@ CREATE ROLE <replaceable class="PARAMETER">name</replaceable> [ [ WITH ] <replac
219219
user. A null password can optionally be written explicitly as
220220
<literal>PASSWORD NULL</literal>.
221221
</para>
222+
<note>
223+
<para>
224+
Specifying an empty string will also set the password to null,
225+
but that was not the case before <productname>PostgreSQL</>
226+
version 10. In earlier versions, an empty string could be used,
227+
or not, depending on the authentication method and the exact
228+
version, and libpq would refuse to use it in any case.
229+
To avoid the ambiguity, specifying an empty string should be
230+
avoided.
231+
</para>
232+
</note>
222233
<para>
223234
The password is always stored encrypted in the system catalogs. The
224235
<literal>ENCRYPTED</> keyword has no effect, but is accepted for

‎src/backend/commands/user.c

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -384,13 +384,36 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
384384

385385
if (password)
386386
{
387-
/* Encrypt the password to the requested format. */
388387
char *shadow_pass;
388+
char *logdetail;
389389

390-
shadow_pass = encrypt_password(Password_encryption, stmt->role,
391-
password);
392-
new_record[Anum_pg_authid_rolpassword - 1] =
393-
CStringGetTextDatum(shadow_pass);
390+
/*
391+
* Don't allow an empty password. Libpq treats an empty password the
392+
* same as no password at all, and won't even try to authenticate. But
393+
* other clients might, so allowing it would be confusing. By clearing
394+
* the password when an empty string is specified, the account is
395+
* consistently locked for all clients.
396+
*
397+
* Note that this only covers passwords stored in the database itself.
398+
* There are also checks in the authentication code, to forbid an
399+
* empty password from being used with authentication methods that
400< 6D40 span class="diff-text-marker">+
* fetch the password from an external system, like LDAP or PAM.
401+
*/
402+
if (password[0] == '\0' ||
403+
plain_crypt_verify(stmt->role, password, "", &logdetail) == STATUS_OK)
404+
{
405+
ereport(NOTICE,
406+
(errmsg("empty string is not a valid password, clearing password")));
407+
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
408+
}
409+
else
410+
{
411+
/* Encrypt the password to the requested format. */
412+
shadow_pass = encrypt_password(Password_encryption, stmt->role,
413+
password);
414+
new_record[Anum_pg_authid_rolpassword - 1] =
415+
CStringGetTextDatum(shadow_pass);
416+
}
394417
}
395418
else
396419
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
@@ -782,13 +805,25 @@ AlterRole(AlterRoleStmt *stmt)
782805
/* password */
783806
if (password)
784807
{
785-
/* Encrypt the password to the requested format. */
786808
char *shadow_pass;
809+
char *logdetail;
787810

788-
shadow_pass = encrypt_password(Password_encryption, rolename,
789-
password);
790-
new_record[Anum_pg_authid_rolpassword - 1] =
791-
CStringGetTextDatum(shadow_pass);
811+
/* Like in CREATE USER, don't allow an empty password. */
812+
if (password[0] == '\0' ||
813+
plain_crypt_verify(rolename, password, "", &logdetail) == STATUS_OK)
814+
{
815+
ereport(NOTICE,
816+
(errmsg("empty string is not a valid password, clearing password")));
817+
new_record_nulls[Anum_pg_authid_rolpassword - 1] = true;
818+
}
819+
else
820+
{
821+
/* Encrypt the password to the requested format. */
822+
shadow_pass = encrypt_password(Password_encryption, rolename,
823+
password);
824+
new_record[Anum_pg_authid_rolpassword - 1] =
825+
CStringGetTextDatum(shadow_pass);
826+
}
792827
new_record_repl[Anum_pg_authid_rolpassword - 1] = true;
793828
}
794829

‎src/backend/libpq/auth.c

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,24 @@ recv_password_packet(Port *port)
688688
(errcode(ERRCODE_PROTOCOL_VIOLATION),
689689
errmsg("invalid password packet size")));
690690

691+
/*
692+
* Don't allow an empty password. Libpq treats an empty password the same
693+
* as no password at all, and won't even try to authenticate. But other
694+
* clients might, so allowing it would be confusing.
695+
*
696+
* Note that this only catches an empty password sent by the client in
697+
* plaintext. There's also a check in CREATE/ALTER USER that prevents an
698+
* empty string from being stored as a user's password in the first place.
699+
* We rely on that for MD5 and SCRAM authentication, but we still need
700+
* this check here, to prevent an empty password from being used with
701+
* authentication methods that check the password against an external
702+
* system, like PAM, LDAP and RADIUS.
703+
*/
704+
if (buf.len == 1)
705+
ereport(ERROR,
706+
(errcode(ERRCODE_INVALID_PASSWORD),
707+
errmsg("empty password returned by client")));
708+
691709
/* Do not echo password to logs, for security. */
692710
elog(DEBUG5, "received password packet");
693711

@@ -2081,12 +2099,6 @@ pam_passwd_conv_proc(int num_msg, const struct pam_message **msg,
20812099
*/
20822100
goto fail;
20832101
}
2084-
if (strlen(passwd) == 0)
2085-
{
2086-
ereport(LOG,
2087-
(errmsg("empty password returned by client")));
2088-
goto fail;
2089-
}
20902102
}
20912103
if ((reply[i].resp = strdup(passwd)) == NULL)
20922104
goto fail;
@@ -2277,6 +2289,8 @@ CheckBSDAuth(Port *port, char *user)
22772289
*/
22782290
retval = auth_userokay(user, NULL, "auth-postgresql", passwd);
22792291

2292+
pfree(passwd);
2293+
22802294
if (!retval)
22812295
return STATUS_ERROR;
22822296

@@ -2407,16 +2421,12 @@ CheckLDAPAuth(Port *port)
24072421
if (passwd == NULL)
24082422
return STATUS_EOF; /* client wouldn't send password */
24092423

2410-
if (strlen(passwd) == 0)
2411-
{
2412-
ereport(LOG,
2413-
(errmsg("empty password returned by client")));
2414-
return STATUS_ERROR;
2415-
}
2416-
24172424
if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
2425+
{
24182426
/* Error message already sent */
2427+
pfree(passwd);
24192428
return STATUS_ERROR;
2429+
}
24202430

24212431
if (port->hba->ldapbasedn)
24222432
{
@@ -2448,6 +2458,7 @@ CheckLDAPAuth(Port *port)
24482458
{
24492459
ereport(LOG,
24502460
(errmsg("invalid character in user name for LDAP authentication")));
2461+
pfree(passwd);
24512462
return STATUS_ERROR;
24522463
}
24532464
}
@@ -2464,6 +2475,7 @@ CheckLDAPAuth(Port *port)
24642475
ereport(LOG,
24652476
(errmsg("could not perform initial LDAP bind for ldapbinddn \"%s\" on server \"%s\": %s",
24662477
port->hba->ldapbinddn, port->hba->ldapserver, ldap_err2string(r))));
2478+
pfree(passwd);
24672479
return STATUS_ERROR;
24682480
}
24692481

@@ -2488,6 +2500,7 @@ CheckLDAPAuth(Port *port)
24882500
ereport(LOG,
24892501
(errmsg("could not search LDAP for filter \"%s\" on server \"%s\": %s",
24902502
filter, port->hba->ldapserver, ldap_err2string(r))));
2503+
pfree(passwd);
24912504
pfree(filter);
24922505
return STATUS_ERROR;
24932506
}
@@ -2508,6 +2521,7 @@ CheckLDAPAuth(Port *port)
25082521
count,
25092522
filter, port->hba->ldapserver, count)));
25102523

2524+
pfree(passwd);
25112525
F438 pfree(filter);
25122526
ldap_msgfree(search_message);
25132527
return STATUS_ERROR;
@@ -2523,6 +2537,7 @@ CheckLDAPAuth(Port *port)
25232537
ereport(LOG,
25242538
(errmsg("could not get dn for the first entry matching \"%s\" on server \"%s\": %s",
25252539
filter, port->hba->ldapserver, ldap_err2string(error))));
2540+
pfree(passwd);
25262541
pfree(filter);
25272542
ldap_msgfree(search_message);
25282543
return STATUS_ERROR;
@@ -2543,6 +2558,7 @@ CheckLDAPAuth(Port *port)
25432558
ereport(LOG,
25442559
(errmsg("could not unbind after searching for user \"%s\" on server \"%s\": %s",
25452560
fulluser, port->hba->ldapserver, ldap_err2string(error))));
2561+
pfree(passwd);
25462562
pfree(fulluser);
25472563
return STATUS_ERROR;
25482564
}
@@ -2553,6 +2569,7 @@ CheckLDAPAuth(Port *port)
25532569
*/
25542570
if (InitializeLDAPConnection(port, &ldap) == STATUS_ERROR)
25552571
{
2572+
pfree(passwd);
25562573
pfree(fulluser);
25572574

25582575
/* Error message already sent */
@@ -2573,10 +2590,12 @@ CheckLDAPAuth(Port *port)
25732590
ereport(LOG,
25742591
(errmsg("LDAP login failed for user \"%s\" on server \"%s\": %s",
25752592
fulluser, port->hba->ldapserver, ldap_err2string(r))));
2593+
pfree(passwd);
25762594
pfree(fulluser);
25772595
return STATUS_ERROR;
25782596
}
25792597

2598+
pfree(passwd);
25802599
pfree(fulluser);
25812600

25822601
return STATUS_OK;
@@ -2720,17 +2739,11 @@ CheckRADIUSAuth(Port *port)
27202739
if (passwd == NULL)
27212740
return STATUS_EOF; /* client wouldn't send password */
27222741

2723-
if (strlen(passwd) == 0)
2724-
{
2725-
ereport(LOG,
2726-
(errmsg("empty password returned by client")));
2727-
return STATUS_ERROR;
2728-
}
2729-
27302742
if (strlen(passwd) > RADIUS_MAX_PASSWORD_LENGTH)
27312743
{
27322744
ereport(LOG,
27332745
(errmsg("RADIUS authentication does not support passwords longer than %d characters", RADIUS_MAX_PASSWORD_LENGTH)));
2746+
pfree(passwd);
27342747
return STATUS_ERROR;
27352748
}
27362749

@@ -2756,9 +2769,15 @@ CheckRADIUSAuth(Port *port)
27562769
*------
27572770
*/
27582771
if (ret == STATUS_OK)
2772+
{
2773+
pfree(passwd);
27592774
return STATUS_OK;
2775+
}
27602776
else if (ret == STATUS_EOF)
2777+
{
2778+
pfree(passwd);
27612779
return STATUS_ERROR;
2780+
}
27622781

27632782
/*
27642783
* secret, port and identifiers either have length 0 (use default),
@@ -2775,6 +2794,7 @@ CheckRADIUSAuth(Port *port)
27752794
}
27762795

27772796
/* No servers left to try, so give up */
2797+
pfree(passwd);
27782798
return STATUS_ERROR;
27792799
}
27802800

‎src/backend/libpq/crypt.c

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,6 @@ get_role_password(const char *role, char **logdetail)
7171

7272
ReleaseSysCache(roleTup);
7373

74-
if (*shadow_pass == '\0')
75-
{
76-
*logdetail = psprintf(_("User \"%s\" has an empty password."),
77-
role);
78-
pfree(shadow_pass);
79-
return NULL; /* empty password */
80-
}
81-
8274
/*
8375
* Password OK, but check to be sure we are not past rolvaliduntil
8476
*/

‎src/test/regress/expected/password.out

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,25 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
7575
regress_passwd5 | md5e73a4b11df52a6068f8b39f90be36023
7676
(5 rows)
7777

78+
-- An empty password is not allowed, in any form
79+
CREATE ROLE regress_passwd_empty PASSWORD '';
80+
NOTICE: empty string is not a valid password, clearing password
81+
ALTER ROLE regress_passwd_empty PASSWORD 'md585939a5ce845f1a1b620742e3c659e0a';
82+
NOTICE: empty string is not a valid password, clearing password
83+
ALTER ROLE regress_passwd_empty PASSWORD 'SCRAM-SHA-256$4096:hpFyHTUsSWcR7O9P$LgZFIt6Oqdo27ZFKbZ2nV+vtnYM995pDh9ca6WSi120=:qVV5NeluNfUPkwm7Vqat25RjSPLkGeoZBQs6wVv+um4=';
84+
NOTICE: empty string is not a valid password, clearing password
85+
SELECT rolpassword FROM pg_authid WHERE rolname='regress_passwd_empty';
86+
rolpassword
87+
-------------
88+
89+
(1 row)
90+
7891
DROP ROLE regress_passwd1;
7992
DROP ROLE regress_passwd2;
8093
DROP ROLE regress_passwd3;
8194
DROP ROLE regress_passwd4;
8295
DROP ROLE regress_passwd5;
96+
DROP ROLE regress_passwd_empty;
8397
-- all entries should have been removed
8498
SELECT rolname, rolpassword
8599
FROM pg_authid

‎src/test/regress/sql/password.sql

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,18 @@ SELECT rolname, regexp_replace(rolpassword, '(SCRAM-SHA-256)\$(\d+):([a-zA-Z0-9+
5959
WHERE rolname LIKE 'regress_passwd%'
6060
ORDER BY rolname, rolpassword;
6161

62+
-- An empty password is not allowed, in any form
63+
CREATE ROLE regress_passwd_empty PASSWORD '';
64+
ALTER ROLE regress_passwd_empty PASSWORD 'md585939a5ce845f1a1b620742e3c659e0a';
65+
ALTER ROLE regress_passwd_empty PASSWORD 'SCRAM-SHA-256$4096:hpFyHTUsSWcR7O9P$LgZFIt6Oqdo27ZFKbZ2nV+vtnYM995pDh9ca6WSi120=:qVV5NeluNfUPkwm7Vqat25RjSPLkGeoZBQs6wVv+um4=';
66+
SELECT rolpassword FROM pg_authid WHERE rolname='regress_passwd_empty';
67+
6268
DROP ROLE regress_passwd1;
6369
DROP ROLE regress_passwd2;
6470
DROP ROLE regress_passwd3;
6571
DROP ROLE regress_passwd4;
6672
DROP ROLE regress_passwd5;
73+
DROP ROLE regress_passwd_empty;
6774

6875
-- all entries should have been removed
6976
SELECT rolname, rolpassword

0 commit comments

Comments
 (0)
0