Re: Extension security improvement: Add support for extensions with an owned schema
On Fri, 27 Sept 2024 at 14:00, Tomas Vondra wrote: > 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? Sadly no. As currently implemented this feature can only really be used by new extensions. There is no upgrade path to it for existing extensions. This is fairly common btw, the only field in the control file that ever gets used after the taken from the control file for ALTER EXTENSION is the version field. e.g. if you change the schema field in the control file of an already installed extension, the extension will not be moved to the new schema unless you DROP + CREATE EXTENSION. After this message I tried messing around a bit with changing an existing extension to become an owned schema (either with a new command or as part of ALTER EXTENSION UPDATE). But it's not as trivial as I hoped for a few reasons: 1. There are multiple states that extensions are currently in all of which need somewhat different conversion strategies. Specifically: relocatable/non-relocatable & schema=pg_catalog/public/actual_extension_schema. 2. There are two important assumptions on the schema for an owned_schema, which we would need to check on an existing schema converting it: a. The owner of the extension should be the owner of the schema b. There are no other objects in the schema appart from the extension. I'll probably continue some efforts to allow for migration, because I agree it's useful. But I don't think it's critical for this feature to be committable. Even if this can only be used by new extensions (that target PG18+ only), I think that would already be very useful. i.e. if in 5 years time I don't have to worry about these security pitfalls for any new extensions that I write then I'll be very happy. Also if an extension really wants to upgrade to an owned schema, then that should be possible by doing some manual catalog hackery in its extension update script, because that's effectively what any automatic conversion would also do. > A couple minor comments: > Doesn't "extension is owned_schema" sound a bit weird? Updated the docs to address all of your feedback I think. > 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. I agree that's probably a better name. Given the amount of effort necessary to update everything I haven't done that yet. I'll think a bit if there's a name I like even better in the meantime, and I'm definitely open to other suggestions. > 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? Because for an owned schema we're not creating objects in an existing schema. We're only renaming the schema that the extension is already in using RenameSchema, so there's no point in checking if the user can create objects in that schema, since the objects are already there. (RenameSchema also checks internally if the current user is the owner of the schema) > Shouldn't binary_upgrade_extension_member still set ext=NULL in the for > loop, the way the original code does that? I don't think that's necessary. It was necessary before to set extobj to NULL, because that variable was set every loop. But now extobj is scoped to the loop, and ext is only set right before the break. So either we set ext in the loop and break out of the loop right away, or ext is still set to the NULL value that's was assigned at the top of the function. > 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. Done v4-0001-Add-support-for-extensions-with-an-owned-schema.patch Description: Binary data
Re: Extension security improvement: Add support for extensions with an owned schema
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 owned_schema if it requires a new dedicated schema for its objects. Such a requirement can make security concerns related to search_path injection much easier to reason about. The default is false, 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 owned_schema. 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
Re: Extension security improvement: Add support for extensions with an owned schema
On Jun 19, 2024, at 13:50, Jelte Fennema-Nio 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
Re: Extension security improvement: Add support for extensions with an owned schema
On Jun 19, 2024, at 11:28, Robert Haas 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
Re: Extension security improvement: Add support for extensions with an owned schema
On Wed, Jun 19, 2024 at 1:50 PM Jelte Fennema-Nio 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
Re: Extension security improvement: Add support for extensions with an owned schema
On Wed, 19 Jun 2024 at 17:22, David G. Johnston wrote: > > On Wed, Jun 19, 2024 at 8:19 AM Jelte Fennema-Nio 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 >= 18) As expected the new pg_upgrade code was severely broken. Attached is a new patch where the pg_upgrade code now actually works. v3-0001-Add-support-for-extensions-with-an-owned-schema.patch Description: Binary data
Re: Extension security improvement: Add support for extensions with an owned schema
On Wed, 19 Jun 2024 at 19:55, Tom Lane wrote: > > Jelte Fennema-Nio writes: > > On Wed, 19 Jun 2024 at 17:28, Robert Haas 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
Re: Extension security improvement: Add support for extensions with an owned schema
Jelte Fennema-Nio writes: > On Wed, 19 Jun 2024 at 17:28, Robert Haas 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
Re: Extension security improvement: Add support for extensions with an owned schema
On Wed, 19 Jun 2024 at 17:28, Robert Haas 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#3ad7a8073d5ef50cfe44e305c38d
Re: Extension security improvement: Add support for extensions with an owned schema
On Sat, Jun 1, 2024 at 8:08 PM Jelte Fennema-Nio 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
Re: Extension security improvement: Add support for extensions with an owned schema
On Wed, Jun 19, 2024 at 8:19 AM Jelte Fennema-Nio 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. David J.
Re: Extension security improvement: Add support for extensions with an owned schema
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. From 37b6fa45bf877bcc15ce76d7e342199b7ca76d50 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Fri, 31 May 2024 02:04:31 -0700 Subject: [PATCH v2] 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. --- doc/src/sgml/extend.sgml | 13 ++ doc/src/sgml/ref/create_extension.sgml| 3 +- src/backend/commands/extension.c | 141 +- src/backend/utils/adt/pg_upgrade_support.c| 20 ++- src/bin/pg_dump/pg_dump.c | 15 +- src/bin/pg_dump/pg_dump.h | 1 + src/include/catalog/pg_extension.h| 1 + src/include/catalog/pg_proc.dat | 2 +- src/include/commands/extension.h | 4 +- src/test/modules/test_extensions/Makefile | 7 +- .../expected/test_extensions.out | 50 +++ src/test/modules/test_extensions/meson.build | 4 + .../test_extensions/sql/test_extensions.sql | 27 .../test_ext_owned_schema--1.0.sql| 2 + .../test_ext_owned_schema.control | 5 + ...test_ext_owned_schema_relocatable--1.0.sql | 2 + .../test_ext_owned_schema_relocatable.control | 4 + 17 files changed, 248 insertions(+), 53 deletions(-) create mode 100644 src/test/modules/test_extensions/test_ext_owned_schema--1.0.sql create mode 100644 src/test/modules/test_extensions/test_ext_owned_schema.control create mode 100644 src/test/modules/test_extensions/test_ext_owned_schema_relocatable--1.0.sql create mode 100644 src/test/modules/test_extensions/test_ext_owned_schema_relocatable.control diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml index 218940ee5ce..36dc692abef 100644 --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -809,6 +809,19 @@ RETURNS anycompatible AS ... + + owned_schema (boolean) + + +An extension is owned_schema if it requires a +new dedicated schema for its objects. Such a requirement can make +security concerns related to search_path injection +much easier to reason about. The default is false, +i.e., the extension can be installed into an existing schema. + + + + schema (string) diff --git a/doc/src/sgml/ref/create_extension.sgml b/doc/src/sgml/ref/create_extension.sgml index ca2b80d669c..6e767c7bfca 100644 --- a/doc/src/sgml/ref/create_extension.sgml +++ b/doc/src/sgml/ref/create_extension.sgml @@ -102,7 +102,8 @@ CREATE EXTENSION [ IF NOT EXISTS ] extension_name The name of the schema in which to install the extension's objects, given that the extension allows its contents to be -relocated. The named schema must already exist. +relocated. The named schema must already exist if the extension's +control file does not specify owned_schema. If not specified, and the extension's control file does not specify a schema either, the current default object creation schema is used. diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index 1643c8c69a0..c9586ad62a1 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -83,6 +83,8 @@ typedef struct ExtensionControlFile * MODULE_PATHNAME */ char *comment; /* comment, if any */ cha
Re: Extension security improvement: Add support for extensions with an owned schema
On Wed, 5 Jun 2024 at 19:53, Jeff Davis 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.
Re: Extension security improvement: Add support for extensions with an owned schema
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
Re: Extension security improvement: Add support for extensions with an owned schema
On Sun, Jun 2, 2024, 02:08 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. 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
