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

At Mon, 30 Jul 2018 09:34:22 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180730003422(dot)GA2878(at)paquier(dot)xyz>
> 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:
> > 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.

There's a case where the database owner differs from catalogs'
owner. ALTER DATABASE OWNER TO can make such configuration.
In this case, the previous code allows REINDEX of a shared
catalog for the database owner who is not the catalog's owner.

Superuser : alice
Database owner : joe
Catalog owner : andy
Schema owner : joe

"REINDEX SCHERMA <the schema>" by joe allows shared catalog to be
reindexed, even he is *not* the owner of the catalog.

The revised patch behaves differently to this case. "REINDEX
SCHERMA <the schema>" by joe *skips* shared catalog since he is
not the owner of the catalog.

# Uh? Alice doesn't involved anywhere..

I feel that just being a database owner doesn't justify to cause
this problem innocently. Catalog owner is also doubious but we
can carefully configure the ownerships to avoid the problem since
only superuser can change it. So I vote +1 for the revised patch.

> > 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.

Even if it is written in the "Notes" section, doesn't the
following need some fix? It is the same for the DATBASE item.

| Parameters
...
| SYSTEM
| Recreate all indexes on system catalogs within the current
| database. *Indexes on shared system catalogs are included*.
| Indexes on user tables are not processed. This form
| of REINDEX cannot be executed inside a transaction block.

> > 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?

This apparently changes the documented behavior and the problem
seems to be a result of a rather malicious/intentional
combination of operatoins (especially named vacuum on a shared
catalog). I vote -0.5 to backpatch unless we categorize this as a
security issue.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center