Re: [patch] fix dblink security hole - Mailing list pgsql-hackers

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);