Re: [Review] fix dblink security hole

On 9/16/08, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> 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/e51f66da0808070653y3d3d065am5dfa258a6273b849@mail.gmail.com
>
> 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