Re: [HACKERS] proposal: schema variables

2018-08-23 10:17 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>:

>
> Hello Pavel,
>
> 2. holding some session based informations, that can be used in security
>> definer functions.
>>
>
> Hmmm, I see our disagreement. My point is that this feature is *NOT* fit
> for security-related uses because if the transaction fails the variable
> would keep the value it had if the transaction had not failed...
>
> 3. Because it is not transactional, then it allows write operation on read
>>
>
> It is not transactional safe, but it is secure in sense a possibility to
>> set a access rights.
>>
>
> This is a misleading play on words. It is secure wrt to access right, but
> unsecure wrt security purposes which is the only point for having such a
> feature in the first place.
>
> I understand, so some patterns are not possible, but when you need hold
>> some keys per session, then this simply solution can be good enough.
>>
>
> Security vs "good enough in some cases" looks bad to me.
>

We don't find a agreement, because you are concentrated on transation, me
on session. And we have different expectations.

> I think it is possible for some more complex patterns,
>>
>
> I'm not sure of any pattern which would be correct wrt security if it
> depends on the success of a transaction.
>
> but then developer should be smarter, and should to enocode state result
>> to content of variable.
>>
>
> I do not see how the developer can be smarter if they need a transactional
> for security but they do not have it.
>
> There is strong benefit - read write access to variables is very cheap and
>> fast.
>>
>
> I'd say that PostgreSQL is about "ACID & security" first, not "cheap &
> fast" first.
>
> I invite any patch to doc (or everywhere) with explanation and about
>> possible risks.
>>
>
> Hmmm... You are the one proposing the feature...
>
> Here is something, thanks for adjusting it to the syntax you are proposing
> and inserting it where appropriate. Possibly in the corresponding CREATE
> doc?
>
> """
> <caution>
> <par>
> Beware that session variables are not transactional.
> This is a concern in a security context where the variable must be set to
> some trusted value depending on the success of the transaction:
> if the transaction fails, the variable keeps its trusted value unduly.
> </par>
>
> <par>
> For instance, the following pattern does NOT work:
>
> <programlisting>
> CREATE USER auditer;
> SET ROLE auditer;
> CREATE SESSION VARIABLE is_audited BOOLEAN DEFAULT FALSE ...;
> -- ensure that only "auditer" can write "is_audited":
> REVOKE ... ON SESSION VARIABLE is_audited FROM ...;
>
> -- create an audit function
> CREATE FUNCTION audit_session(...) SECURITY DEFINER AS $$
> -- record the session and checks in some place...
> -- then tell it was done:
> LET is_audited = TRUE;
> $$;
>
> -- the intention is that other security definier functions can check that
> -- the session is audited by checking on "is_audited", eg:
> CREATE FUNCTION only_for_audited(...) SECURITY DEFINER AS $$
> IF NOT is_audited THEN RAISE "security error";
> -- do protected stuff here.
> $$;
> </programlisting>
>
> The above pattern can be attacked with the following approach:
> <programlisting>
> BEGIN;
> SELECT audit_session(...);
> -- success, "is_audited" is set...
> ROLLBACK;
> -- the audit login has been reverted, but "is_audited" retains its value.
>
> -- any subsequent operation believes wrongly that the session is audited,
> -- but its logging has really been removed by the ROLLBACK.
>
> -- ok but should not:
> SELECT only_for_audited(...);
> </programlisting>
> </par>
> </caution>
> """
>
>
It is good example of not supported pattern. It is not designed for this.
I'll merge this doc.

Note: I am not sure, if I have all relations to described issue, but if I
understand well, then solution can be reset on transaction end, maybe reset
on rollback. This is solvable, I'll look how it is complex.

>
> For the record, I'm "-1" on this feature as proposed, for what it's worth,
> because of the misleading security implications. This feature would just
> help people have their security wrong.
>

I respect your opinion - and I hope so integration of your proposed doc is
good warning for users that would to use not transactional variable like
transactional source.

Regards

Pavel

>
> --
> Fabien.
>
>