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
From d82e9df826b29bf9030dc97551bdd0bef894677d Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 30 Jul 2018 09:29:32 +0900 Subject: [PATCH] Restrict access to reindex of shared catalogs for non-privileged users A database owner running a database-level REINDEX has the possibility to also do the operation on shared system catalogs without being an owner of them, which allows him to block resources it should not have access to. For example, PostgreSQL would go unresponsive and even block authentication if a lock is waited for pg_authid. This commit makes sure that a user running a REINDEX SYSTEM or DATABASE only works on the following relations: - The user is a superuser - The user is the table owner - The user is the database owner, only if the relation worked on is not shared. Reported-by: Lloyd Albin, Jeremy Schneider Author: Michael Paquier Reviewed by: Nathan Bossart, Kyotaro Horiguchi Discussion: https://postgr.es/m/152512087100.19803.12733865831237526...@wrigleys.postgresql.org --- doc/src/sgml/ref/reindex.sgml | 3 ++- src/backend/commands/indexcmds.c | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index a795dfa325..001111c024 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -214,7 +214,8 @@ REINDEX { INDEX | TABLE | DATABASE | SYSTEM } <replaceable class="PARAMETER">nam Reindexing a single index or table requires being the owner of that index or table. Reindexing a database requires being the owner of the database (note that the owner can therefore rebuild indexes of - tables owned by other users). Of course, superusers can always + tables owned by other users). Reindexing a shared catalog requires + being the owner of that shared catalog. Of course, superusers can always reindex anything. </para> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index fb51a79364..414c9cd66f 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1873,6 +1873,17 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user) continue; } + /* + * The table can be reindexed if the user is superuser, the table + * owner, or the database owner (but in the latter case, only if it's + * not a shared relation). pg_class_ownercheck includes the superuser + * case, and we already know that the user has permission to run + * REINDEX on this database. + */ + if (!pg_class_ownercheck(HeapTupleGetOid(tuple), GetUserId()) && + classtuple->relisshared) + continue; + if (HeapTupleGetOid(tuple) == RelationRelationId) continue; /* got it already */ -- 2.18.0
From 848c31a3fec109a55285e6c02e9122d394f787a3 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 30 Jul 2018 09:25:19 +0900 Subject: [PATCH] Restrict access to reindex of shared catalogs for non-privileged users A database owner running a database-level REINDEX has the possibility to also do the operation on shared system catalogs without being an owner of them, which allows him to block resources it should not have access to. For example, PostgreSQL would go unresponsive and even block authentication if a lock is waited for pg_authid. This commit makes sure that a user running a REINDEX SYSTEM or DATABASE only works on the following relations: - The user is a superuser - The user is the table owner - The user is the database owner, only if the relation worked on is not shared. Reported-by: Lloyd Albin, Jeremy Schneider Author: Michael Paquier Reviewed by: Nathan Bossart, Kyotaro Horiguchi Discussion: https://postgr.es/m/152512087100.19803.12733865831237526...@wrigleys.postgresql.org --- doc/src/sgml/ref/reindex.sgml | 3 ++- src/backend/commands/indexcmds.c | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index cabae191bc..82f5dc1317 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -214,7 +214,8 @@ REINDEX { INDEX | TABLE | DATABASE | SYSTEM } <replaceable class="PARAMETER">nam Reindexing a single index or table requires being the owner of that index or table. Reindexing a database requires being the owner of the database (note that the owner can therefore rebuild indexes of - tables owned by other users). Of course, superusers can always + tables owned by other users). Reindexing a shared catalog requires + being the owner of that shared catalog. Of course, superusers can always reindex anything. </para> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 9ee1fd0d51..97b38993d5 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -1869,6 +1869,17 @@ ReindexDatabase(const char *databaseName, bool do_system, bool do_user) continue; } + /* + * The table can be reindexed if the user is superuser, the table + * owner, or the database owner (but in the latter case, only if it's + * not a shared relation). pg_class_ownercheck includes the superuser + * case, and we already know that the user has permission to run + * REINDEX on this database. + */ + if (!pg_class_ownercheck(relid, GetUserId()) && + classtuple->relisshared) + continue; + if (HeapTupleGetOid(tuple) == RelationRelationId) continue; /* got it already */ -- 2.18.0
From ccabd709a8d34ef1f16dc018849c977bd9f5ab53 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 30 Jul 2018 09:13:37 +0900 Subject: [PATCH] Restrict access to reindex of shared catalogs for non-privileged users A database owner running a database-level REINDEX has the possibility to also do the operation on shared system catalogs without being an owner of them, which allows him to block resources it should not have access to. The same goes for a schema owner. For example, PostgreSQL would go unresponsive and even block authentication if a lock is waited for pg_authid. This commit makes sure that a user running a REINDEX SYSTEM, DATABASE or SCHEMA only works on the following relations: - The user is a superuser - The user is the table owner - The user is the database/schema owner, only if the relation worked on is not shared. Reported-by: Lloyd Albin, Jeremy Schneider Author: Michael Paquier Reviewed by: Nathan Bossart, Kyotaro Horiguchi Discussion: https://postgr.es/m/152512087100.19803.12733865831237526...@wrigleys.postgresql.org --- doc/src/sgml/ref/reindex.sgml | 10 +++++++--- src/backend/commands/indexcmds.c | 12 ++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml index 1c21fafb80..2ce103e788 100644 --- a/doc/src/sgml/ref/reindex.sgml +++ b/doc/src/sgml/ref/reindex.sgml @@ -225,9 +225,13 @@ REINDEX [ ( VERBOSE ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } <replacea <para> Reindexing a single index or table requires being the owner of that - index or table. Reindexing a database requires being the owner of - the database (note that the owner can therefore rebuild indexes of - tables owned by other users). Of course, superusers can always + index or table. <command>REINDEX DATABASE</command> and + <command>REINDEX SYSTEM</command> require being the owner of + the database (note that the owner can therefore rebuild indexes of tables + owned by other users). <command>REINDEX SCHEMA</command> requires + being the owner of the schema. Reindexing a shared catalog requires + being the owner of that shared catalog, even if the user is the owner + of the specified schema or database. Of course, superusers can always reindex anything. </para> diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index b9dad9672e..0cb4656aab 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -2415,6 +2415,18 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind, !IsSystemClass(relid, classtuple)) continue; + /* + * The table can be reindexed if the user is superuser, the table + * owner, or the database/schema owner (but in the latter case, only + * if it's not a shared relation). pg_class_ownercheck includes the + * superuser case, and depending on objectKind we already know that + * the user has permission to run REINDEX on this database or schema + * per the permission checks at the beginning of this routine. + */ + if (!pg_class_ownercheck(relid, GetUserId()) && + classtuple->relisshared) + continue; + /* Save the list of relation OIDs in private context */ old = MemoryContextSwitchTo(private_context); -- 2.18.0
signature.asc
Description: PGP signature
