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

On 7/29/18, 7:35 PM, "Michael Paquier" <michael(at)paquier(dot)xyz> wrote:
> 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.

Makes sense.

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

Okay, that seems reasonable to me, too.

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

For 9.3 and 9.4, it might be nice to add the "even if the user is the
owner of the specified database" part, too.

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

Looks good to me. Since REINDEX can be used to block calls to
load_critical_index() from new connections, back-patching seems appropriate.

Nathan