Thread: SQL-standard function bodies and creating SECURITY DEFINER routines securely

SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Erki Eessaar
Date:

Hello

PostgreSQL 14 added the feature: "Allow SQL-language functions and procedures to use SQL-standard function bodies."

If I understand correctly, then in this case the system  will track dependencies between tables and routines that use the tables. Thus, the SECURITY DEFINER routines that use the new approach do not require the following mitigation, i.e., SET search_path= is not needed. The following part of documentation does not mention this.

Here is a small demonstration.

DROP TABLE IF EXISTS T;

CREATE TABLE T(t_id INTEGER,
CONSTRAINT pk_t PRIMARY KEY (t_id));

INSERT INTO T(t_id) VALUES (1), (2);

CREATE OR REPLACE FUNCTION f_find_t_count_with_path_newer() RETURNS bigint
LANGUAGE sql SECURITY DEFINER
SET search_path = public, pg_temp
BEGIN ATOMIC
SELECT Count(*) AS cnt FROM T;
END;

CREATE OR REPLACE FUNCTION f_find_t_count_without_path_newer() RETURNS bigint
LANGUAGE sql SECURITY DEFINER
BEGIN ATOMIC
SELECT Count(*) AS cnt FROM T;
END;

/*I create a fake table in the temporary schema.*/
CREATE TABLE pg_temp.T(t_id INTEGER,
CONSTRAINT pk_t PRIMARY KEY (t_id));

SELECT f_find_t_count_with_path_newer();
Result: 2

SELECT f_find_t_count_without_path_newer();
Result: 2

/*In both cases table T in the schema public was used to return the result.*/

Best regards
Erki Eessaar
On Sat, Dec 25, 2021 at 02:36:27PM +0000, Erki Eessaar wrote:
> 
> Hello
> 
> PostgreSQL 14 added the feature: "Allow SQL-language functions and procedures
> to use SQL-standard function bodies."
> 
> If I understand correctly, then in this case the system  will track
> dependencies between tables and routines that use the tables. Thus, the
> SECURITY DEFINER routines that use the new approach do not require the
> following mitigation, i.e., SET search_path= is not needed. The following part
> of documentation does not mention this.
> 
> https://www.postgresql.org/docs/current/sql-createfunction.html#
> SQL-CREATEFUNCTION-SECURITY
> 
> [elephant] PostgreSQL: Documentation: 14: CREATE FUNCTION
>            Overloading. PostgreSQL allows function overloading; that is, the
>            same name can be used for several different functions so long as
>            they have distinct input argument types.Whether or not you use it,
>            this capability entails security precautions when calling functions
>            in databases where some users mistrust other users; see Section
>            10.3.. Two functions are considered the same if they have the same
>            ...
>            www.postgresql.org

I have written the attached patch to mention this issue about sql_body
functions.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson


Attachment
Bruce Momjian <bruce@momjian.us> writes:
> I have written the attached patch to mention this issue about sql_body
> functions.

Spell-check, please.  Seems OK otherwise.

            regards, tom lane



On Tue, Aug 16, 2022 at 03:34:22PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > I have written the attached patch to mention this issue about sql_body
> > functions.
> 
> Spell-check, please.  Seems OK otherwise.

Just when I think I didn't add enough text to warrant a spell check. 
:-(  Updated patch attached.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson


Attachment

Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Bruce Momjian
Date:
On Tue, Aug 16, 2022 at 03:38:13PM -0400, Bruce Momjian wrote:
> On Tue, Aug 16, 2022 at 03:34:22PM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > I have written the attached patch to mention this issue about sql_body
> > > functions.
> > 
> > Spell-check, please.  Seems OK otherwise.
> 
> Just when I think I didn't add enough text to warrant a spell check. 
> :-(  Updated patch attached.

Patch applied back to PG 10.  Thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson




Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Peter Eisentraut
Date:
On 01.09.22 03:11, Bruce Momjian wrote:
> On Tue, Aug 16, 2022 at 03:38:13PM -0400, Bruce Momjian wrote:
>> On Tue, Aug 16, 2022 at 03:34:22PM -0400, Tom Lane wrote:
>>> Bruce Momjian <bruce@momjian.us> writes:
>>>> I have written the attached patch to mention this issue about sql_body
>>>> functions.
>>>
>>> Spell-check, please.  Seems OK otherwise.
>>
>> Just when I think I didn't add enough text to warrant a spell check.
>> :-(  Updated patch attached.
> 
> Patch applied back to PG 10.  Thanks.

This feature is new in PG 14, so backpatching further than that doesn't 
make sense.



On Thu, Sep 08, 2022 at 01:20:31PM +0200, Peter Eisentraut wrote:
> On 01.09.22 03:11, Bruce Momjian wrote:
> >On Tue, Aug 16, 2022 at 03:38:13PM -0400, Bruce Momjian wrote:
> >>On Tue, Aug 16, 2022 at 03:34:22PM -0400, Tom Lane wrote:
> >>>Bruce Momjian <bruce@momjian.us> writes:
> >>>>I have written the attached patch to mention this issue about sql_body
> >>>>functions.
> >>>
> >>>Spell-check, please.  Seems OK otherwise.

> >Patch applied back to PG 10.  Thanks.
> 
> This feature is new in PG 14, so backpatching further than that doesn't make
> sense.

Even an sql_body function should override search_path, because it may call
other code that reacts to search_path.  Separately, the new sentence is near
the start of a section that addresses more than just search_path.  The section
ends with the "revoke the default PUBLIC privileges" topic, which is no less
relevant to sql_body functions.

Documentation needn't explain cases that make a best practice optional, and it
should explain only valuable ones.  Omitting search_path on sql_body SECURITY
DEFINER functions isn't that valuable.  If it were valuable, the patch's
sentence gives too little detail for the reader to decide what's safe for a
given function.  I think this section should not attempt such detail.  It's
enough to give the best practice, as the documentation did before this change.



Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Bruce Momjian
Date:
On Sun, Sep 11, 2022 at 09:46:47PM -0700, Noah Misch wrote:
> On Thu, Sep 08, 2022 at 01:20:31PM +0200, Peter Eisentraut wrote:
> > On 01.09.22 03:11, Bruce Momjian wrote:
> > >On Tue, Aug 16, 2022 at 03:38:13PM -0400, Bruce Momjian wrote:
> > >>On Tue, Aug 16, 2022 at 03:34:22PM -0400, Tom Lane wrote:
> > >>>Bruce Momjian <bruce@momjian.us> writes:
> > >>>>I have written the attached patch to mention this issue about sql_body
> > >>>>functions.
> > >>>
> > >>>Spell-check, please.  Seems OK otherwise.
> 
> > >Patch applied back to PG 10.  Thanks.
> > 
> > This feature is new in PG 14, so backpatching further than that doesn't make
> > sense.
> 
> Even an sql_body function should override search_path, because it may call
> other code that reacts to search_path.  Separately, the new sentence is near
> the start of a section that addresses more than just search_path.  The section
> ends with the "revoke the default PUBLIC privileges" topic, which is no less
> relevant to sql_body functions.
> 
> Documentation needn't explain cases that make a best practice optional, and it
> should explain only valuable ones.  Omitting search_path on sql_body SECURITY
> DEFINER functions isn't that valuable.  If it were valuable, the patch's
> sentence gives too little detail for the reader to decide what's safe for a
> given function.  I think this section should not attempt such detail.  It's
> enough to give the best practice, as the documentation did before this change.

Agreed, patch reverted in all branches.  Thanks for the feedback.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson




Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Bruce Momjian
Date:
On Tue, Aug 16, 2022 at 03:32:36PM -0400, Bruce Momjian wrote:
> On Sat, Dec 25, 2021 at 02:36:27PM +0000, Erki Eessaar wrote:
> > 
> > Hello
> > 
> > PostgreSQL 14 added the feature: "Allow SQL-language functions and procedures
> > to use SQL-standard function bodies."
> > 
> > If I understand correctly, then in this case the system  will track
> > dependencies between tables and routines that use the tables. Thus, the
> > SECURITY DEFINER routines that use the new approach do not require the
> > following mitigation, i.e., SET search_path= is not needed. The following part
> > of documentation does not mention this.
> > 
> > https://www.postgresql.org/docs/current/sql-createfunction.html#
> > SQL-CREATEFUNCTION-SECURITY
> > 
> > [elephant] PostgreSQL: Documentation: 14: CREATE FUNCTION
> >            Overloading. PostgreSQL allows function overloading; that is, the
> >            same name can be used for several different functions so long as
> >            they have distinct input argument types.Whether or not you use it,
> >            this capability entails security precautions when calling functions
> >            in databases where some users mistrust other users; see Section
> >            10.3.. Two functions are considered the same if they have the same
> >            ...
> >            www.postgresql.org
> 
> I have written the attached patch to mention this issue about sql_body
> functions.

The doc patch was reverted based on feedback in this email thread:


https://www.postgresql.org/message-id/flat/AM9PR01MB8268BF5E74E119828251FD34FE409%40AM9PR01MB8268.eurprd01.prod.exchangelabs.com

If you think we should add new wording, please suggest it, thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson




Hello

I confirmed, that setting search_path is indeed sometimes needed in case of SECURITY DEFINER routines that have SQL-standard bodies. See an example at the end of the letter.

I suggest the following paragraph to the documentation:

Starting from PostgreSQL 14 SQL-standard bodies can be used in SQL-language functions. This form tracks dependencies between the function and objects used in the function body. However, there is still a possibility that such function calls other code that reacts to search path. Thus, as a best practice, SECURITY DEFINER functions with SQL-standard bodies should also override search_path.

Best regards
Erki Eessaar

*************************
/*Table in the schema public.*/
CREATE TABLE public.A(a INTEGER PRIMARY KEY);

/*Table in the schema pg_temp.*/
CREATE TABLE pg_temp.A(a INTEGER PRIMARY KEY);

/*SECURITY DEFINER function without SQL-standard function body.*/
CREATE OR REPLACE FUNCTION f1 () RETURNS VOID
AS $$
INSERT INTO A(a) VALUES (1);
$$ LANGUAGE sql SECURITY DEFINER;

/*SECURITY DEFINER function with SQL-standard function body that invokes
the function without SQL-standard function body. It does not explicitly set the search path.*/
CREATE OR REPLACE FUNCTION f2 () RETURNS VOID
LANGUAGE sql SECURITY DEFINER
BEGIN ATOMIC
SELECT f1();
END;

/*SECURITY DEFINER function with SQL-standard function body that invokes
the function without SQL-standard function body. It does explicitly set the search path.*/
CREATE OR REPLACE FUNCTION f2_secure () RETURNS VOID
LANGUAGE sql SECURITY DEFINER
SET search_path = public, pg_temp
BEGIN ATOMIC
SELECT f1();
END;

SELECT f2();

SELECT * FROM public.A;
/*Result 0 rows.*/

SELECT * FROM pg_temp.A;
/*Result 1 row.*/


DELETE FROM pg_temp.A;

SELECT f2_secure();

SELECT * FROM public.A;
/*Result 1 row. SET search_path in the invoking function had an effect to the invoked function*/

SELECT * FROM pg_temp.A;
/*Result 0 rows.*/





From: Bruce Momjian <bruce@momjian.us>
Sent: Wednesday, September 28, 2022 8:07 PM
To: Erki Eessaar <erki.eessaar@taltech.ee>
Cc: pgsql-docs@lists.postgresql.org <pgsql-docs@lists.postgresql.org>
Subject: Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely
 
On Tue, Aug 16, 2022 at 03:32:36PM -0400, Bruce Momjian wrote:
> On Sat, Dec 25, 2021 at 02:36:27PM +0000, Erki Eessaar wrote:
> >
> > Hello
> >
> > PostgreSQL 14 added the feature: "Allow SQL-language functions and procedures
> > to use SQL-standard function bodies."
> >
> > If I understand correctly, then in this case the system  will track
> > dependencies between tables and routines that use the tables. Thus, the
> > SECURITY DEFINER routines that use the new approach do not require the
> > following mitigation, i.e., SET search_path= is not needed. The following part
> > of documentation does not mention this.
> >
> > https://www.postgresql.org/docs/current/sql-createfunction.html#
> > SQL-CREATEFUNCTION-SECURITY
> >
> > [elephant] PostgreSQL: Documentation: 14: CREATE FUNCTION
> >            Overloading. PostgreSQL allows function overloading; that is, the
> >            same name can be used for several different functions so long as
> >            they have distinct input argument types.Whether or not you use it,
> >            this capability entails security precautions when calling functions
> >            in databases where some users mistrust other users; see Section
> >            10.3.. Two functions are considered the same if they have the same
> >            ...
> >            www.postgresql.org
>
> I have written the attached patch to mention this issue about sql_body
> functions.

The doc patch was reverted based on feedback in this email thread:

        https://www.postgresql.org/message-id/flat/AM9PR01MB8268BF5E74E119828251FD34FE409%40AM9PR01MB8268.eurprd01.prod.exchangelabs.com

If you think we should add new wording, please suggest it, thanks.

--
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Bruce Momjian
Date:
On Fri, Oct  7, 2022 at 08:05:36AM +0000, Erki Eessaar wrote:
> Hello
> 
> I confirmed, that setting search_path is indeed sometimes needed in case of
> SECURITY DEFINER routines that have SQL-standard bodies. See an example at the
> end of the letter.
> 
> I suggest the following paragraph to the documentation:
> 
> Starting from PostgreSQL 14 SQL-standard bodies can be used in SQL-language
> functions. This form tracks dependencies between the function and objects used
> in the function body. However, there is still a possibility that such function
> calls other code that reacts to search path. Thus, as a best practice, SECURITY
> DEFINER functions with SQL-standard bodies should also override search_path.

I think this gets back to what Noah said about this section not needing
to explain all the details but rather give general guidance.  I am not
sure adding the reasons for _why_ you should use search path for
SQL-standard bodies is really adding anything.  Noah, is that accurate?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson




Hello

Another example where explicit search path is needed.

CREATE TABLE public.B(b INTEGER);
CREATE TABLE pg_temp.B(b INTEGER);

CREATE OR REPLACE FUNCTION f3 () RETURNS VOID
LANGUAGE sql SECURITY DEFINER
BEGIN ATOMIC
INSERT INTO B(b) VALUES (1);
END;

SELECT f3();

SELECT * FROM public.B;
/*Result has 0 rows.*/

SELECT * FROM pg_temp.B;
/*Result has 1 row. Function f3 was associated with pg_temp.B because
f3() did not have explicitly set search path.*/

I see now that there are multiple reasons why to still use search path. I agree now that this extra paragaraph is perhaps too confusing and is not needed.

Best regards
Erki Eessaar


From: Bruce Momjian <bruce@momjian.us>
Sent: Friday, October 7, 2022 4:35 PM
To: Erki Eessaar <erki.eessaar@taltech.ee>
Cc: pgsql-docs@lists.postgresql.org <pgsql-docs@lists.postgresql.org>; Noah Misch <noah@leadboat.com>; Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Subject: Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely
 
On Fri, Oct  7, 2022 at 08:05:36AM +0000, Erki Eessaar wrote:
> Hello
>
> I confirmed, that setting search_path is indeed sometimes needed in case of
> SECURITY DEFINER routines that have SQL-standard bodies. See an example at the
> end of the letter.
>
> I suggest the following paragraph to the documentation:
>
> Starting from PostgreSQL 14 SQL-standard bodies can be used in SQL-language
> functions. This form tracks dependencies between the function and objects used
> in the function body. However, there is still a possibility that such function
> calls other code that reacts to search path. Thus, as a best practice, SECURITY
> DEFINER functions with SQL-standard bodies should also override search_path.

I think this gets back to what Noah said about this section not needing
to explain all the details but rather give general guidance.  I am not
sure adding the reasons for _why_ you should use search path for
SQL-standard bodies is really adding anything.  Noah, is that accurate?

--
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

Re: SQL-standard function bodies and creating SECURITY DEFINER routines securely

From
Bruce Momjian
Date:
On Fri, Oct  7, 2022 at 01:50:14PM +0000, Erki Eessaar wrote:
> Hello
> 
> Another example where explicit search path is needed.
> 
> CREATE TABLE public.B(b INTEGER);
> CREATE TABLE pg_temp.B(b INTEGER);
> 
> CREATE OR REPLACE FUNCTION f3 () RETURNS VOID
> LANGUAGE sql SECURITY DEFINER
> BEGIN ATOMIC
> INSERT INTO B(b) VALUES (1);
> END;
> 
> SELECT f3();
> 
> SELECT * FROM public.B;
> /*Result has 0 rows.*/
> 
> SELECT * FROM pg_temp.B;
> /*Result has 1 row. Function f3 was associated with pg_temp.B because
> f3() did not have explicitly set search path.*/
> 
> I see now that there are multiple reasons why to still use search path. I agree
> now that this extra paragaraph is perhaps too confusing and is not needed.

Thanks.  I learned a few things in this discussion and we can revisit it
if we feel there is need of improvement.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson




On Fri, Oct 07, 2022 at 09:35:49AM -0400, Bruce Momjian wrote:
> On Fri, Oct  7, 2022 at 08:05:36AM +0000, Erki Eessaar wrote:
> > I confirmed, that setting search_path is indeed sometimes needed in case of
> > SECURITY DEFINER routines that have SQL-standard bodies. See an example at the
> > end of the letter.
> > 
> > I suggest the following paragraph to the documentation:
> > 
> > Starting from PostgreSQL 14 SQL-standard bodies can be used in SQL-language
> > functions. This form tracks dependencies between the function and objects used
> > in the function body. However, there is still a possibility that such function
> > calls other code that reacts to search path. Thus, as a best practice, SECURITY
> > DEFINER functions with SQL-standard bodies should also override search_path.
> 
> I think this gets back to what Noah said about this section not needing
> to explain all the details but rather give general guidance.  I am not
> sure adding the reasons for _why_ you should use search path for
> SQL-standard bodies is really adding anything.  Noah, is that accurate?

Yes, that's my thinking.  It's hard to make objective decisions about how
deeply to cover each topic in the documentation.  I'm content with the present
state of this particular section, though.