Re: simplifying foreign key/RI checks - Mailing list pgsql-hackers

On Mon, Mar 8, 2021 at 11:41 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Mar 4, 2021 at 5:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >  I guess I'm disturbed by the idea
> > that we'd totally replace the implementation technology for only one
> > variant of foreign key checks.  That means that there'll be a lot
> > of minor details that don't act the same depending on context.  One
> > point I was just reminded of by [1] is that the SPI approach enforces
> > permissions checks on the table access, which I do not see being done
> > anywhere in your patch.  Now, maybe it's fine not to have such checks,
> > on the grounds that the existence of the RI constraint is sufficient
> > permission (the creator had to have REFERENCES permission to make it).
> > But I'm not sure about that.  Should we add SELECT permissions checks
> > to this code path to make it less different?
> >
> > In the same vein, the existing code actually runs the query as the
> > table owner (cf. SetUserIdAndSecContext in ri_PerformCheck), another
> > nicety you haven't bothered with.  Maybe that is invisible for a
> > pure SELECT query but I'm not sure I would bet on it.  At the very
> > least you're betting that the index-related operators you invoke
> > aren't going to care, and that nobody is going to try to use this
> > difference to create a security exploit via a trojan-horse index.
>
> How about we do at the top of ri_ReferencedKeyExists() what
> ri_PerformCheck() always does before executing a query, which is this:
>
>    /* Switch to proper UID to perform check as */
>    GetUserIdAndSecContext(&save_userid, &save_sec_context);
>    SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner,
>                           save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
>                           SECURITY_NOFORCE_RLS);
>
> And then also check the permissions of the switched user on the scan
> target relation's schema (ACL_USAGE) and the relation itself
> (ACL_SELECT).
>
> IOW, this:
>
> +   Oid         save_userid;
> +   int         save_sec_context;
> +   AclResult   aclresult;
> +
> +   /* Switch to proper UID to perform check as */
> +   GetUserIdAndSecContext(&save_userid, &save_sec_context);
> +   SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
> +                          save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
> +                          SECURITY_NOFORCE_RLS);
> +
> +   /* Check namespace permissions. */
> +   aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel),
> +                                     GetUserId(), ACL_USAGE);
> +   if (aclresult != ACLCHECK_OK)
> +       aclcheck_error(aclresult, OBJECT_SCHEMA,
> +                      get_namespace_name(RelationGetNamespace(pk_rel)));
> +   /* Check the user has SELECT permissions on the referenced relation. */
> +   aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(),
> +                                 ACL_SELECT);
> +   if (aclresult != ACLCHECK_OK)
> +       aclcheck_error(aclresult, OBJECT_TABLE,
> +                      RelationGetRelationName(pk_rel));
>
>     /*
>      * Extract the unique key from the provided slot and choose the equality
> @@ -414,6 +436,9 @@ ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel,
>     index_endscan(scan);
>     ExecDropSingleTupleTableSlot(outslot);
>
> +   /* Restore UID and security context */
> +   SetUserIdAndSecContext(save_userid, save_sec_context);
> +
>     /* Don't release lock until commit. */
>     index_close(idxrel, NoLock);

I've included these changes in the updated patch.

> > Shall we mention RLS restrictions?  If we don't worry about that,
> > I think REFERENCES privilege becomes a full bypass of RLS, at
> > least for unique-key columns.
>
> Seeing what check_enable_rls() does when running under the security
> context set by ri_PerformCheck(), it indeed seems that RLS is bypassed
> when executing these RI queries.  The following comment in
> check_enable_rls() seems to say so:
>
>          * InNoForceRLSOperation indicates that we should not apply RLS even
>          * if the table has FORCE RLS set - IF the current user is the owner.
>          * This is specifically to ensure that referential integrity checks
>          * are able to still run correctly.

I've added a comment to note that the new way of "selecting" the
referenced tuple effectively bypasses RLS, as is the case when
selecting via SPI.

> > I wonder also what happens if the referenced table isn't a plain
> > heap with a plain btree index.  Maybe you're accessing it at the
> > right level of abstraction so things will just work with some
> > other access methods, but I'm not sure about that.
>
> I believe that I've made ri_ReferencedKeyExists() use the appropriate
> APIs to scan the index, lock the returned table tuple, etc., but do
> you think we might be better served by introducing a new set of APIs
> for this use case?

I concur that by using the interfaces defined in genam.h and
tableam.h, patch accounts for cases involving other access methods.

That said, I had overlooked one bit in the new code that is specific
to btree AM, which is the hard-coding of BTEqualStrategyNumber in the
following:

        /* Initialize the scankey. */
        ScanKeyInit(&skey[i],
                    pkattno,
                    BTEqualStrategyNumber,
                    regop,
                    pk_vals[i]);

In the updated patch, I've added code to look up the index-specific
strategy number to pass here.

> > Lastly, ri_PerformCheck is pretty careful about not only which
> > snapshot it uses, but which *pair* of snapshots it uses, because
> > sometimes it needs to worry about data changes since the start
> > of the transaction.  You've ignored all of that complexity AFAICS.
> > That's okay (I think) for RI_FKey_check which was passing
> > detectNewRows = false, but for sure it's not okay for
> > ri_Check_Pk_Match.  (I kind of thought we had isolation tests
> > that would catch that, but apparently not.)
>
> Okay, let me closely check the ri_Check_Pk_Match() case and see if
> there's any live bug.

As mentioned in my earlier reply, there doesn't seem to be a need for
ri_Check_Pk_Match() to set the crosscheck snapshot as it is basically
unused.

> > So, this is a cute idea, and the speedup is pretty impressive,
> > but I don't think it's anywhere near committable.  I also wonder
> > whether we really want ri_triggers.c having its own copy of
> > low-level stuff like the tuple-locking code you copied.  Seems
> > like a likely maintenance hazard, so maybe some more refactoring
> > is needed.
>
> Okay, I will see if there's a way to avoid copying too much code.

I thought sharing the tuple-locking code with ExecLockRows(), which
seemed closest in semantics to what the new code is doing, might not
be such a bad idea, but not sure I came up with a great interface for
the shared function.  Actually, there are other places having their
own copies of tuple-locking logic, but they deal with the locking
result in their own unique ways, so I didn't get excited about finding
a way to make the new function accommodate their needs.  I also admit
that I may have totally misunderstood what refactoring you were
referring to in your comment.

Updated patches attached.  Sorry about the delay.

--
Amit Langote
EDB: http://www.enterprisedb.com