From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 10 Sep 2019 19:26:41 +0000 (+0200)
Subject: Fix issue with PAM users losing their password
X-Git-Tag: pgbouncer_1_12_0~32
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b30b49f16d718339f0f7ffee408c48b376c0eb81;p=pgbouncer

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 8a3a6eb..b37d8d4 100644
--- 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 193f5e0..2e2f463 100644
--- 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;
 }