Here is my review of the dblink security hole patch written by Marko Kreen:
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
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
- 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 )
- 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
- Are there dangers? (I
don't think so )
- Have all the bases been covered? (Yes)
- Does the feature work as advertised? (Yes)
- Are there corner cases the author has failed to consider? (I
don't think so)
- Does the patch slow down simple tests? (No)
- If it claims to improve performance, does it? (No)
- Does it slow down other things? (No)
- 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)
- Did the reviewer cover all the things that kind of reviewer is
supposed to do? (should I answer this too)
pgsql-hackers by date
|Next:||From: Ibrar Ahmed||Date: 2008-09-16 08:48:45|
|Subject: [Review] fix dblink security hole|
|Previous:||From: Gregory Stark||Date: 2008-09-16 08:12:17|
|Subject: Re: Coping with nLocks overflow|