Re: SE-PostgreSQL Specifications

On Fri, Jul 31, 2009 at 5:13 PM, Stephen Frost<sfrost(at)snowman(dot)net> wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)kaigai(dot)gr(dot)jp) wrote:
>> Stephen Frost wrote:
>> > Strategy for code changes:
>> >   Patch #1: Move permissions checks currently implemented in other parts
>> >             of the code (eg: tablecmds.c:ATExecChangeOwner()) into
>> >                     aclchk.c.
>> >   Patch #2: Change the aclchk.c function APIs, if necessary, to add
>> >             additional information required for SELinux (eg: the 'old'
>> >                     owner).
>> >   Patch #3: Add SELinux hooks into aclchk.c functions.
>>
>> Thanks for your idea. However, it seems to me this approach eventually
>> requires bigger changeset than what the current security hooks doing.
>
> Yes, it almost certainly will.  However, this is really the approach
> that I think we need to take.  I think what we're beginning to
> understand, collectivly, is something that some of the committers, etc,
> have been saying all along- implementing SELinux in PG requires alot
> more changes to PG than just sprinkling hooks all over the code.  This
> is because we want to do it right, for PG, and for our users.
>
> What this means is that we need to improve the PG code first.  Then,
> when we're happy with those changes, we can add the SELinux hooks and be
> comfortable that they will operate correctly, do what they're intended
> to, and also allow us to extend the system further for the "next big
> privileges thing", if that happens some day.
>
>> In addition, it cannot solve the differences in behavior due to the
>> security model on which they stand.
>>
>> For example, when we create a new table, the native database privilege
>> mechanism checks its permission on the schema object which the new
>> table blongs to. On the other hand, SELinux requires to check
>> "db_table:{create}" permission on the newly created table itself.
>
> That's fine.  We implement a function in aclchk.c which is essentially
> "pg_createTablePriv(schema,newtable)".  The logic for doing the check is
> moved out of tablecmds.c (or where ever) and into that function.  We can
> then easily add to that function the SELinux hook.  Is there some issue
> here which I'm not seeing?  If so, can you please clarify?
>
> Also, I don't see "db_table:{create}" in the documentation anywhere.
> There is a "db_schema:{add_name}", is that what you're referring to?
> How would you check the permissions on an object (and how or why would
> they be different than the default, unless that's also being handled at
> the creation time) which is in the process of being created?
>
>> In my understanding, the purpose of the design specification is to make
>> clear for database folks what the security hooks doin and why the security
>> hooks are deployed here.
>
> I don't think we're going to have much buy-in or success sprinkling
> these hooks all over the PG source code.  It would make maintenance a
> real problem for the PG folks and for those of us trying to work with
> SE.  The SELinux hooks really need to be folded into PG's existing
> authentication system, and if that system needs to be improved or
> expanded, then that's what it requires.
>
>> Linux kernel is a good case in success.
>> Needless to say, most of kernel developer are not security experts.
>> But they defined an explicit specification of the security hooks called
>> LSM, and put these hooks on the strategic points in the kernel.
>> (The very long source code comments describes what is the purpose of this
>> security hook, what arguments should be given and so on for each security
>> hooks.)
>
> Right, we have a number of these already, and they are represented by
> the aclchk.c functions.  The problem that SELinux points out is that
> we've not been consistant with having aclchk.c do the permissions
> checking.  Specifically on the "simple" stuff where we currently require
> owner rights.  What we need to do is improve PG by pulling those checks
> out of the areas they are, spread all around the code, and consolidating
> them into aclchk.c (or a new file if that is deisred).  We're not going
> to get anywhere by trying to add on more hooks all over the code.
>
>> > This initial round, again, should focus on just those
>> > controls/permissions which PostgreSQL already supports.  At this time,
>> > do not stress over finding every "if(superuser())" and moving it to
>> > aclchk.c.  Let's deal with the "clear" situations, such as tablecmds.c
>> > lines 6322-6342 (that entire block, including the if(!superuser()),
>> > should be ripped out and made a function in aclchk.c which is called
>> > with necessary args).  We can go back and "clean up" the other places
>> > where we have explicit superuser() checks later, if necessary.
>>
>> At least, it works correctly as far as the native database privilege
>> mechanism. If the specification of the security hooks is clear, I think
>> the following manner is better than modifying DAC mechanism.
>>
>>   if (!superuser())
>>   {
>>       if (pg_xxx_aclcheck(....) != ACLCHECK_OK)
>>           aclcheck_error(....);
>>   }
>> + sepgsqlCheckXXXX();
>
> Again, this would mean dropping sepgsql() calls all over the code,
> because we don't currently have all of these permission checks done in
> one place.  We need to first consoldiate the permission checks and then
> add the sepgsql() hooks *there*.
>
>> > I've finally had a chance to look through the last set of proposed
>> > patches some, and I've noticed some other issues that need to be
>> > addressed:
>> >
>> > - Let's avoid the changes to heaptuple.c for now.  That's really for
>> >   much later down the road when we implement row-level security.
>>
>> Yes, I can agree to postpone pg_security and corresponding facilities
>> until row-level security.
>
> We might flip this around and use pg_security to track the labels on the
> catalog entries, rather than modifying every catalog table...  That was
> my first thought anyway.
>
>> > - I would expect the dependency system to be used to handle deleting
>> >   things from pg_security, etc, when an object has been deleted (eg: a
>> >   table was dropped).
>>
>> In the patch for v8.4.x series (with fullset functionality), it is
>> already implemented. Any entries within pg_security referenced by the
>> dropped table is reclaimed, when we drop a table.
>> The matter of orphan entries are already solved.
>
> Not correctly, which is what I'm complaining about here.  Right now
> there's a "sepgsqlDropTable" or "sepgsqlDropDB" call or some such in the
> middle of the "dropTable" or "dropDB" command.  Those shouldn't be
> necessary, I don't think, if the dependency system is being properly
> used.  I might be up a tree here, but I'd like to think I'm not.  Those
> hooks should be called from the dependency system, not from the
> 'droptable' command.
>
>> > - Conversly, when new entries need to be added to pg_security, they
>> >   should be done at the same level as other items being added to the
>> >   catalog.  In places like createdb(), where it's all one big function,
>> >   I would recommend splitting out the existing catalog update into a
>> >   separate function, which then makes it much clearer that the code is
>> >   doing: update pg_database table, update pg_security table, update
>> >   pg_dependency table.  That should be done as a separate patch, of
>> >   course.  Remember, we're trying to make small incremental changes that
>> >   make sense by themselves but at the same time will reduce the effort
>> >   required to add SELinux later.
>>
>> Sorry, it's unclear for me what you would like to explain...
>
> The catalog update I'm referring to is the long list of
> MemSet(new_record)
> new_record[blah] = blah
> new_record[blah] = blah
> new_record[blah] = blah
> new_record[blah] = blah
> new_record[blah] = blah
> etc, etc
>
> I would move that out into it's own function (addDBCatalog?).  Then we
> can more cleanly add the "recordSElabel()" function to the overall
> createdb() function.  This is refactoring things to improve PG.
>
>> However, I can agree the step-by-step approach on development.
>> The design specification describes an "ideal image" of SE-PostgreSQL.
>> I understand several phases are necessary to come up to the stage.
>
> Exactly...
>
>> What does the "cacheing the results" mean?
>> If you talked about the caches of access control decision, it is quite
>> different thing from SysCache mechanism. It intends to reduce the number
>> of kernel invocation, not the number of database lookups.
>
> It's still a cache, and if we have a cacheing facility already in place,
> it'd be nice to reuse it rather than adding a whole new one.  Perhaps it
> can be extended to support what you need to cache the access control
> decisions?
>
>        Thanks,
>
>                Stephen

FWIW, pretty much +1 from me on everything in here; I think this is
definitely going in the right direction. It's not the size of the
patches that matter; it's the complexity and difficulty of verifying
that they don't break anything. And it's not cumulative: three easy
patches are better than one hard one, as long as they're really
self-contained.

The idea of restructuring the aclcheck mechanism to support sepgsql
is, IMO, brilliant.

...Robert