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 2eb3bc588, 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 74a308cf5: 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- 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

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 7bee9dd2014c0198777ba754ec06bdfea89655de..724076a3103e52c363fc366e04fa01360918138a 100644 (file)
--- 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, 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 1305de0051a6904877b7d9a059535f2dcf61ffe0..1b4323fe2a609277de8c80400e904ac436e470f8 100644 (file)
--- 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);