CREATE EXTENSION scripts should use CREATE instead of CREATE OR REPLACE

[robe]

Best practices, all our postgis-3.3.sql should be using CREATE instead of CREATE OR REPLACE.

We should start doing this with 3.3.

[strk]

I would like to know more about the rationale for this. Using CREATE without OR REPLACE would imply using a DROP of the function when the function *body* changed across PostGIS versions, which in turn would prevent upgrades when the function is used in views.

Granted we DO now have a way to somehow handle the views (activated via replaces xxxx comments) but that handling would leave stale/old signatures in the database which is not very clean and fail to have the view use the *updated*/new function.

I think we DO WANT to actually _REPLACE_ our own functions, so if that's a dangerous thing to do we'd better handle the dangers, in our upgrade script, and continue to do the actual UPGRADE (and we want support for OR REPLACE in more objects too!)

[robe]

I was thinking that for 3.3.0 (and earlier) we do it just for the CREATE EXTENSION .. call.

I can't think of a situation where someone should be allowed to run CREATE EXTENSION and actually have any postgis functions installed.

For the upgrade CREATE OR REPLACE function is safe as long as we know it would have been installed by CREATE EXTENSION. Cause there is no chance of someone other than a super user having created that function since non-super users can't overwrite other people's functions.

The danger for ALTER EXTENSION here is if a regular untrusted user knows a function exists in postgis in a newer version not yet installed. They go create a function with that signature with the future expectation that postgis will be upgraded. Then when ALTER EXTENSION UPDATE is done, our function would now make a function they own be part of postgis, and they could then change that function putting malicious things in there. If a super user then runs this coopted function, they could accidentally elevate the privileges of said user (cause it would be running under super user rights).

In practice people can easily avoid this by preventing untrusted users from creating things in a schema where postgis is installed. We could also force ownership of all postgis packaged functions at end to be owned by the person running ALTER EXTENSION. I think the force is not a good idea because I suspect DbaaS are looking for that kind of stuff and would treat it as a threat and bale out. It probably would be seen as such too by many vulnerability scanners. So our attempt to mitigate such a thing would look like we're trying cause such a thing to happen.

[strk]

our function would now make a function they own be part of postgis

Woudn't it be much simpler to just FORCE ownership of functions, in the upgrade script ? We would check the ownership of a function which is known to have been in postgis forever (postgis_version?) and set ownership of all functions, after the CREATE OR REPLACE, to that user.

Would this block the kind of attack you describe ?

The enforcement of all function of the extension could be also done in a single final statement, as we can easily get the list of all functions in our extension, and could set ownership of those function match ownership of the extension itself

[robe]

Replying to strk:

our function would now make a function they own be part of postgis

Woudn't it be much simpler to just FORCE ownership of functions, in the upgrade script ? We would check the ownership of a function which is known to have been in postgis forever (postgis_version?) and set ownership of all functions, after the CREATE OR REPLACE, to that user.

Would this block the kind of attack you describe ?

The enforcement of all function of the extension could be also done in a single final statement, as we can easily get the list of all functions in our extension, and could set ownership of those function match ownership of the extension itself

I fear such a step would be flagged by vulnerability scanners as a vulnerability. Cause changing permissions is the kind of thing they are looking for. Although I guess they mostly look for ALTER ROLE so perhaps not. I still think doing CREATE instead of CREATE OR REPLACE for the create extension script is trivial enough don't you think? We don't need to change ownership, just change or regex for the postgis-3.3.0.sql file to replace CREATE OR REPLACE with CREATE.

[strk]

We should have a way to test this now, right ?

[robe]

Replying to strk:

We should have a way to test this now, right ?

test what?

[robe]

I have broken out address_standardizer from this ticket at #5195 and also postgis_tiger_geocoder at #5196

[tbussmann]

The danger for ALTER EXTENSION here is if a regular untrusted user knows a function exists in postgis in a newer version not yet installed. They go create a function with that signature with the future expectation that postgis will be upgraded. Then when ALTER EXTENSION UPDATE is done, our function would now make a function they own be part of postgis, and they could then change that function putting malicious things in there. If a super user then runs this coopted function, they could accidentally elevate the privileges of said user (cause it would be running under super user rights).

That describes what is my understanding of CVE-2022-2625. The upcoming PostgreSQL releases of this week 10.22, 11.17, 12.12, 13.8, 14.5 will therefore forbid to use CREATE OR REPLACE on a function that is not owned by the extension (see postgresql commit b9b21acc766db54d8c337d508d0fe2f5bf2daab0). This breaks the PostGIS regression tests and likely the possibility to upgrade from unpackaged to extension and thus the 2.x → 3.x upgrades where postgis_raster was repackaged. But these deserve their own tickets after being confirmed.

[robe]

So I suppose we can close this ticket out since it's been taken care of upstream.

As far as the breaking unraveling raster from postgis (upgrading from 2.5 to 3+), strk I recall you were working to make sure that we were covered by the above PostgreSQL commit change and your recent changes worked around that. But we might need to backport. I'll test and confirm.

[strk]

The workaround changes are now in all supported branches.

The 2-years old #4648 is still needed or can we close that one too ?

[robe]

yes #4648 is still needed.