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

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;