Thread: 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
Attachment
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
Attachment
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
Attachment
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