Re: Row Level Security − leakproof-ness and performance implications

On Wednesday, February 20, 2019 12:43:50 AM CET Laurenz Albe wrote:
> Pierre Ducroquet wrote:
> > In order to increase our security, we have started deploying row-level
> > security in order to add another safety net if any issue was to happen in
> > our applications.
> > After a careful monitoring of our databases, we found out that a lot of
> > queries started to go south, going extremely slow.
> > The root of these slowdowns is that a lot of the PostgreSQL functions are
> > not marked as leakproof, especially the ones used for operators.
> > In current git master, the following query returns 258 functions that are
> > used by operators returning booleans and not marked leakproof:
> >
> > SELECT proname FROM pg_proc
> >
> > WHERE exists(select 1 from pg_operator where oprcode::name =
> > proname)
> > AND NOT(proleakproof)
> > AND prorettype = 16;
> >
> > Among these, if you have for instance a table like:
> > create table demo_enum (
> >
> > username text,
> > flag my_enum
> >
> > );
> >
> > With an index on (username, flag), the index will only be used on username
> > because flag is an enum and the enum_eq function is not marked leakproof.
> >
> > For simple functions like enum_eq/enum_ne, marking them leakproof is an
> > obvious fix (patch attached to this email, including also textin/textout).
> > And
> The enum_eq part looks totally safe, but the text functions allocate memory,
> so you could create memory pressure, wait for error messages that tell you
> the size of the allocation that failed and this way learn about the data.
>
> Is this a paranoid idea?

This is not paranoid, it depends on your threat model.
In the model we implemented, the biggest threat we consider from an user point
of view is IDOR (Insecure Direct Object Reference): a developer forgetting to
check that its input is sane and matches the other parts of the URL or the
current session user. Exploiting the leak you mentioned in such a situation is
almost impossible, and to be honest the attacker has much easier targets if he
gets to that level…
Maybe leakproof is too simple? Should we be able to specify a 'paranoid-level'
to allow some leaks depending on our threat model?

> > if we had a 'RLS-enabled' context on functions, a way to make a lot of
> > built- in functions leakproof would simply be to prevent them from
> > logging errors containing values.
> >
> > For others, like arraycontains, it's much trickier :[...]
>
> I feel a little out of my depth here, so I won't comment.
>
> > A third issue we noticed is the usage of functional indexes. If you create
> > an index on, for instance, (username, leaky_function(my_field)), and then
> > search on leaky_functon(my_field) = 42, the functional index can be used
> > only if leaky_function is marked leakproof, even if it is never going to
> > be executed on invalid rows thanks to the index. After a quick look at
> > the source code, it also seems complicated to implement since the
> > decision to reject potential dangerous calls to leaky_function is done
> > really early in the process, before the optimizer starts.
>
> If there is a bitmap index scan, the condition will be rechecked, so the
> function will be executed.

That's exactly my point: using a bitmap index scan would be dangerous and thus
should not be allowed, but an index scan works fine. Or the recheck on bitmap
scan must first check the RLS condition before doing its check.

> > I am willing to spend some time on these issues, but I have no specific
> > knowledge of the planner and optimizer, and I fear proper fixes would
> > require much digging there. If you think it would be appropriate for
> > functions like arraycontains or range_contains to require the 'inner'
> > comparison function to be leakproof, or just keep looking at the other
> > functions in pg_proc and leakproof the ones that can be, I would be happy
> > to write the corresponding patches.
>
> Thanks, and I think that every function that can safely be marked leakproof
> is a progress!

Thank you for your comments