Thread: [Review] fix dblink security hole
Hi,
Here is my review of the dblink security hole patch written by Marko Kreen:
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. 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"
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
Here is my review of the dblink security hole patch written by Marko Kreen:
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. 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"
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
Attachment
On 9/16/08, Ibrar Ahmed <ibrar.ahmad@gmail.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
This is no good - the security_check() needs established connection
> I have attached a patch just for the quick thought. Otherwise there is no
> issue with patch.
to work on.
I know it but after putting the superuser check just above the security_check function make this function almost useless and now it doesn't need PGconn parameter.
--
Ibrar Ahmed