Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack

On Sun, Jul 29, 2018 at 04:11:38PM +0000, Bossart, Nathan wrote:
> On 7/27/18, 7:10 PM, "Michael Paquier" <michael(at)paquier(dot)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.
>
> 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:
>
> 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