Re: [patch] fix dblink security hole

Currently dblink allows regular users to initiate libpq connection
to user-provided connection string. This breaks the default
policy that normal users should not be allowed to freely interact
with outside environment.

In addition to breaking standard security policy, dblink exposes
.pgpass/pg_service.conf contents of the OS user database is running
under to the non-privileged database user. (Esp. passwords)

The were already couple of hacks added to libpq and dblink that
disallow the re-use of passwords acquired from .pgpass/pg_service.
The hacks make sure the password is used for login and it came
from connection string.

But they take effect *after* login is finished, thus leaving open
even nastier hole - user can *look* at the passwords by simply
directing the connection to host where is a Postgres-emulator
installed that requests plaintext authentication.

Attached patch fixes the problems by allowing only superusers
to specify connection strings.

Instead of playing with permissions, it hardwires the check to
C code for following reasons:
- encouraging use of SECURITY DEFINER functions to choose
the connection string so they are always under control of admin
- discouraging solutions where normal user can freely pick
connection string
- making dblink security similar to other remote-access solutions
which do not let regular user pick connection string (plproxy / dbi-link)

I understand some of current users may dislike the patch as currently
dblink does not give any support for running it securely.

I've outlined possible future API direction here:

http://archives.postgresql.org/pgsql-hackers/2008-07/msg01302.php

Mainly this involves creating superuser-controlled name->connstr lookup
mechanism. Either specific to dblink or possibly built into core,
so other remote-access solutions can use it also.

--
marko

Marko Kreen escribió:
> Currently dblink allows regular users to initiate libpq connection
> to user-provided connection string. This breaks the default
> policy that normal users should not be allowed to freely interact
> with outside environment.

Since people is now working on implementing the SQL/MED stuff to manage
connections, should we bounce this patch? With luck, the CREATE
CONNECTION (?) stuff will be done for the next commitfest and we can
just switch dblink to use that instead.

http://archives.postgresql.org/message-id/e51f66da0809050539x1b25ebb9t7fd664fd67b9f607@mail.gmail.com

Thoughts? Can we really expect SQL/MED connection mgmt to be done for
the next fest?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Fri, Sep 12, 2008 at 01:14:36PM -0400, Alvaro Herrera wrote:
> Marko Kreen escribió:
> > Currently dblink allows regular users to initiate libpq connection
> > to user-provided connection string. This breaks the default
> > policy that normal users should not be allowed to freely interact
> > with outside environment.
>
> Since people is now working on implementing the SQL/MED stuff to
> manage connections,

I don't see any code for this. Is there some?

> should we bounce this patch? With luck, the CREATE CONNECTION (?)
> stuff will be done for the next commitfest and we can just switch
> dblink to use that instead.

That would be great :)

> http://archives.postgresql.org/message-id/e51f66da0809050539x1b25ebb9t7fd664fd67b9f607@mail.gmail.com
>
> Thoughts? Can we really expect SQL/MED connection mgmt to be done
> for the next fest?

Connection management would be awesome. The whole SQL/MED spec is
gigantic, tho. Should we see about an implementation roadmap for the
parts we care about?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 9/12/08, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Marko Kreen escribió:
>
> > Currently dblink allows regular users to initiate libpq connection
> > to user-provided connection string. This breaks the default
> > policy that normal users should not be allowed to freely interact
> > with outside environment.
>
> Since people is now working on implementing the SQL/MED stuff to manage
> connections, should we bounce this patch? With luck, the CREATE
> CONNECTION (?) stuff will be done for the next commitfest and we can
> just switch dblink to use that instead.
>
> http://archives.postgresql.org/message-id/e51f66da0809050539x1b25ebb9t7fd664fd67b9f607@mail.gmail.com
>
> Thoughts? Can we really expect SQL/MED connection mgmt to be done for
> the next fest?

I will not have time for it. If you want to have it in 8.4,
somebody else needs to step forward.

It should not be that hard actually, for dblink and plproxy only
following is needed (for exact syntax look at sql standard):

- CREATE/ALTER/DROP CONNECTION <name> <details>
- CREATE/DROP USER MAPPING FOR <conn> <user> ... <password>
- system table for connection details
- system table for user mapping - basically access control and passwords
- C API for connection parameter fetching with access control.
It should not try to handle actual connections as it's users may
have different requirements (eg plproxy wants to use async API
for connecting), and anyway it should handle non-Postgres connection
too in the future.
- invalidation mechanism if info in system tables change

The syntax better be SQL-MED compliant (as far as we want to be).

The SQL-MED seems to define further API for both C and SQL, but there
is no need to try to implement those. As there is simply no need for it.

--
marko

Marko Kreen wrote:
> In addition to breaking standard security policy, dblink exposes
> .pgpass/pg_service.conf contents of the OS user database is running
> under to the non-privileged database user. (Esp. passwords)

I took a look and can partially see Marko's point. The scenario exists
within this context:

1. "superuser" installs dblink on db1, running on postgres server
under the "superuser" account
2. "superuser" has .pgpass file
3. the "superuser" .pgpass file is set up with wildcards, e.g.
*:*:*:postgres:mypassword
4. "superuser" creates login for "luser" in db1

This depends on "superuser" to not only make use of .pgpass, but
specifically to use it in an insecure way, i.e. using wildcards to
specify that the login credentials should be sent to any arbitrary
Postgres installation.

So although it may make sense to lock this down for 8.4, I don't agree
with backporting it due to the backward compatibility hit. Also, I think
we still need a way that people who don't allow real end-users directly
in their databases and don't care about Marko's threat scenario can get
their work done with minimal pain.

Attached is my version of a more complete patch. It aims to prevent any
dblink connection by non-superusers. But it also creates "_u" versions
of dblink() and dblink_exec(), and initially revokes privileges from
public in a similar vain. dblink_u(), dblink_exec_u (), and the
previously created dblink_connect_u() are all SECURITY_DEFINER functions
that can be granted to trusted non-superuser logins.

Beyond Marko and I, no one else has publicly weighed in on this. If I
don't hear any objections, I'll apply to cvs HEAD *only* in about 24 hours.

Thanks,

Joe

I'm clearly out of practice -- this time with the attachment
------------------------------------------------------------

Marko Kreen wrote:
> In addition to breaking standard security policy, dblink exposes
> .pgpass/pg_service.conf contents of the OS user database is running
> under to the non-privileged database user. (Esp. passwords)

I took a look and can partially see Marko's point. The scenario exists
within this context:

1. "superuser" installs dblink on db1, running on postgres server
under the "superuser" account
2. "superuser" has .pgpass file
3. the "superuser" .pgpass file is set up with wildcards, e.g.
*:*:*:postgres:mypassword
4. "superuser" creates login for "luser" in db1

This depends on "superuser" to not only make use of .pgpass, but
specifically to use it in an insecure way, i.e. using wildcards to
specify that the login credentials should be sent to any arbitrary
Postgres installation.

So although it may make sense to lock this down for 8.4, I don't agree
with backporting it due to the backward compatibility hit. Also, I think
we still need a way that people who don't allow real end-users directly
in their databases and don't care about Marko's threat scenario can get
their work done with minimal pain.

Attached is my version of a more complete patch. It aims to prevent any
dblink connection by non-superusers. But it also creates "_u" versions
of dblink() and dblink_exec(), and initially revokes privileges from
public in a similar vain. dblink_u(), dblink_exec_u (), and the
previously created dblink_connect_u() are all SECURITY_DEFINER functions
that can be granted to trusted non-superuser logins.

Beyond Marko and I, no one else has publicly weighed in on this. If I
don't hear any objections, I'll apply to cvs HEAD *only* in about 24 hours.

Thanks,

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> I took a look and can partially see Marko's point. The scenario exists
> within this context:

> 1. "superuser" installs dblink on db1, running on postgres server
> under the "superuser" account
> 2. "superuser" has .pgpass file
> 3. the "superuser" .pgpass file is set up with wildcards, e.g.
> *:*:*:postgres:mypassword
> 4. "superuser" creates login for "luser" in db1

> This depends on "superuser" to not only make use of .pgpass, but
> specifically to use it in an insecure way, i.e. using wildcards to
> specify that the login credentials should be sent to any arbitrary
> Postgres installation.

It seems to me that this is a pretty far-fetched scenario; someone
who'd set up his .pgpass that way would be at risk from his own typos,
not just from nefarious users. I'm not sure how far out of our way we
need to go to protect stupid DBAs. But anyway:

The main thing that bothers me about the proposed patch is that it takes
away the security mechanism that existed before. Now you have either no
trust or 100% trust, you don't have the option to trust people who know
a password. That's less secure, not more, if you ask me. Marko's
original patch is just as bad.

If I understand the complaint correctly, it is not that a luser can make
a connection, it is that the password will be sent before dblink rejects
the connection. So really this problem is not specific to dblink ---
what it's saying is that PQconnectionUsedPassword is broken by design
and we should deprecate using that for security purposes.

I think there is an alternative solution, if we are only going to patch
this in 8.4 and up: provide a new libpq conninfo-string option saying
not to use .pgpass, and have dblink add that to the passed-in conninfo
string instead of trying to check after the fact. Then we aren't
changing dblink's API at all, only replacing a leaky security check
with a better one.

regards, tom lane

Tom Lane wrote:
> I think there is an alternative solution, if we are only going to patch
> this in 8.4 and up: provide a new libpq conninfo-string option saying
> not to use .pgpass, and have dblink add that to the passed-in conninfo
> string instead of trying to check after the fact. Then we aren't
> changing dblink's API at all, only replacing a leaky security check
> with a better one.

Good point -- I'll look into that and post something tomorrow. How does
"requirepassword" sound for the option? It is consistent with
"requiressl" but a bit long and hard to read. Maybe "require_password"?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Good point -- I'll look into that and post something tomorrow. How does
> "requirepassword" sound for the option? It is consistent with
> "requiressl" but a bit long and hard to read. Maybe "require_password"?

Well, no, because it's not requiring a password.

Perhaps "ignore_pgpass"?

[ looks at code a moment... ] Actually, there's another possibility.
I see that the code already allows the location of .pgpass to be
specified via the environment variable PGPASSFILE, but very
non-orthogonally fails to have an equivalent conninfo option.
So here's a more concrete proposal: fix it so that pgpassfile is
also a conninfo option, and allow "pgpassfile = none" to silently
suppress use of the pgpass file. (You could almost get there today
with putenv("PGPASSFILE=/dev/null"), except that (a) it would generate
complaints in the postmaster log, and (b) we probably don't want dblink
messing up the backend environment settings for possible other uses
of libpq.)

BTW, a possible hole in this scheme would be if a user could supply a
conninfo string that was intentionally malformed in a way that would
cause a tacked-on pgpassfile option to be ignored by libpq. We might
need to add some validity checks to dblink, or tighten libpq's own
checks.

regards, tom lane

On 9/21/08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
> > Good point -- I'll look into that and post something tomorrow. How does
> > "requirepassword" sound for the option? It is consistent with
> > "requiressl" but a bit long and hard to read. Maybe "require_password"?
>
>
> Well, no, because it's not requiring a password.
>
> Perhaps "ignore_pgpass"?

You need to ignore pg_service also. (And PGPASSWORD)

--
marko

Marko Kreen wrote:
> On 9/21/08, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Joe Conway <mail(at)joeconway(dot)com> writes:
>>> Good point -- I'll look into that and post something tomorrow. How does
>> > "requirepassword" sound for the option? It is consistent with
>> > "requiressl" but a bit long and hard to read. Maybe "require_password"?
>>
>>
>> Well, no, because it's not requiring a password.
>>
>> Perhaps "ignore_pgpass"?
>
> You need to ignore pg_service also. (And PGPASSWORD)

Why? pg_service does not appear to support wildcards, so what is the
attack vector?

And on PGPASSWORD, the fine manual says the following:

PGPASSWORD sets the password used if the server demands password
authentication. Use of this environment variable is not recommended
for security reasons (some operating systems allow non-root users to
see process environment variables via ps); instead consider using the
~/.pgpass file (see Section 30.13).

At the moment the only real issue I can see is .pgpass when wildcards
are used for hostname:port:database.

Joe

Tom Lane wrote:
> BTW, a possible hole in this scheme would be if a user could supply a
> conninfo string that was intentionally malformed in a way that would
> cause a tacked-on pgpassfile option to be ignored by libpq. We might
> need to add some validity checks to dblink, or tighten libpq's own
> checks.

If we push the responsibility back to dblink, we might as well export
conninfo_parse() or some wrapper thereof and let dblink simply check for
a non-null password from the very beginning.

Or perhaps we should modify conninfo_parse() to throw an error if it
sees the same option more than once. Then dblink could prepend
pgpassfile (or ignore_pgpass) to the beginning of the connstr and not
have to worry about being overridden. Not sure if the backward
compatibility hit is worth it though.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> If we push the responsibility back to dblink, we might as well export
> conninfo_parse() or some wrapper thereof and let dblink simply check for
> a non-null password from the very beginning.

That's not totally unreasonable, since we already export the
PQconninfoOption struct ...

> Or perhaps we should modify conninfo_parse() to throw an error if it
> sees the same option more than once. Then dblink could prepend
> pgpassfile (or ignore_pgpass) to the beginning of the connstr and not
> have to worry about being overridden. Not sure if the backward
> compatibility hit is worth it though.

... but I think I like the second one better; multiple specifications of
an option seem like probably a programming error anyway. It's a close
call though. Exporting the parse code might enable other uses besides
this one.

In either case we'd still need a check after connection to verify that
the password actually got *used*, so I guess that
PQconnectionUsedPassword isn't dead, just incomplete.

regards, tom lane

On 9/21/08, Joe Conway <mail(at)joeconway(dot)com> wrote:
> Marko Kreen wrote:
> > You need to ignore pg_service also. (And PGPASSWORD)
>
> Why? pg_service does not appear to support wildcards, so what is the attack
> vector?

"service=foo host=custom"

> And on PGPASSWORD, the fine manual says the following:
>
> PGPASSWORD sets the password used if the server demands password
> authentication. Use of this environment variable is not recommended
> for security reasons (some operating systems allow non-root users to
> see process environment variables via ps); instead consider using the
> ~/.pgpass file (see Section 30.13).

That does not mean it's OK to handle it insecurely.

If you want to solve the immediate problem with hack, then the cleanest
hack would be "no-external-sources-for-connection-details"-hack.

Leaving the less probable paths open is just sloppy attitude.

> At the moment the only real issue I can see is .pgpass when wildcards are
> used for hostname:port:database.

Well, the real issue is that lusers are allowed to freely launch
connections, that's the source for all the other problems.

--
marko

"Marko Kreen" <markokr(at)gmail(dot)com> writes:
> On 9/21/08, Joe Conway <mail(at)joeconway(dot)com> wrote:
>> Why? pg_service does not appear to support wildcards, so what is the attack
>> vector?

> "service=foo host=custom"

The proposal to require a password = foo entry in the conn string seems
to resolve all of these, without taking away useful capability. I don't
think that forbidding use of services altogether is a good thing.

So that seems to tilt the decision towards exposing the conninfo_parse
function. Joe, do you want to have a go at it, or shall I?

regards, tom lane

Tom Lane wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
>> On 9/21/08, Joe Conway <mail(at)joeconway(dot)com> wrote:
>>> Why? pg_service does not appear to support wildcards, so what is the attack
>>> vector?
>
>> "service=foo host=custom"
>
> The proposal to require a password = foo entry in the conn string seems
> to resolve all of these, without taking away useful capability. I don't
> think that forbidding use of services altogether is a good thing.
>
> So that seems to tilt the decision towards exposing the conninfo_parse
> function. Joe, do you want to have a go at it, or shall I?

Agreed. I'll take a stab at it.

Joe

Tom Lane wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
>> On 9/21/08, Joe Conway <mail(at)joeconway(dot)com> wrote:
>>> Why? pg_service does not appear to support wildcards, so what is the attack
>>> vector?
>
>> "service=foo host=custom"
>
> The proposal to require a password = foo entry in the conn string seems
> to resolve all of these, without taking away useful capability. I don't
> think that forbidding use of services altogether is a good thing.
>
> So that seems to tilt the decision towards exposing the conninfo_parse
> function. Joe, do you want to have a go at it, or shall I?

Here's a first shot.

Notes:
1. I have not removed PQconnectionUsedPassword and related. It
is still needed to prevent a non-superuser from logging in
as the superuser if the server does not require authentication.
In that case, any bogus password could be added to the connection
string and be subsequently ignored, if not for this check.
2. I assume this ought to be applied as two separate commits --
one for libpq, and one for dblink.
3. I can't easily verify that I got libpq.sgml perfect; I've gotten out
of sync with the required tool chain for the docs

Comments?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> So that seems to tilt the decision towards exposing the conninfo_parse
>> function. Joe, do you want to have a go at it, or shall I?

> Here's a first shot.

Hmm ... one problem with this is that the caller can't tell
failure-because-out-of-memory from failure-because-string-is-bogus.
That doesn't matter for your proposed dblink patch, but I had been
thinking of documenting that if someone wanted to get an error message
explaining just what was wrong with the conninfo string, they could
try to open a connection with it and use the resulting failure message.
But it's just barely conceivable that the PQconnect call *wouldn't* fail
because out-of-memory. (Not very likely in a sequential application,
but definitely seems possible in a multithreaded app --- some other
thread could release memory meanwhile.) Is it worth having the
PQconninfoParse function pass back the error message to avoid this
corner case? The API would be a lot more ugly, perhaps

int PQconninfoParse(const char *connstr,
PQconninfoOption **options,
char **errmsg)

okay: *options is set, *errmsg is NULL, return true
bogus string: *options is NULL, *errmsg is set, return false
out of memory: both outputs NULL, return false

regards, tom lane

Tom Lane wrote:
> Hmm ... one problem with this is that the caller can't tell
> failure-because-out-of-memory from failure-because-string-is-bogus.

<snip>

> Is it worth having the
> PQconninfoParse function pass back the error message to avoid this
> corner case?

I thought briefly about it, and wasn't sure it would be worth the ugliness.

> The API would be a lot more ugly, perhaps

> int PQconninfoParse(const char *connstr,
> PQconninfoOption **options,
> char **errmsg)
>
> okay: *options is set, *errmsg is NULL, return true
> bogus string: *options is NULL, *errmsg is set, return false
> out of memory: both outputs NULL, return false

conninfo_parse() returns NULL on error, so why not something like:

PQconninfoOption *
PQconninfoParse(const char *conninfo, char **errmsg)
{
PQExpBufferData errorBuf;
bool password_from_string;
PQconninfoOption *connOptions;

initPQExpBuffer(&errorBuf);
connOptions = conninfo_parse(conninfo, &errorBuf,
&password_from_string);

if (!connOptions && errmsg)
*errmsg = pstrdup(errorBuf.data);

termPQExpBuffer(&errorBuf);
return connOptions;
}

If the return value is NULL, use errmsg if you'd like. I'd guess in most
instances you don't even need to bother freeing errmsg as it is in a
limited life memory context.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> If the return value is NULL, use errmsg if you'd like. I'd guess in most
> instances you don't even need to bother freeing errmsg as it is in a
> limited life memory context.

Uh, you're confusing the backend environment with libpq's much more
spartan lifestyle. errmsg will be malloc'd and it will *not* go away
unless the caller free()s it.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> If the return value is NULL, use errmsg if you'd like. I'd guess in most
>> instances you don't even need to bother freeing errmsg as it is in a
>> limited life memory context.
>
> Uh, you're confusing the backend environment with libpq's much more
> spartan lifestyle. errmsg will be malloc'd and it will *not* go away
> unless the caller free()s it.

Yup, just figured that out. Otherwise OK with it?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> Uh, you're confusing the backend environment with libpq's much more
>> spartan lifestyle. errmsg will be malloc'd and it will *not* go away
>> unless the caller free()s it.

> Yup, just figured that out. Otherwise OK with it?

Yeah. We could make one further refinement: callers that don't care
about acquiring an error string can pass NULL for the errmsg parameter.
That tells PQconninfoParse to throw away the errmsg string anyway.
With that, the minimal case isn't much uglier than your original:
just need a NULL arg tacked onto the call.

BTW, the usual method for doing this is just to give the caller back the
errorBuf.data, not incur an additional strdup that could fail.

regards, tom lane

Tom Lane wrote:
> Yeah. We could make one further refinement: callers that don't care
> about acquiring an error string can pass NULL for the errmsg parameter.
> That tells PQconninfoParse to throw away the errmsg string anyway.
> With that, the minimal case isn't much uglier than your original:
> just need a NULL arg tacked onto the call.

True

> BTW, the usual method for doing this is just to give the caller back the
> errorBuf.data, not incur an additional strdup that could fail.

OK, was entirely sure that was safe.

New patch attached.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> New patch attached.

This is close, but you're failing to guard against a few out-of-memory
corner cases (and now that I look, PQconndefaults() is too). The libpq
documentation needs more work than this, too.

I'll make a cleanup pass and commit.

BTW, I'm quite tempted to get rid of pgpass_from_client and simplify the
specification of PQconnectionUsedPassword to be "did the server request
a password?". Thoughts?

regards, tom lane

Joe Conway <mail(at)joeconway(dot)com> writes:
> New patch attached.

erm ... wait a minute. This approach doesn't actually solve the problem
at all, because conninfo_parse is responsible for filling in various
sorts of default values. In particular it would happily pull a password
from the services file or the PGPASSWORD environment variable, and
looking at the array after the fact doesn't tell whether that happened.

Refactoring doesn't seem like an easy way to fix this, because of the
problem that the behavior of pulling up defaults is part of the API
specification for PQconndefaults().

Thoughts?

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> New patch attached.
>
> erm ... wait a minute. This approach doesn't actually solve the problem
> at all, because conninfo_parse is responsible for filling in various
> sorts of default values. In particular it would happily pull a password
> from the services file or the PGPASSWORD environment variable, and
> looking at the array after the fact doesn't tell whether that happened.
>
> Refactoring doesn't seem like an easy way to fix this, because of the
> problem that the behavior of pulling up defaults is part of the API
> specification for PQconndefaults().
>
> Thoughts?

Hmm, I could have sworn I looked for that, and saw it elsewhere. Anyway,
you are obviously correct.

conninfo_parse() is presently only called from a few places -- maybe we
should have conninfo_parse() really just parse, and create a new
conninfo_get_missing() or some such that fills in missing values?

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> Refactoring doesn't seem like an easy way to fix this, because of the
>> problem that the behavior of pulling up defaults is part of the API
>> specification for PQconndefaults().

> conninfo_parse() is presently only called from a few places -- maybe we
> should have conninfo_parse() really just parse, and create a new
> conninfo_get_missing() or some such that fills in missing values?

Doh, I must be too tired, because now that seems obvious. Will set this
aside and try it again tomorrow.

regards, tom lane

Joe Conway wrote:
> Tom Lane wrote:
>> Refactoring doesn't seem like an easy way to fix this, because of the
>> problem that the behavior of pulling up defaults is part of the API
>> specification for PQconndefaults().
>>
>> Thoughts?
>
> Hmm, I could have sworn I looked for that, and saw it elsewhere. Anyway,
> you are obviously correct.
>
> conninfo_parse() is presently only called from a few places -- maybe we
> should have conninfo_parse() really just parse, and create a new
> conninfo_get_missing() or some such that fills in missing values?

Maybe better:

static PQconninfoOption *
conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
bool fill_defaults, bool *password_from_string)

There are only three call sites including the new one. The two originals
could use fill_defaults == true, and PQconninfoParse could use false.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Maybe better:

> static PQconninfoOption *
> conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
> bool fill_defaults, bool *password_from_string)

I'm thinking a separate conninfo_fill_defaults function is better,
though it's not a big deal.

What do you think about getting rid of the password_from_string state
variable? It was always a bit of a kluge, and we don't seem to need
it anymore with this approach.

regards, tom lane

Tom Lane wrote:
>
> What do you think about getting rid of the password_from_string state
> variable? It was always a bit of a kluge, and we don't seem to need
> it anymore with this approach.

It is still used in PQconnectionUsedPassword(). That is still needed to
prevent a non-superuser from logging in as the superuser if the server
does not require authentication. In that case, any bogus password could
be added to the connection string and be subsequently ignored, if not
for this check.

e.g. with a default pg_hba.conf

8<-------------------------------------
psql contrib_regression -U luser
psql (8.4devel)
Type "help" for help.

contrib_regression=> SELECT dblink_connect('password=luser
dbname=contrib_regression');
ERROR: password is required
DETAIL: Non-superuser cannot connect if the server does not request a
password.
HINT: Target server's authentication method must be changed.
8<-------------------------------------

Without PQconnectionUsedPassword() that would have succeeded in logging
in as the superuser, because the password is never actually checked.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> What do you think about getting rid of the password_from_string state
>> variable? It was always a bit of a kluge, and we don't seem to need
>> it anymore with this approach.

> It is still used in PQconnectionUsedPassword(). That is still needed to
> prevent a non-superuser from logging in as the superuser if the server
> does not require authentication.

No, the test to see if the server actually *asked* for the password is
the important part at that end.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> Tom Lane wrote:
>>> What do you think about getting rid of the password_from_string state
>>> variable? It was always a bit of a kluge, and we don't seem to need
>>> it anymore with this approach.
>
>> It is still used in PQconnectionUsedPassword(). That is still needed to
>> prevent a non-superuser from logging in as the superuser if the server
>> does not require authentication.
>
> No, the test to see if the server actually *asked* for the password is
> the important part at that end.

Oh, I see that now. So yes, as far as I can tell, password_from_string
is not used for anything anymore and should be removed.

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tom Lane wrote:
>> No, the test to see if the server actually *asked* for the password is
>> the important part at that end.

> Oh, I see that now. So yes, as far as I can tell, password_from_string
> is not used for anything anymore and should be removed.

Okay. I just committed the patch without that change, but I'll go back
and add it.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>> Tom Lane wrote:
>>> No, the test to see if the server actually *asked* for the password is
>>> the important part at that end.
>
>> Oh, I see that now. So yes, as far as I can tell, password_from_string
>> is not used for anything anymore and should be removed.
>
> Okay. I just committed the patch without that change, but I'll go back
> and add it.

I'm not quite sure I fully understand the consequence of this change.
Does it basically mean that it's not possible to use .pgpass with dblink
for authentication?
The alternative then would be to hardcode the password in your stored
procedures, or store it in a separate table somehow?

--
Tommy Gildseth

Tommy Gildseth wrote:
> Tom Lane wrote:
>> Okay. I just committed the patch without that change, but I'll go back
>> and add it.
>
> I'm not quite sure I fully understand the consequence of this change.
> Does it basically mean that it's not possible to use .pgpass with dblink
> for authentication?

It only applies to 8.4 (which is not yet released) and beyond.

dblink will still work as before for superusers.

> The alternative then would be to hardcode the password in your stored
> procedures, or store it in a separate table somehow?

Trusted non-superusers can be granted permission to use dblink_connect_u().

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> Tommy Gildseth wrote:
>> I'm not quite sure I fully understand the consequence of this change.
>> Does it basically mean that it's not possible to use .pgpass with dblink
>> for authentication?

> It only applies to 8.4 (which is not yet released) and beyond.
> dblink will still work as before for superusers.

The visible, documented behavior actually is not any different from what
it's been in recent PG releases. This change only plugs a possible
security issue that we weren't aware of before, ie, that dblink might
send a password to a server before failing the connect attempt. It will
fail the connect attempt either way, though, so no functionality
changes.

regards, tom lane