Re: POC for a function trust mechanism

On Sun, Aug 12, 2018 at 10:40:30PM -0400, Robert Haas wrote:
> On Wed, Aug 8, 2018 at 1:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > If we had, say, a catalog that provided the desired list of trusted roles
> > for every role, then we could imagine implementing that context change
> > automatically.  Likewise, stuff like autovacuum or REINDEX would want
> > to run with the table owner's list of trusted roles, but the GUC approach
> > doesn't really provide enough infrastructure to know what to do there.
> 
> Yeah, I think these are all good points.  It seems natural for trust
> to be a property of a role, for just the reasons that you mention.
> However, there does also seem to be a use case for varying it
> temporarily on a per-session or per-function basis, and I'm not
> exactly sure how to cater to those needs.

Yep.  A GUC is great for src/bin sessions, since many of those applications
consistently want tight trust settings.

> I wonder if Noah would like to rebase and post his version also.

Sure, attached (last merge at 757c518).  The owner_trustcheck() header comment
is a good starting point for reading it.  Tom Lane later asserted that it's
okay to perform the function trust check in fmgr_info_cxt_security() instead
of doing it in most fmgr_info() callers and some *FunctionCall* callers.
While I suspected he was right, I have not made that change in this rebase.
(I also haven't audited every fmgr_info() call added after -v1.)  When I
shared -v1 with the security team, I included the following submission notes:

===

Here's an implementation.  Design decisions:

1. A role trusts itself implicitly

2. Default pg_auth_trust entry for "public TRUST public"

This effectively disables the new restrictions; concerned administrators will
test their applications with it removed, then remove it.

3. Default pg_auth_trust entry for "public TRUST <bootstrap superuser>"

Almost everyone will leave this untouched.

4. Changing trust for a role requires ADMIN OPTION on the role.

Trust is the next step down from granting role membership.  ("ALTER ROLE
public TRUST ..." does require superuser.)

5. Non-inheritance of trust

Trust is like a reverse permission; I find it helpful to think of "ALTER ROLE
foo TRUST bar" as "GRANT may_supply_functions_to_foo TO bar".  It would be
reasonable to have that trust relationship be effective for INHERITS members
of bar; after all, they could always "ALTER FUNCTION ... OWNER TO bar".  For
now, trust relationships are not subject to inheritance.  This keeps things
simpler to understand and poses no major downsides.  For similar reasons, I
have not made a role trust its own members implicitly.

6. Non-transitivity of trust

If I trust only alice, alice trusts only bob, and I call alice's function that
calls bob's function, should the call succeed?  This is a tough one.  In favor
of "yes", this choice would allow alice to refactor her function without
needing her callers to revise their trust settings.  That is, she could switch
from bob's function to carol's function, and her callers would never notice.
My trust in alice is probably misplaced if she chooses her friends badly.

In favor of "no", making the trust relationship transitive seems to mix two
otherwise-separate concepts: alice's willingness to run code as herself and
alice's willingness to certify third-party functions to her friends.  In
particular, current defects in the system are at odds with conflating those
concepts.  Everyone should trust the bootstrap superuser, but it currently
owns functions like integer_pl_date that are not careful in what they call.
That closed the issue for me; trust is not transitive by default.  Even so, I
think there would be value in a facility for requesting trust transitivity in
specific situations.

7. (Lack of) negative trust entries

There's no way to opt out of a PUBLIC trust relationship; until a superuser
removes the "public TRUST public" entry, no user can achieve anything with
ALTER USER [NO] TRUST.  I considered adding the concept of a negative trust
relationship to remedy this problem; it could also be used to remove the
implicit self-trust for testing purposes.  I have refrained; I don't know
whether the benefits would pay for the extra user-facing complexity.

8. Applicable roles during trust checks

We had examined whether the check could be looser for SECURITY DEFINER
functions, since those can't perform arbitrary actions as the caller.  I
described some challenges then, but a deeper look turned up more:  a SECURITY
DEFINER function can do things like "SET search_path = ..." that affect the
caller.  Consequently, I concluded that all roles on the active call stack
should have a stake in whether a particular function owner is permitted.  Each
trust check walks the call stack and checks every role represented there; if
any lacks the trust relationships to approve the newly-contemplated function
call, the call is rejected.

The stack traversal may and does stop at the edge of a security-restricted
operation; since those restrictions must prevent important session changes,
the stack entries active before the operation started need not be concerned
with the code running therein.  A CREATE FUNCTION option specifying voluntary
use of a security-restricted context would provide the "facility for
requesting trust transitivity" mentioned earlier.

To make walking the role stack possible, I redid the API for changing the
current user ID.  SetUserIdAndSecContext() gives way to PushTransientUser()
and PopTransientUser(); xact.c has its own little API for unwinding this stack
at (sub)transaction abort.  This patch just removes the old APIs, but that
probably wouldn't cut it for the back branches; I do have a design sketch for
reintroducing backward-compatible versions.

9. Trust checks on other object types

Despite trust checks on functions, hostile users can make mischief with
something like "CREATE OPERATOR * (PROCEDURE = int4pl, LEFTARG = int4,
RIGHTARG = int4)".  Therefore, I also caused trust checks to be enforced on
operators themselves.  As I opine in the comments at owner_trustcheck(), where
to stop is more a judgement call than a science.


The following work remains TODO:
- Documentation for the new concepts, syntax and system catalog
- pg_dumpall support
- Testing the performance impact and perhaps adding a cache
- Backward-compatible versions of removed miscinit.c functions