Fix issue with PAM users losing their password

The normal flow when starting a client connection is that set_pool()
is called with an empty password ("").  For normal users, the password
was already set earlier from auth_file or auth_query, so this empty
password is ignored.  For PAM users, the empty password is stored but
the real password is set later when authentication is complete.

The problem in the PAM case is that for the next client connection,
this would overwrite the stored password with an empty password until
the real password would then be re-added later.  If the client
authentication doesn't complete for whatever reason (perhaps server is
down and fast-fail is active), then the correct password is never set.
This would then have clobbered the user's password that might be
useful for server authentication.  There are probably other failure
scenarios.

To fix, call set_pool() with a NULL password instead, and teach
add_pam_user() not to overwrite an existing password if the argument
is NULL.

reported, analysis, and fix proposal by @achix

fixes #285

diff --git a/src/client.c b/src/client.c
index 8a3a6ebed0c01cee9c1d736be877dc5a197806d4..b37d8d459cc6f863ccc83e1beb25d79a197b8041 100644 (file)
--- a/src/client.c
+++ b/src/client.c
@@ -246,6 +246,8 @@ static bool finish_set_pool(PgSocket *client, bool takeover)
 
 bool set_pool(PgSocket *client, const char *dbname, const char *username, const char *password, bool takeover)
 {
+       Assert((password && takeover) || (!password && !takeover));
+
        /* find database */
        client->db = find_database(dbname);
        if (!client->db) {
@@ -279,7 +281,7 @@ bool set_pool(PgSocket *client, const char *dbname, const char *username, const
                        slog_info(client, "login failed: db=%s user=%s", dbname, username);
                return false;
        }
-       if (strlen(password) >= MAX_PASSWORD) {
+       if (password && strlen(password) >= MAX_PASSWORD) {
                disconnect_client(client, true, "password too long");
                if (cf_log_connections)
                        slog_info(client, "login failed: db=%s user=%s", dbname, username);
@@ -511,7 +513,7 @@ static bool decide_startup_pool(PgSocket *client, PktHdr *pkt)
        }
 
        /* find pool */
-       return set_pool(client, dbname, username, "", false);
+       return set_pool(client, dbname, username, NULL, false);
 }
 
 static bool scram_client_first(PgSocket *client, uint32_t datalen, const uint8_t *data)

diff --git a/src/objects.c b/src/objects.c
index 193f5e0b0adfb83732c64e97d3e73eacb70adb27..2e2f463ce32fb334135f2b97e29c172de344e99e 100644 (file)
--- a/src/objects.c
+++ b/src/objects.c
@@ -441,7 +441,8 @@ PgUser *add_pam_user(const char *name, const char *passwd)
                aatree_insert(&pam_user_tree, (uintptr_t)user->name, &user->tree_node);
                user->pool_mode = POOL_INHERIT;
        }
-       safe_strcpy(user->passwd, passwd, sizeof(user->passwd));
+       if (passwd)
+               safe_strcpy(user->passwd, passwd, sizeof(user->passwd));
        return user;
 }