Re: SE-PostgreSQL/Lite Review

Stephen Frost wrote:
> * Greg Smith (greg(at)2ndquadrant(dot)com) wrote:
>> I personally feel that Steven
>> Frost's recent comments here about how the PostgreSQL code makes this
>> harder than it should be really cuts to the core of a next step here.
>> The problem facing us isn't "is SEPostgreSQL the right solution for
>> providing external security checks?"; it's "how can the PostgreSQL code
>> be improved so that integrating external security is easier?" Looking
>
> Thanks for that support, Greg. This was what I was principally trying
> to do with KaiGai and the commitfest patch I reviewed of his last round.
> Unfortunately, the committer comments I received on that patch didn't
> help us outline a path forward, just declared that the approach in the
> current patch wasn't workable. My, now much more optimistic thanks to
> our meeting, view is that the concept of abstracting the access controls
> is solid and a necessary first step; but we need to find a better way to
> implement it.

I agree. SELinux should be one of the user selectable options.
An abstraction layer for enhanced access controls are already adopted in
a few open source projects (Linux kernel, X-window), and gets successed.

> Also thanks to our discussion, I've got a much better handle on how
> SELinux and the general secuirty community views PostgreSQL (and the
> Linux kernel for that matter)- it's an application which consists of a
> set of object managers. That then leads into an approach to address at
> least some of Tom's comments:
>
> Let's start by taking the patch I reviewed and splitting up
> security/access_control.c along object lines. Of course, the individual
> security/<object>_ac.c files would only include the .h's that are
> necessary. This would be a set of much smaller and much more
> managable files which only know about what they should know about- the
> object type they're responsible for.

Basically, agreed. This file had more than 4KL in the last commit fest.

IMO, however, we should consider these issues before splitting up old patch.

(1) Whether the framework should host the default PG model, not only
enhanced security features, or not?
This patch tried to host both of the default PG model and SELinux.
But, the default PG model does not have same origin with label-based
mandatory access controls, such as SELinux, from a historical angle.
So, this patch needed many of unobvious changes to the core routines.

(2) Whether the framework should be comprehensive, or not?
This patch tried to replace all the default PG checks in the core
routines from the begining. It required to review many of unobvious
changes in the core routine. Because I've been repeatedly pointed out
to keep changeset smaller, I'm frightened for the direction.
The coverage of the latest SE-PgSQL/Lite patch is only databases,
schemas, tables and columns. I think it is a good start to deploy
security hooks on the routines related to these objects, rather than
comprehensive support from the begining.

(3) In the future, whether we should allow multiple enhanced securities
at the same time, or not?
It was not clear in the previous commit fest. But, in fact, Linux
kernel does not support multiple security features in same time,
because it just makes security label management complex. (It also
have another reasons, but just now unlear.)
I don't think we have any good reason to activate multiple enhanced
securities at same time.

I think most of folks can agree with (2) and (3). But I expect opinion
is divided at (1).

For example, if we host the default PG checks to create a new database,
all the existing checks shall be moved as follows:

http://code.google.com/p/sepgsql/source/browse/trunk/pgsec/src/backend/security/access_control.c?r=2273#1430

In addition, it has to return the security context to be assigned on
the new database, when it hosts SELinux.
Even if this rework is limited to four kind of object classes, I'm
worry about it overs an acceptable complexity.

If the framework hosts only "enhanced" securities, this hook shall be
implemented as:

/*
* ac_database_create
* It checks permission to create a new database, and returns a security
* label to be assigned on, or NULL if unnecessary.
*
* datName : name of the new database
* srcDatOid : OID of the template database
* datLabel : user given security label, or NULL
*/
Value *
ac_database_create(const char *datName, Oid srcDatOid, Value *datLabel)
{
#ifdef HAVE_SELINUX
if (sepgsql_is_enabled())
return sepgsql_database_create(datName, srcDatOid, datLabel);
#endif
return NULL;
}

In the caller (createdb()), this invocation shall be placed as follows:

:
+ datLabel = ac_database_create(dbname, src_dboid, userDatLabel);
:
+ if (!datLabel)
+ new_record_nulls[Anum_pg_database_datselabel - 1] = true;
+ else
+ new_record[Anum_pg_database_datselabel - 1]
+ = CStringGetTextDatum(strVal(datLabel));
:
simple_heap_insert(pg_database_rel, tuple);

It does not need unobvious logic changes in the default PG models, and
available for vaious kind of label-based security features. (Note that
the core routine handles the security label just as a text data without
special meanings.)

> Clearly, code comments also need to be reviewed and issues with them
> addressed. I'm also not a fan of the "skip permissions check"
> arguments, which are there specifically to address cascade deletions,
> which is a requirment of our PG security model. I'd love to see a
> better solution and am open to suggestions or thoughts about what one
> would be. I know one of KaiGai's concerns in this area was performance,
> butas we often do for PG, perhaps we should consider that second and
> first consider what it would look like if we ignored performance
> concerns. This would change "skip permissions check" to "what object is
> being deleted, and am I a sub-object of that?". I don't feel like I've
> explained that well enough, perhaps someone else could come up with
> another solution, or maybe figure out a better way to describe what I'm
> trying to get with this.

Yep, IIRC, this argument is fixed into "cascade" to provide security
features a hint whether this deletion is due to the cascaded, or not.

If the caller really don't want permission checks, such as cleaning
up the temporary objects, the caller should skip invocation of the
framework, like "if (check_rights)" in DefineIndex().

> Regarding contrib modules- I view them as I do custom code which the
> user writes and loads through dlopen() into the backend process- there
> should be a way for it to do security "right", but it ultimately has to
> be the responsibility of the contrib module. Admins can review what
> contrib modules have been made SELinux/etc aware and choose to install
> only those which they trust.

Yes, correct.
It is the job of administrator to decide what module should be loaded.

It is also a nature of the reference monitor model which widely includes
access control features, such as SELinux and the PG default checks.
Even if the module is not PG checks/SELinux aware, it is considered as
trusted.

>> I have to be honest and say that I'm not optimistic that this is
>> possible or even a good idea to accomplish in the time remaining during
>> this release.
>
> While I agree with you, I wish you hadn't brought it up. :) Mostly
> because I feel like it may discourage people from wanting to spend time
> on it due to desire to focus on things which are likely to make it into
> the next release. That being said, I don't feel that'll be an issue for
> KaiGai or myself, but getting someone beyond us working on this would
> really be great, especially yourself and/or Bruce.

Even if SE-PgSQL/Lite provides a limited feature in this release,
I believe it shall encourage security folks and enable to pull out their
contributions more than myself.
Hopefully, I'd like to merge a meaningful piece of SE-PgSQL. :-)

>> On my side, in addition to helping coordinate everyone pushing in the
>> same direction, I'll also continue trying to shake out some sponsorship
>> funding for additional work out of the people in this country it would
>> benefit. It seems I'm going to keep getting pulled into the middle of
>> this area regularly anyway.
>
> Thank you for that. I'm planning to do the same and will certainly let
> people know, to the extent possible, of anything I'm able to dig up.

Thanks so much!
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>