Skip site navigation (1) Skip section navigation (2)

Re: SE-PostgreSQL Specifications

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>, KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>, pgsql-hackers(at)postgresql(dot)org, Greg Williamson <gwilliamson39(at)yahoo(dot)com>, Sam Mason <sam(at)samason(dot)me(dot)uk>, Joshua Brindle <method(at)manicmethod(dot)com>
Subject: Re: SE-PostgreSQL Specifications
Date: 2009-07-31 23:36:48
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
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

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


In response to


pgsql-hackers by date

Next:From: KaiGai KoheiDate: 2009-08-01 00:22:33
Subject: Re: SE-PostgreSQL Specifications
Previous:From: Robert HaasDate: 2009-07-31 23:34:55
Subject: Re: machine-readable explain output v2

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group