On Mon, Apr 14, 2025 at 12:36 PM Daniil Davydov <3daniss...@gmail.com>
wrote:
> Hi,
>
> During previous commitfest this topic already has been discussed
> within the "Forbid to DROP temp tables of other sessions" thread [1].
> Unfortunately its name doesn't reflect the real problem, so I decided
> to start a new thread, as David G. Johnston advised.
>
> Here are the summary results of the discussion [1] :
> The superuser is only allowed to DROP temporary relations of other
> sessions. Other commands (like SELECT, INSERT, UPDATE, DELETE ...)
> must be forbidden to him. Error message for this case will look like
> this : `could not access temporary relations of other sessions`.
> For now, superuser still can specify such operations because of a bug
> in the code that mistakenly recognizes other session's temp table as
> permanent (I've covered this topic in more detail in [2]). Attached
> patch fixes this bug (targeted on
> b51f86e49a7f119004c0ce5d0be89cdf98309141).
>
> Opened issue:
> Not everyone liked the idea that table's persistence can be assigned
> to table during makeRangeVarXXX calls (inside gram.y).
> My opinion - `As far as "pg_temp_" prefix is reserved by the postgres
> kernel, we can definitely say that we have encountered a temporary
> table when we see this prefix.`
>
> I will be glad to hear your opinion.
>
> --
> Best regards,
> Daniil Davydov
>
> [1]
> https://www.postgresql.org/message-id/CAJDiXgj72Axj0d4ojKdRWG_rnkfs4uWY414NL%3D15sCvh7-9rwg%40mail.gmail.com
> [2]
> https://www.postgresql.org/message-id/CAJDiXgj%2B5UKLWSUT5605rJhuw438NmEKecvhFAF2nnrMsgGK3w%40mail.gmail.com
Hi Daniil,
Your patch for securing cross-session temp table access is a great
improvement. The RVR_OTHER_TEMP_OK flag elegantly handles the DROP case
while keeping the main restriction in place.
For schema name validation, an exact strcmp for "pg_temp" and proper
numeric parsing for "pg_temp_X" would be more precise than the current
prefix check. This would avoid any accidental matches to similarly named
schemas.
The error message could be adjusted to emphasize permissions, like
"permission denied for cross-session temp table access". This would make
the security intent clearer to users.
I noticed the Assert assumes myTempNamespace is always valid. While
correct, a brief comment explaining why this is safe would help future
maintainers. The relpersistence logic could also be centralized in one
place for consistency.
I've added an isolation test to verify the behavior when trying to access
another backend's temp tables. It confirms the restrictions work as
intended while allowing permitted operations.
Thanks for working on this important security enhancement!
Best regards,
Stepan Neretin
From da27bc190faab3853f6a2cc0748f1f5476215001 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davy...@postgrespro.ru>
Date: Mon, 14 Apr 2025 11:01:56 +0700
Subject: [PATCH v6 1/2] Fix accessing other sessions temp tables
---
src/backend/catalog/namespace.c | 56 ++++++++++++++++++++------------
src/backend/commands/tablecmds.c | 3 +-
src/backend/nodes/makefuncs.c | 6 +++-
src/backend/parser/gram.y | 11 ++++++-
src/include/catalog/namespace.h | 2 ++
5 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index d97d632a7ef..f407efd9447 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -499,28 +499,44 @@ RangeVarGetRelidExtended(const RangeVar *relation, LOCKMODE lockmode,
*/
if (relation->relpersistence == RELPERSISTENCE_TEMP)
{
- if (!OidIsValid(myTempNamespace))
- relId = InvalidOid; /* this probably can't happen? */
- else
- {
- if (relation->schemaname)
- {
- Oid namespaceId;
+ Oid namespaceId;
- namespaceId = LookupExplicitNamespace(relation->schemaname, missing_ok);
+ if (relation->schemaname)
+ {
+ namespaceId = LookupExplicitNamespace(relation->schemaname, missing_ok);
+ /*
+ * If the user has specified an existing temporary schema
+ * owned by another user.
+ */
+ if (OidIsValid(namespaceId) && namespaceId != myTempNamespace)
+ {
/*
- * For missing_ok, allow a non-existent schema name to
- * return InvalidOid.
+ * We don't allow users to access temp tables of other
+ * sessions except for the case of dropping tables.
*/
- if (namespaceId != myTempNamespace)
+ if (!(flags & RVR_OTHER_TEMP_OK))
ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("temporary tables cannot specify a schema name")));
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("could not access temporary relations of other sessions")));
}
+ }
+ else
+ {
+ namespaceId = myTempNamespace;
- relId = get_relname_relid(relation->relname, myTempNamespace);
+ /*
+ * If this table was recognized as temporary, it means that we
+ * found it because backend's temporary namespace was specified
+ * in search_path. Thus, MyTempNamespace must contain valid oid.
+ */
+ Assert(OidIsValid(namespaceId));
}
+
+ if (missing_ok && !OidIsValid(namespaceId))
+ relId = InvalidOid;
+ else
+ relId = get_relname_relid(relation->relname, namespaceId);
}
else if (relation->schemaname)
{
@@ -3553,21 +3569,19 @@ get_namespace_oid(const char *nspname, bool missing_ok)
RangeVar *
makeRangeVarFromNameList(const List *names)
{
- RangeVar *rel = makeRangeVar(NULL, NULL, -1);
+ RangeVar *rel;
switch (list_length(names))
{
case 1:
- rel->relname = strVal(linitial(names));
+ rel = makeRangeVar(NULL, strVal(linitial(names)), -1);
break;
case 2:
- rel->schemaname = strVal(linitial(names));
- rel->relname = strVal(lsecond(names));
+ rel = makeRangeVar(strVal(linitial(names)), strVal(lsecond(names)), -1);
break;
case 3:
+ rel = makeRangeVar(strVal(lsecond(names)), strVal(lthird(names)), -1);
rel->catalogname = strVal(linitial(names));
- rel->schemaname = strVal(lsecond(names));
- rel->relname = strVal(lthird(names));
break;
default:
ereport(ERROR,
@@ -3774,6 +3788,8 @@ GetTempNamespaceProcNumber(Oid namespaceId)
return INVALID_PROC_NUMBER; /* no such namespace? */
if (strncmp(nspname, "pg_temp_", 8) == 0)
result = atoi(nspname + 8);
+ else if (strcmp(nspname, "pg_temp") == 0)
+ result = MyProcNumber;
else if (strncmp(nspname, "pg_toast_temp_", 14) == 0)
result = atoi(nspname + 14);
else
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cb811520c29..76406b5a9d9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1623,7 +1623,8 @@ RemoveRelations(DropStmt *drop)
state.heapOid = InvalidOid;
state.partParentOid = InvalidOid;
- relOid = RangeVarGetRelidExtended(rel, lockmode, RVR_MISSING_OK,
+ relOid = RangeVarGetRelidExtended(rel, lockmode,
+ RVR_MISSING_OK | RVR_OTHER_TEMP_OK,
RangeVarCallbackForDropRelation,
&state);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index e2d9e9be41a..62edf24b5c2 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -478,10 +478,14 @@ makeRangeVar(char *schemaname, char *relname, int location)
r->schemaname = schemaname;
r->relname = relname;
r->inh = true;
- r->relpersistence = RELPERSISTENCE_PERMANENT;
r->alias = NULL;
r->location = location;
+ if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
+ r->relpersistence = RELPERSISTENCE_TEMP;
+ else
+ r->relpersistence = RELPERSISTENCE_PERMANENT;
+
return r;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index db43034b9db..1b959e752c3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -19405,7 +19405,11 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
break;
}
- r->relpersistence = RELPERSISTENCE_PERMANENT;
+ if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
+ r->relpersistence = RELPERSISTENCE_TEMP;
+ else
+ r->relpersistence = RELPERSISTENCE_PERMANENT;
+
r->location = position;
return r;
@@ -19445,6 +19449,11 @@ makeRangeVarFromQualifiedName(char *name, List *namelist, int location,
break;
}
+ if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
+ r->relpersistence = RELPERSISTENCE_TEMP;
+ else
+ r->relpersistence = RELPERSISTENCE_PERMANENT;
+
return r;
}
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index 8c7ccc69a3c..9c45a30516e 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -72,6 +72,8 @@ typedef enum RVROption
RVR_MISSING_OK = 1 << 0, /* don't error if relation doesn't exist */
RVR_NOWAIT = 1 << 1, /* error if relation cannot be locked */
RVR_SKIP_LOCKED = 1 << 2, /* skip if relation cannot be locked */
+ RVR_OTHER_TEMP_OK = 1 << 3 /* don't error if relation is temp relation of
+ other session (needed for DROP command) */
} RVROption;
typedef void (*RangeVarGetRelidCallback) (const RangeVar *relation, Oid relId,
--
2.48.1
From c0cbdf310752b51e2c4753bd1e81abfd83c759be Mon Sep 17 00:00:00 2001
From: Stepan Neretin <slp...@gmail.com>
Date: Mon, 28 Jul 2025 10:24:20 +0700
Subject: [PATCH v6 2/2] Prevent cross-session temp table access test
Adds an isolation test verifying that temp tables created in one session
cannot be accessed from another session using the temp schema name.
Session 1 creates a temp table and records its temp schema name in a regular table.
Session 2 attempts to query that temp table via the stored schema name, expecting
an error.
---
src/test/isolation/expected/temp-access.out | 32 +++++++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/temp-access.spec | 32 +++++++++++++++++++++
3 files changed, 65 insertions(+)
create mode 100644 src/test/isolation/expected/temp-access.out
create mode 100644 src/test/isolation/specs/temp-access.spec
diff --git a/src/test/isolation/expected/temp-access.out b/src/test/isolation/expected/temp-access.out
new file mode 100644
index 00000000000..d402add4d83
--- /dev/null
+++ b/src/test/isolation/expected/temp-access.out
@@ -0,0 +1,32 @@
+Parsed test spec with 2 sessions
+
+starting permutation: create_temp_table wait try_access_temp_table
+step create_temp_table:
+ CREATE TEMP TABLE temp_table1(a int);
+ CREATE TABLE IF NOT EXISTS temp_schema_name(schema_name text);
+ TRUNCATE temp_schema_name;
+ INSERT INTO temp_schema_name
+ SELECT n.nspname
+ FROM pg_class c
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE c.relname = 'temp_table1';
+
+step wait:
+ SELECT 1;
+
+?column?
+--------
+ 1
+(1 row)
+
+step try_access_temp_table:
+ DO $$
+ DECLARE
+ nspname TEXT;
+ BEGIN
+ SELECT schema_name INTO nspname FROM temp_schema_name LIMIT 1;
+ EXECUTE format('SELECT * FROM %I.temp_table1', nspname);
+ END
+ $$;
+
+ERROR: could not access temporary relations of other sessions
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index e3c669a29c7..e2d3d330fd0 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -45,6 +45,7 @@ test: lock-update-delete
test: lock-update-traversal
test: inherit-temp
test: temp-schema-cleanup
+test: temp-access
test: insert-conflict-do-nothing
test: insert-conflict-do-nothing-2
test: insert-conflict-do-update
diff --git a/src/test/isolation/specs/temp-access.spec b/src/test/isolation/specs/temp-access.spec
new file mode 100644
index 00000000000..35383fe7b35
--- /dev/null
+++ b/src/test/isolation/specs/temp-access.spec
@@ -0,0 +1,32 @@
+session s1
+step create_temp_table
+{
+ CREATE TEMP TABLE temp_table1(a int);
+ CREATE TABLE IF NOT EXISTS temp_schema_name(schema_name text);
+ TRUNCATE temp_schema_name;
+ INSERT INTO temp_schema_name
+ SELECT n.nspname
+ FROM pg_class c
+ JOIN pg_namespace n ON c.relnamespace = n.oid
+ WHERE c.relname = 'temp_table1';
+}
+
+session s2
+step wait
+{
+ SELECT 1;
+}
+
+step try_access_temp_table
+{
+ DO $$
+ DECLARE
+ nspname TEXT;
+ BEGIN
+ SELECT schema_name INTO nspname FROM temp_schema_name LIMIT 1;
+ EXECUTE format('SELECT * FROM %I.temp_table1', nspname);
+ END
+ $$;
+}
+
+permutation create_temp_table wait try_access_temp_table
--
2.48.1
