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

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