Date: Mon, 30 Jul 2018 09:34:22 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Bossart, Nathan" <bossartn@amazon.com>
Cc: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>,
	"andres@anarazel.de" <andres@anarazel.de>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	"robertmhaas@gmail.com" <robertmhaas@gmail.com>,
	"Schneider, Jeremy" <schnjere@amazon.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
	"lalbin@scharp.org" <lalbin@scharp.org>
Subject: Re: BUG #15182: Canceling authentication due to timeout aka Denial
 of Service Attack
Message-ID: <20180730003422.GA2878@paquier.xyz>
References: <20180726204127.GA1754@paquier.xyz>
 <3D4F2733-92AE-4B74-8912-3C611B027721@amazon.com>
 <20180727023123.GE1754@paquier.xyz>
 <20180727.123702.77822416.horiguchi.kyotaro@lab.ntt.co.jp>
 <20180727041542.GH1754@paquier.xyz>
 <62728D88-85A0-4A9A-A4F8-67D96C1179E0@amazon.com>
 <20180728001012.GC1882@paquier.xyz>
 <6D3CBDB6-C358-49F0-8D00-32E58AD2D3D3@amazon.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="XF85m9dhOBO43t/C"
Content-Disposition: inline
In-Reply-To: <6D3CBDB6-C358-49F0-8D00-32E58AD2D3D3@amazon.com>
Precedence: bulk


--XF85m9dhOBO43t/C
Content-Type: multipart/mixed; boundary="CE+1k2dSO48ffgeK"
Content-Disposition: inline


--CE+1k2dSO48ffgeK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Jul 29, 2018 at 04:11:38PM +0000, Bossart, Nathan wrote:
> On 7/27/18, 7:10 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
> > No problem.  If there are no objections, I am going to fix the REINDEX
> > issue first and back-patch.  Its patch is the least invasive of the
> > set.
>=20
> This seems like a reasonable place to start.  I took a closer look at
> 0003.

Thanks for looking at it!

> This is added to ReindexMultipleTables(), which is used for REINDEX
> SCHEMA, REINDEX SYSTEM, and REINDEX DATABASE.  Presently, REINDEX
> SCHEMA requires being the owner of the schema, and REINDEX SYSTEM and
> REINDEX DATABASE require being the owner of the database.  So, if a
> role is an owner of a database or the pg_catalog schema, they are able
> to reindex shared catalogs like pg_authid.

Yeah, I was testing that yesterday night and bumped on this case when
trying do a REINDEX SCHEMA pg_class.  The point is that you can simplify
the check and remove pg_database_ownercheck as there is already an ACL
check on the database/system/schema at the top of the routine, so you
are already sure that pg_database_ownercheck() or
pg_namespace_ownercheck would return true.  This shaves a few cycles as
well for each relation checked.

> I also noticed that this patch causes shared relations to be skipped
> silently.  Perhaps we should emit a WARNING or DEBUG message when this
> happens, at least for REINDEXOPT_VERBOSE.

That's intentional.  I thought about that as well, but I am hesitant to
do so as we don't bother mentioning the other relations skipped.
REINDEX VERBOSE also shows up what are the tables processed, so it is
easy to guess what are the tables skipped, still more painful.  And the
documentation changes added cover the gap.

> I noticed that there is no mention that the owner of a schema can do
> REINDEX SCHEMA, which seems important to note.  Also, the proposed
> wording might seem slightly ambiguous for the REINDEX DATABASE case.
> It might be clearer to say something like the following:
>=20
>         Reindexing a single index or table requires being the owner of
>         that index of table.  REINDEX DATABASE and REINDEX SYSTEM
>         require being the owner of the database, and REINDEX SCHEMA
>         requires being the owner of the schema (note that the user can
>         therefore rebuild indexes of tables owned by other users).
>         Reindexing a shared catalog requires being the owner of the
>         shared catalog, even if the user is the owner of the specified
>         database or schema.  Of course, superusers can always reindex
>         anything.

+1, I have included your suggestions.  The patch attached applies easily
down to 9.5 where REINDEX SCHEMA was added.  For 9.4 and 9.3, there is
no schema case, still the new check is similar.  The commit message is
slightly changed so as there is no mention of REINDEX SCHEMA.  9.3 needs
a slight change compared to 9.4 as well.

So attached are patches for 9.5~master, 9.4 and 9.3 with commit
messages.  Does that look fine to folks of this thread?
--
Michael

--CE+1k2dSO48ffgeK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-priv-93.patch"
Content-Transfer-Encoding: quoted-printable

=46rom d82e9df826b29bf9030dc97551bdd0bef894677d Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@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.12733865831237526317@wri=
gleys.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 } <replacea=
ble class=3D"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 alwa=
ys
    reindex anything.
   </para>
=20
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.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_sy=
stem, bool do_user)
 				continue;
 		}
=20
+		/*
+		 * 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) =3D=3D RelationRelationId)
 			continue;			/* got it already */
=20
--=20
2.18.0


--CE+1k2dSO48ffgeK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-priv-94.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 848c31a3fec109a55285e6c02e9122d394f787a3 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@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.12733865831237526317@wri=
gleys.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 } <replacea=
ble class=3D"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 alwa=
ys
    reindex anything.
   </para>
=20
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.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_sy=
stem, bool do_user)
 				continue;
 		}
=20
+		/*
+		 * 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) =3D=3D RelationRelationId)
 			continue;			/* got it already */
=20
--=20
2.18.0


--CE+1k2dSO48ffgeK
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="reindex-priv-95-master.patch"
Content-Transfer-Encoding: quoted-printable

=46rom ccabd709a8d34ef1f16dc018849c977bd9f5ab53 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@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.12733865831237526317@wri=
gleys.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 | DAT=
ABASE | SYSTEM } <replacea
=20
   <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 tabl=
es
+   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>
=20
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.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, Reinde=
xObjectType objectKind,
 			!IsSystemClass(relid, classtuple))
 			continue;
=20
+		/*
+		 * 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 =3D MemoryContextSwitchTo(private_context);
=20
--=20
2.18.0


--CE+1k2dSO48ffgeK--

--XF85m9dhOBO43t/C
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlteXQ4ACgkQnvQgOdby
QH1m8Q//YhGLB8x0E6hNCLt/Gvmf3a/DKrJXF+V/ROBjaaTE4wu29mjxqK53p3qG
+4rAura3yu0HPPHyqJ3ZnoCH1ECUshiRCz26VAPpPKj//H1xwXJwPM3Jsc73oQi5
IUcnhGYXpcbk385ZBX+eR1jA9sfclF3gGDCovLXUdx9Ql9iioUV/WC74XP6lR9Q5
0sScimG5+dq3deC/EnAKC/jzBFd11+9ar2UHTBrWKAZ3807UVwpB4duI5bR/Vux5
tg8aU5V1oay/WJbWnsKlCdagTyZexN9uCrEh6dVGcuv0lx0rQ4kS9E9eIRw+iRx1
MISwQUmPkUtyj8s2X7YRaLh1GKeC2C2u4aovv2jE+JYOP0JHVCbrMzcoiP31S22C
GRpOhVzikJmVxM5O1M334rUNUCp7NsuyRQquEViIIZ0hMF2UTLkxZ099cFkwcMeC
4itbz2LLkG09z7tD3BhIRCSRIPtIpsg997DZHdfCiSqo97lVs+lEEnDfxYR/Bglt
Wqa94W/IS9aA11KbPjSk2v1NiCpPYs9Ujc8sV5pNMaZj74fH0QLXEXK90B8JnVJM
dHv2vjRjqY+OrE1f/k2VzEKO+wRG8WgWf+JbRE4KY4R40/HXsRLTGNwOAU8t1mPH
aswo9i9/XL7hKaxB3/tcpUqRyyL3kzWlcNaSYVgcp7Soty/04E0=
=G3/7
-----END PGP SIGNATURE-----

--XF85m9dhOBO43t/C--