Fix dblink_connect() so that it verifies that a password is supplied in the

conninfo string *before* trying to connect to the remote server, not after.
As pointed out by Marko Kreen, in certain not-very-plausible situations
this could result in sending a password from the postgres user's .pgpass file,
or other places that non-superusers shouldn't have access to, to an
untrustworthy remote server.  The cleanest fix seems to be to expose libpq's
conninfo-string-parsing code so that dblink can check for a password option
without duplicating the parsing logic.

Joe Conway, with a little cleanup by Tom Lane

contrib/dblink/dblink.c

@@ -8,7 +8,7 @@
  * Darko Prenosil <Darko.Prenosil@finteh.hr>
  * Shridhar Daithankar <shridhar_daithankar@persistent.co.in>
  *
- * $PostgreSQL: pgsql/contrib/dblink/dblink.c,v 1.74 2008/07/03 03:56:57 joe Exp $
+ * $PostgreSQL: pgsql/contrib/dblink/dblink.c,v 1.75 2008/09/22 13:55:13 tgl Exp $
  * Copyright (c) 2001-2008, PostgreSQL Global Development Group
  * ALL RIGHTS RESERVED;
  *
@@ -93,6 +93,7 @@ static int16 get_attnum_pk_pos(int2vector *pkattnums, int16 pknumatts, int16 key
 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,6 +166,7 @@ typedef struct remoteConnHashEnt
 			else \
 			{ \
 				connstr = conname_or_str; \
+				dblink_connstr_check(connstr); \
 				conn = PQconnectdb(connstr); \
 				if (PQstatus(conn) == CONNECTION_BAD) \
 				{ \
@@ -229,6 +231,9 @@ dblink_connect(PG_FUNCTION_ARGS)
 
 	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,7 +251,7 @@ dblink_connect(PG_FUNCTION_ARGS)
 				 errdetail("%s", msg)));
 	}
 
-	/* check password used if not superuser */
+	/* check password actually used if not superuser */
 	dblink_security_check(conn, rconn);
 
 	if (connname)
@@ -2251,6 +2256,46 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
 	}
 }
 
+/*
+ * For non-superusers, insist that the connstr specify a password.  This
+ * prevents a password from being picked up from .pgpass, a service file,
+ * the environment, etc.  We don't want the postgres user's passwords
+ * to be accessible to non-superusers.
+ */
+static void
+dblink_connstr_check(const char *connstr)
+{
+	if (!superuser())
+	{
+		PQconninfoOption   *options;
+		PQconninfoOption   *option;
+		bool				connstr_gives_password = false;
+
+		options = PQconninfoParse(connstr, NULL);
+		if (options)
+		{
+			for (option = options; option->keyword != NULL; option++)
+			{
+				if (strcmp(option->keyword, "password") == 0)
+				{
+					if (option->val != NULL && option->val[0] != '\0')
+					{
+						connstr_gives_password = true;
+						break;
+					}
+				}
+			}
+			PQconninfoFree(options);
+		}
+
+		if (!connstr_gives_password)
+			ereport(ERROR,
+					(errcode(ERRCODE_S_R_E_PROHIBITED_SQL_STATEMENT_ATTEMPTED),
+					 errmsg("password is required"),
+					 errdetail("Non-superusers 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)
 {

doc/src/sgml/dblink.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/dblink.sgml,v 1.4 2008/04/04 16:57:21 momjian Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/dblink.sgml,v 1.5 2008/09/22 13:55:13 tgl Exp $ -->
 
 <sect1 id="dblink">
  <title>dblink</title>
@@ -140,12 +140,19 @@
     involve a password, then impersonation and subsequent escalation of
     privileges can occur, because the session will appear to have
     originated from the user as which the local <productname>PostgreSQL</>
-    server runs.  Therefore, <function>dblink_connect_u()</> is initially
+    server runs.  Also, even if the remote server does demand a password,
+    it is possible for the password to be supplied from the server
+    environment, such as a <filename>~/.pgpass</> file belonging to the
+    server's user.  This opens not only a risk of impersonation, but the
+    possibility of exposing a password to an untrustworthy remote server.
+    Therefore, <function>dblink_connect_u()</> is initially
     installed with all privileges revoked from <literal>PUBLIC</>,
     making it un-callable except by superusers.  In some situations
     it may be appropriate to grant <literal>EXECUTE</> permission for
     <function>dblink_connect_u()</> to specific users who are considered
-    trustworthy, but this should be done with care.
+    trustworthy, but this should be done with care.  It is also recommended
+    that any <filename>~/.pgpass</> file belonging to the server's user
+    <emphasis>not</> contain any records specifying a wildcard host name.
    </para>
 
    <para>

doc/src/sgml/libpq.sgml

@@ -1,4 +1,4 @@
-<!-- $PostgreSQL: pgsql/doc/src/sgml/libpq.sgml,v 1.263 2008/09/19 20:06:13 tgl Exp $ -->
+<!-- $PostgreSQL: pgsql/doc/src/sgml/libpq.sgml,v 1.264 2008/09/22 13:55:13 tgl Exp $ -->
 
 <chapter id="libpq">
  <title><application>libpq</application> - C Library</title>
@@ -593,7 +593,7 @@ typedef struct
     char   *compiled;  /* Fallback compiled in default value */
     char   *val;       /* Option's current value, or NULL */
     char   *label;     /* Label for field in connect dialog */
-    char   *dispchar;  /* Character to display for this field
+    char   *dispchar;  /* Indicates how to display this field
                           in a connect dialog. Values are:
                           ""        Display entered value as is
                           "*"       Password field - hide value
@@ -624,6 +624,51 @@ typedef struct
      </listitem>
     </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>
+
+      <para>
+       Parses a connection string and returns the resulting options as an
+       array; or returns NULL if there is a problem with the connection
+       string.  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.
+      </para>
+
+      <para>
+       Note that only options explicitly specified in the string will have
+       values set in the result array; no defaults are inserted.
+      </para>
+
+      <para>
+       If <literal>errmsg</> is not NULL, then <literal>*errmsg</> is set
+       to NULL on success, else to a malloc'd error string explaining
+       the problem.  (It is also possible for <literal>*errmsg</> to be
+       set to NULL even when NULL is returned; this indicates an out-of-memory
+       situation.)
+      </para>
+
+      <para>
+       After processing the options array, free it by passing it to
+       <function>PQconninfoFree</function>.  If this is not done, some memory
+       is leaked for each call to <function>PQconninfoParse</function>.
+       Conversely, if an error occurs and <literal>errmsg</> is not NULL,
+       be sure to free the error string using <function>PQfreemem</>.
+      </para>
+
+   </listitem>
+    </varlistentry>
+
     <varlistentry>
      <term><function>PQfinish</function><indexterm><primary>PQfinish</></></term>
      <listitem>
@@ -2985,39 +3030,6 @@ typedef struct {
       </para>
      </listitem>
     </varlistentry>
-
-    <varlistentry>
-     <term>
-      <function>PQfreemem</function>
-      <indexterm>
-       <primary>PQfreemem</primary>
-      </indexterm>
-     </term>
-
-     <listitem>
-      <para>
-       Frees memory allocated by <application>libpq</>.
-       <synopsis>
-        void PQfreemem(void *ptr);
-       </synopsis>
-      </para>
-
-      <para>
-       Frees memory allocated by <application>libpq</>, particularly
-       <function>PQescapeByteaConn</function>,
-       <function>PQescapeBytea</function>,
-       <function>PQunescapeBytea</function>,
-       and <function>PQnotifies</function>.
-       It is particularly important that this function, rather than
-       <function>free()</>, be used on Microsoft Windows.  This is because
-       allocating memory in a DLL and releasing it in the application works
-       only if multithreaded/single-threaded, release/debug, and static/dynamic
-       flags are the same for the DLL and the application.  On non-Microsoft
-       Windows platforms, this function is the same as the standard library
-       function <function>free()</>.
-      </para>
-     </listitem>
-    </varlistentry>
    </variablelist>
 
   </sect2>
@@ -4537,6 +4549,63 @@ char *pg_encoding_to_char(int <replaceable>encoding_id</replaceable>);
   </para>
 
   <variablelist>
+   <varlistentry>
+    <term>
+     <function>PQfreemem</function>
+     <indexterm>
+      <primary>PQfreemem</primary>
+     </indexterm>
+    </term>
+
+    <listitem>
+     <para>
+      Frees memory allocated by <application>libpq</>.
+      <synopsis>
+       void PQfreemem(void *ptr);
+      </synopsis>
+     </para>
+
+     <para>
+      Frees memory allocated by <application>libpq</>, particularly
+      <function>PQescapeByteaConn</function>,
+      <function>PQescapeBytea</function>,
+      <function>PQunescapeBytea</function>,
+      and <function>PQnotifies</function>.
+      It is particularly important that this function, rather than
+      <function>free()</>, be used on Microsoft Windows.  This is because
+      allocating memory in a DLL and releasing it in the application works
+      only if multithreaded/single-threaded, release/debug, and static/dynamic
+      flags are the same for the DLL and the application.  On non-Microsoft
+      Windows platforms, this function is the same as the standard library
+      function <function>free()</>.
+     </para>
+    </listitem>
+   </varlistentry>
+
+   <varlistentry>
+    <term>
+     <function>PQconninfoFree</function>
+     <indexterm>
+      <primary>PQconninfoFree</primary>
+     </indexterm>
+    </term>
+
+    <listitem>
+     <para>
+      Frees the data structures allocated by
+      <function>PQconndefaults</> or <function>PQconninfoParse</>.
+      <synopsis>
+       void PQconninfoFree(PQconninfoOption *connOptions);
+      </synopsis>
+     </para>
+
+     <para>
+      A simple <function>PQfreemem</function> will not do for this, since
+      the array contains references to subsidiary strings.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term>
      <function>PQencryptPassword</function>

src/interfaces/libpq/exports.txt

@@ -1,4 +1,4 @@
-# $PostgreSQL: pgsql/src/interfaces/libpq/exports.txt,v 1.21 2008/09/19 20:06:13 tgl Exp $
+# $PostgreSQL: pgsql/src/interfaces/libpq/exports.txt,v 1.22 2008/09/22 13:55:14 tgl Exp $
 # Functions to be exported by libpq DLLs
 PQconnectdb               1
 PQsetdbLogin              2
@@ -151,3 +151,4 @@ PQsetInstanceData         148
 PQresultInstanceData      149
 PQresultSetInstanceData   150
 PQfireResultCreateEvents  151
+PQconninfoParse           152

src/interfaces/libpq/fe-connect.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/interfaces/libpq/fe-connect.c,v 1.360 2008/09/17 04:31:08 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/interfaces/libpq/fe-connect.c,v 1.361 2008/09/22 13:55:14 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -232,7 +232,8 @@ static PGconn *makeEmptyPGconn(void);
 static void freePGconn(PGconn *conn);
 static void closePGconn(PGconn *conn);
 static PQconninfoOption *conninfo_parse(const char *conninfo,
-			   PQExpBuffer errorMessage, bool *password_from_string);
+			   PQExpBuffer errorMessage, bool use_defaults,
+			   bool *password_from_string);
 static char *conninfo_getval(PQconninfoOption *connOptions,
 				const char *keyword);
 static void defaultNoticeReceiver(void *arg, const PGresult *res);
@@ -376,7 +377,7 @@ connectOptions1(PGconn *conn, const char *conninfo)
 	/*
 	 * Parse the conninfo string
 	 */
-	connOptions = conninfo_parse(conninfo, &conn->errorMessage,
+	connOptions = conninfo_parse(conninfo, &conn->errorMessage, true,
 								 &conn->pgpass_from_client);
 	if (connOptions == NULL)
 	{
@@ -542,7 +543,9 @@ connectOptions2(PGconn *conn)
  *		PQconndefaults
  *
  * Parse an empty string like PQconnectdb() would do and return the
- * working connection options array.
+ * resulting connection options array, ie, all the default values that are
+ * available from the environment etc.  On error (eg out of memory),
+ * NULL is returned.
  *
  * Using this function, an application may determine all possible options
  * and their current default values.
@@ -561,7 +564,10 @@ PQconndefaults(void)
 	PQconninfoOption *connOptions;
 
 	initPQExpBuffer(&errorBuf);
-	connOptions = conninfo_parse("", &errorBuf, &password_from_string);
+	if (errorBuf.data == NULL)
+		return NULL;			/* out of memory already :-( */
+	connOptions = conninfo_parse("", &errorBuf, true,
+								 &password_from_string);
 	termPQExpBuffer(&errorBuf);
 	return connOptions;
 }
@@ -3102,18 +3108,56 @@ parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage)
 }
 
 
+/*
+ *		PQconninfoParse
+ *
+ * Parse a string like PQconnectdb() would do and return the
+ * resulting connection options array.  NULL is returned on failure.
+ * The result contains only options specified directly in the string,
+ * not any possible default values.
+ *
+ * If errmsg isn't NULL, *errmsg is set to NULL on success, or a malloc'd
+ * string on failure (use PQfreemem to free it).  In out-of-memory conditions
+ * both *errmsg and the result could be NULL.
+ *
+ * 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;
+
+	if (errmsg)
+		*errmsg = NULL;			/* default */
+	initPQExpBuffer(&errorBuf);
+	if (errorBuf.data == NULL)
+		return NULL;			/* out of memory already :-( */
+	connOptions = conninfo_parse(conninfo, &errorBuf, false,
+								 &password_from_string);
+	if (connOptions == NULL && errmsg)
+		*errmsg = errorBuf.data;
+	else
+		termPQExpBuffer(&errorBuf);
+	return connOptions;
+}
+
 /*
  * Conninfo parser routine
  *
  * If successful, a malloc'd PQconninfoOption array is returned.
  * If not successful, NULL is returned and an error message is
  * left in errorMessage.
+ * Defaults are supplied (from a service file, environment variables, etc)
+ * for unspecified options, but only if use_defaults is TRUE.
  * *password_from_string is set TRUE if we got a password from the
  * conninfo string, otherwise FALSE.
  */
 static PQconninfoOption *
 conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
-			   bool *password_from_string)
+			   bool use_defaults, bool *password_from_string)
 {
 	char	   *pname;
 	char	   *pval;
@@ -3293,6 +3337,12 @@ conninfo_parse(const char *conninfo, PQExpBuffer errorMessage,
 	/* Done with the modifiable input string */
 	free(buf);
 
+	/*
+	 * Stop here if caller doesn't want defaults filled in.
+	 */
+	if (!use_defaults)
+		return options;
+
 	/*
 	 * If there's a service spec, use it to obtain any not-explicitly-given
 	 * parameters.

src/interfaces/libpq/libpq-fe.h

@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-fe.h,v 1.143 2008/09/17 04:31:08 tgl Exp $
+ * $PostgreSQL: pgsql/src/interfaces/libpq/libpq-fe.h,v 1.144 2008/09/22 13:55:14 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -164,6 +164,7 @@ typedef struct _PQprintOpt
 
 /* ----------------
  * Structure for the conninfo parameter definitions returned by PQconndefaults
+ * or PQconninfoParse.
  *
  * All fields except "val" point at static strings which must not be altered.
  * "val" is either NULL or a malloc'd current-value string.  PQconninfoFree()
@@ -177,7 +178,7 @@ typedef struct _PQconninfoOption
 	char	   *compiled;		/* Fallback compiled in default value	*/
 	char	   *val;			/* Option's current value, or NULL		 */
 	char	   *label;			/* Label for field in connect dialog	*/
-	char	   *dispchar;		/* Character to display for this field in a
+	char	   *dispchar;		/* Indicates how to display this field in a
 								 * connect dialog. Values are: "" Display
 								 * entered value as is "*" Password field -
 								 * hide value "D"  Debug option - don't show
@@ -243,7 +244,10 @@ extern void PQfinish(PGconn *conn);
 /* get info about connection options known to PQconnectdb */
 extern PQconninfoOption *PQconndefaults(void);
 
-/* free the data structure returned by PQconndefaults() */
+/* parse connection options in same way as PQconnectdb */
+extern PQconninfoOption *PQconninfoParse(const char *conninfo, char **errmsg);
+
+/* free the data structure returned by PQconndefaults() or PQconninfoParse() */
 extern void PQconninfoFree(PQconninfoOption *connOptions);
 
 /*