Skip to content

Commit df8020b

Browse files
tglsfdctglsfdc
committed
Teach libpq to handle arbitrary-length lines in .pgpass files.
Historically there's been a hard-wired assumption here that no line of a .pgpass file could be as long as NAMEDATALEN*5 bytes. That's a bit shaky to start off with, because (a) there's no reason to suppose that host names fit in NAMEDATALEN, and (b) this figure fails to allow for backslash escape characters. However, it fails completely if someone wants to use a very long password, and we're now hearing reports of people wanting to use "security tokens" that can run up to several hundred bytes. Another angle is that the file is specified to allow comment lines, but there's no reason to assume that long comment lines aren't possible. Rather than guessing at what might be a more suitable limit, let's replace the fixed-size buffer with an expansible PQExpBuffer. That adds one malloc/free cycle to the typical use-case, but that's surely pretty cheap relative to the I/O this code has to do. Also, add TAP test cases to exercise this code, because there was no test coverage before. This reverts most of commit 2eb3bc5, as there's no longer a need for a warning message about overlength .pgpass lines. (I kept the explicit check for comment lines, though.) In HEAD and v13, this also fixes an oversight in 74a308c: there's not much point in explicit_bzero'ing the line buffer if we only do so in two of the three exit paths. Back-patch to all supported branches, except that the test case only goes back to v10 where src/test/authentication/ was added. Discussion: https://postgr.es/m/4187382.1598909041@sss.pgh.pa.us
1 parent b235936 commit df8020b

File tree

2 files changed

+84
-44
lines changed

2 files changed

+84
-44
lines changed

‎src/interfaces/libpq/fe-connect.c

Lines changed: 58 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -6459,9 +6459,7 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
64596459
{
64606460
FILE *fp;
64616461
struct stat stat_buf;
6462-
6463-
#define LINELEN NAMEDATALEN*5
6464-
char buf[LINELEN];
6462+
PQExpBufferData buf;
64656463

64666464
if (dbname == NULL || dbname[0] == '\0')
64676465
return NULL;
@@ -6517,63 +6515,81 @@ passwordFromFile(const char *hostname, const char *port, const char *dbname,
65176515
if (fp == NULL)
65186516
return NULL;
65196517

6518+
/* Use an expansible buffer to accommodate any reasonable line length */
6519+
initPQExpBuffer(&buf);
6520+
65206521
while (!feof(fp) && !ferror(fp))
65216522
{
6522-
char *t = buf,
6523-
*ret,
6524-
*p1,
6525-
*p2;
6526-
int len;
6523+
/* Make sure there's a reasonable amount of room in the buffer */
6524+
if (!enlargePQExpBuffer(&buf, 128))
6525+
break;
65276526

6528-
if (fgets(buf, sizeof(buf), fp) == NULL)
6527+
/* Read some data, appending it to what we already have */
6528+
if (fgets(buf.data + buf.len, buf.maxlen - buf.len, fp) == NULL)
65296529
break;
6530+
buf.len += strlen(buf.data + buf.len);
65306531

6531-
len = strlen(buf);
6532+
/* If we don't yet have a whole line, loop around to read more */
6533+
if (!(buf.len > 0 && buf.data[buf.len - 1] == '\n') && !feof(fp))
6534+
continue;
65326535

6533-
/* Remove trailing newline */
6534-
if (len > 0 && buf[len - 1] == '\n')
6536+
/* ignore comments */
6537+
if (buf.data[0] != '#')
65356538
{
6536-
buf[--len] = '\0';
6537-
/* Handle DOS-style line endings, too, even when not on Windows */
6538-
if (len > 0 && buf[len - 1] == '\r')
6539-
buf[--len] = '\0';
6540-
}
6539+
char *t = buf.data;
6540+
int len = buf.len;
65416541

6542-
if (len == 0)
6543-
continue;
6542+
/* Remove trailing newline */
6543+
if (len > 0 && t[len - 1] == '\n')
6544+
{
6545+
t[--len] = '\0';
6546+
/* Handle DOS-style line endings, too */
6547+
if (len > 0 && t[len - 1] == '\r')
6548+
t[--len] = '\0';
6549+
}
65446550

6545-
if ((t = pwdfMatchesString(t, hostname)) == NULL ||
6546-
(t = pwdfMatchesString(t, port)) == NULL ||
6547-
(t = pwdfMatchesString(t, dbname)) == NULL ||
6548-
(t = pwdfMatchesString(t, username)) == NULL)
6549-
continue;
6551+
if (len > 0 &&
6552+
(t = pwdfMatchesString(t, hostname)) != NULL &&
6553+
(t = pwdfMatchesString(t, port)) != NULL &&
6554+
(t = pwdfMatchesString(t, dbname)) != NULL &&
6555+
(t = pwdfMatchesString(t, username)) != NULL)
6556+
{
6557+
/* Found a match. */
6558+
char *ret,
6559+
*p1,
6560+
*p2;
65506561

6551-
/* Found a match. */
6552-
ret = strdup(t);
6553-
fclose(fp);
6562+
ret = strdup(t);
65546563

6555-
if (!ret)
6556-
{
6557-
/* Out of memory. XXX: an error message would be nice. */
6558-
return NULL;
6559-
}
6564+
fclose(fp);
6565+
termPQExpBuffer(&buf);
65606566

6561-
/* De-escape password. */
6562-
for (p1 = p2 = ret; *p1 != ':' && *p1 != '\0'; ++p1, ++p2)
6563-
{
6564-
if (*p1 == '\\' && p1[1] != '\0')
6565-
++p1;
6566-
*p2 = *p1;
6567+
if (!ret)
6568+
{
6569+
/* Out of memory. XXX: an error message would be nice. */
6570+
return NULL;
6571+
}
6572+
6573+
/* De-escape password. */
6574+
for (p1 = p2 = ret; *p1 != ':' && *p1 != '\0'; ++p1, ++p2)
6575+
{
6576+
if (*p1 == '\\' && p1[1] != '\0')
6577+
++p1;
6578+
*p2 = *p1;
6579+
}
6580+
*p2 = '\0';
6581+
6582+
return ret;
6583+
}
65676584
}
6568-
*p2 = '\0';
65696585

6570-
return ret;
6586+
/* No match, reset buffer to prepare for next line. */
6587+
buf.len = 0;
65716588
}
65726589

65736590
fclose(fp);
6591+
termPQExpBuffer(&buf);
65746592
return NULL;
6575-
6576-
#undef LINELEN
65776593
}
65786594

65796595

‎src/test/authentication/t/001_password.pl

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
}
1818
else
1919
{
20-
plan tests => 8;
20+
plan tests => 11;
2121
}
2222

2323

@@ -45,7 +45,9 @@ sub test_role
4545

4646
$status_string = 'success' if ($expected_res eq 0);
4747

48-
my $res = $node->psql('postgres', undef, extra_params => [ '-U', $role ]);
48+
local $Test::Builder::Level = $Test::Builder::Level + 1;
49+
50+
my $res = $node->psql('postgres', undef, extra_params => [ '-U', $role, '-w' ]);
4951
is($res, $expected_res,
5052
"authentication $status_string for method $method, role $role");
5153
return;
@@ -86,3 +88,25 @@ sub test_role
8688
reset_pg_hba($node, 'md5');
8789
test_role($node, 'scram_role', 'md5', 0);
8890
test_role($node, 'md5_role', 'md5', 0);
91+
92+
# Test .pgpass processing; but use a temp file, don't overwrite the real one!
93+
my $pgpassfile = "${TestLib::tmp_check}/pgpass";
94+
95+
delete $ENV{"PGPASSWORD"};
96+
$ENV{"PGPASSFILE"} = $pgpassfile;
97+
98+
append_to_file($pgpassfile, qq!
99+
# 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.
100+
*:*:postgres:scram_role:pass:this is not part of the password.
101+
!);
102+
chmod 0600, $pgpassfile or die;
103+
104+
reset_pg_hba($node, 'password');
105+
test_role($node, 'scram_role', 'password from pgpass', 0);
106+
test_role($node, 'md5_role', 'password from pgpass', 2);
107+
108+
append_to_file($pgpassfile, qq!
109+
*:*:*:md5_role:p\\ass
110+
!);
111+
112+
test_role($node, 'md5_role', 'password from pgpass', 0);

0 commit comments

Comments
 (0)