Bizarre behavior in libpq's searching of ~/.pgpass

I wrote

I noticed that there's some strange coding in libpq's choice of
what hostname to use for searching ~/.pgpass for a password.
...

So my first thought was that we should go back to the pre-v10 behavior
of considering only the host parameter, which it looks like would only
require removing the "if" bit above.

But on second thought, I'm not clear that the pre-v10 behavior is really
all that sane either. What it means is that if you specify only hostaddr,
the code will happily grab your localhost password and send it off to
whatever server hostaddr references. This is unlikely to be helpful,
and it could even be painted as a security breach --- the remote server
could ask for your password in plaintext and then capture it.

What seems like a saner definition is "use host if it's specified
(nonempty), else use hostaddr if it's specified (nonempty), else
fall back to localhost". That avoids sending a password somewhere
it doesn't belong, and allows a useful ~/.pgpass lookup in cases
where only hostaddr is given -- you just need to make an entry
with the numeric IP address in the host column.

I think it's not too late to make v11 work that way, but I wonder
what we ought to do in v10. Comments?

Here's a proposed patch to adopt that behavior. I'm still of mixed
mind whether to push this into v10 ... but we definitely need some
change in v10, because it's not acting as per its docs.

regards, tom lane

On 07/29/2018 11:15 PM, Tom Lane wrote:

Here's a proposed patch to adopt that behavior. I'm still of mixed
mind whether to push this into v10 ... but we definitely need some
change in v10, because it's not acting as per its docs.

Is there actually a useful use case working in v10 and broken by your
proposed patch? I can't think of any, and even if there is one it's
likely no one is aware of it as the docs were not updated. Considering
this could also be painted as a security issue (people generally don't
connect from servers to random machines, but still ...), I'd vote for
pushing this to v10.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Jul 27, 2018 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I noticed that there's some strange coding in libpq's choice of
what hostname to use for searching ~/.pgpass for a password.
Historically (pre-v10), it just used the pghost parameter:

conn->pgpass = PasswordFromFile(conn->pghost, conn->pgport,
conn->dbName, conn->pguser);

no ifs, ands, or buts, except for the fact that PasswordFromFile
replaces its hostname parameter with "localhost" if it's null or
matches the default socket directory. This is per the documentation
(see sections 34.1.2 and 34.15).

Since v10 we've got this:

char *pwhost = conn->connhost[i].host;

if (conn->connhost[i].type == CHT_HOST_ADDRESS &&
conn->connhost[i].host != NULL &&
conn->connhost[i].host[0] != '\0')
pwhost = conn->connhost[i].hostaddr;

conn->connhost[i].password =
passwordFromFile(pwhost,
conn->connhost[i].port,
conn->dbName,
conn->pguser,
conn->pgpassfile);

Now that's just bizarre on its face: take hostaddr if it's specified,
but only if host is also specified? And it certainly doesn't match
the documentation.

Yeah, that's bad code. The intent was that if you set host=a,b you
probably want to use either 'a' or 'b' as the thing to look up in
.pgpass, not 'a,b', but the implementation leaves something to be
desired.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jul 27, 2018 at 11:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I noticed that there's some strange coding in libpq's choice of
what hostname to use for searching ~/.pgpass for a password.

Yeah, that's bad code. The intent was that if you set host=a,b you
probably want to use either 'a' or 'b' as the thing to look up in
.pgpass, not 'a,b', but the implementation leaves something to be
desired.

Right. Closely related to that is that the existing code in
passwordFromFile behaves differently for null vs. empty hostname.
This is bad on its face, because nowhere else in libpq do we treat
empty-string parameters differently from unset ones, but it's particularly
a mess for the comma-separated-list case because then it's impossible
for one of the alternatives to be NULL. In the already-posted patch
I fixed that so that an empty alternative is replaced by DefaultHost
in passwordFromFile, and likewise for port (though I think the latter
case may be unreachable).

But now that I look at it, it seems like the code in connectOptions2
has also Gotten It Wrong. Shouldn't the replacement of "unspecified"
cases by DEFAULT_PGSOCKET_DIR/DefaultHost also happen on an entry-by-
entry basis, so that "host=foo," would behave as though the empty
entry were "localhost"?

regards, tom lane

I wrote:

But now that I look at it, it seems like the code in connectOptions2
has also Gotten It Wrong. Shouldn't the replacement of "unspecified"
cases by DEFAULT_PGSOCKET_DIR/DefaultHost also happen on an entry-by-
entry basis, so that "host=foo," would behave as though the empty
entry were "localhost"?

Here's an updated patch that fixes that aspect too. Although this
might seem independent, it's not really: this version of the patch
eliminates the corner case where we have neither a host or hostaddr
to search for. The check for empty hostname in passwordFromFile is
thereby dead code, though it seemed better style to leave it in.

(Basically, the point here is to guarantee that passwordFromFile has
the same idea of what host/port we are going to connect to as the
actual connection code does. That was not true before, either for
the host or the port :-()

regards, tom lane