From: | "Marko Kreen" <markokr(at)gmail(dot)com> |
---|---|
To: | "Ibrar Ahmed" <ibrar(dot)ahmad(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: [Review] fix dblink security hole |
Date: | 2008-09-16 10:26:17 |
Message-ID: | e51f66da0809160326x76eb4d0bm34f33968c5620937@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2008-09-16 10:27:20 | proposal - GROUPING SETS |
Previous Message | Andrew Dunstan | 2008-09-16 10:05:32 | Re: no XLOG during COPY? |