Hi all, This is a second thread I am spawning for the previous thread "Canceling authentication due to timeout aka Denial of Service Attack": https://www.postgresql.org/message-id/152512087100.19803.12733865831237526317%40wrigleys.postgresql.org
And this time the discussion is about TRUNCATE, as the former thread discussed about a family of failures hence it is harder to catch the proper attention. The problem is that when we look for the relation OID of the relation to truncate, we don't use a callback with RangeVarGetRelidExtended, causing a lock acquire attempt to happen before checking the privileges on the relation for the user running the command. Attached is a patch I have been working on which refactors the code of TRUNCATE in such a way that we check for privileges before trying to acquire a lock, without any user-facing impact (I have reworked a couple of comments compared to the last version). This includes a set of tests showing the new behavior. Like cbe24a6, perhaps we would not want to back-patch it? Based on the past history (and the consensus being reached for the REINDEX case would be to patch only HEAD), I would be actually incline to not back-patch this stuff and qualify that as an improvement. That's also less work for me at commit :) Thoughts? -- Michael
From 9946e5d2025cd6b8ec5baa214dab06a32f3b20a8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 6 Aug 2018 18:43:20 +0200
Subject: [PATCH] Refactor TRUNCATE execution to avoid too early lock lookups
A caller of TRUNCATE can perform early lookup obtention which can cause
other sessions to block on the request done, causing potentially DOS
attacks as even a non-privileged user can attempt a truncation of a
critical catalog table to block even all incoming connection attempts.
This commit fixes the case of TRUNCATE so as RangeVarGetRelidExtended is
used with a callback doing the necessary checks at an earlier stage,
avoiding locking issues at early stages, so as a failure happens for
unprivileged users instead of waiting on a lock.
---
src/backend/commands/tablecmds.c | 84 +++++++++++++++----
.../isolation/expected/truncate-conflict.out | 45 ++++++++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/truncate-conflict.spec | 27 ++++++
4 files changed, 139 insertions(+), 18 deletions(-)
create mode 100644 src/test/isolation/expected/truncate-conflict.out
create mode 100644 src/test/isolation/specs/truncate-conflict.spec
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index eb2d33dd86..fd240cb635 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -300,7 +300,10 @@ struct DropRelationCallbackState
#define child_dependency_type(child_is_partition) \
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
-static void truncate_check_rel(Relation rel);
+static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_activity(Relation rel);
+static void RangeVarCallbackForTruncate(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List **supconstr,
int *supOidCount);
@@ -1336,15 +1339,25 @@ ExecuteTruncate(TruncateStmt *stmt)
bool recurse = rv->inh;
Oid myrelid;
- rel = heap_openrv(rv, AccessExclusiveLock);
- myrelid = RelationGetRelid(rel);
+ myrelid = RangeVarGetRelidExtended(rv, AccessExclusiveLock,
+ 0, RangeVarCallbackForTruncate,
+ NULL);
+
+ /* open the relation, we already hold a lock on it */
+ rel = heap_open(myrelid, NoLock);
+
/* don't throw error for "TRUNCATE foo, foo" */
if (list_member_oid(relids, myrelid))
{
heap_close(rel, AccessExclusiveLock);
continue;
}
- truncate_check_rel(rel);
+
+ /*
+ * RangeVarGetRelidExtended has done some basic checks with its
+ * callback, and only remain session-level checks.
+ */
+ truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, myrelid);
/* Log this relation only if needed for logical decoding */
@@ -1367,7 +1380,9 @@ ExecuteTruncate(TruncateStmt *stmt)
/* find_all_inheritors already got lock */
rel = heap_open(childrelid, NoLock);
- truncate_check_rel(rel);
+ truncate_check_rel(RelationGetRelid(rel), rel->rd_rel);
+ truncate_check_activity(rel);
+
rels = lappend(rels, rel);
relids = lappend_oid(relids, childrelid);
/* Log this relation only if needed for logical decoding */
@@ -1450,7 +1465,8 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
ereport(NOTICE,
(errmsg("truncate cascades to table \"%s\"",
RelationGetRelationName(rel))));
- truncate_check_rel(rel);
+ truncate_check_rel(relid, rel->rd_rel);
+ truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, relid);
/* Log this relation only if needed for logical decoding */
@@ -1700,38 +1716,48 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_logged,
}
/*
- * Check that a given rel is safe to truncate. Subroutine for ExecuteTruncate
+ * Check that a given relation is safe to truncate. Subroutine for
+ * ExecuteTruncate and RangeVarCallbackForTruncate.
*/
static void
-truncate_check_rel(Relation rel)
+truncate_check_rel(Oid relid, Form_pg_class reltuple)
{
AclResult aclresult;
+ char *relname = NameStr(reltuple->relname);
/*
* Only allow truncate on regular tables and partitioned tables (although,
* the latter are only being included here for the following checks; no
* physical truncation will occur in their case.)
*/
- if (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ if (reltuple->relkind != RELKIND_RELATION &&
+
+ reltuple->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- errmsg("\"%s\" is not a table",
- RelationGetRelationName(rel))));
+ errmsg("\"%s\" is not a table", relname)));
/* Permissions checks */
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
- ACL_TRUNCATE);
+ aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, get_relkind_objtype(rel->rd_rel->relkind),
- RelationGetRelationName(rel));
+ aclcheck_error(aclresult, get_relkind_objtype(reltuple->relkind),
+ relname);
- if (!allowSystemTableMods && IsSystemRelation(rel))
+ if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied: \"%s\" is a system catalog",
- RelationGetRelationName(rel))));
+ relname)));
+}
+/*
+ * Set of extra sanity checks to check if a given relation is safe to
+ * truncate. This is split with truncate_check_rel as
+ * RangeVarCallbackForTruncate can only call the former.
+ */
+static void
+truncate_check_activity(Relation rel)
+{
/*
* Don't allow truncate on temp tables of other backends ... their local
* buffer manager is not going to cope.
@@ -13419,6 +13445,28 @@ RangeVarCallbackOwnsTable(const RangeVar *relation,
aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(get_rel_relkind(relId)), relation->relname);
}
+/*
+ * Callback to RangeVarGetRelidExtended() for TRUNCATE processing.
+ */
+static void
+RangeVarCallbackForTruncate(const RangeVar *relation,
+ Oid relId, Oid oldRelId, void *arg)
+{
+ HeapTuple tuple;
+
+ /* 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);
+
+ truncate_check_rel(relId, (Form_pg_class) GETSTRUCT(tuple));
+
+ ReleaseSysCache(tuple);
+}
+
/*
* Callback to RangeVarGetRelidExtended(), similar to
* RangeVarCallbackOwnsTable() but without checks on the type of the relation.
diff --git a/src/test/isolation/expected/truncate-conflict.out b/src/test/isolation/expected/truncate-conflict.out
new file mode 100644
index 0000000000..6938528380
--- /dev/null
+++ b/src/test/isolation/expected/truncate-conflict.out
@@ -0,0 +1,45 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_begin s1_catalog_lookup s2_auth s2_truncate s1_commit
+step s1_begin: BEGIN;
+step s1_catalog_lookup: SELECT count(*) >= 0 FROM pg_stat_activity;
+?column?
+
+t
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE pg_authid;
+ERROR: permission denied for table pg_authid
+step s1_commit: COMMIT;
+
+starting permutation: s1_begin s2_auth s2_truncate s1_catalog_lookup s1_commit
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE pg_authid;
+ERROR: permission denied for table pg_authid
+step s1_catalog_lookup: SELECT count(*) >= 0 FROM pg_stat_activity;
+?column?
+
+t
+step s1_commit: COMMIT;
+
+starting permutation: s1_begin s2_auth s1_catalog_lookup s2_truncate s1_commit
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s1_catalog_lookup: SELECT count(*) >= 0 FROM pg_stat_activity;
+?column?
+
+t
+step s2_truncate: TRUNCATE pg_authid;
+ERROR: permission denied for table pg_authid
+step s1_commit: COMMIT;
+
+starting permutation: s2_auth s2_truncate s1_begin s1_catalog_lookup s1_commit
+step s2_auth: SET ROLE regress_truncate_conflict;
+step s2_truncate: TRUNCATE pg_authid;
+ERROR: permission denied for table pg_authid
+step s1_begin: BEGIN;
+step s1_catalog_lookup: SELECT count(*) >= 0 FROM pg_stat_activity;
+?column?
+
+t
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index d5594e80e2..48ae740739 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -76,3 +76,4 @@ test: partition-key-update-2
test: partition-key-update-3
test: partition-key-update-4
test: plpgsql-toast
+test: truncate-conflict
diff --git a/src/test/isolation/specs/truncate-conflict.spec b/src/test/isolation/specs/truncate-conflict.spec
new file mode 100644
index 0000000000..f70aca9f69
--- /dev/null
+++ b/src/test/isolation/specs/truncate-conflict.spec
@@ -0,0 +1,27 @@
+# Test for lock lookup conflicts with TRUNCATE when working on unowned
+# relations, particularly catalogs should trigger an ERROR for all the
+# scenarios present here.
+
+setup
+{
+ CREATE ROLE regress_truncate_conflict;
+}
+
+teardown
+{
+ DROP ROLE regress_truncate_conflict;
+}
+
+session "s1"
+step "s1_begin" { BEGIN; }
+step "s1_catalog_lookup" { SELECT count(*) >= 0 FROM pg_stat_activity; }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+step "s2_auth" { SET ROLE regress_truncate_conflict; }
+step "s2_truncate" { TRUNCATE pg_authid; }
+
+permutation "s1_begin" "s1_catalog_lookup" "s2_auth" "s2_truncate" "s1_commit"
+permutation "s1_begin" "s2_auth" "s2_truncate" "s1_catalog_lookup" "s1_commit"
+permutation "s1_begin" "s2_auth" "s1_catalog_lookup" "s2_truncate" "s1_commit"
+permutation "s2_auth" "s2_truncate" "s1_begin" "s1_catalog_lookup" "s1_commit"
--
2.18.0
signature.asc
Description: PGP signature
