From: | Daniil Davydov <3danissimo(at)gmail(dot)com> |
---|---|
To: | Stepan Neretin <slpmcf(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Fix bug with accessing to temporary tables of other sessions |
Date: | 2025-07-29 09:35:00 |
Message-ID: | CAJDiXghNBp=MEUdE3pxSLtULoHmkt4WyA4ZEReoE+ihAfX1uxQ@mail.gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On Mon, Jul 28, 2025 at 10:43 AM Stepan Neretin <slpmcf(at)gmail(dot)com> wrote:
>
>
> 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.
>
Thanks for looking into it!
> 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 don't think that such an error message will be more appropriate. We
want to forbid this operation not because of "permission" reasons, but
because of the danger of this operation.
Yes, some people insist that dropping other sessions' temp tables might
be useful in some cases, but it is a "last resort" solution.
Even with this patch, DROP of other session temp tables can lead to
an error. I wrote about it here [1].
> I noticed the Assert assumes myTempNamespace is always valid. While correct, a brief comment explaining why this is safe would help future maintainers.
Well, v5 patch already contains comment for this assert :
/*
* If this table was recognized as temporary, it means that we
* found it because the backend's temporary namespace was specified
* in search_path. Thus, MyTempNamespace must contain valid oid.
*/
> The relpersistence logic could also be centralized in one place for consistency.
I don't see a reason to separate this logic into a new function, because
there will be no more cases when it will be useful to us.
> 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.
Some time ago I also created a test for this situation, see patch in this [2]
message. it worked in a similar way (but covered more test cases).
It caused a mixed reaction from people, so I decided to abandon this idea.
I guess it might be a discussion point in the future, but first I'd like to
settle the core logic of the patch.
I attach a v7 patch to this letter. No changes yet, just rebased on the newest
commit in master branch.
[1] https://www.postgresql.org/message-id/flat/CAJDiXghoi-FM4d5XVZzUyiuhv8DDm9JdGOU8KC47emasqi1GUw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAJDiXgi9CWaZCVcHmvAT604RrAqDN5zpOYxZq92adqkPq5QbnQ%40mail.gmail.com
--
Best regards,
Daniil Davydov
Attachment | Content-Type | Size |
---|---|---|
v7-0001-Fix-accessing-other-sessions-temp-tables.patch | text/x-patch | 6.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jelte Fennema-Nio | 2025-07-29 09:35:36 | Re: Extension security improvement: Add support for extensions with an owned schema |
Previous Message | sunil s | 2025-07-29 09:29:35 | Re: Unnecessary delay in streaming replication due to replay lag |