Thread: Bug in handling default privileges inside extension update scripts

Greetings,

* Mats Kindahl (mats@timescale.com) wrote:
> * To be able to read the configuration tables, "reader" need to have
>   SELECT privileges.
>
> * Since the new role is added by the user and not by the extension,
>   the grants have to be dumped as well. Otherwise, a restore of the
>   data will have wrong privileges.
>
> * Since new configuration tables could be added by an update of the
>   extension, it is necessary to make sure that these privileges are
>   added to new tables when updating. Typically, this means changing
>   the default privileges on the schema for the configuration files.

If the extension is updated, I think it's entirely reasonable to expect
an admin to have to go in and update the relevant permissions on any new
tables that have come into existance and, as I've said elsewhere, I
don't think that schema-level default privs should be applied to tables
created by extensions.  Sadly, no one else seems to have an opinion
regarding that and so there hasn't been a change in that, yet, but
that's the source of the issue imv.

If you want to comment on that, I'd suggest doing so on that thread:

https://www.postgresql.org/message-id/20200205034454.GU3195@tamriel.snowman.net

Thanks,

Stephen

Greetings,

* Mats Kindahl (mats@timescale.com) wrote:
> On Thu, Apr 22, 2021 at 5:15 PM Stephen Frost <sfrost@snowman.net> wrote:
> > * Mats Kindahl (mats@timescale.com) wrote:
> > > * To be able to read the configuration tables, "reader" need to have
> > >   SELECT privileges.
> > >
> > > * Since the new role is added by the user and not by the extension,
> > >   the grants have to be dumped as well. Otherwise, a restore of the
> > >   data will have wrong privileges.
> > >
> > > * Since new configuration tables could be added by an update of the
> > >   extension, it is necessary to make sure that these privileges are
> > >   added to new tables when updating. Typically, this means changing
> > >   the default privileges on the schema for the configuration files.
> >
> > If the extension is updated, I think it's entirely reasonable to expect
> > an admin to have to go in and update the relevant permissions on any new
> > tables that have come into existance and, as I've said elsewhere, I
> > don't think that schema-level default privs should be applied to tables
> > created by extensions.  Sadly, no one else seems to have an opinion
> > regarding that and so there hasn't been a change in that, yet, but
> > that's the source of the issue imv.
>
> That is a different way to solve it, but I think that is a little
> unintuitive. I am actually proposing to still assign default privileges,
> but not add them to initprivs, to make sure that they are treated the same
> way before and after an update.

Yes, I understood your suggestion, but I did, and still do, disagree
with that approach- how is an admin supposed to correctly guess what
permissions would be appropriate for new tables being added during an
upgrade of an extension?  Not to mention that extensions routinely get
added to existing schemas and I don't think it's at all obvious to
users that tables, functions, etc, added by an extension into a schema
should get the default privileges for that schema (and that could even
lead to security issues, I suspect...), not to mention that you have to
wonder if the privileges installed by the extension should be applied
*first*, and default privs after, or if the default privileges should
be first and the extension's privileges after.  As it's currently the
latter, it's rather complicated as the extension has no idea what to
expect the privileges on the object to be and so how can it sensibly set
privileges on it..?

More and more it looks clear to me that this is really just broken and
we need to stop applying default privs to objects created by extensions.

Thanks,

Stephen

Greetings,

* Mats Kindahl (mats@timescale.com) wrote:
> On Mon, Apr 26, 2021 at 7:29 PM Stephen Frost <sfrost@snowman.net> wrote:
> > * Mats Kindahl (mats@timescale.com) wrote:
> > > On Thu, Apr 22, 2021 at 5:15 PM Stephen Frost <sfrost@snowman.net>
> > wrote:
> > > > * Mats Kindahl (mats@timescale.com) wrote:
> > > > > * To be able to read the configuration tables, "reader" need to have
> > > > >   SELECT privileges.
> > > > >
> > > > > * Since the new role is added by the user and not by the extension,
> > > > >   the grants have to be dumped as well. Otherwise, a restore of the
> > > > >   data will have wrong privileges.
> > > > >
> > > > > * Since new configuration tables could be added by an update of the
> > > > >   extension, it is necessary to make sure that these privileges are
> > > > >   added to new tables when updating. Typically, this means changing
> > > > >   the default privileges on the schema for the configuration files.
> > > >
> > > > If the extension is updated, I think it's entirely reasonable to expect
> > > > an admin to have to go in and update the relevant permissions on any
> > new
> > > > tables that have come into existance and, as I've said elsewhere, I
> > > > don't think that schema-level default privs should be applied to tables
> > > > created by extensions.  Sadly, no one else seems to have an opinion
> > > > regarding that and so there hasn't been a change in that, yet, but
> > > > that's the source of the issue imv.
> > >
> > > That is a different way to solve it, but I think that is a little
> > > unintuitive. I am actually proposing to still assign default privileges,
> > > but not add them to initprivs, to make sure that they are treated the
> > same
> > > way before and after an update.
>
> Before delving into the discussion below, my main issue originally is that
> the default privileges are used to set the initprivs for objects created by
> the extension, causing them to not be dumped after an update of the
> extension. From the thread that you mentioned above, it seems like you
> agree that this should indeed not be the case for the reasons I outlined.
> Did I understand it correctly? If so, I can create a patch for that part at
> least.

If default privileges aren't set for objects created by an extension, as
I feel should be the case, then there's no concern around them being set
in initprivs or impacting the dump/restore case.  I wouldn't agree with
a patch that applied default privileges but then excluded them from
initprivs, as I said before.

> > Yes, I understood your suggestion, but I did, and still do, disagree
> > with that approach- how is an admin supposed to correctly guess what
> > permissions would be appropriate for new tables being added during an
> > upgrade of an extension?
>
> I am not really sure in what situation an admin will not be able to decide
> on the grants for extension objects (beyond what the extension decides are
> required).

Consider this:

An admin reviews v1 of extension ABC which installs some tables and
functions and decides that all users should be allowed to modify those
tables and so they set DEFAULT PRIVILEGES on the schema for everyone to
be able to modify the installed tables.

The ABC extension is then updated to add a new table in a v2 which
should *not* be able to be modified by any user, and the extension
author doesn't GRANT any privileges for that table since they know that
the *default* for a table is that only the owning role can modify the
table.  When that extension is updated, however, the DEFAULT PRIVILEGES
will end up GRANT'ing everyone access to that table anyway.  That
doesn't strike me as at all sane.

> The example I gave is because a non-superuser should be able to back up the
> tables and I think that the statements are quite clear in what they mean:
> assigning grants to tables directly is for the purpose of the currently
> existing objects, and default privileges are for future objects.

Allowing a non-superuser to back up a database is actually something
that's been added to v14 in the form of the pg_read_all_data role.

When it comes to default privileges, there's no shortage of extensions
which modify the privileges as part of the script that is run- but they
can only do that sanely when they know what privileges the object will
have initially.  If we have DEFAULT PRIVILEGES going on, there's no way
for the extension to know what privileges the object has during the
initial or the upgrade script.

> In the example given, the reason adding additional grants and/or default
> privileges is because the user needs to read the objects for backup
> purpose. It is quite clearly expressed using the statements in the example.
> To help me understand what problem you see, do you have an example where
> this is not as clear and where the water is muddier about what privileges
> to assign?

Any DEFAULT PRIVILEGES that get assigned behind the back of the
extension when the object is created means that the extension isn't able
to properly set the privileges on the objects that the extension has
created and that can lead to cases like the extension having a private
table that users are able to modify anyway, which is not a good thing.

> > Not to mention that extensions routinely get
> > added to existing schemas and I don't think it's at all obvious to
> > users that tables, functions, etc, added by an extension into a schema
> > should get the default privileges for that schema (and that could even
> > lead to security issues, I suspect...),
>
> Well... a user that wants to assign specific privileges to extension
> objects and keep them "under control" is probably going to use a dedicated
> schema for the extension(s) to separate user data from extension data (for
> example, in a hosted environment). Sure, users *can* use the public schema
> for all extensions (I think that is what you meant by "extensions routinely
> gets added to existing schemas"), but that will quickly be unmanageable for
> precisely the reason that it is hard to separate extension data from user
> data, so not sure if that is a good reason to not allow default privileges
> on extension objects. It feels like optimizing for the rare case.

It's not an optimization, it's a matter of doing what makes sense and is
not going to be a surprise to extension authors and users, and I don't
buy that having DEFAULT PRIVILEGES applied to objects created by
extensions is sensible to either of those groups.

> I am not sure how this could cause a security issue though. My reasoning is
> this: escalations of privileges can happen for objects created by the
> extension if you have default privileges on the schema, so this would mean
> that some role can get access to new objects created by the extension, but
> this was the purpose of assigning default privileges on the schema in the
> first place so it cannot be unexpected.

> Do you have some ideas or suspicions for how unexpected escalations could
> occur?

I've outlined at least one way this could cause problems above.  I don't
agree that having DEFAULT PRIVILEGES assigned to objects created by
extensions is expected.  Indeed, I'm fairly confident that it's entirely
*unexpected* by extension authors, including for core extensions like
adminpack, that DEFAULT PRIVILEGES might interfere with the privileges
that it's attempting to explicitly assign (such as REVOKE'ing EXECUTE on
sensitive functions from PUBLIC).

At least, when I wrote those REVOKEs into that exact extension, I sure
wasn't imagining that DEFAULT PRIVILEGES might be assigned to the schema
where those functions were being created and which would end up
GRANT'ing access to those very sensitive functions.  Maybe you can argue
that the user installing that extension into such a schema intended it,
but as an extension author, I surely didn't and that's enough of a point
of surprise that I'm fairly well convinced that we shouldn't have
objects being created by extensions have DEFAULT PRIVILEGES applied.

> not to mention that you have to
> > wonder if the privileges installed by the extension should be applied
> > *first*, and default privs after, or if the default privileges should
> > be first and the extension's privileges after.
>
> Forgive my ignorance, but I am not sure how that makes a difference. Could
> you elaborate?

The order in which privileges are applied makes a difference in terms of
what the resulting privileges on the object end up being..

> > As it's currently the
> > latter, it's rather complicated as the extension has no idea what to
> > expect the privileges on the object to be and so how can it sensibly set
> > privileges on it..?
>
> Again, forgive my ignorance, but isn't the expectation of the extension
> that it has superuser privileges to the database where it is being created?

First, no, extensions can be installed by non-superusers in the first
place, and second, I don't see how that has much to do with the question
at hand..

Thanks,

Stephen