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

I'd like to bump this old bug that Lloyd filed for more discussion. It
seems serious enough to me that we should at least talk about it.

Anyone with simply the login privilege and the ability to run SQL can
instantly block all new incoming connections to a DB including new
superuser connections.

session 1:
select pg_sleep(9999999999) from pg_stat_activity;

session 2:
vacuum full pg_authid; -or- truncate table pg_authid;

(there are likely other SQL you could run in session 2 as well.)

-------- Forwarded Message --------
Subject: BUG #15182: Canceling authentication due to timeout aka Denial
of Service Attack
Date: Mon, 30 Apr 2018 20:41:11 +0000
From: PG Bug reporting form <noreply@postgresql.org>
Reply-To: lalbin@scharp.org, pgsql-bugs@lists.postgresql.org
To: pgsql-bugs@lists.postgresql.org
CC: lalbin@scharp.org

The following bug has been logged on the website:

Bug reference: 15182
Logged by: Lloyd Albin
Email address: lalbin@scharp.org
PostgreSQL version: 10.3
Operating system: OpenSUSE
Description:
Over the last several weeks our developers caused a Denial of Service Attack
against ourselves by accident. When looking at the log files, I noticed that
we had authentication timeouts during these time periods. In researching the
problem I found this is due to locks being held on shared system catalog
items, aka system catalog items that are shared between all databases on the
same cluster/server. This can be caused by beginning a long running
transaction that queries pg_stat_activity, pg_roles, pg_database, etc and
then another connection that runs either a REINDEX DATABASE, REINDEX SYSTEM,
or VACUUM FULL. This issue is of particular importance to database resellers
who use the same cluster/server for multiple clients, as two clients can
cause this issue to happen inadvertently or a single client can either cause
it to happen maliciously or inadvertently. Note: The large cloud providers
give each of their clients their own cluster/server so this will not affect
across cloud clients but can affect an individual client. The problem is
that traditional hosting companies will have all clients from one or more
web servers share the same PostgreSQL cluster/server. This means that one or
two clients could inadvertently stop all the other clients from being able
to connect to their databases until the first client does either a COMMIT or
ROLLBACK of their transaction which they could hold open for hours, which is
what happened to us internally.

In Connection 1 we need to BEGIN a transaction and then query a shared
system item; pg_authid, pg_database, etc; or a view that depends on a shared
system item; pg_stat_activity, pg_roles, etc. Our developers were accessing
pg_roles.

Connection 1 (Any database, Any User)
BEGIN;
SELECT * FROM pg_stat_activity;

Connection 2 (Any database will do as long as you are the database owner)
REINDEX DATABASE postgres;

Connection 3 (Any Database, Any User)
psql -h sqltest-alt -d sandbox

All future Connection 3's will hang for however long the transaction in
Connection 1 runs. In our case this was hours and denied everybody else the
ability to log into the server until Connection 1 was committed. psql will
just hang for hours, even overnight in my testing, but our apps would get
the "Canceling authentication due to timeout" after 1 minute.

Connection 2 can also do any of these commands to also cause the same
issue:
REINDEX SYSTEM postgres;
VACUUM FULL pg_authid;
vacuumdb -f -h sqltest-alt -d lloyd -U lalbin

Even worse is that the VACUUM FULL pg_authid; can be started by an
unprivileged user and it will wait for the AccessShareLock by connection 1
to be released before returning the error that you don't have permission to
perform this action, so even an unprivileged user can cause this to happen.
The privilege check needs to happen before the waiting for the
AccessExclusiveLock happens.

This bug report has been simplified and shorted drastically. To read the
full information about this issue please see my blog post:
http://lloyd.thealbins.com/Canceling%20authentication%20due%20to%20timeout

Lloyd Albin
Database Administrator
Statistical Center for HIV/AIDS Research and Prevention (SCHARP)
Fred Hutchinson Cancer Research Center

--
Jeremy Schneider
Database Engineer
Amazon Web Services

On Fri, Jul 20, 2018 at 2:17 AM, Jeremy Schneider <schnjere@amazon.com>
wrote:

I'd like to bump this old bug that Lloyd filed for more discussion. It
seems serious enough to me that we should at least talk about it.

Anyone with simply the login privilege and the ability to run SQL can
instantly block all new incoming connections to a DB including new
superuser connections.

So.. don't VACUUM FULL pg_authid without lock_timeout?

I can come up with dozens of ways to achieve the same effect, all of them
silly.

.m

On Thu, Jul 19, 2018 at 7:17 PM, Jeremy Schneider <schnjere@amazon.com> wrote:

I'd like to bump this old bug that Lloyd filed for more discussion. It
seems serious enough to me that we should at least talk about it.

Anyone with simply the login privilege and the ability to run SQL can
instantly block all new incoming connections to a DB including new
superuser connections.

session 1:
select pg_sleep(9999999999) from pg_stat_activity;

session 2:
vacuum full pg_authid; -or- truncate table pg_authid;

(there are likely other SQL you could run in session 2 as well.)

ExecuteTruncate needs to be refactored to use RangeVarGetRelidExtended
with a non-NULL callback rather than heap_openrv, and
expand_vacuum_rel needs to use RangeVarGetRelidExtended with a
callback instead of RangeVarGetRelid. See
cbe24a6dd8fb224b9585f25b882d5ffdb55a0ba5 as an example of what to do.
I fixed a large number of cases of this problem back around that time,
but then ran out of steam and had to move onto other things before I
got them all. Patches welcome.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jul 20, 2018 at 5:56 PM, Marko Tiikkaja <marko@joh.to> wrote:

On Fri, Jul 20, 2018 at 2:17 AM, Jeremy Schneider <schnjere@amazon.com>
wrote:

I'd like to bump this old bug that Lloyd filed for more discussion. It
seems serious enough to me that we should at least talk about it.

Anyone with simply the login privilege and the ability to run SQL can
instantly block all new incoming connections to a DB including new
superuser connections.

So.. don't VACUUM FULL pg_authid without lock_timeout?

That's like saying the solution to a security hole is for no one to attempt
to exploit it.

Note that you do not need to have permissions to do the vacuum full. This
works merely from the attempt to do so, before the permissions are checked.

Cheers,

Jeff

On Mon, Jul 23, 2018 at 11:29:33AM -0400, Robert Haas wrote:

ExecuteTruncate needs to be refactored to use RangeVarGetRelidExtended
with a non-NULL callback rather than heap_openrv, and
expand_vacuum_rel needs to use RangeVarGetRelidExtended with a
callback instead of RangeVarGetRelid. See
cbe24a6dd8fb224b9585f25b882d5ffdb55a0ba5 as an example of what to do.
I fixed a large number of cases of this problem back around that time,
but then ran out of steam and had to move onto other things before I
got them all. Patches welcome.

Thanks for pointing those out, I looked at both code paths recently for
some other work... The amount of work does not consist just in using
for example RangeVarCallbackOwnsRelation for VACUUM and TRUNCATE. There
are a couple of reasons behind that:
- While it would make sense, at least to me, to make VACUUM fall into if
allow_system_table_mods is allowed, that's not the case of ANALYZE as I
think that we should be able to call ANALYZE on a system catalog as
well. So we would basically a new flavor of
RangeVarCallbackOwnsRelation for VACUUM which makes this difference
between vacuum and analyze with an argument in the callback, the options
of VacuumStmt would be nice. This would not be used by autovacuum
anyway, but adding an assertion and mentioning that in the comments
would not hurt. There is an argument for just restricting VACUUM FULL
as well and not plain VACUUM, as that's the one hurting here.
- TRUNCATE is closer to a solution, as it has its own flavor of relation
checks with truncate_check_rel. So the callback would replace
truncate_check_rel but CheckTableNotInUse should be moved out of it.
TRUNCATE already uses allow_system_table_mods for its checks.

Thoughts?
--
Michael

On July 23, 2018 9:14:03 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:

- While it would make sense, at least to me, to make VACUUM fall into
if
allow_system_table_mods is allowed,

I might be mis-parsing this due to typos. Are you actually suggesting vacuum on system tables should depend on that GUC? If so, why? That's seems like a terrible idea. It's pretty normal to occasionally have to vacuum them?

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Mon, Jul 23, 2018 at 09:17:53PM -0700, Andres Freund wrote:

I might be mis-parsing this due to typos. Are you actually suggesting
vacuum on system tables should depend on that GUC? If so, why? That's
seems like a terrible idea. It's pretty normal to occasionally have
to vacuum them?

Oh, yes, that would be bad. My mind has slipped here. I have seen
manual VACUUMs on system catalogs for applications using many temp
tables... So we would want to have only VACUUM FULL being conditionally
happening? The question comes then about what to do when a VACUUM FULL
is run without a list of relations because expand_vacuum_rel() is not
actually the only problem. Would we want to ignore system tables as
well except if allow_system_table_mods is on? When no relation list is
specified, get_all_vacuum_rels() builds the list of relations which
causes vacuum_rel() to complain on try_relation_open(), so patching
just expand_vacuum_rel() solves only half of the problem for manual
VACUUMs.
--
Michael

On July 23, 2018 9:50:10 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Jul 23, 2018 at 09:17:53PM -0700, Andres Freund wrote:

I might be mis-parsing this due to typos. Are you actually suggesting
vacuum on system tables should depend on that GUC? If so, why? That's
seems like a terrible idea. It's pretty normal to occasionally have
to vacuum them?

Oh, yes, that would be bad. My mind has slipped here. I have seen
manual VACUUMs on system catalogs for applications using many temp
tables... So we would want to have only VACUUM FULL being
conditionally
happening? The question comes then about what to do when a VACUUM FULL
is run without a list of relations because expand_vacuum_rel() is not
actually the only problem. Would we want to ignore system tables as
well except if allow_system_table_mods is on? When no relation list is
specified, get_all_vacuum_rels() builds the list of relations which
causes vacuum_rel() to complain on try_relation_open(), so patching
just expand_vacuum_rel() solves only half of the problem for manual
VACUUMs.

I think any such restriction is entirely unacceptable. FULL or not.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Mon, Jul 23, 2018 at 09:51:54PM -0700, Andres Freund wrote:

On July 23, 2018 9:50:10 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:

Oh, yes, that would be bad. My mind has slipped here. I have seen
manual VACUUMs on system catalogs for applications using many temp
tables... So we would want to have only VACUUM FULL being
conditionally
happening? The question comes then about what to do when a VACUUM FULL
is run without a list of relations because expand_vacuum_rel() is not
actually the only problem. Would we want to ignore system tables as
well except if allow_system_table_mods is on? When no relation list is
specified, get_all_vacuum_rels() builds the list of relations which
causes vacuum_rel() to complain on try_relation_open(), so patching
just expand_vacuum_rel() solves only half of the problem for manual
VACUUMs.

I think any such restriction is entirely unacceptable. FULL or not.

Well, letting any users take an exclusive lock on system catalogs at
will is not acceptable either, so two possible answers would be to fail
or skip such relations. The first concept applies if a relation list is
given by the user, and the second if no list is given.

Do you have any thoughts on the matter?
--
Michael

On Tue, Jul 24, 2018 at 02:23:02PM +0900, Michael Paquier wrote:

Well, letting any users take an exclusive lock on system catalogs at
will is not acceptable either, so two possible answers would be to fail
or skip such relations. The first concept applies if a relation list is
given by the user, and the second if no list is given.

The first sentence is incorrect. That's actually "letting any users
attempt to take an exclusive lock which makes others to be stuck as
well".
--
Michael

On 2018-Jul-24, Michael Paquier wrote:

On Mon, Jul 23, 2018 at 11:29:33AM -0400, Robert Haas wrote:

ExecuteTruncate needs to be refactored to use RangeVarGetRelidExtended
with a non-NULL callback rather than heap_openrv, and
expand_vacuum_rel needs to use RangeVarGetRelidExtended with a
callback instead of RangeVarGetRelid. See
cbe24a6dd8fb224b9585f25b882d5ffdb55a0ba5 as an example of what to do.
I fixed a large number of cases of this problem back around that time,
but then ran out of steam and had to move onto other things before I
got them all. Patches welcome.

Thanks for pointing those out, I looked at both code paths recently for
some other work... The amount of work does not consist just in using
for example RangeVarCallbackOwnsRelation for VACUUM and TRUNCATE.

I don't think we're forced to reuse the existing callbacks -- maybe
write a specific callback for each case, if really needed. But anyway
like Andres I don't think this is related to allow_system_table_mods at
all; you just need to do the checks in the right order, no?

But I don't see why RangeVarCallbackOwnsTable isn't sufficient.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jul 24, 2018 at 01:34:13AM -0400, Alvaro Herrera wrote:

But I don't see why RangeVarCallbackOwnsTable isn't sufficient.

The set of relkinds checked by truncate_check_rel and
RangeVarCallbackOwnsTable is different (toast and matviews). And in the
case of VACUUM, partitioned tables can call RangeVarGetRelidExtended
when one is listed as part of a manual VACUUM command.
--
Michael

At Tue, 24 Jul 2018 14:23:02 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20180724052302.GB4736@paquier.xyz>

On Mon, Jul 23, 2018 at 09:51:54PM -0700, Andres Freund wrote:

On July 23, 2018 9:50:10 PM PDT, Michael Paquier <michael@paquier.xyz> wrote:

Oh, yes, that would be bad. My mind has slipped here. I have seen
manual VACUUMs on system catalogs for applications using many temp
tables... So we would want to have only VACUUM FULL being
conditionally
happening? The question comes then about what to do when a VACUUM FULL
is run without a list of relations because expand_vacuum_rel() is not
actually the only problem. Would we want to ignore system tables as
well except if allow_system_table_mods is on? When no relation list is
specified, get_all_vacuum_rels() builds the list of relations which
causes vacuum_rel() to complain on try_relation_open(), so patching
just expand_vacuum_rel() solves only half of the problem for manual
VACUUMs.

I think any such restriction is entirely unacceptable. FULL or not.

Well, letting any users attempt to take an exclusive lock which
makes others to be stuck as well is not acceptable either, so
two possible answers would be to fail
or skip such relations. The first concept applies if a relation list is
given by the user, and the second if no list is given.

Do you have any thoughts on the matter?

I'm not sure what is the exact problem here. If it is that a
non-privilege user can stuck on VACUUM FULL on the scenario, we
should just give up VACUUM_FULLing on unprivileged relations
*before* trying to take a lock on it. There's no reason for
allowing VACUUM FULL on a relation on that the same user is not
allowed to run VACUUM. I don't think it's a prbolem that
superuser can be involed in the blocking scenario running VACUUM
FULL.

I may be misunderstanding something because (really ?) it's still
extremely hot in Japan, today.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jul 24, 2018 at 06:27:16PM +0900, Kyotaro HORIGUCHI wrote:

I may be misunderstanding something because (really ?) it's still
extremely hot in Japan, today.

It is better since yesterday, in exchange of a typhoon heading straight
to Tokyo :)
--
Michael

Hi,

So, I have spent the last couple of days trying to figure out a nice
solution for VACUUM, TRUNCATE and REINDEX, and attached is a set of
three patches to address the issues of this thread:
1) 0001 does the work for TRUNCATE, but using RangeVarGetRelidExtended
with a custom callback based on the existing truncate_check_rel(). I
had to split the session-level checks into a separate routine as no
Relation is available, but that finishes by being a neat result without
changing any user-facing behavior.
2) 0002 does the work for VACUUM. It happens that vacuum_rel() already
does all the skip-related checks we need to know about to decide if a
relation needs to be vacuum'ed or not, so I refactored things as
follows:
2-1) For a VACUUM manual listing relations, issue an ERROR if it cannot
be vacuum'ed. Previously vacuum_rel() would just log a WARNING and call
it a day *after* locking the relation. But as we need to rely on
RangeVarGetRelidExtended() an ERROR is necessary. The ERROR happens
only if VACUUM FULL is used.
2-2) When a relation list is not specified in a manual VACUUM command,
then the decision to skip the relation is done in get_all_vacuum_rels()
when building the relation list with the pg_class lookup. This logs a
DEBUG message when the relation is skipped, which is more information
that what we have now. The checks need to happen again in vacuum_rel as
the VACUUM work could be spawned across multiple transactions, where a
WARNING is logged.
3) REINDEX is already smart enough to check for ownership of relations
if one is manually listed and reports an ERROR. However it can cause
the instance to be stuck when doing a database-wide REINDEX on a
database using just the owner of this database. In this case it seems
to me that we need to make ReindexMultipleTables in terms of ACL
checks, as per 0003.

I quite like the shape of the patches proposed here, and the refactoring
is I think pretty clear. Each patch can be treated independently as
well. Comments are welcome. (Those patches are not indented yet, which
does not matter much at this stage anyway.)

Thanks,
--
Michael

I took a look at 0001.

On 7/26/18, 12:24 AM, "Michael Paquier" <michael@paquier.xyz> wrote:

1) 0001 does the work for TRUNCATE, but using RangeVarGetRelidExtended
with a custom callback based on the existing truncate_check_rel(). I
had to split the session-level checks into a separate routine as no
Relation is available, but that finishes by being a neat result without
changing any user-facing behavior.

Splitting the checks like this seems reasonable. As you pointed out,
it doesn't change the behavior of the session checks, which AFAICT
aren't necessary for the kind of permissions checks we want to add to
the RangeVarGetRelidExtended() call.

-		myrelid = RelationGetRelid(rel);
+		myrelid = RangeVarGetRelidExtended(rv, AccessExclusiveLock,
+										   false, RangeVarCallbackForTruncate,
+										   NULL);

Should the flags argument be 0 instead of false?

+	/* Nothing to do if the relation was not found. */
+	if (!OidIsValid(relId))
+		return;
+
+	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relId));
+	if (!HeapTupleIsValid(tuple))	/* should not happen */
+		elog(ERROR, "cache lookup failed for relation %u", relId);

The first time we use this callback, the relation won't be locked, so
isn't it possible that we won't get a valid tuple here? I did notice
that callbacks like RangeVarCallbackForRenameRule,
RangeVarCallbackForPolicy, and RangeVarCallbackForRenameTrigger assume
that the relation can be concurrently dropped, but
RangeVarCallbackOwnsRelation does not. Instead, we assume that the
syscache search will succeed if the given OID is valid. Is this a
bug, or am I missing something?

Nathan

On 7/26/18, 10:07 AM, "Bossart, Nathan" <bossartn@amazon.com> wrote:

The first time we use this callback, the relation won't be locked, so
isn't it possible that we won't get a valid tuple here? I did notice
that callbacks like RangeVarCallbackForRenameRule,
RangeVarCallbackForPolicy, and RangeVarCallbackForRenameTrigger assume
that the relation can be concurrently dropped, but
RangeVarCallbackOwnsRelation does not. Instead, we assume that the
syscache search will succeed if the given OID is valid. Is this a
bug, or am I missing something?

Please pardon the noise. I see that we don't accept invalidation
messages until later on in RangeVarGetRelidExtended(), at which point
we'll retry and get InvalidOid for concurrently dropped relations.

Nathan

On Thu, Jul 26, 2018 at 03:06:59PM +0000, Bossart, Nathan wrote:

I took a look at 0001.

Thanks for the lookup. 0003 is the most simple in the set by the way.

On 7/26/18, 12:24 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
-	myrelid = RelationGetRelid(rel);
+	myrelid = RangeVarGetRelidExtended(rv, AccessExclusiveLock,
+	    false, RangeVarCallbackForTruncate, NULL);

Should the flags argument be 0 instead of false?

Yes, those should be 0. All patches are missing that. It does not have
a bad consequence on the patch, still that's incorrect.
--
Michael

On 7/26/18, 3:42 PM, "Michael Paquier" <michael@paquier.xyz> wrote:

Thanks for the lookup. 0003 is the most simple in the set by the way.

Minus a possible documentation update, 0003 seems almost ready, too.

For 0002, would adding vacuum_skip_rel() before and after
try_relation_open() in vacuum_rel() be enough? That way, we could
avoid emitting an ERROR for only the VACUUM FULL case (and skip it
with a WARNING like everything else).

Nathan