Thread: [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
Attachment
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 usersmay have different requirements (eg plproxy wants to use async API for connecting), and anyway it should handle non-Postgresconnection 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
Index: dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.74
diff -c -r1.74 dblink.c
*** dblink.c 3 Jul 2008 03:56:57 -0000 1.74
--- dblink.c 10 Aug 2008 04:59:05 -0000
***************
*** 93,99 ****
static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
! static void dblink_security_check(PGconn *conn, remoteConn *rconn);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
/* Global */
--- 93,99 ----
static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
! static void dblink_security_check(void);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
/* Global */
***************
*** 164,169 ****
--- 164,170 ----
} \
else \
{ \
+ dblink_security_check(); \
connstr = conname_or_str; \
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
***************
*** 175,181 ****
errmsg("could not establish connection"), \
errdetail("%s", msg))); \
} \
- dblink_security_check(conn, rconn); \
freeconn = true; \
} \
} while (0)
--- 176,181 ----
***************
*** 229,234 ****
--- 229,237 ----
if (connname)
rconn = (remoteConn *) palloc(sizeof(remoteConn));
+
+ /* only connect if superuser */
+ dblink_security_check();
conn = PQconnectdb(connstr);
MemoryContextSwitchTo(oldcontext);
***************
*** 246,254 ****
errdetail("%s", msg)));
}
- /* check password used if not superuser */
- dblink_security_check(conn, rconn);
-
if (connname)
{
rconn->conn = conn;
--- 249,254 ----
***************
*** 2232,2253 ****
}
static void
! dblink_security_check(PGconn *conn, remoteConn *rconn)
{
if (!superuser())
{
! if (!PQconnectionUsedPassword(conn))
! {
! PQfinish(conn);
! if (rconn)
! pfree(rconn);
!
! ereport(ERROR,
! (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
! errmsg("password is required"),
! errdetail("Non-superuser cannot connect if the server does not request a password."),
! errhint("Target server's authentication method must be changed.")));
! }
}
}
--- 2232,2246 ----
}
static void
! dblink_security_check()
{
if (!superuser())
{
! ereport(ERROR,
! (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
! errmsg("superuser is required"),
! errdetail("Non-superuser cannot connect remotely."),
! errhint("Use dblink_connect_u to connect as superuser.")));
}
}
Index: dblink.sql.in
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.sql.in,v
retrieving revision 1.17
diff -c -r1.17 dblink.sql.in
*** dblink.sql.in 5 Apr 2008 02:44:42 -0000 1.17
--- dblink.sql.in 11 Aug 2008 03:44:34 -0000
***************
*** 3,10 ****
-- Adjust this setting to control where the objects get created.
SET search_path = public;
! -- dblink_connect now restricts non-superusers to password
! -- authenticated connections
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
--- 3,9 ----
-- Adjust this setting to control where the objects get created.
SET search_path = public;
! -- dblink_connect now rejects all non-superusers
CREATE OR REPLACE FUNCTION dblink_connect (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***************
*** 16,23 ****
LANGUAGE C STRICT;
-- dblink_connect_u allows non-superusers to use
! -- non-password authenticated connections, but initially
! -- privileges are revoked from public
CREATE OR REPLACE FUNCTION dblink_connect_u (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
--- 15,21 ----
LANGUAGE C STRICT;
-- dblink_connect_u allows non-superusers to use
! -- connections, but initially privileges are revoked from public
CREATE OR REPLACE FUNCTION dblink_connect_u (text)
RETURNS text
AS 'MODULE_PATHNAME','dblink_connect'
***************
*** 202,204 ****
--- 200,229 ----
RETURNS text
AS 'MODULE_PATHNAME', 'dblink_error_message'
LANGUAGE C STRICT;
+
+ -- dblink_u and dblink_exec_u allows non-superusers to use
+ -- connect strings, but initially privileges are revoked from public
+
+ CREATE OR REPLACE FUNCTION dblink_u (text, text)
+ RETURNS setof record
+ AS 'MODULE_PATHNAME','dblink_record'
+ LANGUAGE C STRICT SECURITY DEFINER;
+ REVOKE ALL ON FUNCTION dblink_u (text, text) FROM public;
+
+ CREATE OR REPLACE FUNCTION dblink_u (text, text, boolean)
+ RETURNS setof record
+ AS 'MODULE_PATHNAME','dblink_record'
+ LANGUAGE C STRICT SECURITY DEFINER;
+ REVOKE ALL ON FUNCTION dblink_u (text, text, boolean) FROM public;
+
+ CREATE OR REPLACE FUNCTION dblink_exec_u (text, text)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_exec'
+ LANGUAGE C STRICT SECURITY DEFINER;
+ REVOKE ALL ON FUNCTION dblink_exec_u (text, text) FROM public;
+
+ CREATE OR REPLACE FUNCTION dblink_exec_u (text, text, boolean)
+ RETURNS text
+ AS 'MODULE_PATHNAME','dblink_exec'
+ LANGUAGE C STRICT SECURITY DEFINER;
+ REVOKE ALL ON FUNCTION dblink_exec_u (text, text, boolean) FROM public;
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 notrecommended for security reasons (some operating systems allow non-root users to see process environment variables viaps); 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
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
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: > 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
"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
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
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
Index: contrib/dblink/dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.74
diff -c -r1.74 dblink.c
*** contrib/dblink/dblink.c 3 Jul 2008 03:56:57 -0000 1.74
--- contrib/dblink/dblink.c 22 Sep 2008 00:34:17 -0000
***************
*** 93,98 ****
--- 93,99 ----
static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
+ static void dblink_connstr_check(const char *connstr);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
***************
*** 165,170 ****
--- 166,172 ----
else \
{ \
connstr = conname_or_str; \
+ dblink_connstr_check(connstr); \
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
{ \
***************
*** 229,234 ****
--- 231,239 ----
if (connname)
rconn = (remoteConn *) palloc(sizeof(remoteConn));
+
+ /* check password in connection string if not superuser */
+ dblink_connstr_check(connstr);
conn = PQconnectdb(connstr);
MemoryContextSwitchTo(oldcontext);
***************
*** 246,252 ****
errdetail("%s", msg)));
}
! /* check password used if not superuser */
dblink_security_check(conn, rconn);
if (connname)
--- 251,257 ----
errdetail("%s", msg)));
}
! /* check password actually used if not superuser */
dblink_security_check(conn, rconn);
if (connname)
***************
*** 2252,2257 ****
--- 2257,2293 ----
}
static void
+ dblink_connstr_check(const char *connstr)
+ {
+ if (!superuser())
+ {
+ PQconninfoOption *options;
+ PQconninfoOption *option;
+ bool conn_used_password = false;
+
+ options = PQconninfoParse(connstr);
+ for (option = options; option->keyword != NULL; option++)
+ {
+ if (strcmp(option->keyword, "password") == 0)
+ {
+ if (option->val != NULL && option->val[0] != '\0')
+ {
+ conn_used_password = true;
+ break;
+ }
+ }
+ }
+ PQconninfoFree(options);
+
+ if (!conn_used_password)
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser must provide a password in the connection string.")));
+ }
+ }
+
+ static void
dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail)
{
int level;
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.263
diff -c -r1.263 libpq.sgml
*** doc/src/sgml/libpq.sgml 19 Sep 2008 20:06:13 -0000 1.263
--- doc/src/sgml/libpq.sgml 21 Sep 2008 23:08:27 -0000
***************
*** 625,630 ****
--- 625,658 ----
</varlistentry>
<varlistentry>
+ <term><function>PQconninfoParse</function><indexterm><primary>PQconninfoParse</></></term>
+ <listitem>
+ <para>
+ Returns parsed connection options from the provided connection string.
+
+ <synopsis>
+ PQconninfoOption *PQconninfoParse(const char *conninfo);
+ </synopsis>
+
+ <para>
+ Returns a connection options array. This can be used to determine
+ the <function>PQconnectdb</function> options in the provided
+ connection string. The return value points to an array of
+ <structname>PQconninfoOption</structname> structures, which ends
+ with an entry having a null <structfield>keyword</> pointer. The
+ null pointer is returned if memory could not be allocated.
+ </para>
+
+ <para>
+ After processing the options array, free it by passing it to
+ <function>PQconninfoFree</function>. If this is not done, a small amount of memory
+ is leaked for each call to <function>PQconndefaults</function>.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><function>PQfinish</function><indexterm><primary>PQfinish</></></term>
<listitem>
<para>
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.21
diff -c -r1.21 exports.txt
*** src/interfaces/libpq/exports.txt 19 Sep 2008 20:06:13 -0000 1.21
--- src/interfaces/libpq/exports.txt 21 Sep 2008 23:32:56 -0000
***************
*** 151,153 ****
--- 151,154 ----
PQresultInstanceData 149
PQresultSetInstanceData 150
PQfireResultCreateEvents 151
+ PQconninfoParse 152
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.360
diff -c -r1.360 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 17 Sep 2008 04:31:08 -0000 1.360
--- src/interfaces/libpq/fe-connect.c 21 Sep 2008 22:56:40 -0000
***************
*** 3101,3106 ****
--- 3101,3127 ----
return 0;
}
+ /*
+ * PQconninfoParse
+ *
+ * Parse a string like PQconnectdb() would do and return the
+ * working connection options array.
+ *
+ * NOTE: the returned array is dynamically allocated and should
+ * be freed when no longer needed via PQconninfoFree().
+ */
+ PQconninfoOption *
+ PQconninfoParse(const char *conninfo)
+ {
+ PQExpBufferData errorBuf;
+ bool password_from_string;
+ PQconninfoOption *connOptions;
+
+ initPQExpBuffer(&errorBuf);
+ connOptions = conninfo_parse(conninfo, &errorBuf, &password_from_string);
+ termPQExpBuffer(&errorBuf);
+ return connOptions;
+ }
/*
* Conninfo parser routine
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.143
diff -c -r1.143 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 17 Sep 2008 04:31:08 -0000 1.143
--- src/interfaces/libpq/libpq-fe.h 21 Sep 2008 22:57:37 -0000
***************
*** 243,248 ****
--- 243,251 ----
/* get info about connection options known to PQconnectdb */
extern PQconninfoOption *PQconndefaults(void);
+ /* parse connection options in same way as PQconnectdb */
+ extern PQconninfoOption *PQconninfoParse(const char *conninfo);
+
/* free the data structure returned by PQconndefaults() */
extern void PQconninfoFree(PQconninfoOption *connOptions);
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 truebogus string: *options is NULL, *errmsg is set, return falseout 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
Tom Lane wrote: > 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. Yup, just figured that out. Otherwise OK with it? Joe
Joe Conway <mail@joeconway.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
Index: contrib/dblink/dblink.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/dblink/dblink.c,v
retrieving revision 1.74
diff -c -r1.74 dblink.c
*** contrib/dblink/dblink.c 3 Jul 2008 03:56:57 -0000 1.74
--- contrib/dblink/dblink.c 22 Sep 2008 02:09:39 -0000
***************
*** 93,98 ****
--- 93,99 ----
static HeapTuple get_tuple_of_interest(Oid relid, int2vector *pkattnums, int16 pknumatts, char **src_pkattvals);
static Oid get_relid_from_relname(text *relname_text);
static char *generate_relation_name(Oid relid);
+ static void dblink_connstr_check(const char *connstr);
static void dblink_security_check(PGconn *conn, remoteConn *rconn);
static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
***************
*** 165,170 ****
--- 166,172 ----
else \
{ \
connstr = conname_or_str; \
+ dblink_connstr_check(connstr); \
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
{ \
***************
*** 229,234 ****
--- 231,239 ----
if (connname)
rconn = (remoteConn *) palloc(sizeof(remoteConn));
+
+ /* check password in connection string if not superuser */
+ dblink_connstr_check(connstr);
conn = PQconnectdb(connstr);
MemoryContextSwitchTo(oldcontext);
***************
*** 246,252 ****
errdetail("%s", msg)));
}
! /* check password used if not superuser */
dblink_security_check(conn, rconn);
if (connname)
--- 251,257 ----
errdetail("%s", msg)));
}
! /* check password actually used if not superuser */
dblink_security_check(conn, rconn);
if (connname)
***************
*** 2252,2257 ****
--- 2257,2293 ----
}
static void
+ dblink_connstr_check(const char *connstr)
+ {
+ if (!superuser())
+ {
+ PQconninfoOption *options;
+ PQconninfoOption *option;
+ bool conn_used_password = false;
+
+ options = PQconninfoParse(connstr, NULL);
+ for (option = options; option->keyword != NULL; option++)
+ {
+ if (strcmp(option->keyword, "password") == 0)
+ {
+ if (option->val != NULL && option->val[0] != '\0')
+ {
+ conn_used_password = true;
+ break;
+ }
+ }
+ }
+ PQconninfoFree(options);
+
+ if (!conn_used_password)
+ ereport(ERROR,
+ (errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+ errmsg("password is required"),
+ errdetail("Non-superuser must provide a password in the connection string.")));
+ }
+ }
+
+ static void
dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail)
{
int level;
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.263
diff -c -r1.263 libpq.sgml
*** doc/src/sgml/libpq.sgml 19 Sep 2008 20:06:13 -0000 1.263
--- doc/src/sgml/libpq.sgml 22 Sep 2008 02:08:50 -0000
***************
*** 625,630 ****
--- 625,661 ----
</varlistentry>
<varlistentry>
+ <term><function>PQconninfoParse</function><indexterm><primary>PQconninfoParse</></></term>
+ <listitem>
+ <para>
+ Returns parsed connection options from the provided connection string.
+
+ <synopsis>
+ PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+ </synopsis>
+
+ <para>
+ Returns a connection options array. This can be used to determine
+ the <function>PQconnectdb</function> options in the provided
+ connection string. The return value points to an array of
+ <structname>PQconninfoOption</structname> structures, which ends
+ with an entry having a null <structfield>keyword</> pointer. The
+ null pointer is returned if an error occurs. In this case,
+ <literal>errmsg</literal> contains the error message. Passing
+ <literal>NULL</literal> for <literal>errmsg</literal> tells
+ <function>PQconninfoParse</function> to throw away the error message.
+ </para>
+
+ <para>
+ After processing the options array, free it by passing it to
+ <function>PQconninfoFree</function>. If this is not done, a small amount of memory
+ is leaked for each call to <function>PQconndefaults</function>.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><function>PQfinish</function><indexterm><primary>PQfinish</></></term>
<listitem>
<para>
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.21
diff -c -r1.21 exports.txt
*** src/interfaces/libpq/exports.txt 19 Sep 2008 20:06:13 -0000 1.21
--- src/interfaces/libpq/exports.txt 21 Sep 2008 23:32:56 -0000
***************
*** 151,153 ****
--- 151,154 ----
PQresultInstanceData 149
PQresultSetInstanceData 150
PQfireResultCreateEvents 151
+ PQconninfoParse 152
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.360
diff -c -r1.360 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 17 Sep 2008 04:31:08 -0000 1.360
--- src/interfaces/libpq/fe-connect.c 22 Sep 2008 02:01:33 -0000
***************
*** 3101,3106 ****
--- 3101,3129 ----
return 0;
}
+ /*
+ * PQconninfoParse
+ *
+ * Parse a string like PQconnectdb() would do and return the
+ * working connection options array.
+ *
+ * NOTE: the returned array is dynamically allocated and should
+ * be freed when no longer needed via PQconninfoFree().
+ */
+ 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 = errorBuf.data;
+ termPQExpBuffer(&errorBuf);
+ return connOptions;
+ }
/*
* Conninfo parser routine
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.143
diff -c -r1.143 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 17 Sep 2008 04:31:08 -0000 1.143
--- src/interfaces/libpq/libpq-fe.h 22 Sep 2008 01:45:24 -0000
***************
*** 243,248 ****
--- 243,251 ----
/* get info about connection options known to PQconnectdb */
extern PQconninfoOption *PQconndefaults(void);
+ /* parse connection options in same way as PQconnectdb */
+ extern PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+
/* free the data structure returned by PQconndefaults() */
extern void PQconninfoFree(PQconninfoOption *connOptions);
Joe Conway <mail@joeconway.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@joeconway.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@joeconway.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@joeconway.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@joeconway.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@joeconway.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@joeconway.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@joeconway.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@joeconway.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@joeconway.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