Thread: Loose ends after CVE-2020-14350 (extension installation hazards)

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

On 08/14/20 14:50, Tom Lane wrote:
>     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.

This reminds me of the way the SQL standard overloads WITH to supply
lexically-scoped settings of things, as well as CTEs, mentioned a while
back. [1]

Would this provide additional incentive to implement that syntax,
generalized to support arbitrary GUCs and not just the handful of
specific settings the standard uses it for?

Regards,
-Chap



[1] https://www.postgresql.org/message-id/5AAEAE0F.20006%40anastigmatix.net

Chapman Flack <chap@anastigmatix.net> writes:
> On 08/14/20 14:50, Tom Lane wrote:
>> 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.

> This reminds me of the way the SQL standard overloads WITH to supply
> lexically-scoped settings of things, as well as CTEs, mentioned a while
> back. [1]
> Would this provide additional incentive to implement that syntax,
> generalized to support arbitrary GUCs and not just the handful of
> specific settings the standard uses it for?

Hmm.  I see a few things not to like about that:

(1) It's hard to see how the WITH approach could work for GUCs
that need to take effect during raw parsing, such as the much-detested
standard_conforming_strings.  Ideally we'd not have any such GUCs, for
the reasons explained at the top of gram.y, but I dunno that we'll ever
get there.

(2) We only have WITH for DML (SELECT/INSERT/UPDATE/DELETE), not utility
commands.  Maybe that's enough for the cases at hand.  Or maybe we'd be
willing to do whatever's needful to handle WITH attached to a utility
command, but that could be a pretty large addition of work.

(3) If the SQL syntax is really just "WITH variable value [, ...]"
then I'm afraid we're going to have a lot of parse-ambiguity problems
with wedging full SET syntax into that.  The ability for the righthand
side to be a comma-separated list is certainly going to go out the
window, and we have various other special cases like "SET TIME ZONE"
that aren't going to work.  Again, maybe we don't need a full solution,
but it seems like it's gonna be a kluge.

(4) You'd need to repeat the WITH for each SQL command, IIUC.  Could
get tedious.

So maybe this is worth doing just for more standards compliance, but
it doesn't really seem like a nicer solution than subtransaction-
scoped SET.

            regards, tom lane

On 08/14/20 15:38, Tom Lane wrote:

> (3) If the SQL syntax is really just "WITH variable value [, ...]"
> then I'm afraid we're going to have a lot of parse-ambiguity problems
> with wedging full SET syntax into that.  The ability for the righthand

There is precedent in the SET command for having one general syntax
usable for any GUC, and specialized ones for a few 'special' GUCs
(search_path, client_encoding, timezone).

Maybe WITH could be done the same way, inventing some less thorny syntax
for the general case

   WITH (foo = bar, baz), (quux = 42), XMLBINARY BASE64, a AS (SELECT...)

and treating just the few like XMLBINARY that appear in the standard
as having equivalent specialized productions?

The only examples of the syntax in the standard that are coming to mind
right now are those I've seen in the SQL/XML part, but I feel like I have
seen others, as if the committee kind of likes their WITH local-setting-
of-something syntax, and additional examples may continue to appear.

Regards,
-Chap

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