Обсуждение: 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 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].
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 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.

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, 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:

http://postgr.es/m/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

"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

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], 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...

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:

https://www.postgresql.org/message-id/flat/20180809190443.GA14011%40momjian.us#3dda365965c7d95007e58b7551161442
https://www.postgresql.org/message-id/CA%2BTgmoaHpmz9-7ybB17B2qpDoqsi7%3DOWigc-3VBctb6B_x8bKA%40mail.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 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:
> 
> https://www.postgresql.org/message-id/flat/20180809190443.GA14011%40momjian.us#3dda365965c7d95007e58b7551161442
> https://www.postgresql.org/message-id/CA%2BTgmoaHpmz9-7ybB17B2qpDoqsi7%3DOWigc-3VBctb6B_x8bKA%40mail.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.

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 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.