Re: Non-superuser subscription owners

Greetings,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Tue, Feb 28, 2023 at 4:01 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> > Or default expressions, I presume. If we at least agree on this point,
> > then I think we should try to find a way to treat these other hunks of
> > code in a secure way (which I think is what Andres was suggesting).
>
> Yeah, or any other expressions. Basically impose restrictions when the
> user running the code is not the same as the user who provided the
> code.

Would this have carve-outs for things like "except if the user providing
the code is trusted/superuser"? Seems like that would be necessary for
the function to be able to do more-or-less anything, but then I worry
that there's superuser-owned code which could leak information or be
used by a malicious owner as that code would still be running as the
invoking user.. Perhaps we could say that the function also has to be
leakproof, but that isn't quite the same issue and therefore it seems
like we'd need to decorate all of the functions with another flag that's
allowed to be run in this manner.

Random thought- what if the function has a NOTIFY in it with a payload
of some kind of sensitive information?

> > Unless the trusted roles defaults to '*', then I think it will still
> > break some things.
>
> Definitely. IMHO, it's OK to break some things, certainly in a major
> release and maybe even in a minor release. But we don't want to break
> more things that we really need to break. And as you say, we want the
> restrictions to be comprehensible.

Really hard to say if whatever this is is OK for back-patching and
breaking minor releases without knowing exactly what is getting broken
... but it'd have to be a very clear edge case of what gets broken for
it to be sensible for breaking in a minor release without a very clear
vulnerability or such that's being fixed with a simple work-around.
Just making all auditing triggers break in a minor release certainly
wouldn't be acceptable, as an example that I imagine we all agree with.

> > Each of these ideas and sub-ideas affect the semantics, and should be
> > documented. But how do we document that some code runs as you, some as
> > the person who wrote it, sometimes we obey SECURITY INVOKER and
> > sometimes we ignore it and use DEFINER semantics, some code is outside
> > a function and always executes as the invoker, some code has some
> > security flags, and some code has more security flags, code can change
> > between the time you look at it and the time it runs, and it's all
> > filtered through GUCs with their own privilege sub-language?
> >
> > OK, let's assume that we have all of that documented, then how do we
> > guide users on what reasonable best practices are for the GUC settings,
> > etc.? Or do we just say "this is mechanically how all these parts work,
> > good luck assembling it into a secure system!". [ Note: I feel like
> > this is the state we are in now. Even if technically we don't have live
> > security bugs that I'm aware of, we are setting users up for security
> > problems. ]
> >
> > On the other hand, if we focus on executing code as the user who wrote
> > it in most places, then the documentation will be something like: "you
> > defined the table, you wrote the code, it runs as you, here are some
> > best practices for writing secure code". And we have some different
> > documentation for writing a cool SECURITY INVOKER function and how to
> > get other users to trust you enough to run it. That sounds a LOT more
> > understandable for users.
>
> What I was imagining is that we would document something like: A table
> can have executable code associated with it in a variety of ways. For
> example, it can have triggers, default expressions, check constraints,
> or row-level security filters. In most cases, these expressions are
> executed with the privileges of the user performing the operation on
> the table, except when SECURITY DEFINER functions are used. Because
> these expressions are set by the table owner and executed by the users
> accessing the table, there is a risk that the table owner could
> include malicious code that usurps the privileges of the user
> accessing the table. For this reason, these expressions are, by
> default, restricted from doing <things>. If you want to allow those
> operations, you can <something>.

Well, one possible answer to 'something' might be 'use SECURITY DEFINER
functions which are owned by a role allowed to do <things>'. Note that
that doesn't have to be the table owner though, it could be a much more
constrained role. That approach would allow us to leverage the existing
GRANT/RLS/et al system for what's allowed and avoid having to create new
things like a complex permission system inside of a GUC for users to
have to understand.

> I agree that running code as the table owner is helpful in a bunch of
> scenarios, but I also don't think it fixes everything. You earlier
> mentioned that switching to the table owner seems to be just a way of
> turning SECURITY INVOKER into SECURITY DEFINER in random places, or
> maybe that's not exactly what you said but that's what I took from it.
> And I think that's right. If we just slather user context switches
> everywhere, I'm not actually very sure that's going to be
> comprehensible behavior: if my trigger function is SECURITY INVOKER,
> why is it getting executed as me, not the inserting user? I also think
> there are plenty of cases where that could just replace the current
> set of security problems with a new set of security problems. If the
> trigger function is SECURITY INVOKER, then the user who wrote it
> doesn't have to worry about securing it against attacks by users
> accessing the table; it's just running with the permissions of the
> user performing the DML. Maybe there are correctness issues if you
> don't lock down search_path etc., but there's no security compromise
> because there's no user ID switching. As soon as you magically turn
> that into a SECURITY DEFINER function, you've provided a way for the
> users performing DML to attack the table owner.

I agree that we don't want to just turn "SECURITY INVOKER function when
run as a trigger" into SECURITY DEFINER, and that SECURITY DEFINER
functions need to be able to be written in a mannor that limits the risk
of them being able to be abused to gain control of the role which owns
the function (the latter being something we've worked on but should
certainly continue to improve on, independently of any of this..).

Along the same general vein of "don't break things", perhaps an approach
would be a GUC that users can enable that says "don't allow code that
does something dangerous (again, need to figure out how to do that..)
when it's written by someone else to run with my privileges (and
therefore isn't a SECURITY DEFINER function)". The idea here being that
we want to encourage users to enable that, maybe we eventually enable it
by default in a new major version, and push people in the direction of
writing secure SECURITY DEFINER functions for the cases where they
actually need the trigger, or such, to do something beyond whatever we
define as being 'safe'. This keeps the GUC as a simple on/off or enum
like row_security, fails the action when something not-safe is being
attempted, and gives the flexibility of our existing GRANT/RLS system
for the case where a SECURITY DEFINER function is created to perform the
operation. This does still need some supporting functions like 'calling
role' or such because there could be many many roles doing an INSERT
into a table which runs a trigger and that trigger runs as some other
role, but the other role could be one that has more privileges than the
INSERT'ing role and therefore it needs to implement additional checks on
the operation to limit what the INSERT'ing role is allowed to do.

I do worry about asking function authors to effectively rewrite these
kinds of permission checks and wonder if there's a way we could make it
easier for them- perhaps a kind of function that's SECURITY DEFINER in
that it runs as the owner of the function, but it sets a flag saying
"only allow things that the function owner is allowed to do AND the
calling user is allowed to do", similar to the 'intersection of
privileges' idea mentioned elsewhere on this thread.

Thanks,

Stephen