Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Tom Lane wrote: Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Tom Lane wrote: Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
On 9/21/08, Tom Lane [EMAIL PROTECTED] wrote: Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Marko Kreen wrote: On 9/21/08, Tom Lane [EMAIL PROTECTED] wrote: Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
On 9/21/08, Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Tom Lane wrote: Marko Kreen [EMAIL PROTECTED] writes: On 9/21/08, Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Marko Kreen [EMAIL PROTECTED] writes: On 9/21/08, Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Tom Lane wrote:
Marko Kreen [EMAIL PROTECTED] writes:
On 9/21/08, Joe Conway [EMAIL PROTECTED] 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 - 1.74
--- contrib/dblink/dblink.c 22 Sep 2008 00:34:17 -
***
*** 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;
+ boolconn_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 - 1.263
--- doc/src/sgml/libpq.sgml 21 Sep 2008 23:08:27 -
***
*** 625,630
--- 625,658
/varlistentry
varlistentry
+ termfunctionPQconninfoParse/functionindextermprimaryPQconninfoParse///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 functionPQconnectdb/function options in the provided
+connection string. The return value points to an array of
+structnamePQconninfoOption/structname structures, which ends
+with an entry having a null structfieldkeyword/ pointer. The
+null pointer is returned if memory could not be allocated.
+ /para
+
+ para
+
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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;
boolpassword_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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Tom Lane wrote: Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 - 1.74
--- contrib/dblink/dblink.c 22 Sep 2008 02:09:39 -
***
*** 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;
+ boolconn_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 - 1.263
--- doc/src/sgml/libpq.sgml 22 Sep 2008 02:08:50 -
***
*** 625,630
--- 625,661
/varlistentry
varlistentry
+ termfunctionPQconninfoParse/functionindextermprimaryPQconninfoParse///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 functionPQconnectdb/function options in the provided
+connection string. The return value points to an array of
+structnamePQconninfoOption/structname structures, which ends
+with an entry having a null structfieldkeyword/ pointer. The
+null pointer is returned if an error occurs. In this case,
+literalerrmsg/literal contains the error message. Passing
+literalNULL/literal for literalerrmsg/literal tells
+functionPQconninfoParse/function to throw away the error message.
+ /para
+
+ para
+After processing the options array, free it by passing it to
+functionPQconninfoFree/function. If this is not done, a small amount of memory
+is leaked for each call to functionPQconndefaults/function.
+ /para
+
+/listitem
+ /varlistentry
+
+ varlistentry
termfunctionPQfinish/functionindextermprimaryPQfinish///term
listitem
para
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Tom Lane wrote: Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 - 1.74
--- dblink.c 10 Aug 2008 04:59:05 -
***
*** 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
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
Joe Conway [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
On 9/12/08, Alvaro Herrera [EMAIL PROTECTED] 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/[EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [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/[EMAIL PROTECTED] Thoughts? Can we really expect SQL/MED connection mgmt to be done for the next fest? -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] fix dblink security hole
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/[EMAIL PROTECTED] 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 [EMAIL PROTECTED] http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: [EMAIL PROTECTED] Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
