Re: SECURITY LABEL on shared database object

I signed up to do a review on $subject patch for the commitfest. In
order to do that, I want to get SELinux and contrib/sepgsql properly set
up so that I can test. I ran into a problem when trying to do:

cd contrib/sepgsql
make install (succeeds)
make installcheck (fails)

I get this:

============== creating database "contrib_regression" ==============
ERROR: could not determine which collation to use for string
comparison
HINT: Use the COLLATE clause to set the collation explicitly.
command failed: "/usr/local/pgsql-head/bin/psql" -X -c "CREATE
DATABASE \"contrib_regression\" TEMPLATE=template0" "postgres"
make: *** [installcheck] Error 2

So I installed sepgsql into the postgres database anyway and do this:

postgres=# SELECT sepgsql_restorecon(NULL);
ERROR: could not determine which collation to use for string
comparison
HINT: Use the COLLATE clause to set the collation explicitly.

Ok, so now I go look at the docs to figure out what exactly a "COLLATE
clause" is. Only searching the online docs brings up no hits on the
keyword COLLATE". Google brings me to TODO wiki page:

http://wiki.postgresql.org/wiki/Todo:Collate

But that isn't much help either. Grepping the source gets hits in 9.1
and master. So I guess:

1) COLLATE clause is a new feature in 9.1?
2) The doc search feature on postgresql.org does not search the 9.1
documentation?

I looked in the 9.1 docs in SQL Commands->SELECT and could find no
reference to COLLATE. Can anyone point me to some documentation that
would explain what that error message means and how to resolve it?

Thanks,

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

On 06/29/2011 04:18 PM, Joe Conway wrote:
> 1) COLLATE clause is a new feature in 9.1?
> 2) The doc search feature on postgresql.org does not search the 9.1
> documentation?
>
> I looked in the 9.1 docs in SQL Commands->SELECT and could find no
> reference to COLLATE. Can anyone point me to some documentation that
> would explain what that error message means and how to resolve it?

A little more information. It seems that sepgsql_restorecon calls
GetSecurityLabel in the backend. Here's the backtrace:

8<-----------------
#0 errfinish (dummy=0) at elog.c:374
#1 0x00000000007b7c70 in varstr_cmp (arg1=0x7faeb724efa9 "selinux",
len1=7, arg2=0x16a1d8c "selinux~", len2=7, collid=0) at varlena.c:1312
#2 0x00000000007b7f01 in text_cmp (arg1=0x7faeb724efa8, arg2=0x16a1d88,
collid=0) at varlena.c:1468
#3 0x00000000007b8424 in bttextcmp (fcinfo=0x7ffff08e2b30) at
varlena.c:1610
#4 0x0000000000819c93 in FunctionCall2Coll (flinfo=0x7ffff08e30a0,
collation=0, arg1=140388373688232, arg2=23731592) at fmgr.c:1319
#5 0x000000000049a905 in _bt_compare (rel=0x7faeb6f71540, keysz=3,
scankey=0x7ffff08e3090, page=0x7faeb724cfc0 "", offnum=1) at nbtsearch.c:406
#6 0x000000000049a2c6 in _bt_binsrch (rel=0x7faeb6f71540, buf=74,
keysz=3, scankey=0x7ffff08e3000, nextkey=0 '\000') at nbtsearch.c:285
#7 0x000000000049b17b in _bt_first (scan=0x169ce28,
dir=ForwardScanDirection) at nbtsearch.c:877
#8 0x0000000000498971 in btgettuple (fcinfo=0x7ffff08e3af0) at nbtree.c:315
#9 0x0000000000819c93 in FunctionCall2Coll (flinfo=0x168e0d8,
collation=0, arg1=23711272, arg2=1) at fmgr.c:1319
#10 0x000000000048eeed in index_getnext (scan=0x169ce28,
direction=ForwardScanDirection) at indexam.c:487
#11 0x000000000048da81 in systable_getnext (sysscan=0x16a1020) at
genam.c:315
#12 0x00000000007fa322 in SearchCatCache (cache=0x1613700, v1=1,
v2=1262, v3=23731592, v4=0) at catcache.c:1201
#13 0x00000000008091c0 in SearchSysCache (cacheId=44, key1=1, key2=1262,
key3=23731592, key4=0) at syscache.c:859
#14 0x00000000005a7016 in GetSecurityLabel (object=0x7ffff08e43d0,
provider=0x7faeb9713830 "selinux") at seclabel.c:157
8<-----------------

The third key passed to SearchSysCache is CStringGetTextDatum(provider).
Ultimately FunctionCall2Coll gets called with collation == 0 and
varstr_cmp fails due to the ambiguity.

Is there something new that should be used in place of
CStringGetTextDatum that would convey a collation here?

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

On 06/29/2011 05:34 PM, Joe Conway wrote:
> The third key passed to SearchSysCache is CStringGetTextDatum(provider).
> Ultimately FunctionCall2Coll gets called with collation == 0 and
> varstr_cmp fails due to the ambiguity.
>
> Is there something new that should be used in place of
> CStringGetTextDatum that would convey a collation here?

(Sorry about replying to myself again...)

In fmgr.h we have:

8<-------------------------
/* These macros allow the collation argument to be omitted (with a
default of
* InvalidOid, ie, no collation). They exist mostly for backwards
* compatibility of source code.
*/
#define DirectFunctionCall1(func, arg1) \
DirectFunctionCall1Coll(func, InvalidOid, arg1)
#define DirectFunctionCall2(func, arg1, arg2) \
DirectFunctionCall2Coll(func, InvalidOid, arg1, arg2)
#define DirectFunctionCall3(func, arg1, arg2, arg3) \
DirectFunctionCall3Coll(func, InvalidOid, arg1, arg2, arg3)
[...]
8<--------------------------

Perhaps instead of InvalidOid here we should be using DEFAULT_COLLATION_OID?

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

Thanks for your reviewing, and Sorry for this debugging burden.

The origin of matter is, as you mentioned, collation to be used for system
catalog scan when we reference it via syscache.
So, the following chunk should be added, as I did in the userspace access
vector patch - part.1.

The attached patch is fixed version.

@@ -934,8 +935,7 @@ CatalogCacheInitializeCache(CatCache *cache)
/* Fill in sk_strategy as well --- always standard equality */
cache->cc_skey[i].sk_strategy = BTEqualStrategyNumber;
cache->cc_skey[i].sk_subtype = InvalidOid;
- /* Currently, there are no catcaches on collation-aware data types */
- cache->cc_skey[i].sk_collation = InvalidOid;
+ cache->cc_skey[i].sk_collation = DEFAULT_COLLATION_OID;

CACHE4_elog(DEBUG2, "CatalogCacheInitializeCache %s %d %p",
cache->cc_relname,

Thanks,

2011/6/30 Joe Conway <mail(at)joeconway(dot)com>:
> I signed up to do a review on $subject patch for the commitfest. In
> order to do that, I want to get SELinux and contrib/sepgsql properly set
> up so that I can test. I ran into a problem when trying to do:
>
>    cd contrib/sepgsql
>    make install                       (succeeds)
>    make installcheck                  (fails)
>
> I get this:
>
>    ============== creating database "contrib_regression" ==============
>    ERROR:  could not determine which collation to use for string
>    comparison
>    HINT:  Use the COLLATE clause to set the collation explicitly.
>    command failed: "/usr/local/pgsql-head/bin/psql" -X -c "CREATE
>    DATABASE \"contrib_regression\" TEMPLATE=template0" "postgres"
>    make: *** [installcheck] Error 2
>
> So I installed sepgsql into the postgres database anyway and do this:
>
>    postgres=# SELECT sepgsql_restorecon(NULL);
>    ERROR:  could not determine which collation to use for string
>    comparison
>    HINT:  Use the COLLATE clause to set the collation explicitly.
>
> Ok, so now I go look at the docs to figure out what exactly a "COLLATE
> clause" is. Only searching the online docs brings up no hits on the
> keyword COLLATE". Google brings me to TODO wiki page:
>
>    http://wiki.postgresql.org/wiki/Todo:Collate
>
> But that isn't much help either. Grepping the source gets hits in 9.1
> and master. So I guess:
>
> 1) COLLATE clause is a new feature in 9.1?
> 2) The doc search feature on postgresql.org does not search the 9.1
>   documentation?
>
> I looked in the 9.1 docs in SQL Commands->SELECT and could find no
> reference to COLLATE. Can anyone point me to some documentation that
> would explain what that error message means and how to resolve it?
>
> Thanks,
>
> Joe
>
> --
> Joe Conway
> credativ LLC: http://www.credativ.us
> Linux, PostgreSQL, and general Open Source
> Training, Service, Consulting, & 24x7 Support
>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
> The origin of matter is, as you mentioned, collation to be used for system
> catalog scan when we reference it via syscache.
> So, the following chunk should be added, as I did in the userspace access
> vector patch - part.1.

> @@ -934,8 +935,7 @@ CatalogCacheInitializeCache(CatCache *cache)
> /* Fill in sk_strategy as well --- always standard equality */
> cache->cc_skey[i].sk_strategy = BTEqualStrategyNumber;
> cache->cc_skey[i].sk_subtype = InvalidOid;
> - /* Currently, there are no catcaches on collation-aware data types */
> - cache->cc_skey[i].sk_collation = InvalidOid;
> + cache->cc_skey[i].sk_collation = DEFAULT_COLLATION_OID;

I removed such a hunk from a previous patch of yours, and I don't like
it any better this time. This is just a hack that will result in
masking bugs.

Consider using a non-collation-aware datatype instead, such as NAME.

regards, tom lane

2011/7/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>> The origin of matter is, as you mentioned, collation to be used for system
>> catalog scan when we reference it via syscache.
>> So, the following chunk should be added, as I did in the userspace access
>> vector patch - part.1.
>
>>   @@ -934,8 +935,7 @@ CatalogCacheInitializeCache(CatCache *cache)
>>           /* Fill in sk_strategy as well --- always standard equality */
>>           cache->cc_skey[i].sk_strategy = BTEqualStrategyNumber;
>>           cache->cc_skey[i].sk_subtype = InvalidOid;
>>   -       /* Currently, there are no catcaches on collation-aware data types */
>>   -       cache->cc_skey[i].sk_collation = InvalidOid;
>>   +       cache->cc_skey[i].sk_collation = DEFAULT_COLLATION_OID;
>
> I removed such a hunk from a previous patch of yours, and I don't like
> it any better this time.  This is just a hack that will result in
> masking bugs.
>
> Consider using a non-collation-aware datatype instead, such as NAME.
>
I agree that pg_(sh)seclabel.provider field shall not need more than
NAMEDATALEN.

How about re-define pg_seclabel.provider field also; currently defined as TEXT?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch re-defines pg_shseclabel.provider as NameData,
instead of Text,
and revert changes to catcache.c about collation.

Rest of parts are not changed.

Thanks,

2011/7/2 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2011/7/2 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> writes:
>>> The origin of matter is, as you mentioned, collation to be used for system
>>> catalog scan when we reference it via syscache.
>>> So, the following chunk should be added, as I did in the userspace access
>>> vector patch - part.1.
>>
>>>   @@ -934,8 +935,7 @@ CatalogCacheInitializeCache(CatCache *cache)
>>>           /* Fill in sk_strategy as well --- always standard equality */
>>>           cache->cc_skey[i].sk_strategy = BTEqualStrategyNumber;
>>>           cache->cc_skey[i].sk_subtype = InvalidOid;
>>>   -       /* Currently, there are no catcaches on collation-aware data types */
>>>   -       cache->cc_skey[i].sk_collation = InvalidOid;
>>>   +       cache->cc_skey[i].sk_collation = DEFAULT_COLLATION_OID;
>>
>> I removed such a hunk from a previous patch of yours, and I don't like
>> it any better this time.  This is just a hack that will result in
>> masking bugs.
>>
>> Consider using a non-collation-aware datatype instead, such as NAME.
>>
> I agree that pg_(sh)seclabel.provider field shall not need more than
> NAMEDATALEN.
>
> How about re-define pg_seclabel.provider field also; currently defined as TEXT?
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>