[v9.2] Fix Leaky View Problem

On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:

The (2) is new stuff from the revision in commit-fest 1st. It enables to
supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
allowed to distribute across security barrier. Only superuser can set this
option.

"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
Also, it could be read as "Don't allow leaks in this function". Could we
instead use something like TRUSTED or something akin to it being allowed to
do more than safer functions? It then describes its level of behaviour
rather than what it promises not to do.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2011/9/7 Thom Brown <thom@linux.com>:

On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:

The (2) is new stuff from the revision in commit-fest 1st. It enables to
supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
allowed to distribute across security barrier. Only superuser can set this
option.

"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
 Also, it could be read as "Don't allow leaks in this function".  Could we
instead use something like TRUSTED or something akin to it being allowed to
do more than safer functions?  It then describes its level of behaviour
rather than what it promises not to do.

Thanks for your comment. I'm not a native English specker, so it is helpful.

"TRUSTED" sounds meaningful for me, however, it is confusable with a concept
of "trusted procedure" in label-based MAC. It is not only SELinux,
Oracle's label
based security also uses this term to mean a procedure that switches user's
credential during its execution.
http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm

So, how about "CREDIBLE", instead of "TRUSTED"?

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2011/9/7 Thom Brown <thom@linux.com>:

On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:

The (2) is new stuff from the revision in commit-fest 1st. It enables to
supply "NOLEAKY" option on CREATE FUNCTION statement, then the function

is

allowed to distribute across security barrier. Only superuser can set

this

option.

"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin

English.

Also, it could be read as "Don't allow leaks in this function". Could

we

instead use something like TRUSTED or something akin to it being allowed

to

do more than safer functions? It then describes its level of behaviour
rather than what it promises not to do.

Thanks for your comment. I'm not a native English specker, so it is
helpful.

"TRUSTED" sounds meaningful for me, however, it is confusable with a
concept
of "trusted procedure" in label-based MAC. It is not only SELinux,
Oracle's label
based security also uses this term to mean a procedure that switches user's
credential during its execution.

http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm

So, how about "CREDIBLE", instead of "TRUSTED"?

I can't say I'm keen on that alternative, but I'm probably not the one to
participate in bike-shedding here, so I'll leave comment to you hackers. :)

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote:

On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2011/9/7 Thom Brown <thom@linux.com>:

On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:

The (2) is new stuff from the revision in commit-fest 1st. It enables
to
supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
is
allowed to distribute across security barrier. Only superuser can set
this
option.

"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
English.
 Also, it could be read as "Don't allow leaks in this function".  Could
we
instead use something like TRUSTED or something akin to it being allowed
to
do more than safer functions?  It then describes its level of behaviour
rather than what it promises not to do.

Thanks for your comment. I'm not a native English specker, so it is
helpful.

"TRUSTED" sounds meaningful for me, however, it is confusable with a
concept
of "trusted procedure" in label-based MAC. It is not only SELinux,
Oracle's label
based security also uses this term to mean a procedure that switches
user's
credential during its execution.

 http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm

So, how about "CREDIBLE", instead of "TRUSTED"?

I can't say I'm keen on that alternative, but I'm probably not the one to
participate in bike-shedding here, so I'll leave comment to you hackers. :)

I think TRUSTED actually does a reasonably good job capturing what
we're after here, although I do share a bit of KaiGai's nervousness
about terminological confusion. Still, I'd be inclined to go that way
if we can't come up with anything better. CREDIBLE is definitely the
wrong idea: that means "believable", which sounds more like a
statement about the function's results than about its side-effects. I
thought about TACITURN, since we need the error messages to not be
excessively informative, but that doesn't do a good job characterizing
the hazard created by side-effects, or the potential for abuse due to
- for example - deliberate division by zero. I also thought about
PURE, which is a term that's sometimes used to describe code that
throws no errors and has no side effects, and comes pretty close to
our actual requirement here, but doesn't necessarily convey that a
security concern is involved. Yet another idea would be to use a
variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with
the idea of a trusted procedure, but I'm not that excited about that
idea despite have no real specific gripe with it other than length.
So at the moment I am leaning toward TRUSTED.

Anyone else want to bikeshed?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> wrote:

Anyone else want to bikeshed?

I'm not sure they beat TRUSTED, but:

SECURE
OPAQUE

-Kevin

2011/9/7 Robert Haas <robertmhaas@gmail.com>:

On Wed, Sep 7, 2011 at 9:39 AM, Thom Brown <thom@linux.com> wrote:

On 7 September 2011 14:34, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

2011/9/7 Thom Brown <thom@linux.com>:

On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:

The (2) is new stuff from the revision in commit-fest 1st. It enables
to
supply "NOLEAKY" option on CREATE FUNCTION statement, then the function
is
allowed to distribute across security barrier. Only superuser can set
this
option.

"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin
English.
 Also, it could be read as "Don't allow leaks in this function".  Could
we
instead use something like TRUSTED or something akin to it being allowed
to
do more than safer functions?  It then describes its level of behaviour
rather than what it promises not to do.

Thanks for your comment. I'm not a native English specker, so it is
helpful.

"TRUSTED" sounds meaningful for me, however, it is confusable with a
concept
of "trusted procedure" in label-based MAC. It is not only SELinux,
Oracle's label
based security also uses this term to mean a procedure that switches
user's
credential during its execution.

 http://download.oracle.com/docs/cd/B28359_01/network.111/b28529/storproc.htm

So, how about "CREDIBLE", instead of "TRUSTED"?

I can't say I'm keen on that alternative, but I'm probably not the one to
participate in bike-shedding here, so I'll leave comment to you hackers. :)

I think TRUSTED actually does a reasonably good job capturing what
we're after here, although I do share a bit of KaiGai's nervousness
about terminological confusion.  Still, I'd be inclined to go that way
if we can't come up with anything better.  CREDIBLE is definitely the
wrong idea: that means "believable", which sounds more like a
statement about the function's results than about its side-effects.  I
thought about TACITURN, since we need the error messages to not be
excessively informative, but that doesn't do a good job characterizing
the hazard created by side-effects, or the potential for abuse due to
- for example - deliberate division by zero.  I also thought about
PURE, which is a term that's sometimes used to describe code that
throws no errors and has no side effects, and comes pretty close to
our actual requirement here, but doesn't necessarily convey that a
security concern is involved.  Yet another idea would be to use a
variant of TRUSTED, such as TRUSTWORTHY, just to avoid confusion with
the idea of a trusted procedure, but I'm not that excited about that
idea despite have no real specific gripe with it other than length.
So at the moment I am leaning toward TRUSTED.

Anyone else want to bikeshed?

I also become leaning toward TRUSTED, although we still have a bit risk of
terminology confusion, because I assume it is quite rare case to set this
option by DBA and we will able to expect DBAs who try to this option have
correct knowledge about background of the leaky-view problem.

I'll submit the patch again.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On 2011-09-07 16:02, Kevin Grittner wrote:

Robert Haas<robertmhaas@gmail.com> wrote:

Anyone else want to bikeshed?

I'm not sure they beat TRUSTED, but:

SECURE
OPAQUE

SAVE

-- Yeb

On Wed, Sep 07, 2011 at 02:09:15PM +0100, Thom Brown wrote:

On 24 August 2011 13:38, Kohei Kaigai <Kohei.Kaigai@emea.nec.com> wrote:

The (2) is new stuff from the revision in commit-fest 1st. It enables to
supply "NOLEAKY" option on CREATE FUNCTION statement, then the function is
allowed to distribute across security barrier. Only superuser can set this
option.

"NOLEAKY" doesn't really sound appropriate as it sounds like pidgin English.
Also, it could be read as "Don't allow leaks in this function". Could we
instead use something like TRUSTED or something akin to it being allowed to
do more than safer functions? It then describes its level of behaviour
rather than what it promises not to do.

I liked NOLEAKY for its semantics, though I probably would have spelled it
"LEAKPROOF". PostgreSQL will trust the function to implement a specific,
relatively-unintuitive security policy. We want the function implementers to
read that policy closely and not rely on any intuition they have about the
"trusted" term of art. Our use of TRUSTED in CREATE LANGUAGE is more
conventional, I think, as is the trusted nature of SECURITY DEFINER. In that
vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
NOLEAKY would not attract the same unwarranted attention.

Noah Misch <noah@leadboat.com> writes:

I liked NOLEAKY for its semantics, though I probably would have spelled it
"LEAKPROOF". PostgreSQL will trust the function to implement a specific,
relatively-unintuitive security policy. We want the function implementers to
read that policy closely and not rely on any intuition they have about the
"trusted" term of art. Our use of TRUSTED in CREATE LANGUAGE is more
conventional, I think, as is the trusted nature of SECURITY DEFINER. In that
vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
NOLEAKY would not attract the same unwarranted attention.

I agree that TRUSTED is a pretty bad choice here because of the high
probability that people will think it means something else than what
it really means. LEAKPROOF isn't too bad.

regards, tom lane

2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>:

Noah Misch <noah@leadboat.com> writes:

I liked NOLEAKY for its semantics, though I probably would have spelled it
"LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
relatively-unintuitive security policy.  We want the function implementers to
read that policy closely and not rely on any intuition they have about the
"trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
NOLEAKY would not attract the same unwarranted attention.

I agree that TRUSTED is a pretty bad choice here because of the high
probability that people will think it means something else than what
it really means.  LEAKPROOF isn't too bad.

It seems to me LEAKPROOF is never confusable for everyone, and
no conflicts with other concept, although it was not in my vocaburary.

If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On 09/07/2011 12:05 PM, Tom Lane wrote:

I agree that TRUSTED is a pretty bad choice here because of the high
probability that people will think it means something else than what
it really means.

Agreed.

LEAKPROOF isn't too bad.

It's fairly opaque unless you know the context, although that might be
said of some of our other terms too. Someone coming across it is going
to think "What would it leak?"

cheers

andrew

2011/9/7 Andrew Dunstan <andrew@dunslane.net>:

LEAKPROOF isn't too bad.

It's fairly opaque unless you know the context, although that might be said
of some of our other terms too. Someone coming across it is going to think
"What would it leak?"

It is introduced in the documentation. I'll add a point to this
chapter in the introduction of this keyword.

http://developer.postgresql.org/pgdocs/postgres/rules-privileges.html

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Andrew Dunstan <andrew@dunslane.net> writes:

On 09/07/2011 12:05 PM, Tom Lane wrote:

LEAKPROOF isn't too bad.

It's fairly opaque unless you know the context, although that might be
said of some of our other terms too. Someone coming across it is going
to think "What would it leak?"

Well, the whole point is that we want people to RTFM instead of assuming
they know what it means ...

regards, tom lane

I updated the patches of fix-leaky-view problem, according to the
previous discussion.
The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
test cases were added. Rest of stuffs are unchanged.

For convenience of reviewer, below is summary of these patches:

The Part-1 implements corresponding SQL syntax stuffs which are
"security_barrier"
reloption of views, and "LEAKPROOF" option on creation of functions to be stored
new pg_proc.proleakproof field.

The Part-2 tries to tackles a leaky-view scenarios by functions with
very tiny cost
estimation value. It was same one we had discussed in the commitfest-1st.
It prevents to launch functions earlier than ones come from inside of views with
"security_barrier" option.

The Part-3 tries to tackles a leaky-view scenarios by functions that references
one side of join loop. It prevents to distribute qualifiers including
functions without
"leakproof" attribute into relations across security-barrier.

Thanks,

2011/9/7 Kohei KaiGai <kaigai@kaigai.gr.jp>:

2011/9/7 Tom Lane <tgl@sss.pgh.pa.us>:

Noah Misch <noah@leadboat.com> writes:

I liked NOLEAKY for its semantics, though I probably would have spelled it
"LEAKPROOF".  PostgreSQL will trust the function to implement a specific,
relatively-unintuitive security policy.  We want the function implementers to
read that policy closely and not rely on any intuition they have about the
"trusted" term of art.  Our use of TRUSTED in CREATE LANGUAGE is more
conventional, I think, as is the trusted nature of SECURITY DEFINER.  In that
vein, folks who actually need SECURITY DEFINER might first look at TRUSTED;
NOLEAKY would not attract the same unwarranted attention.

I agree that TRUSTED is a pretty bad choice here because of the high
probability that people will think it means something else than what
it really means.  LEAKPROOF isn't too bad.

It seems to me LEAKPROOF is never confusable for everyone, and
no conflicts with other concept, although it was not in my vocaburary.

If no better idea anymore, I'll submit the patch again; with LEAKPROOF keyword.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

--
KaiGai Kohei <kaigai@kaigai.gr.jp>

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

I updated the patches of fix-leaky-view problem, according to the
previous discussion.
The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
test cases were added. Rest of stuffs are unchanged.

You have a leftover reference to NOLEAKY.

For convenience of reviewer, below is summary of these patches:

The Part-1 implements corresponding SQL syntax stuffs which are
"security_barrier"
reloption of views, and "LEAKPROOF" option on creation of functions to be stored
new pg_proc.proleakproof field.

The way you have this implemented, we just blow away all view options
whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want?
If a security_barrier view gets accidentally turned into a
non-security_barrier view, doesn't that create a security_hole?

I'm also wondering if the way you're using ResetViewOptions() is the
right way to handle this anyhow. Isn't that going to update pg_class
twice? I guess that's probably harmless from a performance
standpoint, but wouldn't it be better not to? I guess we could define
something like AT_ReplaceRelOptions to handle this case.

The documentation in general is not nearly adequate, at least IMHO.

I'm a bit nervous about storing security_barrier in the RTE. What
happens to stored rules if the security_barrier option gets change
later?

More when I've had more time to look at this...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote:

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The Part-1 implements corresponding SQL syntax stuffs which are
"security_barrier"
reloption of views, and "LEAKPROOF" option on creation of functions to be stored
new pg_proc.proleakproof field.

The way you have this implemented, we just blow away all view options
whenever we do CREATE OR REPLACE VIEW. Is that the behavior we want?
If a security_barrier view gets accidentally turned into a
non-security_barrier view, doesn't that create a security_hole?

I think CREATE OR REPLACE needs to keep meaning just that, never becoming
"replace some characteristics, merge others". The consequence is less than
delightful here, but I don't have an idea that avoids this problem without
running afoul of some previously-raised design constraint.

On Sat, Sep 24, 2011 at 5:37 PM, Noah Misch <noah@leadboat.com> wrote:

On Fri, Sep 23, 2011 at 06:25:01PM -0400, Robert Haas wrote:

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

The Part-1 implements corresponding SQL syntax stuffs which are
"security_barrier"
reloption of views, and "LEAKPROOF" option on creation of functions to be stored
new pg_proc.proleakproof field.

The way you have this implemented, we just blow away all view options
whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
If a security_barrier view gets accidentally turned into a
non-security_barrier view, doesn't that create a security_hole?

I think CREATE OR REPLACE needs to keep meaning just that, never becoming
"replace some characteristics, merge others".  The consequence is less than
delightful here, but I don't have an idea that avoids this problem without
running afoul of some previously-raised design constraint.

Hmm, you might be right, although I'm not sure we've been 100%
consistent about that, since we previously made CREATE OR REPLACE
LANGUAGE not replace the owner with the current user.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas 09/25/11 10:58 AM >>>

I'm not sure we've been 100% consistent about that, since we
previously made CREATE OR REPLACE LANGUAGE not replace the owner
with the current user.

I think we've been consistent in *not* changing security on an
object when it is replaced.

test=# create user someoneelse;
CREATE ROLE
test=# create user yetanother;
CREATE ROLE
test=# create function one() returns int language sql as 'select 1;';
CREATE FUNCTION
test=# alter function one() owner to someoneelse;
ALTER FUNCTION
test=# revoke execute on function one() from public;
REVOKE
test=# create or replace function one() returns int language plpgsql as
$$begin return 1; end;$$;
CREATE FUNCTION
test=# \df+ one()
List of
functions
Schema | Name | Result data type | Argument data types | Type |
Volatility | Owner | Language | Source code | Description

--------+------+------------------+---------------------+--------+------------+-------------+----------+----------------------+-------------
public | one | integer | | normal |
volatile | someoneelse | plpgsql | begin return 1; end; |
(1 row)

test=# set role yetanother;
SET
test=> select one();
ERROR: permission denied for function one

-Kevin

2011/9/24 Robert Haas <robertmhaas@gmail.com>:

On Mon, Sep 12, 2011 at 3:31 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:

I updated the patches of fix-leaky-view problem, according to the
previous discussion.
The "NOLEAKY" option was replaced by "LEAKPROOF" option, and several regression
test cases were added. Rest of stuffs are unchanged.

You have a leftover reference to NOLEAKY.

Oops, I'll fix it.

For convenience of reviewer, below is summary of these patches:

The Part-1 implements corresponding SQL syntax stuffs which are
"security_barrier"
reloption of views, and "LEAKPROOF" option on creation of functions to be stored
new pg_proc.proleakproof field.

The way you have this implemented, we just blow away all view options
whenever we do CREATE OR REPLACE VIEW.  Is that the behavior we want?
If a security_barrier view gets accidentally turned into a
non-security_barrier view, doesn't that create a security_hole?

I'm also wondering if the way you're using ResetViewOptions() is the
right way to handle this anyhow.  Isn't that going to update pg_class
twice?  I guess that's probably harmless from a performance
standpoint, but wouldn't it be better not to?  I guess we could define
something like AT_ReplaceRelOptions to handle this case.

IIRC, we had a discussion that the behavior should follow the case
when a view is newly defined, even if it would be replaced actually.
However, it seems to me consistent way to keep existing setting
as long as user does not provide new option explicitly.
If so, I think AT_ReplaceRelOptions enables to simplify the code
to implement such a behavior.

The documentation in general is not nearly adequate, at least IMHO.

Do you think the description is poor to introduce the behavior changes
corresponding to security_barrier option?
OK, I'll try to update the documentation.

I'm a bit nervous about storing security_barrier in the RTE.  What
happens to stored rules if the security_barrier option gets change
later?

The rte->security_barrier is evaluated when a query referencing security
views get expanded. So, rte->security_barrier is not stored to catalog.
Even if security_barrier option was changed after PREPARE statement
with references to security view, our existing mechanism re-evaluate
the query on EXECUTE, thus, it shall be executed as we expected.
(As an aside, I didn't know this mechanism and surprised at EXECUTE
works correctly, even if VIEW definition was changed after PREPARE.)

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>