[patch] fix dblink security hole

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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com

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

On 9/12/08, Alvaro Herrera <alvherre@commandprompt.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@joeconway.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@joeconway.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@sss.pgh.pa.us> wrote:

Joe Conway <mail@joeconway.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@sss.pgh.pa.us> wrote:

Joe Conway <mail@joeconway.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@joeconway.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@joeconway.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@gmail.com> writes:

On 9/21/08, Joe Conway <mail@joeconway.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@gmail.com> writes:

On 9/21/08, Joe Conway <mail@joeconway.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@gmail.com> writes:

On 9/21/08, Joe Conway <mail@joeconway.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@joeconway.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@joeconway.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