Re: Re: Missing REVOKE in SQL for functions with SECURITY DEFINER (or any function, really)

Thanks - seems to work well, so I've committed the patch to 1.16 and master.

Erwin; would appreciate any additional QA you can find time for :-)

On Fri, Apr 19, 2013 at 1:47 PM, Dhiraj Chawla
<dhiraj.chawla@enterprisedb.com> wrote:
> Hi Dave,
>
> Please find attached the patch that fixes this issue related to security
> definer not provided.
>
> According to our discussion based on
> http://www.postgresql.org/docs/9.2/static/sql-grant.html the fix that I have
> made is as follows:
>
> When a user creates a new function, database or language, we show their
> default privileges in the dialog box, thus giving the user a chance to
> revoke them if they wish to. If the user chooses to revoke them, then in the
> reverse engineered code in the sql pane, the revoke statement will be
> visible. For this I have modified the GetGrant function of pgObject to
> handle this.
>
> Thus we ourselves don't revoke the default grant given to function, database
> or language object, but we make sure that they are visible to the user, so
> that he/she can take an informed decision.
>
> In this patch I have also taken care that if the owner of an object revokes
> all privileges from himself, then that should also be seen in the reverse
> engineered sql in the sql pane.
>
> Let me know your views on this patch.
>
> Thanks,
>
>
>
> On Mon, Apr 8, 2013 at 5:08 PM, Dave Page <dpage@pgadmin.org> wrote:
>>
>> Dhiraj, can you look into this please?
>>
>> Thanks.
>>
>> On Sun, Apr 7, 2013 at 11:02 PM, Erwin Brandstetter
>> <brandstetter@falter.at> wrote:
>> > Hi developers!
>> >
>> > I have been missing in action for a while, so I am not sure whether
>> > anybody
>> > even uses trac any more.
>> > Either way, I just ran into this bug once again and checked to find it
>> > still
>> > open:
>> > http://code.pgadmin.org/trac/ticket/88
>> >
>> > Basically, REVOKE EXECUTE ON FUNCTION is omitted in the DDL script.
>> > To reproduce:
>> >
>> > CREATE OR REPLACE FUNCTION foo() RETURNS int AS 'SELECT 1' LANGUAGE sql;
>> > REVOKE EXECUTE ON FUNCTION foo() FROM public;
>> >
>> > This is a **potential security hazard** and it has been open for (at
>> > least)
>> > over a year now.
>> >
>> > Regards
>> > Erwin
>> >
>> >
>> >
>> > On 27.02.2012 23:53, Erwin Brandstetter wrote:
>> >
>> > On 27.02.2012 23:38, Erwin Brandstetter wrote:
>> >
>> > Hi developers!
>> >
>> > Congratulations on the many bug fixes in the latest release!
>> > I think I found another serious problem.
>> >
>> > Testing with pgAdmin 1.14.2 on Windows XP. Server is PostgreSQL 9.1 on
>> > Devian Squeeze.
>> >
>> > There is a security hazard lingering in the reverse engineered SQL of
>> > the
>> > latest version 1.14.2 (and versions before it).
>> >
>> > As summed up here
>> >
>> >
>> > http://www.postgresql.org/docs/current/interactive/sql-createfunction.html#SQL-CREATEFUNCTION-SECURITY
>> > the execute privilege is granted to PUBLIC by default. It needs to be
>> > revoked for security critical functions.
>> >
>> > I quote the manual:
>> >
>> > Another point to keep in mind is that by default, execute privilege is
>> > granted to PUBLIC for newly created functions (see GRANT for more
>> > information). Frequently you will wish to restrict use of a security
>> > definer
>> > function to only some users. To do that, you must revoke the default
>> > PUBLIC
>> > privileges and then grant execute privilege selectively.
>> >
>> >
>> > This goes wrong with pgAdmin 1.14.2. Consider this test case, executed
>> > as
>> > superuser postgres:
>> >
>> > CREATE OR REPLACE FUNCTION foo ()
>> >   RETURNS void AS
>> > $BODY$
>> > BEGIN
>> >     PERFORM 1;
>> > END;
>> > $BODY$
>> >   LANGUAGE plpgsql VOLATILE SECURITY DEFINER;
>> > ALTER FUNCTION foo() SET search_path=public, pg_temp;
>> > REVOKE ALL ON FUNCTION foo() FROM PUBLIC;
>> > GRANT EXECUTE ON FUNCTION foo() TO ief;
>> >
>> >
>> > The reverse engineered SQL looks like this
>> >
>> > -- Function: foo()
>> >
>> > -- DROP FUNCTION foo();
>> >
>> > CREATE OR REPLACE FUNCTION foo()
>> >   RETURNS void AS
>> > $BODY$
>> >
>> > BEGIN
>> > PERFORM 1;
>> > END;
>> > $BODY$
>> >   LANGUAGE plpgsql VOLATILE SECURITY DEFINER
>> >   COST 100;
>> > ALTER FUNCTION foo() SET search_path=public, pg_temp;
>> >
>> > ALTER FUNCTION foo()
>> >   OWNER TO postgres;
>> > GRANT EXECUTE ON FUNCTION foo() TO postgres;
>> > GRANT EXECUTE ON FUNCTION foo() TO ief;
>> >
>> >
>> > The REVOKE statement is missing, which is a serious security hazard. A
>> > recreated function will be open to the the public.
>> >
>> > Regards
>> > Erwin
>> >
>> >
>> > I reopened ticket #88 for that
>> >     http://code.pgadmin.org/trac/ticket/88#comment:2
>> > because it seemed closely related.
>> >
>> > Regards
>> > Erwin
>> >
>> >
>> >
>>
>>
>>
>> --
>> Dave Page
>> Blog: http://pgsnake.blogspot.com
>> Twitter: @pgsnake
>>
>> EnterpriseDB UK: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>
>
>
>
> --
> regards,
>
> Dhiraj Chawla
> Software Engineer
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91-20-30589522



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company