Thread: Extension security improvement: Add support for extensions with an owned schema

Writing the sql migration scripts that are run by CREATE EXTENSION and
ALTER EXTENSION UPDATE are security minefields for extension authors.
One big reason for this is that search_path is set to the schema of the
extension while running these scripts, and thus if a user with lower
privileges can create functions or operators in that schema they can do
all kinds of search_path confusion attacks if not every function and
operator that is used in the script is schema qualified. While doing
such schema qualification is possible, it relies on the author to never
make a mistake in any of the sql files. And sadly humans have a tendency
to make mistakes.

This patch adds a new "owned_schema" option to the extension control
file that can be set to true to indicate that this extension wants to
own the schema in which it is installed. What that means is that the
schema should not exist before creating the extension, and will be
created during extension creation. This thus gives the extension author
an easy way to use a safe search_path, while still allowing all objects
to be grouped together in a schema. The implementation also has the
pleasant side effect that the schema will be automatically dropped when
the extension is dropped.

One way in which certain extensions currently hack around the
non-existence of this feature is by using the approach that pg_cron
uses: Setting the schema to pg_catalog and running "CREATE SCHEMA
pg_cron" from within the extension script. While this works, it's
obviously a hack, and a big downside of it is that it doesn't allow
users to choose the schema name used by the extension.

PS. I have never added fields to pg_catalag tables before, so there's
a clear TODO in the pg_upgrade code related to that. If anyone has
some pointers for me to look at to address that one that would be
helpful, if not I'll probably figure it out myself. All other code is
in pretty finished state, although I'm considering if
AlterExtensionNamespace should maybe be split a bit somehow, because
owned_schema skips most of the code in that function.

On Sun, Jun 2, 2024, 02:08 Jelte Fennema-Nio <me@jeltef.nl> wrote:
> This patch adds a new "owned_schema" option to the extension control
> file that can be set to true to indicate that this extension wants to
> own the schema in which it is installed.

Huge +1

Many managed PostgreSQL services block superuser access, but provide a
way for users to trigger a create/alter extension as superuser. There
have been various extensions whose SQL scripts can be tricked into
calling a function that was pre-created in the extension schema. This
is usually done by finding an unqualified call to a pg_catalog
function/operator, and overloading it with one whose arguments types
are a closer match for the provided values, which then takes
precedence regardless of search_path order. The custom function can
then do something like "alter user foo superuser".

The sequence of steps assumes the user already has some kind of admin
role and is gaining superuser access to their own database server.
However, the superuser implicitly has shell access, so it gives
attackers an additional set of tools to poke around in the managed
service. For instance, they can change the way the machine responds to
control plane requests, which can sometimes trigger further
escalations. In addition, many applications use the relatively
privileged default user, which means SQL injection issues can also
escalate into superuser access and beyond.

There are some static analysis tools like
https://github.com/timescale/pgspot that address this issue, though it
seems like a totally unnecessary hole. Using schema = pg_catalog,
relocatable = false, and doing an explicit create schema (without "if
not exists") plugs the hole by effectively disabling extension
schemas. For extensions I'm involved in, I consider this to be a hard
requirement.

I think Jelte's solution is preferable going forward, because it
preserves the flexibility that extension schemas were meant to
provide, and makes the potential hazards of reusing a schema more
explicit.

cheers,
Marco

On Sat, 2024-06-01 at 17:08 -0700, Jelte Fennema-Nio wrote:
> This patch adds a new "owned_schema" option to the extension control
> file that can be set to true to indicate that this extension wants to
> own the schema in which it is installed. What that means is that the
> schema should not exist before creating the extension, and will be
> created during extension creation. This thus gives the extension
> author
> an easy way to use a safe search_path, while still allowing all
> objects
> to be grouped together in a schema. The implementation also has the
> pleasant side effect that the schema will be automatically dropped
> when
> the extension is dropped.

Is this orthogonal to relocatability?

When you say "an easy way to use a safe search_path": the CREATE
EXTENSION command already sets the search_path, so your patch just
ensures that it's empty (and therefore safe) first, right?

Should we go further and try to prevent creating objects in an
extension-owned schema with normal SQL?

Approximately how many extensions should be adjusted to use
owned_schema=true? What are the reasons an extension would not want to
own the schema in which the objects are created? I assume some would
still create objects in pg_catalog, but ideally we'd come up with a
better solution to that as well.

This protects the extension script, but I'm left wondering if we could
do something here to make it easier to protect extension functions
called from outside the extension script, also. It would be nice if we
could implicitly tack on a "SET search_path TO @extschema@, pg_catalog,
pg_temp" to each function in the extension. I'm not proposing that, but
perhaps a related idea might work. Probably outside the scope of your
proposal.

Regards,
    Jeff Davis

On Wed, 5 Jun 2024 at 19:53, Jeff Davis <pgsql@j-davis.com> wrote:
> Is this orthogonal to relocatability?

It's fairly orthogonal, but it has some impact on relocatability: You
can only relocate to a schema name that does not exist yet (while
currently you can only relocate to a schema that already exists). This
is because, for owned_schema=true, relocation is not actually changing
the schema of the extension objects, it only renames the existing
schema to the new name.

> When you say "an easy way to use a safe search_path": the CREATE
> EXTENSION command already sets the search_path, so your patch just
> ensures that it's empty (and therefore safe) first, right?

Correct: **safe** is the key word in that sentence. Without
owned_schema, you get an **unsafe** search_path by default unless you
go out of your way to set "schema=pg_catalog" in the control file.

> Should we go further and try to prevent creating objects in an
> extension-owned schema with normal SQL?

That would be nice for sure, but security wise it doesn't matter
afaict. Only the creator of the extension would be able to add stuff
in the extension-owned schema anyway, so there's no privilege
escalation concern there.

> Approximately how many extensions should be adjusted to use
> owned_schema=true?

Adjusting existing extensions would be hard at the moment, because the
current patch does not introduce a migration path. But basically I
think for most new extension installs (even of existing extensions) it
would be fine if owned_schema=true would be the default. I didn't
propose (yet) to make it the default though, to avoid discussing the
tradeoff of security vs breaking installation for an unknown amount of
existing extensions.

I think having a generic migration path would be hard, due to the many
ways in which extensions can now be installed. But I think we might be
able to add one fairly easily for relocatable extensions: e.g. "ALTER
EXTESION SET SCHEMA new_schema OWNED_SCHEMA", which would essentially
do CREATE SCHEMA new_schema + move all objects from old_schema to
new_schema. And even for non-relocatable one you could do something
like:

CREATE SCHEMA temp_schema_{random_id};
-- move all objects from ext_schema to temp_schema_{random_id};
DROP SCHEMA ext_schema; -- if this fails, ext_schema was not empty
ALTER SCHEMA temp_schema_{random_id} RENAME TO ext_schema;

> What are the reasons an extension would not want to
> own the schema in which the objects are created? I assume some would
> still create objects in pg_catalog, but ideally we'd come up with a
> better solution to that as well.

Some extensions depend on putting stuff into the public schema. But
yeah it would be best if they didn't.

> This protects the extension script, but I'm left wondering if we could
> do something here to make it easier to protect extension functions
> called from outside the extension script, also. It would be nice if we
> could implicitly tack on a "SET search_path TO @extschema@, pg_catalog,
> pg_temp" to each function in the extension. I'm not proposing that, but
> perhaps a related idea might work. Probably outside the scope of your
> proposal.

Yeah, this proposal definitely doesn't solve all security problems
with extensions. And indeed what you're proposing would solve another
major issue, another option would be to default to the "safe"
search_path that you proposed a while back. But yeah I agree that it's
outside of the scope of this proposal. I feel like if we try to solve
every security problem at once, probably nothing gets solved instead.
That's why I tried to keep this proposal very targeted, i.e. have this
be step 1 of an N step plan to make extensions more secure by default.

Attached is an updated version of this patch that fixes a few issues
that CI reported (autoconf, compiler warnings and broken docs).

I also think I changed the pg_upgrade to do the correct thing, but I'm
not sure how to test this (even manually). Because part of it would
only be relevant once we support upgrading from PG18. So for now the
upgrade_code I haven't actually run.

On Sat, Jun 1, 2024 at 8:08 PM Jelte Fennema-Nio <me@jeltef.nl> wrote:
> Writing the sql migration scripts that are run by CREATE EXTENSION and
> ALTER EXTENSION UPDATE are security minefields for extension authors.
> One big reason for this is that search_path is set to the schema of the
> extension while running these scripts, and thus if a user with lower
> privileges can create functions or operators in that schema they can do
> all kinds of search_path confusion attacks if not every function and
> operator that is used in the script is schema qualified. While doing
> such schema qualification is possible, it relies on the author to never
> make a mistake in any of the sql files. And sadly humans have a tendency
> to make mistakes.

I agree that this is a problem. I also think that the patch might be a
reasonable solution (but I haven't reviewed it).

But I wonder if there might also be another possible approach: could
we, somehow, prevent object references in extension scripts from
resolving to anything other than the system catalogs and the contents
of that extension? Perhaps with a control file setting to specify a
list of trusted extensions which we're also allowed to reference?

I have a feeling that this might be pretty annoying to implement, and
if that is true, then never mind. But if it isn't that annoying to
implement, it would make a lot of unsafe extensions safe by default,
without the extension author needing to take any action. Which could
be pretty cool. It would also make it possible for extensions to
safely share a schema, if desired.

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

On Wed, 19 Jun 2024 at 17:28, Robert Haas <robertmhaas@gmail.com> wrote:
> But I wonder if there might also be another possible approach: could
> we, somehow, prevent object references in extension scripts from
> resolving to anything other than the system catalogs and the contents
> of that extension?

This indeed does sound like the behaviour that pretty much every
existing extension wants to have. One small addition/clarification
that I would make to your definition: fully qualified references to
other objects should still be allowed.

I do think, even if we have this, there would be other good reasons to
use "owned schemas" for extension authors. At least the following two:
1. To have a safe search_path that can be used in SET search_path on a
function (see also [1]).
2. To make it easy for extension authors to avoid conflicts with other
extensions/UDFs.

> Perhaps with a control file setting to specify a
> list of trusted extensions which we're also allowed to reference?

I think we could simply use the already existing "requires" field from
the control file. i.e. you're allowed to reference only your own
extension

> I have a feeling that this might be pretty annoying to implement, and
> if that is true, then never mind.

Based on a quick look it's not trivial, but also not super bad.
Basically it seems like in src/backend/catalog/namespace.c, every time
we loop over activeSearchPath and CurrentExtensionObject is set, then
we should skip any item that's not stored in pg_catalog, unless
there's a DEPENDENCY_EXTENSION pg_depend entry for the item (and that
pg_depend entry references the extension or the requires list).

There's quite a few loops over activeSearchPath in namespace.c, but
they all seem pretty similar. So while a bunch of code would need to
be changed, the changes could probably be well encapsulated in a
function.

[1]:
https://www.postgresql.org/message-id/flat/00d8f046156e355ec0eb49585408bafc8012e4a5.camel%40j-davis.com#3ad66667a8073d5ef50cfe44e305c38d

Jelte Fennema-Nio <me@jeltef.nl> writes:
> On Wed, 19 Jun 2024 at 17:28, Robert Haas <robertmhaas@gmail.com> wrote:
>> I have a feeling that this might be pretty annoying to implement, and
>> if that is true, then never mind.

> Based on a quick look it's not trivial, but also not super bad.
> Basically it seems like in src/backend/catalog/namespace.c, every time
> we loop over activeSearchPath and CurrentExtensionObject is set, then
> we should skip any item that's not stored in pg_catalog, unless
> there's a DEPENDENCY_EXTENSION pg_depend entry for the item (and that
> pg_depend entry references the extension or the requires list).

We could change the lookup rules that apply during execution of
an extension script, but we already restrict search_path at that
time so I'm not sure how much further this'd move the goalposts.

The *real* problem IMO is that if you create a PL function or
(old-style) SQL function within an extension, execution of that
function is not similarly protected.

            regards, tom lane

On Wed, 19 Jun 2024 at 19:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Jelte Fennema-Nio <me@jeltef.nl> writes:
> > On Wed, 19 Jun 2024 at 17:28, Robert Haas <robertmhaas@gmail.com> wrote:
> >> I have a feeling that this might be pretty annoying to implement, and
> >> if that is true, then never mind.
>
> > Based on a quick look it's not trivial, but also not super bad.
> > Basically it seems like in src/backend/catalog/namespace.c, every time
> > we loop over activeSearchPath and CurrentExtensionObject is set, then
> > we should skip any item that's not stored in pg_catalog, unless
> > there's a DEPENDENCY_EXTENSION pg_depend entry for the item (and that
> > pg_depend entry references the extension or the requires list).
>
> We could change the lookup rules that apply during execution of
> an extension script, but we already restrict search_path at that
> time so I'm not sure how much further this'd move the goalposts.

The point I tried to make in my first email is that this restricted
search_path you mention, is not very helpful at preventing privilege
escalations. Since it's often possible for a non-superuser to create
functions in one of the schemas in this search_path, e.g. by having
the non-superuser first create the schema & create some functions in
it, and then asking the DBA/control plane to create the extension in
that schema.

My patch tries to address that problem by creating the schema of the
extension during extension creation, and failing if it already exists.
Thus implicitly ensuring that a non-superuser cannot mess with the
schema.

The proposal from Robert tries to instead address by changing the
lookup rules during execution of an extension script to be more strict
than they would be outside of it (i.e. even if a function/operator
matches search_path it might still not be picked).

> The *real* problem IMO is that if you create a PL function or
> (old-style) SQL function within an extension, execution of that
> function is not similarly protected.

That's definitely a big problem too, and that's the problem that [1]
tries to fix. But first the lookup in extension scripts would need to
be made secure, because it doesn't seem very useful (security wise) to
use the same lookup mechanism in functions as we do in extension
scripts, if the lookup in extension scripts is not secure in the first
place. I think the method of making the lookup secure in my patch
would transfer over well, because it adds a way for a safe search_path
to exist, so all that's needed is for the PL function to use that
search_path. Robbert's proposal would be more difficult I think. When
executing a PL function from an extension we'd need to use the same
changed lookup rules that we'd use during the extension script of that
extension. I think that should be possible, but it's definitely more
involved.

[1]:
https://www.postgresql.org/message-id/flat/CAE9k0P%253DFcZ%253Dao3ZpEq29BveF%252B%253D27KBcRT2HFowJxoNCv02dHLA%2540mail.gmail.com

On Wed, 19 Jun 2024 at 17:22, David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Wed, Jun 19, 2024 at 8:19 AM Jelte Fennema-Nio <me@jeltef.nl> wrote:
>
>>
>> Because part of it would
>> only be relevant once we support upgrading from PG18. So for now the
>> upgrade_code I haven't actually run.
>
>
> Does it apply against v16?  If so, branch off there, apply it, then upgrade from the v16 branch to master.


I realized it's possible to do an "upgrade" with pg_upgrade from v17
to v17. So I was able to test both the pre and post PG18 upgrade logic
manually by changing the version in this line:

if (fout->remoteVersion >= 180000)

As expected the new pg_upgrade code was severely broken. Attached is a
new patch where the pg_upgrade code now actually works.

On Wed, Jun 19, 2024 at 1:50 PM Jelte Fennema-Nio <me@jeltef.nl> wrote:
> I do think, even if we have this, there would be other good reasons to
> use "owned schemas" for extension authors. At least the following two:
> 1. To have a safe search_path that can be used in SET search_path on a
> function (see also [1]).
> 2. To make it easy for extension authors to avoid conflicts with other
> extensions/UDFs.

(1) is a very good point. (2) I don't know about one way or the other.

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

On Jun 19, 2024, at 11:28, Robert Haas <robertmhaas@gmail.com> wrote:

> But I wonder if there might also be another possible approach: could
> we, somehow, prevent object references in extension scripts from
> resolving to anything other than the system catalogs and the contents
> of that extension? Perhaps with a control file setting to specify a
> list of trusted extensions which we're also allowed to reference?

It would also have to allow access to other extensions it depends upon.

D

On Jun 19, 2024, at 13:50, Jelte Fennema-Nio <me@jeltef.nl> wrote:

> This indeed does sound like the behaviour that pretty much every
> existing extension wants to have. One small addition/clarification
> that I would make to your definition: fully qualified references to
> other objects should still be allowed.

Would be tricky for referring to objects from other extensions  with no defined schema, or are relatable.

> 1. To have a safe search_path that can be used in SET search_path on a
> function (see also [1]).
> 2. To make it easy for extension authors to avoid conflicts with other
> extensions/UDFs.

These would indeed be nice improvements IMO.

Best,

David

Hi,

I've spent a bit of time looking at this patch. It seems there's a clear
consensus that having "owned schemas" for extensions would be good for
security. To me it also seems as a convenient way to organize stuff. It
was possible to create extensions in a separate schema before, ofc, but
that's up to the DBA. With this the extension author to enforce that.

One thing that's not quite clear to me is what's the correct way for
existing extensions to switch to an "owned schema". Let's say you have
an extension. How do you transition to this? Can you just add it to the
control file and then some magic happens?

A couple minor comments:


1) doc/src/sgml/extend.sgml

  An extension is <firstterm>owned_schema</firstterm> if it requires a
  new dedicated schema for its objects. Such a requirement can make
  security concerns related to <literal>search_path</literal> injection
  much easier to reason about. The default is <literal>false</literal>,
  i.e., the extension can be installed into an existing schema.

Doesn't "extension is owned_schema" sound a bit weird? I'd probably say
"extension may own a schema" or something like that.

Also, "requires a new dedicated schema" is a bit ambiguous. It's not
clear if it means the schema is expected to exist, or if it creates the
schema itself.

And perhaps it should clarify what "much easier to reason about" means.
That's pretty vague, and as a random extension author I wouldn't know
about the risks to consider. Maybe there's a section about this stuff
that we could reference?


2) doc/src/sgml/ref/create_extension.sgml

  relocated.  The named schema must already exist if the extension's
  control file does not specify <literal>owned_schema</literal>.

Seems a bit unclear, I'd say having "owned_schema = false" in the
control file still qualifies as "specifies owned_schema". So might be
better to say it needs to be set to true?

Also, perhaps "dedicated_schema" would be better than "owned_schema"? I
mean, the point is not that it's "owned" by the extension, but that
there's nothing else in it. But that's nitpicking.


3) src/backend/commands/extension.c

I'm not sure why AlterExtensionNamespace moves the privilege check. Why
should it not check the privilege for owned schema too?


4) src/bin/pg_dump/pg_dump.c

checkExtensionMembership has typo "owned_schem".

Shouldn't binary_upgrade_extension_member still set ext=NULL in the for
loop, the way the original code does that?

The long if conditions might need some indentation, I guess. pgindent
leaves them like this, but 100 columns seems a bit too much. I'd do a
line break after each condition, I guess.




regards

-- 
Tomas Vondra