Horton

Thread

Commits

  1. Use @extschema:name@ notation in contrib transform modules.

  2. pg_freespacemap: Fix declaration of pg_freespace(regclass)

  3. contrib/pageinspect: Use SQL-standard function bodies.

  4. contrib/xml2: Use SQL-standard function bodies.

  5. contrib/citext: Use SQL-standard function bodies.

  6. contrib/earthdistance: Use SQL-standard function bodies.

  7. contrib/lo: Use SQL-standard function bodies

  8. xml2: Add tests for functions xpath_nodeset() and xpath_list()

  9. contrib/lo: Add test for function lo_oid()

  10. pg_freespacemap: Use SQL-standard function bodies

  11. Add @extschema:name@ and no_relocate options to extensions.

  12. Make contrib modules' installation scripts more secure.

  1. Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-13T22:26:34Z

    Attached are some draft patches to convert almost all of the
contrib modules' SQL functions to use SQL-standard function bodies.
The point of this is to remove the residual search_path security
hazards that we couldn't fix in commits 7eeb1d986 et al.  Since
a SQL-style function body is fully parsed at creation time,
its object references are not subject to capture by the run-time
search path.  Possibly there are small performance benefits too,
though I've not tried to measure that.

I've not touched the documentation yet.  I suppose that we can
tone down the warnings added by 7eeb1d986 quite a bit, maybe
replacing them with just "be sure to use version x.y or later".
However I think we may still need an assumption that earthdistance
and cube are in the same schema --- any comments on that?

I'd like to propose squeezing these changes into v14, even though
we're past feature freeze.  Reason one is that this is less a
new feature than a security fix; reason two is that this provides
some non-artificial test coverage for the SQL-function-body feature.

BTW, there still remain a couple of old-style SQL functions in
contrib/adminpack and contrib/lo.  AFAICS those are unconditionally
secure, so I didn't bother with them.

Thoughts?

			regards, tom lane

  2. Re: Converting contrib SQL functions to new style

    Noah Misch <noah@leadboat.com> — 2021-04-14T02:08:27Z

    On Tue, Apr 13, 2021 at 06:26:34PM -0400, Tom Lane wrote:
> Attached are some draft patches to convert almost all of the
> contrib modules' SQL functions to use SQL-standard function bodies.
> The point of this is to remove the residual search_path security
> hazards that we couldn't fix in commits 7eeb1d986 et al.  Since
> a SQL-style function body is fully parsed at creation time,
> its object references are not subject to capture by the run-time
> search path.

Are there any inexact matches in those function/operator calls?  Will that
matter more or less than it does today?

> However I think we may still need an assumption that earthdistance
> and cube are in the same schema --- any comments on that?

That part doesn't change, indeed.

> I'd like to propose squeezing these changes into v14, even though
> we're past feature freeze.  Reason one is that this is less a
> new feature than a security fix; reason two is that this provides
> some non-artificial test coverage for the SQL-function-body feature.

Dogfooding like this is good.  What about the SQL-language functions that
initdb creates?

  3. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T03:11:13Z

    Noah Misch <noah@leadboat.com> writes:
> On Tue, Apr 13, 2021 at 06:26:34PM -0400, Tom Lane wrote:
>> Attached are some draft patches to convert almost all of the
>> contrib modules' SQL functions to use SQL-standard function bodies.
>> The point of this is to remove the residual search_path security
>> hazards that we couldn't fix in commits 7eeb1d986 et al.  Since
>> a SQL-style function body is fully parsed at creation time,
>> its object references are not subject to capture by the run-time
>> search path.

> Are there any inexact matches in those function/operator calls?  Will that
> matter more or less than it does today?

I can't claim to have looked closely for inexact matches.  It should
matter less than today, since there's a hazard only during creation
(with a somewhat-controlled search path) and not during use.  But
that doesn't automatically eliminate the issue.

>> I'd like to propose squeezing these changes into v14, even though
>> we're past feature freeze.  Reason one is that this is less a
>> new feature than a security fix; reason two is that this provides
>> some non-artificial test coverage for the SQL-function-body feature.

> Dogfooding like this is good.  What about the SQL-language functions that
> initdb creates?

Hadn't thought about those, but converting them seems like a good idea.

			regards, tom lane

  4. Re: Converting contrib SQL functions to new style

    Noah Misch <noah@leadboat.com> — 2021-04-14T12:58:11Z

    On Tue, Apr 13, 2021 at 11:11:13PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Tue, Apr 13, 2021 at 06:26:34PM -0400, Tom Lane wrote:
> >> Attached are some draft patches to convert almost all of the
> >> contrib modules' SQL functions to use SQL-standard function bodies.
> >> The point of this is to remove the residual search_path security
> >> hazards that we couldn't fix in commits 7eeb1d986 et al.  Since
> >> a SQL-style function body is fully parsed at creation time,
> >> its object references are not subject to capture by the run-time
> >> search path.
> 
> > Are there any inexact matches in those function/operator calls?  Will that
> > matter more or less than it does today?
> 
> I can't claim to have looked closely for inexact matches.  It should
> matter less than today, since there's a hazard only during creation
> (with a somewhat-controlled search path) and not during use.  But
> that doesn't automatically eliminate the issue.

Once CREATE EXTENSION is over, things are a great deal safer under this
proposal, as you say.  I suspect it makes CREATE EXTENSION more hazardous.
Today, typical SQL commands in extension creation scripts don't activate
inexact argument type matching.  You were careful to make each script clear
the search_path around commands deviating from that (commit 7eeb1d9).  I think
"CREATE FUNCTION plus1dot1(int) RETURNS numeric LANGUAGE SQL RETURN $1 + 1.1;"
in a trusted extension script would constitute a security vulnerability, since
it can lock in the wrong operator.

  5. Re: Converting contrib SQL functions to new style

    Robert Haas <robertmhaas@gmail.com> — 2021-04-14T13:55:56Z

    On Wed, Apr 14, 2021 at 8:58 AM Noah Misch <noah@leadboat.com> wrote:
> Once CREATE EXTENSION is over, things are a great deal safer under this
> proposal, as you say.  I suspect it makes CREATE EXTENSION more hazardous.
> Today, typical SQL commands in extension creation scripts don't activate
> inexact argument type matching.  You were careful to make each script clear
> the search_path around commands deviating from that (commit 7eeb1d9).  I think
> "CREATE FUNCTION plus1dot1(int) RETURNS numeric LANGUAGE SQL RETURN $1 + 1.1;"
> in a trusted extension script would constitute a security vulnerability, since
> it can lock in the wrong operator.

I don't understand how that can happen, unless we've failed to secure
the search_path. And, if we've failed to secure the search_path, I
think we are in a lot of trouble no matter what else we do.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  6. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T14:49:37Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 14, 2021 at 8:58 AM Noah Misch <noah@leadboat.com> wrote:
>> Once CREATE EXTENSION is over, things are a great deal safer under this
>> proposal, as you say.  I suspect it makes CREATE EXTENSION more hazardous.
>> Today, typical SQL commands in extension creation scripts don't activate
>> inexact argument type matching.  You were careful to make each script clear
>> the search_path around commands deviating from that (commit 7eeb1d9).  I think
>> "CREATE FUNCTION plus1dot1(int) RETURNS numeric LANGUAGE SQL RETURN $1 + 1.1;"
>> in a trusted extension script would constitute a security vulnerability, since
>> it can lock in the wrong operator.

> I don't understand how that can happen, unless we've failed to secure
> the search_path. And, if we've failed to secure the search_path, I
> think we are in a lot of trouble no matter what else we do.

The situation of interest is where you are trying to install an extension
into a schema that also contains malicious objects.  We've managed to make
most of the commands you might use in an extension script secure against
that situation, and Noah wants to hold SQL-function creation to that same
standard.

My concern in this patch is rendering SQL functions safe against untrusted
search_path at *time of use*, which is really an independent security
concern.

If you're willing to assume there's nothing untrustworthy in your
search_path, then there's no issue and nothing to fix.  Unfortunately,
that seems like a rather head-in-the-sand standpoint.

			regards, tom lane

  7. Re: Converting contrib SQL functions to new style

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-14T17:18:30Z

    
> On Apr 13, 2021, at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> However I think we may still need an assumption that earthdistance
> and cube are in the same schema --- any comments on that?

This is probably not worth doing, and we are already past feature freeze, but adding syntax to look up the namespace of an extension might help.  The problem seems to be that we can't syntactically refer to the schema of an extension.  We have to instead query pg_catalog.pg_extension joined against pg_catalog.pg_namespace and then interpolate the namespace name into strings that get executed, which is ugly.

This syntax is perhaps a non-starter, but conceptually something like:

-CREATE DOMAIN earth AS cube
+CREATE DOMAIN earthdistance::->earth AS cube::->cube

Then we'd perhaps extend RangeVar with an extensionname field and have either a schemaname or an extensionname be looked up in places where we currently lookup schemas, adding a catcache for extensions.  (Like I said, probably not worth doing.)


We could get something like this working just inside the CREATE EXTENSION command if we expanded on the @extschema@ idea a bit.  At first I thought this idea would suffer race conditions with concurrent modifications of pg_extension or pg_namespace, but it looks like we already have a snapshot when processing the script file, so:

-CREATE DOMAIN earth AS cube
+CREATE DOMAIN @@earthdistance@@::earth AS @@cube@@::cube

or such, with @@foo@@ being parsed out, looked up in pg_extension join pg_namespace, and substituted back in.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  8. Re: Converting contrib SQL functions to new style

    Robert Haas <robertmhaas@gmail.com> — 2021-04-14T17:19:55Z

    On Wed, Apr 14, 2021 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The situation of interest is where you are trying to install an extension
> into a schema that also contains malicious objects.  We've managed to make
> most of the commands you might use in an extension script secure against
> that situation, and Noah wants to hold SQL-function creation to that same
> standard.

Oh, I was forgetting that the creation schema has to be first in your
search path. :-(

Does the idea of allowing the creation schema to be set separately
have any legs? Because it seems like that would help here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  9. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T17:36:22Z

    Mark Dilger <mark.dilger@enterprisedb.com> writes:
>> On Apr 13, 2021, at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However I think we may still need an assumption that earthdistance
>> and cube are in the same schema --- any comments on that?

> This is probably not worth doing, and we are already past feature
> freeze, but adding syntax to look up the namespace of an extension might
> help.

Yeah, that idea was discussed before (perhaps only in private
security-team threads, though).  We didn't do anything about it because
at the time there didn't seem to be pressing need, but in the context
of SQL function bodies there's an obvious use-case.

> We could get something like this working just inside the CREATE EXTENSION command if we expanded on the @extschema@ idea a bit.  At first I thought this idea would suffer race conditions with concurrent modifications of pg_extension or pg_namespace, but it looks like we already have a snapshot when processing the script file, so:

> -CREATE DOMAIN earth AS cube
> +CREATE DOMAIN @@earthdistance@@::earth AS @@cube@@::cube

Right, extending the @extschema@ mechanism is what was discussed,
though I think I'd lean towards something like @extschema:cube@
to denote the schema of a referenced extension "cube".

I'm not sure this is useful enough to break feature freeze for,
but I'm +1 for investigating it for v15.

			regards, tom lane

  10. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T17:41:44Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 14, 2021 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The situation of interest is where you are trying to install an extension
>> into a schema that also contains malicious objects.  We've managed to make
>> most of the commands you might use in an extension script secure against
>> that situation, and Noah wants to hold SQL-function creation to that same
>> standard.

> Oh, I was forgetting that the creation schema has to be first in your
> search path. :-(

> Does the idea of allowing the creation schema to be set separately
> have any legs? Because it seems like that would help here.

Doesn't help that much, because you still have to reference objects
already created by your own extension, so it's hard to see how the
target schema won't need to be in the path.

[ thinks for awhile ... ]

Could we hack things so that extension scripts are only allowed to
reference objects created (a) by the system, (b) earlier in the
same script, or (c) owned by one of the declared prerequisite
extensions?  Seems like that might provide a pretty bulletproof
defense against trojan-horse objects, though I'm not sure how much
of a pain it'd be to implement.

			regards, tom lane

  11. Re: Converting contrib SQL functions to new style

    Robert Haas <robertmhaas@gmail.com> — 2021-04-14T17:56:34Z

    On Wed, Apr 14, 2021 at 1:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Doesn't help that much, because you still have to reference objects
> already created by your own extension, so it's hard to see how the
> target schema won't need to be in the path.

Oh, woops.

> Could we hack things so that extension scripts are only allowed to
> reference objects created (a) by the system, (b) earlier in the
> same script, or (c) owned by one of the declared prerequisite
> extensions?  Seems like that might provide a pretty bulletproof
> defense against trojan-horse objects, though I'm not sure how much
> of a pain it'd be to implement.

That doesn't seem like a crazy idea, but the previous idea of having
some magic syntax that means "the schema where extension FOO is" seems
like it might be easier to implement and more generally useful. If we
taught the core system that %!!**&^%?(earthdistance) means "the schema
where the earthdistance is located" that syntax might get some use
even outside of extension creation scripts, which seems like it could
be a good thing, just because code that is used more widely is more
likely to have been debugged to the point where it actually works.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  12. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T18:03:56Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Apr 14, 2021 at 1:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Could we hack things so that extension scripts are only allowed to
>> reference objects created (a) by the system, (b) earlier in the
>> same script, or (c) owned by one of the declared prerequisite
>> extensions?  Seems like that might provide a pretty bulletproof
>> defense against trojan-horse objects, though I'm not sure how much
>> of a pain it'd be to implement.

> That doesn't seem like a crazy idea, but the previous idea of having
> some magic syntax that means "the schema where extension FOO is" seems
> like it might be easier to implement and more generally useful.

I think that's definitely useful, but it's not a fix for the
reference-capture problem unless you care to assume that the other
extension's schema is free of trojan-horse objects.  So I'm thinking
that we really ought to pursue both ideas.

This may mean that squeezing these contrib changes into v14 is a lost
cause.  We certainly shouldn't try to do what I suggest above for
v14; but without it, these changes are just moving the security
issue to a different place rather than eradicating it completely.

			regards, tom lane

  13. Re: Converting contrib SQL functions to new style

    Andrew Dunstan <andrew@dunslane.net> — 2021-04-14T19:32:28Z

    On 4/14/21 2:03 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, Apr 14, 2021 at 1:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Could we hack things so that extension scripts are only allowed to
>>> reference objects created (a) by the system, (b) earlier in the
>>> same script, or (c) owned by one of the declared prerequisite
>>> extensions?  Seems like that might provide a pretty bulletproof
>>> defense against trojan-horse objects, though I'm not sure how much
>>> of a pain it'd be to implement.
>> That doesn't seem like a crazy idea, but the previous idea of having
>> some magic syntax that means "the schema where extension FOO is" seems
>> like it might be easier to implement and more generally useful.
> I think that's definitely useful, but it's not a fix for the
> reference-capture problem unless you care to assume that the other
> extension's schema is free of trojan-horse objects.  So I'm thinking
> that we really ought to pursue both ideas.
>
> This may mean that squeezing these contrib changes into v14 is a lost
> cause.  We certainly shouldn't try to do what I suggest above for
> v14; but without it, these changes are just moving the security
> issue to a different place rather than eradicating it completely.
>
> 			



Is there anything else we should be doing along the eat your own dogfood
line that don't have these security implications?


cheers


andrew


-- 

Andrew Dunstan
EDB: https://www.enterprisedb.com

  14. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-04-14T20:13:12Z

    Andrew Dunstan <andrew@dunslane.net> writes:
> On 4/14/21 2:03 PM, Tom Lane wrote:
>> This may mean that squeezing these contrib changes into v14 is a lost
>> cause.  We certainly shouldn't try to do what I suggest above for
>> v14; but without it, these changes are just moving the security
>> issue to a different place rather than eradicating it completely.

> Is there anything else we should be doing along the eat your own dogfood
> line that don't have these security implications?

We can still convert the initdb-created SQL functions to new style,
since there's no security threat during initdb.  I'll make a patch
for that soon.

			regards, tom lane

  15. Re: Converting contrib SQL functions to new style

    Vik Fearing <vik@postgresfriends.org> — 2021-04-14T21:47:53Z

    On 4/14/21 7:36 PM, Tom Lane wrote:
> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>>> On Apr 13, 2021, at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> However I think we may still need an assumption that earthdistance
>>> and cube are in the same schema --- any comments on that?
> 
>> This is probably not worth doing, and we are already past feature
>> freeze, but adding syntax to look up the namespace of an extension might
>> help.
> 
> Yeah, that idea was discussed before (perhaps only in private
> security-team threads, though).  We didn't do anything about it because
> at the time there didn't seem to be pressing need, but in the context
> of SQL function bodies there's an obvious use-case.
> 
>> We could get something like this working just inside the CREATE EXTENSION command if we expanded on the @extschema@ idea a bit.  At first I thought this idea would suffer race conditions with concurrent modifications of pg_extension or pg_namespace, but it looks like we already have a snapshot when processing the script file, so:
> 
>> -CREATE DOMAIN earth AS cube
>> +CREATE DOMAIN @@earthdistance@@::earth AS @@cube@@::cube
> 
> Right, extending the @extschema@ mechanism is what was discussed,
> though I think I'd lean towards something like @extschema:cube@
> to denote the schema of a referenced extension "cube".
> 
> I'm not sure this is useful enough to break feature freeze for,
> but I'm +1 for investigating it for v15.
Just like we have a pseudo "$user" schema, could we have a pseudo
"$extension" catalog?  That should avoid changing grammar rules too much.

CREATE TABLE unaccented_words (
    word "$extension".citext.citext,
    CHECK (word = "$extension".unaccent.unaccent(word)
);

-- 
Vik Fearing

  16. Re: Converting contrib SQL functions to new style

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-04-14T22:18:48Z

    
> On Apr 14, 2021, at 2:47 PM, Vik Fearing <vik@postgresfriends.org> wrote:
> 
> On 4/14/21 7:36 PM, Tom Lane wrote:
>> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>>>> On Apr 13, 2021, at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> However I think we may still need an assumption that earthdistance
>>>> and cube are in the same schema --- any comments on that?
>> 
>>> This is probably not worth doing, and we are already past feature
>>> freeze, but adding syntax to look up the namespace of an extension might
>>> help.
>> 
>> Yeah, that idea was discussed before (perhaps only in private
>> security-team threads, though).  We didn't do anything about it because
>> at the time there didn't seem to be pressing need, but in the context
>> of SQL function bodies there's an obvious use-case.
>> 
>>> We could get something like this working just inside the CREATE EXTENSION command if we expanded on the @extschema@ idea a bit.  At first I thought this idea would suffer race conditions with concurrent modifications of pg_extension or pg_namespace, but it looks like we already have a snapshot when processing the script file, so:
>> 
>>> -CREATE DOMAIN earth AS cube
>>> +CREATE DOMAIN @@earthdistance@@::earth AS @@cube@@::cube
>> 
>> Right, extending the @extschema@ mechanism is what was discussed,
>> though I think I'd lean towards something like @extschema:cube@
>> to denote the schema of a referenced extension "cube".
>> 
>> I'm not sure this is useful enough to break feature freeze for,
>> but I'm +1 for investigating it for v15.
> Just like we have a pseudo "$user" schema, could we have a pseudo
> "$extension" catalog?  That should avoid changing grammar rules too much.
> 
> CREATE TABLE unaccented_words (
>    word "$extension".citext.citext,
>    CHECK (word = "$extension".unaccent.unaccent(word)
> );

Having a single variable $extension might help in many cases, but I don't see how to use it to handle the remaining cross-extension references, such as earthdistance needing to reference cube.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  17. Re: Converting contrib SQL functions to new style

    Vik Fearing <vik@postgresfriends.org> — 2021-04-15T00:18:56Z

    On 4/15/21 12:18 AM, Mark Dilger wrote:
> 
> 
>> On Apr 14, 2021, at 2:47 PM, Vik Fearing <vik@postgresfriends.org> wrote:
>>
>> On 4/14/21 7:36 PM, Tom Lane wrote:
>>> Mark Dilger <mark.dilger@enterprisedb.com> writes:
>>>>> On Apr 13, 2021, at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>> However I think we may still need an assumption that earthdistance
>>>>> and cube are in the same schema --- any comments on that?
>>>
>>>> This is probably not worth doing, and we are already past feature
>>>> freeze, but adding syntax to look up the namespace of an extension might
>>>> help.
>>>
>>> Yeah, that idea was discussed before (perhaps only in private
>>> security-team threads, though).  We didn't do anything about it because
>>> at the time there didn't seem to be pressing need, but in the context
>>> of SQL function bodies there's an obvious use-case.
>>>
>>>> We could get something like this working just inside the CREATE EXTENSION command if we expanded on the @extschema@ idea a bit.  At first I thought this idea would suffer race conditions with concurrent modifications of pg_extension or pg_namespace, but it looks like we already have a snapshot when processing the script file, so:
>>>
>>>> -CREATE DOMAIN earth AS cube
>>>> +CREATE DOMAIN @@earthdistance@@::earth AS @@cube@@::cube
>>>
>>> Right, extending the @extschema@ mechanism is what was discussed,
>>> though I think I'd lean towards something like @extschema:cube@
>>> to denote the schema of a referenced extension "cube".
>>>
>>> I'm not sure this is useful enough to break feature freeze for,
>>> but I'm +1 for investigating it for v15.
>> Just like we have a pseudo "$user" schema, could we have a pseudo
>> "$extension" catalog?  That should avoid changing grammar rules too much.
>>
>> CREATE TABLE unaccented_words (
>>    word "$extension".citext.citext,
>>    CHECK (word = "$extension".unaccent.unaccent(word)
>> );
> 
> Having a single variable $extension might help in many cases, but I don't see how to use it to handle the remaining cross-extension references, such as earthdistance needing to reference cube.


Sorry, I hadn't realized that was a real example so I made up my own.

Basically my idea is to use the fully qualified catalog.schema.object
syntax where the catalog is a special "$extension" value (meaning we
would have to forbid that as an actual database name) and the schema is
the name of the extension whose schema we want.  The object is then just
the object.


CREATE DOMAIN earth AS "$extension".cube.cube
  CONSTRAINT not_point check("$extension".cube.cube_is_point(value))
  CONSTRAINT not_3d check("$extension".cube.cube_dim(value <= 3)
  ...;


CREATE FUNCTION earth_box(earth, float8)
 RETURNS "$extension".cube.cube
 LANGUAGE sql
 IMMUTABLE PARALLEL SAFE STRICT
RETURN "$extension".cube.cube_enlarge($1, gc_to_sec($2), 3);


If I had my druthers, we would spell it pg_extension instead of
"$extension" because I hate double-quoting identifiers, but that's just
bikeshedding and has little to do with the concept itself.
-- 
Vik Fearing

  18. Re: Converting contrib SQL functions to new style

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-04-15T17:23:13Z

    On 2021-Apr-15, Vik Fearing wrote:

> CREATE DOMAIN earth AS "$extension".cube.cube
>   CONSTRAINT not_point check("$extension".cube.cube_is_point(value))
>   CONSTRAINT not_3d check("$extension".cube.cube_dim(value <= 3)
>   ...;

I find this syntax pretty weird -- here, the ".cube." part of the
identifier is acting as an argument of sorts for the preceding
$extension thingy.  This looks very surprising.

Something similar to OPERATOR() syntax may be more palatable:

 CREATE DOMAIN earth AS PG_EXTENSION_SCHEMA(cube).cube
   CONSTRAINT not_point check(PG_EXTENSION_SCHEMA(cube).cube_is_point(value))
   CONSTRAINT not_3d check(PG_EXTENSION_SCHEMA(cube).cube_dim(value <= 3)
   ...;

Here, the PG_EXTENSION_SCHEMA() construct expands into the schema of the
given extension.  This looks more natural to me, since the extension
that acts as argument to PG_EXTENSION_SCHEMA() does look like an
argument.

I don't know if the parser would like this, though.

-- 
Álvaro Herrera       Valdivia, Chile

  19. Re: Converting contrib SQL functions to new style

    Noah Misch <noah@leadboat.com> — 2021-04-16T01:40:04Z

    On Wed, Apr 14, 2021 at 02:03:56PM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Apr 14, 2021 at 1:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Could we hack things so that extension scripts are only allowed to
> >> reference objects created (a) by the system, (b) earlier in the
> >> same script, or (c) owned by one of the declared prerequisite
> >> extensions?  Seems like that might provide a pretty bulletproof
> >> defense against trojan-horse objects, though I'm not sure how much
> >> of a pain it'd be to implement.

Good idea.

> > That doesn't seem like a crazy idea, but the previous idea of having
> > some magic syntax that means "the schema where extension FOO is" seems
> > like it might be easier to implement and more generally useful.
> 
> I think that's definitely useful, but it's not a fix for the
> reference-capture problem unless you care to assume that the other
> extension's schema is free of trojan-horse objects.

I could see using that, perhaps in a non-SQL-language function.  I agree it
solves different problems.

  20. Re: Converting contrib SQL functions to new style

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-01T07:26:14Z

    On 14.04.21 00:26, Tom Lane wrote:
> Attached are some draft patches to convert almost all of the
> contrib modules' SQL functions to use SQL-standard function bodies.

This first patch is still the patch of record in CF 2021-09, but from 
the subsequent discussion, it seems more work is being contemplated.

  21. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-01T17:27:35Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 14.04.21 00:26, Tom Lane wrote:
>> Attached are some draft patches to convert almost all of the
>> contrib modules' SQL functions to use SQL-standard function bodies.

> This first patch is still the patch of record in CF 2021-09, but from 
> the subsequent discussion, it seems more work is being contemplated.

Yeah, it looks like we already did the unconditionally-safe part
(i.e. making initdb-created SQL functions use new style, cf 767982e36).

The rest of this is stuck pending investigation of the ideas about
making new-style function creation safer when the creation-time path
isn't secure, so I suppose we should mark it RWF rather than leaving
it in the queue.  Will go do that.

			regards, tom lane

  22. Re: Converting contrib SQL functions to new style

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2024-10-29T07:28:55Z

    Le mercredi 1 septembre 2021, 19:27:35 heure normale d’Europe centrale Tom 
Lane a écrit :
> The rest of this is stuck pending investigation of the ideas about
> making new-style function creation safer when the creation-time path
> isn't secure, so I suppose we should mark it RWF rather than leaving
> it in the queue.  Will go do that.

Sorry to revive such an old thread but ... since the introduction of the 
@extschema:name@ syntax in 72a5b1fc880481914da2d4233077438dd87840ca we can now 
proceed with this or am I missing something ?

I've updated the previous patches to convert them to the new-style, added one 
for lo as well. Most of them are just patching against a newer extension 
version than what was provided at the time, except for earthdistance which now 
references everything explicitly by pg_catalog or @extschema:cube@. 

This is a bit tangential to the initial subject, but take the opportunity to 
also update the CREATE DOMAIN statement in earthdistance extension to 
explicitly reference the cube schema.

Best regards,

--
Ronan Dunklau

  23. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-04T16:10:01Z

    Ronan Dunklau <ronan.dunklau@aiven.io> writes:
> Le mercredi 1 septembre 2021, 19:27:35 heure normale d’Europe centrale Tom 
> Lane a écrit :
>> The rest of this is stuck pending investigation of the ideas about
>> making new-style function creation safer when the creation-time path
>> isn't secure, so I suppose we should mark it RWF rather than leaving
>> it in the queue.  Will go do that.

> Sorry to revive such an old thread but ... since the introduction of the 
> @extschema:name@ syntax in 72a5b1fc880481914da2d4233077438dd87840ca we can now 
> proceed with this or am I missing something ?

Yeah, seems like we could make it work with that.

> I've updated the previous patches to convert them to the new-style, added one 
> for lo as well.

The cfbot says many of these fail regression tests --- lots of

 CREATE EXTENSION citext SCHEMA s;
+ERROR:  extension "citext" has no installation script nor update path for version "1.8"

and such.  I didn't poke into why, but maybe you missed "git add'ing"
some changes?

			regards, tom lane

  24. Re: Converting contrib SQL functions to new style

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2024-11-05T09:14:02Z

    Le lundi 4 novembre 2024, 17:10:01 heure normale d’Europe centrale Tom Lane a 
écrit :
> The cfbot says many of these fail regression tests --- lots of
> 
>  CREATE EXTENSION citext SCHEMA s;
> +ERROR:  extension "citext" has no installation script nor update path for
> version "1.8"
> 
> and such.  I didn't poke into why, but maybe you missed "git add'ing"
> some changes?
> 

Sorry you're right I missed one for xml2. But most importantly I forgot to 
update the meson build files for every one of them, using only the Makefile...

Please find attached a new version which should make this right. 

Regards,

--
Ronan Dunklau

  25. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2024-11-06T00:51:09Z

    On Tue, Nov 05, 2024 at 10:14:02AM +0100, Ronan Dunklau wrote:
> Sorry you're right I missed one for xml2. But most importantly I forgot to 
> update the meson build files for every one of them, using only the Makefile...

If you want to catch that easily in the future, you can also set up
the CI within your own repo.  This has saved me many times and the
cfbot sometimes takes a few days to report back.  This usually reports
within 15 minutes.

I was wondering what was going on here, and this patch comes down to
switching all these definitions from that:
CREATE FUNCTION lo_oid(lo) RETURNS pg_catalog.oid AS
'SELECT $1::pg_catalog.oid' LANGUAGE SQL STRICT IMMUTABLE PARALLEL SAFE;

To that:
+CREATE OR REPLACE FUNCTION lo_oid(lo) RETURNS pg_catalog.oid
+LANGUAGE SQL STRICT IMMUTABLE PARALLEL SAFE
+RETURN (SELECT $1::pg_catalog.oid);

This makes the executions more robust run-time search_path checks.  Is
that something that should be considered for a backpatch, actually?
--
Michael

  26. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-11-06T01:05:16Z

    Michael Paquier <michael@paquier.xyz> writes:
> I was wondering what was going on here, and this patch comes down to
> switching all these definitions from that:
> CREATE FUNCTION lo_oid(lo) RETURNS pg_catalog.oid AS
> 'SELECT $1::pg_catalog.oid' LANGUAGE SQL STRICT IMMUTABLE PARALLEL SAFE;

> To that:
> +CREATE OR REPLACE FUNCTION lo_oid(lo) RETURNS pg_catalog.oid
> +LANGUAGE SQL STRICT IMMUTABLE PARALLEL SAFE
> +RETURN (SELECT $1::pg_catalog.oid);

Right.

> This makes the executions more robust run-time search_path checks.  Is
> that something that should be considered for a backpatch, actually?

No, I don't think so.  For one thing, it would not help existing
installations unless they issue "ALTER EXTENSION UPDATE", which
people are not likely to do in a minor update.  But also, we don't
know of live attacks against these functions with their current
definitions, so I don't think this is urgent.

			regards, tom lane

  27. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2024-11-06T05:52:16Z

    On Tue, Nov 05, 2024 at 08:05:16PM -0500, Tom Lane wrote:
> No, I don't think so.  For one thing, it would not help existing
> installations unless they issue "ALTER EXTENSION UPDATE", which
> people are not likely to do in a minor update.  But also, we don't
> know of live attacks against these functions with their current
> definitions, so I don't think this is urgent.

OK, thanks.
--
Michael

  28. Re: Converting contrib SQL functions to new style

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2024-11-06T09:51:29Z

    Le mercredi 6 novembre 2024, 06:52:16 heure normale d’Europe centrale Michael 
Paquier a écrit :
> On Tue, Nov 05, 2024 at 08:05:16PM -0500, Tom Lane wrote:
> > No, I don't think so.  For one thing, it would not help existing
> > installations unless they issue "ALTER EXTENSION UPDATE", which
> > people are not likely to do in a minor update.  But also, we don't
> > know of live attacks against these functions with their current
> > definitions, so I don't think this is urgent.
> 
> OK, thanks.

For most of them I agree, but one side effect of the current implementation is 
that we have a bug when pg_upgrad'ing if earthdistance is installed: 
https://www.postgresql.org/message-id/flat/
152106914669.1223.5104148605998271987%40wrigleys.postgresql.org

The new version of earthdistance fixes that problem, so this one would be worth 
a back patch to 16, giving users the opportunity to update earthdistance 
before running pg_upgrade.

Thanks for the tip about the CI, will make sure to have it setup before next 
time.

--
Ronan Dunklau

  29. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2024-11-07T01:06:37Z

    On Wed, Nov 06, 2024 at 10:51:29AM +0100, Ronan Dunklau wrote:
> For most of them I agree, but one side effect of the current implementation is 
> that we have a bug when pg_upgrad'ing if earthdistance is installed: 
> https://www.postgresql.org/message-id/flat/
> 152106914669.1223.5104148605998271987%40wrigleys.postgresql.org
> 
> The new version of earthdistance fixes that problem, so this one would be worth 
> a back patch to 16, giving users the opportunity to update earthdistance 
> before running pg_upgrade.

Good point.  Checking all these contrib updates one-by-one is an ant's
work, but I'll see if I can get at least some of them done on HEAD.

> Thanks for the tip about the CI, will make sure to have it setup before next 
> time.

See also "Enabling cirrus-ci in a github repository" in
src/tools/ci/README.
--
Michael

  30. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2024-11-12T08:30:30Z

    On Thu, Nov 07, 2024 at 10:06:37AM +0900, Michael Paquier wrote:
> Good point.  Checking all these contrib updates one-by-one is an ant's
> work, but I'll see if I can get at least some of them done on HEAD.

I've begun looking at that a bit, and there are a couple of things
that we could do better with xml2 in 0005 at least in the context of
this patch: xpath_nodeset() and xpath_list() don't have any test
coverage.  That's not an issue directly related to this patch, but
perhaps we should add something for the functions that we are
manipulating after this upgrade path at least?  That's one way to 
automatically make sure that these changes work the same way as the
original.

The same argument comes up with lo_oid() in 0006. 

0004 for pg_freespace is fine regarding that for example as we have
calls of pg_freespace(regclass) in its sql/.  I've applied it to begin
with something.

Tomas Vondra has posted a patch for a bug fix with pageinspect, so
this would create some conflicts noise for him if 0003 was applied
today, so let's wait a bit:
https://www.postgresql.org/message-id/3385a58f-5484-49d0-b790-9a198a0bf236@vondra.me

0001 and 0002 are much larger than the 4 others, and I'm lacking the
steam to check them in more details today.
--
Michael

  31. Re: Converting contrib SQL functions to new style

    Ronan Dunklau <ronan.dunklau@aiven.io> — 2024-11-13T08:15:08Z

    Le mardi 12 novembre 2024, 09:30:30 heure normale d’Europe centrale Michael 
Paquier a écrit :
> On Thu, Nov 07, 2024 at 10:06:37AM +0900, Michael Paquier wrote:
> > Good point.  Checking all these contrib updates one-by-one is an ant's
> > work, but I'll see if I can get at least some of them done on HEAD.
> 
> I've begun looking at that a bit, and there are a couple of things
> that we could do better with xml2 in 0005 at least in the context of
> this patch: xpath_nodeset() and xpath_list() don't have any test
> coverage.  That's not an issue directly related to this patch, but
> perhaps we should add something for the functions that we are
> manipulating after this upgrade path at least?  That's one way to
> automatically make sure that these changes work the same way as the
> original.
> 
> The same argument comes up with lo_oid() in 0006.

Ok, please find attached a new complete patch series including tests for the 
uncovered functions. Tests pass both before and after the move to SQL-body 
functions.


> 
> 0004 for pg_freespace is fine regarding that for example as we have
> calls of pg_freespace(regclass) in its sql/.  I've applied it to begin
> with something.

Thank you for this one, removed from the new series.

> 
> Tomas Vondra has posted a patch for a bug fix with pageinspect, so
> this would create some conflicts noise for him if 0003 was applied
> today, so let's wait a bit:
> https://www.postgresql.org/message-id/3385a58f-5484-49d0-b790-9a198a0bf236@v
> ondra.me

Agreed, let's wait on this one. I did not include it in the series. 

> 
> 0001 and 0002 are much larger than the 4 others, and I'm lacking the
> > steam to check them in more details today.

Thanks !

--
Ronan Dunklau

  32. Re: Converting contrib SQL functions to new style

    Peter Eisentraut <peter@eisentraut.org> — 2024-11-13T08:39:06Z

    On 13.11.24 09:15, Ronan Dunklau wrote:
> Le mardi 12 novembre 2024, 09:30:30 heure normale d’Europe centrale Michael
> Paquier a écrit :
>> On Thu, Nov 07, 2024 at 10:06:37AM +0900, Michael Paquier wrote:
>>> Good point.  Checking all these contrib updates one-by-one is an ant's
>>> work, but I'll see if I can get at least some of them done on HEAD.
>>
>> I've begun looking at that a bit, and there are a couple of things
>> that we could do better with xml2 in 0005 at least in the context of
>> this patch: xpath_nodeset() and xpath_list() don't have any test
>> coverage.  That's not an issue directly related to this patch, but
>> perhaps we should add something for the functions that we are
>> manipulating after this upgrade path at least?  That's one way to
>> automatically make sure that these changes work the same way as the
>> original.
>>
>> The same argument comes up with lo_oid() in 0006.
> 
> Ok, please find attached a new complete patch series including tests for the
> uncovered functions. Tests pass both before and after the move to SQL-body
> functions.

By the way, if we're going to touch all these extension script files to 
make them more modern SQL-like, we could also use named parameters more. 
  For example,

+CREATE OR REPLACE FUNCTION regexp_match(string citext, pattern citext) 
RETURNS TEXT[]
+LANGUAGE SQL IMMUTABLE STRICT PARALLEL SAFE
+RETURN pg_catalog.regexp_match( $1::pg_catalog.text, 
$2::pg_catalog.text, 'i' );

could be

+CREATE OR REPLACE FUNCTION regexp_match(string citext, pattern citext) 
RETURNS TEXT[]
+LANGUAGE SQL IMMUTABLE STRICT PARALLEL SAFE
+RETURN pg_catalog.regexp_match( string::pg_catalog.text, 
pattern::pg_catalog.text, 'i' );

etc.

  33. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2024-11-13T23:26:33Z

    On Wed, Nov 13, 2024 at 09:39:06AM +0100, Peter Eisentraut wrote:
> By the way, if we're going to touch all these extension script files to make
> them more modern SQL-like, we could also use named parameters more.

Sounds like a good idea to do.  Thanks for the suggestion.
--
Michael

  34. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2024-11-14T04:30:43Z

    On Wed, Nov 13, 2024 at 09:15:08AM +0100, Ronan Dunklau wrote:
> Ok, please find attached a new complete patch series including tests for the 
> uncovered functions. Tests pass both before and after the move to SQL-body 
> functions.

In 0001 for the tests of contrib/xml2/, you have forgotten to update
the alternate output used when compiling with xml2 and without
libxslt.  Fixed that, then applied.

0002 was OK as-is, so applied.

0006 to switch contrib/lo/ is so short that using named arguments does
not change much IMO, so I've left it as you proposed, then applied it.
pg_freespace with two functions is small enough that it does not seem
worth bothering more, as well.

Remains the cases of citext, pageinspect, xml2 and earthdistance.

The argument of using named parameters is appealing to self-document
the functions, but it seems to me that it gets much better once we
also do the same for all the functions of a given extension, not only
these whose body is changed.  This information does not show up in a
\dx+, it does in a \df. 

Doing that step-by-step is better than nothing, hence limiting the use
of named parameters for only the functions whose body is rewritten is
fine by me, as a first step, as long as the names are used rather the
dollar parameter numbers.  I'd suggest to do take the bonus step of
applying the same rule to all the other functions so as everything
applies with the same extension update in a single major release.
Perhaps on top of the patches already proposed?  There is no need for
an extra version bump if all that is done in the same development
cycle.

Thoughts?
--
Michael

  35. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-14T18:00:00Z

    Michael Paquier <michael@paquier.xyz> writes:
> Doing that step-by-step is better than nothing, hence limiting the use
> of named parameters for only the functions whose body is rewritten is
> fine by me, as a first step, as long as the names are used rather the
> dollar parameter numbers.  I'd suggest to do take the bonus step of
> applying the same rule to all the other functions so as everything
> applies with the same extension update in a single major release.
> Perhaps on top of the patches already proposed?  There is no need for
> an extra version bump if all that is done in the same development
> cycle.

FWIW, I don't agree at all with doing argument name addition in
this patchset.  Certainly that's a fine thing to do, but it's an
independent issue and should be handled in an independent patch.

I see that the cfbot is unhappy because it doesn't understand
that some of the patches have been applied already.  I am going
to go ahead and get the earthdistance one done, because we have
a live problem report about that [1].  I'll rebase and repost
the remainder afterwards.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/6a6439f1-8039-44e2-8fb9-59028f7f2014%40mailbox.org

  36. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-14T21:34:30Z

    I wrote:
> I see that the cfbot is unhappy because it doesn't understand
> that some of the patches have been applied already.  I am going
> to go ahead and get the earthdistance one done, because we have
> a live problem report about that [1].  I'll rebase and repost
> the remainder afterwards.

Pushed the earthdistance patch, backpatching to v16 as already
discussed upthread.  While it's still fresh in mind, a few
comments about that:

The search path during execution of earthdistance's scripts
is going to be pg_catalog (implicitly first), then earthdistance's
schema, then cube's schema (if different), then pg_temp (to make
sure it's not implicitly before the others).  So that means the
ground rules for object references are:

* References to pg_catalog objects are unconditionally secure,
with or without qualification, if they have simple names (such
as datatypes).  Calls of functions and operators in pg_catalog
are unconditionally secure if the argument data types exactly match
the function or operator.  If implicit casting is required, there's
a hazard of the reference being captured by a better match further
down the search path.  That can be fixed by adding explicit schema
qualification, but I think usually it's better to make the argument
data types match by explicitly casting them.  In particular the
syntax for schema-qualifying an operator is painful enough that
nobody wants to do that.

* References to earthdistance's own objects are again unconditionally
secure for simple names (given no conflicts with pg_catalog).  For
calls of functions and operators, the only safe answer is to make the
argument data types match: schema qualification isn't enough, since
there might be hostile objects in earthdistance's installation schema.

* References to cube's objects require an @extschema:cube@
qualification to ensure they aren't captured by hostile objects
in earthdistance's schema, *and* we have to be careful about
exact argument data type matches to prevent capture by
hostile objects in cube's schema.

Ronan had added some "pg_catalog." qualifications to the previous
version of the patch, but I found all of them to be unnecessary
per these ground rules, and probably not good style because they
convey an impression of schema safety that is entirely false if
the operators aren't likewise qualified.

Also, I realized while looking at the patch that fixing the
functions wasn't sufficient, because we also had

CREATE DOMAIN earth AS cube
  CONSTRAINT not_point check(cube_is_point(value))
  CONSTRAINT not_3d check(cube_dim(value) <= 3)
  CONSTRAINT on_surface check(abs(cube_distance(value, '(0)'::cube) /
  earth() - '1'::float8) < '10e-7'::float8);

The reference to type cube is potentially subvertible, and so are the
cube function calls.  We could fix the constraint expressions (at
great cost) in the 1.2 update script by dropping and re-adding the
domain constraints, but there's nothing the update script can do to
change the domain's base type.

What I did about this was to add @extschema:cube@ decoration in this
command in the 1.1 base script.  I don't think this is a violation of
the normal rule that extension scripts mustn't change post-release,
because the intended results are the same.  (That is, really the rule
is "the results of installation mustn't change".)  This fixes things
for fresh installations including dump/restore updates.  It won't do
anything for you if you're trying to pg_upgrade an already-trojaned
earthdistance extension, but it's better than not addressing the issue
at all.

I think we could now mark earthdistance as trusted (probably only
in HEAD), but it'd be good for someone else to verify it.

			regards, tom lane

  37. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-14T22:08:19Z

    Here's the remaining two patches in the current set.  This is just
to pacify the cfbot: I've not done anything to them, just verified
that they still apply and pass regression.

			regards, tom lane

  38. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-29T18:56:55Z

    I wrote:
> Here's the remaining two patches in the current set.  This is just
> to pacify the cfbot: I've not done anything to them, just verified
> that they still apply and pass regression.

I reviewed and pushed these.  Let's not forget that the pageinspect
one is still pending, though.  We were waiting on Tomas' fix, which
is now pushed at 957ba9ff1, so I suppose it needs a rebase.

			regards, tom lane

  39. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-12-29T20:01:16Z

    I wrote:
> I reviewed and pushed these.  Let's not forget that the pageinspect
> one is still pending, though.  We were waiting on Tomas' fix, which
> is now pushed at 957ba9ff1, so I suppose it needs a rebase.

Actually ... that one's quite trivial, so I went ahead and pushed it.

The submitted patch had the functions as PARALLEL SAFE, but I think
they have to remain PARALLEL RESTRICTED because they use
tuple_data_split(), cf pageinspect--1.10--1.11.sql.

			regards, tom lane

  40. Re: Converting contrib SQL functions to new style

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-04T20:14:55Z

    I happened to notice that there's more that we can do to harden
contrib modules: the transform modules for hstore and ltree
currently have disclaimers about having to install them in the
same schema as the underlying modules.  AFAICS that can be
fixed trivially now, by using the @extschema:name@ mechanism.

In the attached I just modified the 1.0 installation scripts
rather than making update scripts.  As for the problem with
earthdistance's CREATE DOMAIN command, there's no way for
an update script to rescue an already-trojaned definition,
and the script change does not affect the results in any
non-compromised case.

(The transform modules for bool and jsonb don't need to be
touched, since those data types are in pg_catalog which
will always be the front of the search path.)

I also noticed some text in extend.sgml that needs to be
updated.

			regards, tom lane

  41. Re: Converting contrib SQL functions to new style

    Noah Misch <noah@leadboat.com> — 2025-01-06T19:04:28Z

    On Tue, Nov 12, 2024 at 05:30:30PM +0900, Michael Paquier wrote:
> 0004 for pg_freespace is fine regarding that for example as we have
> calls of pg_freespace(regclass) in its sql/.  I've applied it to begin
> with something.

That commit (3f323eb) contains a generate_series(int, bigint) call.  Following
https://www.postgresql.org/docs/current/ddl-schemas.html#DDL-SCHEMAS-PATTERNS
is enough in v17, but it wouldn't be enough after that commit.  An attacker
can achieve persistent capture of the inexact call:

set search_path = pg_catalog;
create or replace function public.generate_series(int, bigint) returns bigint
  language plpgsql as $$
BEGIN
	RAISE NOTICE 'owned';
	RETURN 0;
END
$$;
create extension pg_freespacemap schema public;
select public.pg_freespace(0);
\sf public.pg_freespace(regclass)

Per postgr.es/m/3489827.1618411777@sss.pgh.pa.us and
postgr.es/m/1471865.1734212070@sss.pgh.pa.us one requirement for migrating to
SQL-standard function bodies is removing these inexact-match function and
operator calls.  Here, one could either write pg_catalog.generate_series or
make the argument types match exactly.

  42. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2025-01-08T02:32:00Z

    On Mon, Jan 06, 2025 at 11:04:28AM -0800, Noah Misch wrote:
> Per postgr.es/m/3489827.1618411777@sss.pgh.pa.us and
> postgr.es/m/1471865.1734212070@sss.pgh.pa.us one requirement for migrating to
> SQL-standard function bodies is removing these inexact-match function and
> operator calls.  Here, one could either write pg_catalog.generate_series or
> make the argument types match exactly.

Thanks for the report.

Tom has used casts for xml2 in 667368fd26de, and also combined some
pg_catalog for schema qualifications in citext for example.  In this
case forcing the data types seems more elegant to me.

What do you think about the attached?
--
Michael

  43. Re: Converting contrib SQL functions to new style

    Noah Misch <noah@leadboat.com> — 2025-01-08T02:52:33Z

    On Wed, Jan 08, 2025 at 11:32:00AM +0900, Michael Paquier wrote:
> On Mon, Jan 06, 2025 at 11:04:28AM -0800, Noah Misch wrote:
> > Per postgr.es/m/3489827.1618411777@sss.pgh.pa.us and
> > postgr.es/m/1471865.1734212070@sss.pgh.pa.us one requirement for migrating to
> > SQL-standard function bodies is removing these inexact-match function and
> > operator calls.  Here, one could either write pg_catalog.generate_series or
> > make the argument types match exactly.
> 
> Thanks for the report.
> 
> Tom has used casts for xml2 in 667368fd26de, and also combined some
> pg_catalog for schema qualifications in citext for example.  In this
> case forcing the data types seems more elegant to me.

> --- a/contrib/pg_freespacemap/pg_freespacemap--1.2--1.3.sql
> +++ b/contrib/pg_freespacemap/pg_freespacemap--1.2--1.3.sql
> @@ -9,5 +9,5 @@ RETURNS SETOF RECORD
>  LANGUAGE SQL PARALLEL SAFE
>  BEGIN ATOMIC
>    SELECT blkno, pg_freespace($1, blkno) AS avail
> -  FROM generate_series(0, pg_relation_size($1) / current_setting('block_size')::bigint - 1) AS blkno;
> +  FROM generate_series(0::bigint, pg_relation_size($1) / current_setting('block_size'::text)::bigint - 1::bigint) AS blkno;
>  END;

It's more optimal to write '0'::bigint.  That generates a CONST node, whereas
0::bigint generates a FUNCEXPR calling the cast function.  No other concerns.

  44. Re: Converting contrib SQL functions to new style

    Michael Paquier <michael@paquier.xyz> — 2025-01-08T04:18:36Z

    On Tue, Jan 07, 2025 at 06:52:33PM -0800, Noah Misch wrote:
> It's more optimal to write '0'::bigint.  That generates a CONST node, whereas
> 0::bigint generates a FUNCEXPR calling the cast function.  No other concerns.

Makes sense, done this way.
--
Michael