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

[Noah Misch]

On Fri, Oct 07, 2022 at 09:35:49AM -0400, Bruce Momjian wrote:
> On Fri, Oct  7, 2022 at 08:05:36AM +, 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.

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

[Erki Eessaar]

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 
Sent: Friday, October 7, 2022 4:35 PM
To: Erki Eessaar 
Cc: pgsql-docs@lists.postgresql.org ; Noah 
Misch ; Peter Eisentraut 
Subject: Re: SQL-standard function bodies and creating SECURITY DEFINER 
routines securely

On Fri, Oct  7, 2022 at 08:05:36AM +, 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  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

[Bruce Momjian]

On Fri, Oct  7, 2022 at 01:50:14PM +, 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  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

[Bruce Momjian]

On Fri, Oct  7, 2022 at 08:05:36AM +, 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  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

[Erki Eessaar]

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 
Sent: Wednesday, September 28, 2022 8:07 PM
To: Erki Eessaar 
Cc: 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 +, 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<http://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  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

[Bruce Momjian]

On Tue, Aug 16, 2022 at 03:32:36PM -0400, Bruce Momjian wrote:
> On Sat, Dec 25, 2021 at 02:36:27PM +, 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  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

[Bruce Momjian]

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  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  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

[Noah Misch]

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  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

[Peter Eisentraut]

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  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.

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

[Bruce Momjian]

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  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  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

[Bruce Momjian]

On Tue, Aug 16, 2022 at 03:34:22PM -0400, Tom Lane wrote:
> Bruce Momjian  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  https://momjian.us
  EDB  https://enterprisedb.com

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

diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 7e6d52c7dc..35869bf6ba 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -779,7 +779,10 @@ SELECT * FROM dup(42);

 Because a SECURITY DEFINER function is executed
 with the privileges of the user that owns it, care is needed to
-ensure that the function cannot be misused.  For security,
+ensure that the function cannot be misused.  This is particularly
+important for non-sql_body functions because
+their function bodies are evaluated at run-time, not creation time.
+For security,
  should be set to exclude any schemas
 writable by untrusted users.  This prevents
 malicious users from creating objects (e.g., tables, functions, and

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

[Tom Lane]

Bruce Momjian  writes:
> I have written the attached patch to mention this issue about sql_body
> functions.

Spell-check, please.  Seems OK otherwise.

regards, tom lane

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

[Bruce Momjian]

On Sat, Dec 25, 2021 at 02:36:27PM +, 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  https://momjian.us
  EDB  https://enterprisedb.com

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

diff --git a/doc/src/sgml/ref/create_function.sgml b/doc/src/sgml/ref/create_function.sgml
index 7e6d52c7dc..976d4caad6 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -779,7 +779,10 @@ SELECT * FROM dup(42);

 Because a SECURITY DEFINER function is executed
 with the privileges of the user that owns it, care is needed to
-ensure that the function cannot be misused.  For security,
+ensure that the function cannot be misused.  This is particularly
+important for non-sql_body functions because
+their function bodies are evaulated at run-time, not creation time.
+For security,
  should be set to exclude any schemas
 writable by untrusted users.  This prevents
 malicious users from creating objects (e.g., tables, functions, and

SQL-standard function bodies and creating SECURITY DEFINER routines securely

[Erki Eessaar]

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
[https://www.postgresql.org/media/img/about/press/elephant.png]
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
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