Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fslEf-0005rD-4E for pgsql-hackers@arkaria.postgresql.org; Thu, 23 Aug 2018 08:44:57 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fslEd-00086p-KS for pgsql-hackers@arkaria.postgresql.org; Thu, 23 Aug 2018 08:44:55 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fslEd-00086d-64 for pgsql-hackers@lists.postgresql.org; Thu, 23 Aug 2018 08:44:55 +0000 Received: from mail-wm0-x22d.google.com ([2a00:1450:400c:c09::22d]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fslEa-0000f9-UD for pgsql-hackers@lists.postgresql.org; Thu, 23 Aug 2018 08:44:54 +0000 Received: by mail-wm0-x22d.google.com with SMTP id o18-v6so4937689wmc.0 for ; Thu, 23 Aug 2018 01:44:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Ba4aoqKqOqj/6JZ+npitsR+NVhQZ7eLV6qsQc6gKrdk=; b=m+CqZDea5I/p/ve/AWI30sl3NFvd1Aj3dhC9Otl9MYTB9TGN/s7B8I/eKLHoCz8dt9 xomy9Luxkao/Pdtbv29ZBLsjcLLbmIzAXmh7wKYSKRG4v5vcuuP/m3480lHa/Kjjk/Eb zimcFwDZdC2Plrsp3csbp/3wv+28jGokfZU1xUmnOnI+T9HmoMLB1mNpFeYAS3E10cRz QevwzZppQcEB+mmgZ6dz/zVj3bduMCC+oiUHixzskx6IXY7GWfNNXRwNyBQP3efbyLiL 6wJZ8CAsRFDQlpEfo3RQ/jkWZ1O4DZcyLXduN2Ij85erMU/h6i+s8VfnR387kUhF/O7G BlfA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Ba4aoqKqOqj/6JZ+npitsR+NVhQZ7eLV6qsQc6gKrdk=; b=uGrmyI68eMIkev5qs6LAFvK4K5tgf8OYvAGbnlqsWPgtY+Kh2JGiWxU4l4l1ktE3rk PKlBcqbL7/Eb08bSYWXjOfddVFmydPQqNXWRlm7nD6Q/CcedfPszv3cHD9IUsUD2ZVOG XJhQgGdjbjB/yAo05+uwtKsXmb6Fs9r53I3aW2fq7gx8ru0E14zBWvykjJMlTxCDqGqb M0C34a41PHSkYjZa4zdJcChSpuE4o5g0DKAJdks4DiFrpggaTilsWfvLYEVwerNwr7Nt xElp8Ize7W3J2CeO911Bps/uHcHzXRM1wHTIynDB6KEHcAuKNQhNIHyi7WXaQ+Uea6ys laKw== X-Gm-Message-State: APzg51CyP7thcYm8ZKB0VE3sEmYC7EhKlDFpxsNDr7kq9280oohAo0r5 MZc7fFYTyVwBINKTPJIeKcmDRtPHO5Bd0Z0ez/shDQ== X-Google-Smtp-Source: ANB0VdZ/gwoM0ozJ1Ic7MDIAao9aTF5c0ta2n7b2Pk70q4ZEyOTw8QI92h9219zTYDo7hci9Plv0B0pZ5POOkvTLcuU= X-Received: by 2002:a1c:3411:: with SMTP id b17-v6mr4427440wma.85.1535013890525; Thu, 23 Aug 2018 01:44:50 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a5d:428a:0:0:0:0:0 with HTTP; Thu, 23 Aug 2018 01:44:10 -0700 (PDT) In-Reply-To: References: <623395617.20171113141500@gf.microolap.com> <28924bcc-9242-9798-e4e8-9d83cea3fef6@dalibo.com> From: Pavel Stehule Date: Thu, 23 Aug 2018 10:44:10 +0200 Message-ID: Subject: Re: [HACKERS] proposal: schema variables To: Fabien COELHO Cc: Gilles Darold , PostgreSQL Hackers Content-Type: multipart/alternative; boundary="000000000000ab8d2d057416456a" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000ab8d2d057416456a Content-Type: text/plain; charset="UTF-8" 2018-08-23 10:17 GMT+02:00 Fabien COELHO : > > 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? > > """ > > > 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. > > > > For instance, the following pattern does NOT work: > > > 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. > $$; > > > The above pattern can be attacked with the following approach: > > 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(...); > > > > """ > > 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. > > --000000000000ab8d2d057416456a Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


2018-08-23 10:17 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr&g= t;:

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 fo= r 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<= br>

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 u= nsecure wrt security purposes which is the only point for having such a fea= ture in the first place.

I understand, so some patterns are not possible, but when you need hold som= e 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 agr= eement, because you are concentrated on transation, me on session. And we h= ave 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 d= epends 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 do= c?

"""
<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 $$
=C2=A0 -- record the session and checks in some place...
=C2=A0 -- then tell it was done:
=C2=A0 LET is_audited =3D 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 $$
=C2=A0 IF NOT is_audited THEN RAISE "security error";
=C2=A0 -- 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 it= s 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 pa= ttern. It is not designed for this. I'll merge this doc.

Note: I am not sure, if I have all relations to described i= ssue, but if I understand well, then solution can be reset on transaction e= nd, maybe reset on rollback. This is solvable, I'll look how it is comp= lex.
=C2=A0

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

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

Regards

=
Pavel



--
Fabien.


--000000000000ab8d2d057416456a--