Hi,
Here is my review of the dblink security hole patch written by Marko Kreen:
http://archives.postgresql.org/message-id/[EMAIL PROTECTED]
1 - The patch applies cleanly to the latest GIT repository.
2 - Regression passed before/after the patch.
3 - I have played around with the patch a little and haven't found any
issue.
[Code review]
4 - In my opinion we should not add the superuser check in "DBLINK_GET_CONN"
macro and in "dblink_connect" function because we already have a function
named "dblink_security_check" and there is a superuser check in the
function. In my opinion we should use this function and throw an error if
non super user is detected, because calling superuser function in multiple
places is not a good idea. Another point is that if we are not using the
function "dblink_security_check" then we should delete that function because
after the patch there will be no effect of this function (I think).
I have attached a patch just for the quick thought. Otherwise there is no
issue with patch.
[Reviewing a Patch] from the link
"http://wiki.postgresql.org/wiki/Reviewing_a_Patch";<http://wiki.postgresql.org/wiki/Reviewing_a_Patch>
Submission review
-----------------
- Is the patch in the standard form? ( Yes )
- Does it apply cleanly to the current CVS HEAD? ( I checked it with the
latest git repository)
- Does it include reasonable tests, docs, etc? ( In my opinion
there should be couple of test cases )
Usability review
----------------------
- Does the patch actually implement that? (Yes)
- Do we want that? (Not sure about that because we
are restricting non super user to use dblink )
- Do we already have it?
- Does it follow SQL spec, or the community-agreed behavior? (Looks like)
- Are there dangers? (I don't
think so )
- Have all the bases been covered? (Yes)
Feature test
------------
- Does the feature work as advertised? (Yes)
- Are there corner cases the author has failed to consider? (I don't
think so)
Performance review
------------------
- Does the patch slow down simple tests? (No)
- If it claims to improve performance, does it? (No)
- Does it slow down other things? (No)
Architecture review
-------------------
- Is everything done in a way that fits together coherently with other
features/modules? (I have added comments above)
- Are there interdependencies than can cause
problems? (I don't think so)
Review review
-------------
- Did the reviewer cover all the things that kind of reviewer is supposed
to do? (should I answer this too)
--
Ibrar Ahmed
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index 0e01470..0f90f1e 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -164,6 +164,7 @@ typedef struct remoteConnHashEnt
} \
else \
{ \
+ dblink_security_check(conn, rconn); \
connstr = conname_or_str; \
conn = PQconnectdb(connstr); \
if (PQstatus(conn) == CONNECTION_BAD) \
@@ -175,7 +176,6 @@ typedef struct remoteConnHashEnt
errmsg("could not establish connection"), \
errdetail("%s", msg))); \
} \
- dblink_security_check(conn, rconn); \
freeconn = true; \
} \
} while (0)
@@ -215,6 +215,8 @@ dblink_connect(PG_FUNCTION_ARGS)
PGconn *conn = NULL;
remoteConn *rconn = NULL;
+ dblink_security_check(conn, rconn);
+
DBLINK_INIT;
if (PG_NARGS() == 2)
@@ -246,9 +248,6 @@ dblink_connect(PG_FUNCTION_ARGS)
errdetail("%s", msg)));
}
- /* check password used if not superuser */
- dblink_security_check(conn, rconn);
-
if (connname)
{
rconn->conn = conn;
@@ -2236,6 +2235,11 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
{
if (!superuser())
{
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("only superuser can specify connect string / call \"dblink_connect()\"")));
+#if 0
+
if (!PQconnectionUsedPassword(conn))
{
PQfinish(conn);
@@ -2248,6 +2252,7 @@ dblink_security_check(PGconn *conn, remoteConn *rconn)
errdetail("Non-superuser cannot connect if the server does not request a password."),
errhint("Target server's authentication method must be changed.")));
}
+#endif
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
