Allowing to create LEAKPROOF functions to non-superuser

Andrey Borodin <x4mmm@yandex-team.ru> writes:

Currently only superuser is allowed to create LEAKPROOF functions because leakproof functions can see tuples which have not yet been filtered out by security barrier views or row level security policies.

Yeah.

But managed cloud services typically do not provide superuser roles.

This is not a good argument for relaxing superuser requirements.

regards, tom lane

On 4/12/21 10:37 PM, Tom Lane wrote:

Andrey Borodin <x4mmm@yandex-team.ru> writes:

Currently only superuser is allowed to create LEAKPROOF functions
because leakproof functions can see tuples which have not yet been
filtered out by security barrier views or row level security
policies.

Yeah.

But managed cloud services typically do not provide superuser
roles.

This is not a good argument for relaxing superuser requirements.

I guess for the cloud services it's not an issue - they're mostly
concerned about manageability and restricting access to the OS. It's
unfortunate that we tie the this capability to being superuser, so maybe
the right solution would be to introduce a separate role with this
privilege?

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks for so quick response, Tom!

12 апр. 2021 г., в 23:37, Tom Lane <tgl@sss.pgh.pa.us> написал(а):

But managed cloud services typically do not provide superuser roles.

This is not a good argument for relaxing superuser requirements.

Ok, let's put aside question about relaxing requirements in upstream.

Do I risk having some extra superusers in my installation if I allow everyone to create LEAKPROOF functions?

Thanks!

Best regards, Andrey Borodin.

Hi,

On 2021-04-12 16:37:01 -0400, Tom Lane wrote:

Andrey Borodin <x4mmm@yandex-team.ru> writes:

Currently only superuser is allowed to create LEAKPROOF functions
because leakproof functions can see tuples which have not yet been
filtered out by security barrier views or row level security
policies.

Yeah.

But managed cloud services typically do not provide superuser roles.

This is not a good argument for relaxing superuser requirements.

IDK. I may have been adjacent to people operating database-as-a-service
for too long, but ISTM there's decent reasons for (and also against) not
providing full superuser access. Even outside of managed services it
seems like a decent idea to split the "can execute native code" role
from the "administers an application" role. That reduces the impact a
bug in the application can incur.

There's certain things that are pretty intrinsically "can execute native
code", like defining new 'C' functions, arbitrary ALTER SYSTEM,
arbitrary file reads/writes, etc. Splitting them off from superuser is a
fools errand. But it's not at all clear why adding LEAKPROOF to
functions falls into that category?

Greetings,

Andres Freund

Hi,

On 2021-04-12 22:42:03 +0200, Tomas Vondra wrote:

It's unfortunate that we tie the this capability to being superuser,
so maybe the right solution would be to introduce a separate role with
this privilege?

Perhaps DB owner + BYPASSRLS would be enough?

Greetings,

Andres Freund

Thanks, Tomas!

12 апр. 2021 г., в 23:42, Tomas Vondra <tomas.vondra@enterprisedb.com> написал(а):

I guess for the cloud services it's not an issue - they're mostly
concerned about manageability and restricting access to the OS.

In fact, we would happily give a client access to an OS too. It's a client's VM after all and all software is open source. But it opens a way to attack control plane. Which in turn opens a way for clients to attack each other. And we really do not want it.

Thanks!

Best regards, Andrey Borodin.

Hi,

On 2021-04-12 23:51:02 +0300, Andrey Borodin wrote:

Do I risk having some extra superusers in my installation if I allow
everyone to create LEAKPROOF functions?

I think that depends on what you define "superuser" to exactly
be. Defining it as "has a path to executing arbitrary native code", I
don't think, if implemented sensibly, allowing to set LEAKPROOF on new
functions would equate superuser permissions. But you soon after might
hit further limitations where lifting them would have such a risk,
e.g. defining new types with in/out functions.

Greetings,

Andres Freund

13 апр. 2021 г., в 00:01, Andres Freund <andres@anarazel.de> написал(а):

Hi,

On 2021-04-12 23:51:02 +0300, Andrey Borodin wrote:

Do I risk having some extra superusers in my installation if I allow
everyone to create LEAKPROOF functions?

I think that depends on what you define "superuser" to exactly
be. Defining it as "has a path to executing arbitrary native code", I
don't think, if implemented sensibly, allowing to set LEAKPROOF on new
functions would equate superuser permissions.

Thanks!

But you soon after might
hit further limitations where lifting them would have such a risk,
e.g. defining new types with in/out functions.

I think, real extensibility of a managed DB service is a very distant challenge.
Currently we just allow-list extensions.

Best regards, Andrey Borodin.

Andres Freund <andres@anarazel.de> writes:

On 2021-04-12 23:51:02 +0300, Andrey Borodin wrote:

Do I risk having some extra superusers in my installation if I allow
everyone to create LEAKPROOF functions?

I think that depends on what you define "superuser" to exactly
be. Defining it as "has a path to executing arbitrary native code", I
don't think, if implemented sensibly, allowing to set LEAKPROOF on new
functions would equate superuser permissions. But you soon after might
hit further limitations where lifting them would have such a risk,
e.g. defining new types with in/out functions.

I think the issue here is more that superuser = "able to break the
security guarantees of the database". I doubt that falsely labeling
a function LEAKPROOF can get you more than the ability to read data
you're not supposed to be able to read ... but that ability is then
available to all users, or at least all users who can execute the
function in question. So it definitely is a fairly serious security
hazard, and one that's not well modeled by role labels. If you
give somebody e.g. pg_read_all_data privileges, you don't expect
that that means they can give it to other users.

regards, tom lane

Hi,

On 2021-04-12 17:14:20 -0400, Tom Lane wrote:

I doubt that falsely labeling a function LEAKPROOF can get you more
than the ability to read data you're not supposed to be able to read
... but that ability is then available to all users, or at least all
users who can execute the function in question. So it definitely is a
fairly serious security hazard, and one that's not well modeled by
role labels. If you give somebody e.g. pg_read_all_data privileges,
you don't expect that that means they can give it to other users.

A user with BYPASSRLS can create public security definer functions
returning data. If the concern is a BYPASSRLS user intentionally
exposing data, then there's not a meaningful increase to allow defining
LEAKPROOF functions.

To me the more relevant concern is that it's hard to determine
LEAKPROOF-ness and that many use-cases for BYPASSRLS do not require the
target to have the technical chops to determine if a function actually
is leakproof. But that seems more an argument for providing a separate
control over allowing to specify LEAKPROOF than against separating it
from superuser.

Greetings,

Andres Freund

On Mon, Apr 12, 2021 at 02:35:27PM -0700, Andres Freund wrote:

On 2021-04-12 17:14:20 -0400, Tom Lane wrote:

I doubt that falsely labeling a function LEAKPROOF can get you more
than the ability to read data you're not supposed to be able to read
... but that ability is then available to all users, or at least all
users who can execute the function in question. So it definitely is a
fairly serious security hazard, and one that's not well modeled by
role labels. If you give somebody e.g. pg_read_all_data privileges,
you don't expect that that means they can give it to other users.

I do expect that, essentially. Like Andres describes for BYPASSRLS, they can
create and GRANT a SECURITY DEFINER function that performs an arbitrary query
and returns a refcursor (or stores the data to a table of the caller's
choosing, etc.). Unlike BYPASSRLS, they can even make pg_read_all_data own
the function, making the situation persist after one drops the actor's role
and that role's objects.

A user with BYPASSRLS can create public security definer functions
returning data. If the concern is a BYPASSRLS user intentionally
exposing data, then there's not a meaningful increase to allow defining
LEAKPROOF functions.

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF. I would not consult datdba, because datdba currently has
no special read abilities. It feels too weird to let BYPASSRLS start
affecting non-RLS access controls. A reasonable person may assume that
BYPASSRLS has no consequences until someone uses CREATE POLICY. That said, I
wouldn't be horrified if BYPASSRLS played a part. BYPASSRLS, like
pg_read_all_data, clearly isn't something to grant lightly.

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

On Mon, Apr 12, 2021 at 02:35:27PM -0700, Andres Freund wrote:

On 2021-04-12 17:14:20 -0400, Tom Lane wrote:

I doubt that falsely labeling a function LEAKPROOF can get you more
than the ability to read data you're not supposed to be able to read
... but that ability is then available to all users, or at least all
users who can execute the function in question. So it definitely is a
fairly serious security hazard, and one that's not well modeled by
role labels. If you give somebody e.g. pg_read_all_data privileges,
you don't expect that that means they can give it to other users.

I do expect that, essentially. Like Andres describes for BYPASSRLS, they can
create and GRANT a SECURITY DEFINER function that performs an arbitrary query
and returns a refcursor (or stores the data to a table of the caller's
choosing, etc.). Unlike BYPASSRLS, they can even make pg_read_all_data own
the function, making the situation persist after one drops the actor's role
and that role's objects.

Yes. I think that if someone can read all the data, it's unworkable to
suppose that they can't find a way to delegate that ability to others.
If nothing else, a station wagon full of tapes has a lot of bandwidth.

A user with BYPASSRLS can create public security definer functions
returning data. If the concern is a BYPASSRLS user intentionally
exposing data, then there's not a meaningful increase to allow defining
LEAKPROOF functions.

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF. I would not consult datdba, because datdba currently has
no special read abilities. It feels too weird to let BYPASSRLS start
affecting non-RLS access controls. A reasonable person may assume that
BYPASSRLS has no consequences until someone uses CREATE POLICY. That said, I
wouldn't be horrified if BYPASSRLS played a part. BYPASSRLS, like
pg_read_all_data, clearly isn't something to grant lightly.

I agree that datdba doesn't seem like quite the right thing, but I'm
not sure I agree with the rest. How can we say that leakproof is a
non-RLS access control? Its only purpose is to keep RLS secure, so I
guess I'd be inclined to think that of the two, BYPASSRLS is more
closely related to the topic at hand.

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

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF. I would not consult datdba, because datdba currently has
no special read abilities. It feels too weird to let BYPASSRLS start
affecting non-RLS access controls. A reasonable person may assume that
BYPASSRLS has no consequences until someone uses CREATE POLICY. That said, I
wouldn't be horrified if BYPASSRLS played a part. BYPASSRLS, like
pg_read_all_data, clearly isn't something to grant lightly.

I agree that datdba doesn't seem like quite the right thing, but I'm
not sure I agree with the rest. How can we say that leakproof is a
non-RLS access control? Its only purpose is to keep RLS secure, so I
guess I'd be inclined to think that of the two, BYPASSRLS is more
closely related to the topic at hand.

Umm ... I'm pretty sure LEAKPROOF also affects optimization around
"security barrier" views, which I wouldn't call RLS. Out of these
options, I'd prefer granting the ability to pg_read_all_data.

regards, tom lane

On Mon, Apr 19, 2021 at 4:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF. I would not consult datdba, because datdba currently has
no special read abilities. It feels too weird to let BYPASSRLS start
affecting non-RLS access controls. A reasonable person may assume that
BYPASSRLS has no consequences until someone uses CREATE POLICY. That said, I
wouldn't be horrified if BYPASSRLS played a part. BYPASSRLS, like
pg_read_all_data, clearly isn't something to grant lightly.

I agree that datdba doesn't seem like quite the right thing, but I'm
not sure I agree with the rest. How can we say that leakproof is a
non-RLS access control? Its only purpose is to keep RLS secure, so I
guess I'd be inclined to think that of the two, BYPASSRLS is more
closely related to the topic at hand.

Umm ... I'm pretty sure LEAKPROOF also affects optimization around
"security barrier" views, which I wouldn't call RLS. Out of these
options, I'd prefer granting the ability to pg_read_all_data.

Oops, I forgot about security_barrier views, which is rather
embarrassing since I committed them. So, yeah, I agree:
pg_read_all_data is better.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:

On Mon, Apr 19, 2021 at 4:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF. I would not consult datdba, because datdba currently has
no special read abilities. It feels too weird to let BYPASSRLS start
affecting non-RLS access controls. A reasonable person may assume that
BYPASSRLS has no consequences until someone uses CREATE POLICY. That said, I
wouldn't be horrified if BYPASSRLS played a part. BYPASSRLS, like
pg_read_all_data, clearly isn't something to grant lightly.

I agree that datdba doesn't seem like quite the right thing, but I'm
not sure I agree with the rest. How can we say that leakproof is a
non-RLS access control? Its only purpose is to keep RLS secure, so I
guess I'd be inclined to think that of the two, BYPASSRLS is more
closely related to the topic at hand.

Umm ... I'm pretty sure LEAKPROOF also affects optimization around
"security barrier" views, which I wouldn't call RLS. Out of these
options, I'd prefer granting the ability to pg_read_all_data.

Oops, I forgot about security_barrier views, which is rather
embarrassing since I committed them. So, yeah, I agree:
pg_read_all_data is better.

I'm not really sure that attaching it to pg_read_all_data makes sense..

In general, I've been frustrated by the places where we lump privileges
together rather than having a way to distinctly GRANT capabilities
independently- that's more-or-less exactly what lead me to work on
implementing the role system in the first place, and later the
predefined roles.

I do think it's good to reduce the number of places that require
superuser, in general, but I'm not sure that marking functions as
leakproof as a non-superuser makes sense.

Here's what I'd ask Andrey- what's the actual use-case here? Are these
cases where users are actually adding new functions which they believe
are leakproof where those functions don't require superuser already to
be created? Clearly if they're in a untrusted language and you have to
be a superuser to install them in the first place then they should just
mark the function as leakproof when they install it. If these are
trusted language functions, I'd be curious to actually see them as I
have doubts about if they're actually leakproof..

Or is the actual use-case here that they just want to mark functions we
know aren't leakproof as leakproof anyway because they aren't getting
the performance they want?

If it's the latter, as I suspect it is, then I don't really think the
use-case justifies any change on our part- the right answer is to make
those functions actually leakproof, or write ones which are.

Thanks,

Stephen

On Mon, Apr 19, 2021 at 05:38:43PM -0400, Stephen Frost wrote:

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF.

I'm not really sure that attaching it to pg_read_all_data makes sense..

In general, I've been frustrated by the places where we lump privileges
together rather than having a way to distinctly GRANT capabilities
independently- that's more-or-less exactly what lead me to work on
implementing the role system in the first place, and later the
predefined roles.

This would be no more lumpy than e.g. pg_read_all_stats. However, I could
live with a separate pg_change_leakproof (or whatever name).

Here's what I'd ask Andrey- what's the actual use-case here? Are these
cases where users are actually adding new functions which they believe
are leakproof where those functions don't require superuser already to
be created? Clearly if they're in a untrusted language and you have to
be a superuser to install them in the first place then they should just
mark the function as leakproof when they install it. If these are
trusted language functions, I'd be curious to actually see them as I
have doubts about if they're actually leakproof..

Or is the actual use-case here that they just want to mark functions we
know aren't leakproof as leakproof anyway because they aren't getting
the performance they want?

Hearing those answers would be interesting, but it shouldn't change decisions.
A reasonable person can write an actually-leakproof function and wish to mark
it LEAKPROOF, independent of whether that happened in the case that prompted
this thread.

20 апр. 2021 г., в 02:38, Stephen Frost <sfrost@snowman.net> написал(а):

Here's what I'd ask Andrey- what's the actual use-case here? Are these
cases where users are actually adding new functions which they believe
are leakproof where those functions don't require superuser already to
be created? Clearly if they're in a untrusted language and you have to
be a superuser to install them in the first place then they should just
mark the function as leakproof when they install it. If these are
trusted language functions, I'd be curious to actually see them as I
have doubts about if they're actually leakproof..

Or is the actual use-case here that they just want to mark functions we
know aren't leakproof as leakproof anyway because they aren't getting
the performance they want?

If it's the latter, as I suspect it is, then I don't really think the
use-case justifies any change on our part- the right answer is to make
those functions actually leakproof, or write ones which are.

Customer was restoring pg_dump of on-premise ERP known as 1C (something like TurboTax) with this add-on [0]https://github.com/ivan816/simple-1c/blob/f2e5ce78b98f70f30039fd3de79308a59d432fc2/Simple1C/Impl/Sql/SchemaMapping/Simple1cSchemaCreator.cs#L74

CREATE FUNCTION simple1c.date_from_guid(varchar(36)) RETURNS timestamp LANGUAGE plpgsql IMMUTABLE LEAKPROOF STRICT

I'm not 1C-expert (programmed it a bit to get few bucks when I was a student), but seems like this function simple1c.date_from_guid() can be used in DSL queries. It have no obvious side effects. Maybe we could hack it by exploiting timestamp overflow, but I doubt it's practically usable.

Thanks!

Best regards, Andrey Borodin.

[0]: https://github.com/ivan816/simple-1c/blob/f2e5ce78b98f70f30039fd3de79308a59d432fc2/Simple1C/Impl/Sql/SchemaMapping/Simple1cSchemaCreator.cs#L74

Greetings,

* Andrey Borodin (x4mmm@yandex-team.ru) wrote:

20 апр. 2021 г., в 02:38, Stephen Frost <sfrost@snowman.net> написал(а):
Here's what I'd ask Andrey- what's the actual use-case here? Are these
cases where users are actually adding new functions which they believe
are leakproof where those functions don't require superuser already to
be created? Clearly if they're in a untrusted language and you have to
be a superuser to install them in the first place then they should just
mark the function as leakproof when they install it. If these are
trusted language functions, I'd be curious to actually see them as I
have doubts about if they're actually leakproof..

Or is the actual use-case here that they just want to mark functions we
know aren't leakproof as leakproof anyway because they aren't getting
the performance they want?

If it's the latter, as I suspect it is, then I don't really think the
use-case justifies any change on our part- the right answer is to make
those functions actually leakproof, or write ones which are.

Customer was restoring pg_dump of on-premise ERP known as 1C (something like TurboTax) with this add-on [0]

CREATE FUNCTION simple1c.date_from_guid(varchar(36)) RETURNS timestamp LANGUAGE plpgsql IMMUTABLE LEAKPROOF STRICT

I'm not 1C-expert (programmed it a bit to get few bucks when I was a student), but seems like this function simple1c.date_from_guid() can be used in DSL queries. It have no obvious side effects. Maybe we could hack it by exploiting timestamp overflow, but I doubt it's practically usable.

Erm, it's very clearly not leakproof and will happily return information
about the value passed in during some error cases...

Thanks,

Stephen

Greetings,

* Noah Misch (noah@leadboat.com) wrote:

On Mon, Apr 19, 2021 at 05:38:43PM -0400, Stephen Frost wrote:

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF.

I'm not really sure that attaching it to pg_read_all_data makes sense..

In general, I've been frustrated by the places where we lump privileges
together rather than having a way to distinctly GRANT capabilities
independently- that's more-or-less exactly what lead me to work on
implementing the role system in the first place, and later the
predefined roles.

This would be no more lumpy than e.g. pg_read_all_stats. However, I could
live with a separate pg_change_leakproof (or whatever name).

There's been already some disagreements about pg_read_all_stats, so I
don't think that is actually a good model to look at.

I have doubts about users generally being able to write actually
leakproof functions (this case being an example of someone writing a
function that certainly wasn't leakproof but marking it as such
anyway...), though I suppose it's unlikely that it's any worse than the
cases of people writing SECURITY DEFINER functions that aren't careful
enough, of which I've seen plenty of.

I would think the role/capability would be 'pg_mark_function_leakproof'
or similar though, and allow a user who had that role to either create
leakproof functions (provided they have access to create the function in
the first place) or to mark an existing function as leakproof (but
requiring them to be a member of the role which owns the function).

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Andrey Borodin (x4mmm@yandex-team.ru) wrote:

Customer was restoring pg_dump of on-premise ERP known as 1C (something like TurboTax) with this add-on [0]

Erm, it's very clearly not leakproof and will happily return information
about the value passed in during some error cases...

Yeah, that's pretty much a poster-child example for NOT letting
random users fool with leakproofness settings.

regards, tom lane

On Sun, Apr 25, 2021 at 02:40:54PM -0400, Stephen Frost wrote:

* Noah Misch (noah@leadboat.com) wrote:

On Mon, Apr 19, 2021 at 05:38:43PM -0400, Stephen Frost wrote:

On Fri, Apr 16, 2021 at 3:57 AM Noah Misch <noah@leadboat.com> wrote:

Hence, I do find it reasonable to let pg_read_all_data be sufficient for
setting LEAKPROOF.

I'm not really sure that attaching it to pg_read_all_data makes sense..

In general, I've been frustrated by the places where we lump privileges
together rather than having a way to distinctly GRANT capabilities
independently- that's more-or-less exactly what lead me to work on
implementing the role system in the first place, and later the
predefined roles.

This would be no more lumpy than e.g. pg_read_all_stats. However, I could
live with a separate pg_change_leakproof (or whatever name).

There's been already some disagreements about pg_read_all_stats, so I
don't think that is actually a good model to look at.

I have doubts about users generally being able to write actually
leakproof functions (this case being an example of someone writing a
function that certainly wasn't leakproof but marking it as such
anyway...), though I suppose it's unlikely that it's any worse than the
cases of people writing SECURITY DEFINER functions that aren't careful
enough, of which I've seen plenty of.

Making "it's hard to do well" imply "only superusers get to try" doesn't
mitigate a risk; it multiplies risks.

I would think the role/capability would be 'pg_mark_function_leakproof'
or similar though, and allow a user who had that role to either create
leakproof functions (provided they have access to create the function in
the first place) or to mark an existing function as leakproof (but
requiring them to be a member of the role which owns the function).

That's fine.