Loose ends after CVE-2020-14350 (extension installation hazards) - Mailing list pgsql-hackers

Yesterday's releases included some fixes meant to make it harder
for a malicious user to sabotage an extension installation/update
script.  There are some things remaining to be done in the area,
though:

1. We don't have a way to make things adequately secure for extensions
that depend on other extensions.  As an example, suppose that Alice
installs the "cube" extension into schema1 and then installs
"earthdistance" into schema2.  The earthdistance install script will
run with search_path "schema2, schema1".  If schema2 is writable by
Bob, then Bob can create a type "schema2.cube" ahead of time, and
that will capture all the references to cube in the earthdistance
script.  Bob can similarly capture the references to cube functions
in earthdistance's domain constraints.  A partial solution to this
might be to extend the @extschema@ notation to allow specification
of a depended-on extension's schema.  Inventing freely, we could
imagine writing earthdistance's domain creation command like

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

This is pretty tedious and error-prone; but as long as one is careful
to write only exact matches of function and operator argument types,
it's safe against CVE-2018-1058-style attacks, even if both extensions
are in publicly writable schemas.

However, in itself this can only fix references that are resolved during
execution of the extension script.  I don't see a good way to use the
idea to make earthdistance's SQL functions fully secure.  It won't do
to write, say,

CREATE FUNCTION ll_to_earth(float8, float8)
...
AS 'SELECT @extschema(cube)@.cube(...)';

because this will not survive somebody doing "ALTER EXTENSION cube SET
SCHEMA schema3".  I don't have a proposal for what to do about that.
Admittedly, we already disclaim security if you run queries with a
search_path that contains any untrusted schemas ... but it would be
nice if extensions could be written that (in themselves) are safe
regardless.  Peter E's proposal for parsing SQL function bodies at
creation time could perhaps fix this for SQL functions, but we still
have the issue for other PLs.

2. For the most part, the sorts of DDL commands you might use in an
extension script are not very subject to CVE-2018-1058-style attacks
because they specify relevant functions and operators exactly.  As long
as the objects you want are at the front of the search_path, there's no
way for an attacker to inject another, better match.  I did find one
exception that is not fixed as of today: lookup_agg_function() allows
inexact argument type matches for an aggregate's support functions,
so it could be possible to capture a reference if the intended support
function doesn't exactly match the aggregate's declared input and
transition data types.  This doesn't occur in any contrib modules
fortunately.  I do not see a way to make this better without breaking
the ability to use polymorphic support functions in a non-polymorphic
aggregate, since those are inherently not exact matches.

3. As Christoph Berg noted, the fixes in some extension update scripts
mean that plpgsql has to be installed while those scripts run.  How
much do we care, and if we do, what should we do about it?  I suggested
a band-aid fix of updating the base install scripts so that users don't
typically need to run the update scripts, but that's just a band-aid.
Maybe we could extend SQL enough so that plpgsql isn't needed to do
what those scripts have to do.  I'd initially thought of doing the
search path save-and-restore via

    SAVEPOINT s1;
    SET LOCAL search_path = pg_catalog, pg_temp;
    ... protected code here ...
    RELEASE SAVEPOINT s1;

but this does not work because SET LOCAL persists to the end of the
outer transaction.  Maybe we could invent a variant that only lasts
for the current subtransaction.

4. I noticed while testing that hstore--1.0--1.1.sql is completely
useless nowadays, so it might as well get dropped.  It fails with a
syntax error in every still-supported server version, since "=>" is
no longer a legal operator name.  There's no way to load hstore 1.0
into a modern server because of that, either.

Thoughts?

            regards, tom lane