Re: revamp row-security tracking

On Thu, 21 Nov 2024 at 18:00, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> In light of CVE-2024-10976, which was fixed by commit cd7ab57, I'd like to
> propose a bigger change to this area of the code that aims to future-proof
> it a bit. Instead of requiring hackers to carefully cart around whether a
> query references a table with RLS enabled, I think we should instead
> accumulate such information globally and require higher-level routines like
> fireRIRrules() and inline_set_returning_function() to inspect it as needed.
>
> The attached patch accomplishes this by establishing a global queue of
> row-security "nest levels" that the aforementioned higher-level callers can
> use.

I'm not convinced that this is an improvement.

The code in check_sql_fn_retval() is building a Query struct from
scratch, so it seems perfectly natural for it to be responsible for
setting all the required fields, based on the information it has
available. With this patch, check_sql_fn_retval() is returning a
potentially incorrectly marked Query at the end of the querytree list,
which the caller is responsible for fixing up, which doesn't seem
ideal.

I'm also not a fan of using global variables in this way, or the
resulting need to hook into the transaction management system to tidy
up. The end result is that the places where the flag is set are moved
further away from where RLS policies are applied, which IMO makes the
code much harder to follow.

There is exactly one place where RLS policies are applied, and it
seems much more natural for it to have responsibility for setting this
flag. I think that a slightly neater way for it to handle that would
be to modify fireRIRrules(), adding an extra parameter "bool
*hasRowSecurity" that it would set to true if RLS is enabled for the
query it is rewriting. Doing that forces all callers to think about
whether or not that affects some outer query. For example,
ApplyRetrieveRule() would then do:

rule_action = fireRIRrules(rule_action, activeRIRs,
&parsetree->hasRowSecurity);

rather than having a separate second step to update the flag on
"parsetree", and similarly for fireRIRrules()'s recursive calls to
itself. If, in the future, it becomes necessary to invoke
fireRIRrules() on more parts of a Query, it's then much more likely
that the new code won't forget to update the parent query's flag.

Regards,
Dean