Re: SE-PostgreSQL Specifications - Mailing list pgsql-hackers

KaiGai,

* KaiGai Kohei (kaigai@kaigai.gr.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