On 9/16/08, Ibrar Ahmed <[EMAIL PROTECTED]> wrote: > Here is my review of the dblink security hole patch written by Marko Kreen:
Thanks for the review. > 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. No, that would keep the security hole as the password leak happens in PQconnectdb(). The dblink_security_check() is ran after the connection is already established, to fix a single use case. > 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). That is a good point - the check is mostly pointless now. Although it can be argued that it protects clueless admins who create SECURITY DEFINER wrapper around dblink_connect(), it can be said if admin knowingly wants to do that, on what grounds do we disallow it? > I have attached a patch just for the quick thought. Otherwise there is no > issue with patch. This is no good - the security_check() needs established connection to work on. > - Does it include reasonable tests, docs, etc? ( In my opinion > there should be couple of test cases ) Yes, that may be good idea. I don't know what is the policy of creating users in regtest, can we do it freely? -- marko -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
