Thread: Remove line length restriction in passwordFromFile()
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
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
hereto exercise handling of long lines in the file. This very long comment is just here to exercise handling of long
linesin the file. This very long comment is just here to exercise handling of long lines in the file. This very long
commentis 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);
On 2020/09/01 6:24, Tom Lane wrote: > 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. AFAIR, there were proposals to increase the maximum length of password so far, but we could not do that because we failed to get the consensus about that change. But if we get the clear use-case requiring longer password and reach the consensus, that's good news. I agree with the change. > 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. Yes, some client tools have 100 bytes length restriction for the password. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2020/09/01 10:00, Fujii Masao wrote: > > > On 2020/09/01 6:24, Tom Lane wrote: >> 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. > > AFAIR, there were proposals to increase the maximum length of password so far, > but we could not do that because we failed to get the consensus about > that change. But if we get the clear use-case requiring longer password and > reach the consensus, that's good news. I agree with the change. > >> 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. The patch looks good to me, except the following minor thing. + if (fgets(buf.data + buf.len, buf.maxlen - buf.len - 1, fp) == NULL) IIUC fgets() reads the data with the specified size - 1, so ISTM that -1 of "buf.maxlen - buf.len - 1" is not necessary. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> The patch looks good to me, except the following minor thing.
> + if (fgets(buf.data + buf.len, buf.maxlen - buf.len - 1, fp) == NULL)
> IIUC fgets() reads the data with the specified size - 1, so ISTM that -1 of
> "buf.maxlen - buf.len - 1" is not necessary.
Good point, I was being unduly conservative. Thanks for reviewing
the patch!
regards, tom lane
On 2020/09/02 0:14, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> writes:
>> The patch looks good to me, except the following minor thing.
>> + if (fgets(buf.data + buf.len, buf.maxlen - buf.len - 1, fp) == NULL)
>> IIUC fgets() reads the data with the specified size - 1, so ISTM that -1 of
>> "buf.maxlen - buf.len - 1" is not necessary.
>
> Good point, I was being unduly conservative. Thanks for reviewing
> the patch!
Thanks for committing the patch!
The patch was back-patched to v13, but v13 release note still has the following item.
Tighten libpq's overlength-line handling and comment detection for .pgpass files (Fujii Masao)
This should be changed to the following or something?
Teach libpq to handle arbitrary-length lines in .pgpass files (Tom Lane)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> The patch was back-patched to v13, but v13 release note still has the following item.
> Tighten libpq's overlength-line handling and comment detection for .pgpass files (Fujii Masao)
> This should be changed to the following or something?
> Teach libpq to handle arbitrary-length lines in .pgpass files (Tom Lane)
Hm. Actually, since that went further back than v13, I don't think
it should appear in the v13 notes at all; it will be material for
the next quarterly update release notes.
We could adjust the release-note entry for your patch to say
"Improve comment detection for .pgpass files", or we could decide
it's not worth documenting that part and just drop the entry.
regards, tom lane
On 2020/09/10 1:48, Tom Lane wrote: > Fujii Masao <masao.fujii@oss.nttdata.com> writes: >> The patch was back-patched to v13, but v13 release note still has the following item. > >> Tighten libpq's overlength-line handling and comment detection for .pgpass files (Fujii Masao) > >> This should be changed to the following or something? > >> Teach libpq to handle arbitrary-length lines in .pgpass files (Tom Lane) > > Hm. Actually, since that went further back than v13, I don't think > it should appear in the v13 notes at all; it will be material for > the next quarterly update release notes. > > We could adjust the release-note entry for your patch to say > "Improve comment detection for .pgpass files", or we could decide > it's not worth documenting that part and just drop the entry. "Improve comment detection for .pgpass files" is also the material for minor version release because that change was also back-patched? If so, I agree to drop the entry. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2020/09/10 1:48, Tom Lane wrote:
>> We could adjust the release-note entry for your patch to say
>> "Improve comment detection for .pgpass files", or we could decide
>> it's not worth documenting that part and just drop the entry.
> "Improve comment detection for .pgpass files" is also the material for
> minor version release because that change was also back-patched?
> If so, I agree to drop the entry.
Hm, in a quick search I only see 2eb3bc588 which was not back-patched
... which commits are you thinking of?
regards, tom lane
On 2020/09/10 2:16, Tom Lane wrote: > Fujii Masao <masao.fujii@oss.nttdata.com> writes: >> On 2020/09/10 1:48, Tom Lane wrote: >>> We could adjust the release-note entry for your patch to say >>> "Improve comment detection for .pgpass files", or we could decide >>> it's not worth documenting that part and just drop the entry. > >> "Improve comment detection for .pgpass files" is also the material for >> minor version release because that change was also back-patched? >> If so, I agree to drop the entry. > > Hm, in a quick search I only see 2eb3bc588 which was not back-patched > ... which commits are you thinking of? I thought your commit b55b4dad99 included the improvement on comment detection and was back-patched.... Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2020/09/10 2:16, Tom Lane wrote:
>> Hm, in a quick search I only see 2eb3bc588 which was not back-patched
>> ... which commits are you thinking of?
> I thought your commit b55b4dad99 included the improvement on comment detection and was back-patched....
Oh, right, I did include the check for '#' in what was back-patched.
I debated whether to do that or not, and was misremembering my decision.
So yeah, it seems there's no need to document 2eb3bc588 in the v13
notes. I'll try to remember to give you part credit for b55b4dad9
when I do the November notes.
regards, tom lane