[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-hackers
Subject:    Remove line length restriction in passwordFromFile()
From:       Tom Lane <tgl () sss ! pgh ! pa ! us>
Date:       2020-08-31 21:24:01
Message-ID: 4187382.1598909041 () sss ! pgh ! pa ! us
[Download RAW message or body]

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4187369.1598909023.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

Per the discussion at [1], we're now aware of actual use-cases for
password strings approaching a kilobyte in length.  I think this puts
the final nail in the coffin of the idea that passwordFromFile() can
use a fixed-length line buffer.  Therefore, commit 2eb3bc588 (which
added a warning for overlength lines) seems rather misguided in
hindsight.  What we should do instead is fix that code so it has no
hard upper bound on the line length.  Even if you want to say that
we'll set a particular limit on how long the password field can be,
there's no good upper bound for the length of the hostname field;
so ISTM that just getting out of the business of a fixed-size buffer
is the sanest way.

Hence, the attached proposed patch does that, and for good measure
adds some testing of this formerly untested code.

Since we now have an actual user complaint, I'm inclined to back-patch
this all the way.

As noted in the other thread, there may be some other changes needed
to support long passwords, but this is clearly required.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CAOhmDze1nqG2vfegpSsTFCgaiFRsqgjO6yLsbmhroz2zGmJHog%40mail.gmail.com



------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="remove-fixed-buffer-in-passwordFromFile.patch";
	charset="us-ascii"
Content-ID: <4187369.1598909023.2@sss.pgh.pa.us>
Content-Description: remove-fixed-buffer-in-passwordFromFile.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 7bee9dd201..ab0973d5b0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -6937,10 +6937,7 @@ passwordFromFile(const char *hostname, const char *port, const \
char *dbname,  {
 	FILE	   *fp;
 	struct stat stat_buf;
-	int			line_number = 0;
-
-#define LINELEN NAMEDATALEN*5
-	char		buf[LINELEN];
+	PQExpBufferData buf;
 
 	if (dbname == NULL || dbname[0] == '\0')
 		return NULL;
@@ -6996,89 +6993,77 @@ passwordFromFile(const char *hostname, const char *port, \
const char *dbname,  if (fp == NULL)
 		return NULL;
 
+	/* Use an expansible buffer to accommodate any reasonable line length */
+	initPQExpBuffer(&buf);
+
 	while (!feof(fp) && !ferror(fp))
 	{
-		char	   *t = buf,
-				   *ret,
-				   *p1,
-				   *p2;
-		int			len;
-		int			buflen;
+		/* Make sure there's a reasonable amount of room in the buffer */
+		if (!enlargePQExpBuffer(&buf, 128))
+			break;
 
-		if (fgets(buf, sizeof(buf), fp) == NULL)
+		/* Read some data, appending it to what we already have */
+		if (fgets(buf.data + buf.len, buf.maxlen - buf.len - 1, fp) == NULL)
 			break;
+		buf.len += strlen(buf.data + buf.len);
 
-		line_number++;
-		buflen = strlen(buf);
-		if (buflen >= sizeof(buf) - 1 && buf[buflen - 1] != '\n')
+		/* If we don't yet have a whole line, loop around to read more */
+		if (!(buf.len > 0 && buf.data[buf.len - 1] == '\n') && !feof(fp))
+			continue;
+
+		/* ignore comments */
+		if (buf.data[0] != '#')
 		{
-			char		rest[LINELEN];
-			int			restlen;
+			char	   *t = buf.data;
+			int			len;
 
-			/*
-			 * Warn if this password setting line is too long, because it's
-			 * unexpectedly truncated.
-			 */
-			if (buf[0] != '#')
-				fprintf(stderr,
-						libpq_gettext("WARNING: line %d too long in password file \"%s\"\n"),
-						line_number, pgpassfile);
+			/* strip trailing newline and carriage return */
+			len = pg_strip_crlf(t);
 
-			/* eat rest of the line */
-			while (!feof(fp) && !ferror(fp))
+			if (len > 0 &&
+				(t = pwdfMatchesString(t, hostname)) != NULL &&
+				(t = pwdfMatchesString(t, port)) != NULL &&
+				(t = pwdfMatchesString(t, dbname)) != NULL &&
+				(t = pwdfMatchesString(t, username)) != NULL)
 			{
-				if (fgets(rest, sizeof(rest), fp) == NULL)
-					break;
-				restlen = strlen(rest);
-				if (restlen < sizeof(rest) - 1 || rest[restlen - 1] == '\n')
-					break;
-			}
-		}
-
-		/* ignore comments */
-		if (buf[0] == '#')
-			continue;
-
-		/* strip trailing newline and carriage return */
-		len = pg_strip_crlf(buf);
+				/* Found a match. */
+				char	   *ret,
+						   *p1,
+						   *p2;
 
-		if (len == 0)
-			continue;
+				ret = strdup(t);
 
-		if ((t = pwdfMatchesString(t, hostname)) == NULL ||
-			(t = pwdfMatchesString(t, port)) == NULL ||
-			(t = pwdfMatchesString(t, dbname)) == NULL ||
-			(t = pwdfMatchesString(t, username)) == NULL)
-			continue;
+				fclose(fp);
+				explicit_bzero(buf.data, buf.maxlen);
+				termPQExpBuffer(&buf);
 
-		/* Found a match. */
-		ret = strdup(t);
-		fclose(fp);
+				if (!ret)
+				{
+					/* Out of memory. XXX: an error message would be nice. */
+					return NULL;
+				}
 
-		if (!ret)
-		{
-			/* Out of memory. XXX: an error message would be nice. */
-			explicit_bzero(buf, sizeof(buf));
-			return NULL;
-		}
+				/* De-escape password. */
+				for (p1 = p2 = ret; *p1 != ':' && *p1 != '\0'; ++p1, ++p2)
+				{
+					if (*p1 == '\\' && p1[1] != '\0')
+						++p1;
+					*p2 = *p1;
+				}
+				*p2 = '\0';
 
-		/* De-escape password. */
-		for (p1 = p2 = ret; *p1 != ':' && *p1 != '\0'; ++p1, ++p2)
-		{
-			if (*p1 == '\\' && p1[1] != '\0')
-				++p1;
-			*p2 = *p1;
+				return ret;
+			}
 		}
-		*p2 = '\0';
 
-		return ret;
+		/* No match, reset buffer to prepare for next line. */
+		buf.len = 0;
 	}
 
 	fclose(fp);
-	explicit_bzero(buf, sizeof(buf));
+	explicit_bzero(buf.data, buf.maxlen);
+	termPQExpBuffer(&buf);
 	return NULL;
-
-#undef LINELEN
 }
 
 
diff --git a/src/test/authentication/t/001_password.pl \
b/src/test/authentication/t/001_password.pl index 1305de0051..1b4323fe2a 100644
--- a/src/test/authentication/t/001_password.pl
+++ b/src/test/authentication/t/001_password.pl
@@ -17,7 +17,7 @@ if (!$use_unix_sockets)
 }
 else
 {
-	plan tests => 10;
+	plan tests => 13;
 }
 
 
@@ -45,7 +45,9 @@ sub test_role
 
 	$status_string = 'success' if ($expected_res eq 0);
 
-	my $res = $node->psql('postgres', undef, extra_params => [ '-U', $role ]);
+	local $Test::Builder::Level = $Test::Builder::Level + 1;
+
+	my $res = $node->psql('postgres', undef, extra_params => [ '-U', $role, '-w' ]);
 	is($res, $expected_res,
 		"authentication $status_string for method $method, role $role");
 	return;
@@ -96,3 +98,26 @@ test_role($node, 'scram_role', 'scram-sha-256', 2);
 reset_pg_hba($node, 'scram-sha-256');
 $ENV{"PGCHANNELBINDING"} = 'require';
 test_role($node, 'scram_role', 'scram-sha-256', 2);
+
+# Test .pgpass processing; but use a temp file, don't overwrite the real one!
+my $pgpassfile = "${TestLib::tmp_check}/pgpass";
+
+delete $ENV{"PGPASSWORD"};
+delete $ENV{"PGCHANNELBINDING"};
+$ENV{"PGPASSFILE"} = $pgpassfile;
+
+append_to_file($pgpassfile, qq!
+# This very long comment is just here to exercise handling of long lines in the \
file. This very long comment is just here to exercise handling of long lines in the \
file. This very long comment is just here to exercise handling of long lines in the \
file. This very long comment is just here to exercise handling of long lines in the \
file. This very long comment is just here to exercise handling of long lines in the \
file. +*:*:postgres:scram_role:pass:this is not part of the password.
+!);
+chmod 0600, $pgpassfile or die;
+
+reset_pg_hba($node, 'password');
+test_role($node, 'scram_role', 'password from pgpass', 0);
+test_role($node, 'md5_role',   'password from pgpass', 2);
+
+append_to_file($pgpassfile, qq!
+*:*:*:md5_role:p\\ass
+!);
+
+test_role($node, 'md5_role',   'password from pgpass', 0);

------- =_aaaaaaaaaa0--


[prev in list] [next in list] [prev in thread] [next in thread]