Date: Wed, 14 Oct 2020 01:46:15 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Loose ends after CVE-2020-14350 (extension installation hazards)
Message-ID: <20201014084615.GA1972206@rfd.leadboat.com>
References: <653936.1597431032@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <653936.1597431032@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Fri, Aug 14, 2020 at 02:50:32PM -0400, Tom Lane wrote:
> 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.

Another challenge is verifying that the body qualified everything.  Via a
simple matter of programming, CREATE EXTENSION could verify that CREATE-time
code observes schema qualification rules.  That would not extend to function
bodies.

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

Yes.  Even when safety is not a concern, it's a quality problem for the
functions to error out when search_path lacks some schema.  As you know, we
get recurring reports about that, e.g.
https://www.postgresql.org/message-id/flat/16534-69f25077c45f34a5%40postgresql.org

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

Yes.  The SQL-specific feature could do enough to let a future version of
earthdistance be trusted.

> 2. [...] 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.

Should CREATE AGGREGATE support "FINALFUNC = foo(sometype)" input to constrain
the lookup?  (It does accept the syntax, but "sometype" is unused and need not
even denote an extant type.)

> 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 propose not caring at all.  Since we have dump/reload of "REVOKE USAGE ON
LANGUAGE plpgsql FROM PUBLIC", extensions requiring plpgsql are fine.  (It
could be a problem in a "superuser = false" extension, but core isn't doing
those.)  Even saddling plpgsql with a pin dependency would be fine.

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

The chance of this getting reported from the field has been dropping for
several years.  It's negligible now.

Thanks,
nm