SE-DBUS updates

[Matthew Rickard]

On Mon, 2004-07-26 at 10:32, Havoc Pennington wrote:
> On Mon, 2004-07-26 at 08:42, Matthew Rickard wrote:
> > I see you've implemented this option in your next patch.  I can test
> > this stuff out, but it seems selinux.[ch] didn't make it into your
> > patch.  Can you redo the diff and send it again?
> 
> Doh. Attached.

I've made some comments below from an initial look over your patch. 
Mostly the changes look good, but you were right -- it didn't compile :)

In configure.in the header check should be:
        AC_EGREP_HEADER(DBUS__ACQUIRE_SVC, av_permissions.h
                        have_selinux=yes, have_selinux=no)
av_permissions.h is the header with the class/permission info in it.

In selinux.c
>  if (!bus_connection_read_selinux_context (owner, &con))
it looks like you forgot to change owner to connection.

Now this one I'm not quite sure the cause.  In 
>  bus_selinux_id_table_free_value (BusSELinuxID *sid)
It seems that sometimes the sid passed in is NULL, which causes
bus_selinux_id_unref to throw an assertion.  I solved this by simply
testing for NULL before calling bus_selinux_id_unref.  However, I'm
still not sure why this is ever being passed NULL, so I don't know if
this is the correct solution.

>  bus_selinux_allows_send (DBusConnection     *sender,
>                           DBusConnection     *proposed_recipient)
proposed_recipient can be NULL to represent the bus driver.  The
bus_selinux_allows_send was not testing this, and therefore throwing an
assertion in bus_connection_get_selinux_id().  Adding a simple check
here that uses the bus_sid on a NULL proposed_recipient corrected this.

For the 
>  _dbus_assert_not_reached ("FIXME the avc_context_to_sid() error 
>  handling");
The only error avc_context_to_sid should fail on is ENOMEM, so I think
it should be OK to just return NULL.

However, in
>  _dbus_assert_not_reached ("bus_selinux_init_id doesn't properly
>  indicate OOM");
it can fail on errors other than ENOMEM (like failing to get the
connection context).  How should we handle this?

In bus_selinux_id_table_insert() the key/sid were being cleared before
the _dbus_verbose() displayed them.  I switched the order here.

Also, some minor changes to just kill some compiler warnings.  In
bus_selinux_id_ref/unref cast it back to security_id_t with
SELINUX_SID_FROM_BUS.  In bus_selinux_id_table_insert I assign retval to
FALSE to start so that it won't complain that it could be used
uninitialized.

I also noticed that rather than using security_context_t for context
types you are simply using char* instead.  Is there any particular
reason for this (since we do have the SELinux headers available in
selinux.c)?

Thanks for updating the man page with the new SELinux information.  I
filled in the FIXME items there for the class/permissions.

I've attached a diff that applies on top of the patch you sent to show
the few changes that I made.  Hopefully we can get this ready in time to
be included in the next D-BUS release.


Matt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbus-selinux-incremental.patch
Type: text/x-patch
Size: 4648 bytes
Desc: not available
Url : http://freedesktop.org/pipermail/dbus/attachments/20040726/bf8095ec/dbus-selinux-incremental.bin