Обсуждение: REINDEX and shared catalogs

Hi all,

This is a continuation of the thread "Canceling authentication due to
timeout aka Denial of Service Attack", which is here to focus on the
case of REINDEX:
https://www.postgresql.org/message-id/20180730003422.GA2878%40paquier.xyz

As visibly the set of patches I proposed on this thread is not
attracting the proper attention, I have preferred beginning a new thread
so as this can get a proper review and agreement.

Per the original thread, it is not difficult to block loading of
critical indexes, authentication being one, with a couple of SQLs:
TRUNCATE, VACUUM and REINDEX as reported.

VACUUM and TRUNCATE will have their own thread later depending on my
time available, and actually those refer to the problem where a relation
lock is attempted to be taken before checking if the user running the
command has the privileges to do so, and if the user has no privilege on
the relation, then the session would wait on a lock but fail later.
However REINDEX is different.

In the case of REINDEX, we *allow* shared catalogs to be reindexed.
Hence, if a user is a database owner, he would also be able to reindex
critical indexes on shared catalogs, where blocking authentication is
possible just with sessions connected to the database reindexed.  For a
schema, the situation is basically worse since 9.5 as a schema owner can
do the same with lighter permissions.  One can just run "SELECT * FROM
pg_stat_activity" in a transaction block in session 1, run REINDEX in
session 2, and cause the system to refuse new connections.  This is
documented as well.

Attached is a set of patches I proposed on the original thread, which
skips shared catalogs if the user running REINDEX is not an owner of
it.  This is a behavior change, and as I have a hard time believing that
anybody can take advantage of the current situation, I would like also
to see this stuff back-patched, as anybody doing shared hosting of
Postgres is most likely fixing the hole one way or another.  However, I
am sure as well that many folks here would not like that.

This thread is here to gather opinions and to help reaching a consensus,
as I would like to do something at least on HEAD for future releases.

reindex-priv-93.patch is for REL9_3_STABLE, reindex-priv-94.patch for
REL9_4_STABLE and reindex-priv-95-master.patch for 9.5~master.

Thanks for reading!
--
Michael

On 8/5/18, 4:12 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Attached is a set of patches I proposed on the original thread, which
> skips shared catalogs if the user running REINDEX is not an owner of
> it.  This is a behavior change, and as I have a hard time believing that
> anybody can take advantage of the current situation, I would like also
> to see this stuff back-patched, as anybody doing shared hosting of
> Postgres is most likely fixing the hole one way or another.  However, I
> am sure as well that many folks here would not like that.
>
> This thread is here to gather opinions and to help reaching a consensus,
> as I would like to do something at least on HEAD for future releases.

+1 for fixing this on master.  Upon reading the versioning policy,
which states that "minor releases fix only frequently-encountered,
security, and data corruption bugs..." [0], I gather that back-
patching such permissions changes might not be possible unless it is
treated as a security issue.  While I would love to see this fixed for
older versions, I don't anticipate much support for back-patching.
Kyotaro Horiguchi and Robert Haas have already voted against it in the
previous thread, anyway.

Nathan

[0] https://www.postgresql.org/support/versioning/

On Mon, Aug 6, 2018 at 2:40 AM, Michael Paquier <michael@paquier.xyz> wrote:
> In the case of REINDEX, we *allow* shared catalogs to be reindexed.
> Hence, if a user is a database owner, he would also be able to reindex
> critical indexes on shared catalogs, where blocking authentication is
> possible just with sessions connected to the database reindexed.  For a
> schema, the situation is basically worse since 9.5 as a schema owner can
> do the same with lighter permissions.  One can just run "SELECT * FROM
> pg_stat_activity" in a transaction block in session 1, run REINDEX in
> session 2, and cause the system to refuse new connections.  This is
> documented as well.

In my opinion, the behavior change is probably OK, but not back-patchable.

I think that the documentation could be phrased more clearly.  If I
understand the proposed semantics, something like this might be about
right:

   Reindexing a single index or table requires being the owner of that
   index or table.  Reindexing a schema or database requires being the
   owner of that schema or database.  Note that is therefore sometimes
   possible for non-superusers to rebuild indexes of tables owner by other
   users; however, as a special exception, when <command>REINDEX
   DATABASE</command> or <command>REINDEX SCHEMA</> is
   issued by a non-superuser, indexes on shared catalogs will be skipped
   unless the user owns the catalog (which typically won't be the case).
   Of course, superusers can always reindex anything.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Aug 08, 2018 at 12:25:03PM +0530, Robert Haas wrote:
> In my opinion, the behavior change is probably OK, but not
> back-patchable.

Thanks.  I see three votes in favor of not back-patching (you,
Horiguchi-san and Nathan), so that won't happen.

> I think that the documentation could be phrased more clearly.  If I
> understand the proposed semantics, something like this might be about
> right:
>
>    Reindexing a single index or table requires being the owner of that
>    index or table.  Reindexing a schema or database requires being the
>    owner of that schema or database.  Note that is therefore sometimes
>    possible for non-superusers to rebuild indexes of tables owner by other
>    users; however, as a special exception, when <command>REINDEX
>    DATABASE</command> or <command>REINDEX SCHEMA</> is
>    issued by a non-superuser, indexes on shared catalogs will be skipped
>    unless the user owns the catalog (which typically won't be the case).
>    Of course, superusers can always reindex anything.

I quite like what you are proposing here.  I'll reuse that, I hope you
don't mind ;)
--
Michael

On 2018-Aug-05, Michael Paquier wrote:

> Attached is a set of patches I proposed on the original thread, which
> skips shared catalogs if the user running REINDEX is not an owner of
> it.  This is a behavior change, and as I have a hard time believing that
> anybody can take advantage of the current situation, I would like also
> to see this stuff back-patched, as anybody doing shared hosting of
> Postgres is most likely fixing the hole one way or another.  However, I
> am sure as well that many folks here would not like that.

I agree that it would be good to have it fixed in released versions, but
I also agree that such a change could cause trouble in production for
some.  Is the "no backpatch" idea that you will push this to both pg11
and master?  That would get my vote.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Aug-05, Michael Paquier wrote:
>> Attached is a set of patches I proposed on the original thread, which
>> skips shared catalogs if the user running REINDEX is not an owner of
>> it.  This is a behavior change, and as I have a hard time believing that
>> anybody can take advantage of the current situation, I would like also
>> to see this stuff back-patched, as anybody doing shared hosting of
>> Postgres is most likely fixing the hole one way or another.  However, I
>> am sure as well that many folks here would not like that.

> I agree that it would be good to have it fixed in released versions, but
> I also agree that such a change could cause trouble in production for
> some.  Is the "no backpatch" idea that you will push this to both pg11
> and master?  That would get my vote.

Same here.  I am not excited about putting such a change into stable
branches, mainly because the existing behavior has been there for
twenty years without any previous complaints.  So it's not *that* big
a problem.  But it's not too late for v11, I think.

            regards, tom lane

On Wed, Aug 08, 2018 at 02:39:00PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> I agree that it would be good to have it fixed in released versions, but
>> I also agree that such a change could cause trouble in production for
>> some.  Is the "no backpatch" idea that you will push this to both pg11
>> and master?  That would get my vote.
>
> Same here.  I am not excited about putting such a change into stable
> branches, mainly because the existing behavior has been there for
> twenty years without any previous complaints.  So it's not *that* big
> a problem.  But it's not too late for v11, I think.

We are talking a lot about the definition of what a stable branch is,
aren't we? ;p

By no-backpatch, I only implied patching v12, but that would not be a
huge amount of efforts to get that into v11, so I can do that as well.
Any objections to doing that?
--
Michael

On 8/8/18, 2:16 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> By no-backpatch, I only implied patching v12, but that would not be a
> huge amount of efforts to get that into v11, so I can do that as well.
> Any objections to doing that?

+1 for back-patching to v11.

Nathan

On Wed, Aug 08, 2018 at 07:36:22PM +0000, Bossart, Nathan wrote:
> On 8/8/18, 2:16 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> By no-backpatch, I only implied patching v12, but that would not be a
>> huge amount of efforts to get that into v11, so I can do that as well.
>> Any objections to doing that?
>
> +1 for back-patching to v11.

OK, I have been able to grab some time and pushed this thing to HEAD and
REL_11_STABLE.  Thanks all!
--
Michael