Bug 105330

In addition to that it seems that when the security classes and/or AV are reordered in the policy that dbus misbehave.

(In reply to Laurent Bigonville from comment #0)
> ATM, when selinux_set_mapping() fails due to an incomplete policy (or no
> policy at all) dbus-daemon exits even in permissive mode.

Patch welcome - but someone who knows SELinux (so, not me) will need to review it for SELinux correctness. I'm not completely clear on whether 

If SELinux is not being enforced, *and* dbus-daemon will behave correctly without the mapping, then perhaps it doesn't need to be mandatory. If you're contributing a patch to change this behaviour, please explain what would happen if the mapping couldn't be loaded, and why that behaviour would not be a security or correctness issue.

> I'm actually wondering if that call is needed at all these days. As we could
> use, I think, avc_context_to_sid(), string_to_security_class() and
> string_to_av_perm() to get the needed info.
> 
> An other solution would be to use selinux_check_access() instead of
> avc_has_perm() (selinux_check_access() uses avc_has_perm() internally and
> the function mentioned above internally), the problem is that way we cannot
> use the cache, I think again, and we might have a performance hit.

I'll defer to SELinux experts' recommendations for this.

> selinux_set_mapping() was introduced by:
>     selinux: Use selinux_set_mapping() to avoid hardcoded constants for
> policy

See Bug #71187 for discussion (unfortunately the commit message lists the attachment URL and not the bug URL, but you can get from attachment to bug via a URL like <https://bugs.freedesktop.org/attachment.cgi?id=88719&action=edit>).

(In reply to Simon McVittie from comment #2)
> Patch welcome - but someone who knows SELinux (so, not me) will need to
> review it for SELinux correctness. I'm not completely clear on whether 

... these numbers are considered to be API or not.

(I'm clearly also not completely caffeinated yet.)

I've a non-tested patch waiting here on top of my patch for #92831

Created attachment 137821 [details] [review]
Stop using selinux_set_mapping() function

Currently, if the "dbus" security class or the associated AV doesn't
exist, dbus-daemon fails to initialize and exits immediately. This can
be a problem for people developping their own policy or trying to
access a machine where, for some reasons, there is not policy defined
at all.

The code here copy the behaviour of the selinux_check_access() function.
We cannot use this function here as it doesn't allow us to define a
cache.

(In reply to Simon McVittie from comment #3)
> (In reply to Simon McVittie from comment #2)
> > Patch welcome - but someone who knows SELinux (so, not me) will need to
> > review it for SELinux correctness. I'm not completely clear on whether 
> 
> ... these numbers are considered to be API or not.
> 
The numbers are not considered API, the names of the class and the AV are though

Comment on attachment 137821 [details] [review]
Stop using selinux_set_mapping() function

Review of attachment 137821 [details] [review]:
-----------------------------------------------------------------

This looks reasonable to me, but someone who knows SELinux should review it. (Colin?)

::: bus/selinux.c
@@ +367,3 @@
>  		   DBusString          *auxdata)
>  {
> +  int rc;

If this is being used to save the value of errno, the conventional name for that purpose in the dbus codebase is "saved_errno".

Created attachment 138021 [details] [review]
Stop using selinux_set_mapping() function

Currently, if the "dbus" security class or the associated AV doesn't
exist, dbus-daemon fails to initialize and exits immediately. Also the
security classes or access vector cannot be reordered in the policy.
This can be a problem for people developping their own policy or trying
to access a machine where, for some reasons, there is not policy defined
at all.

The code here copy the behaviour of the selinux_check_access() function.
We cannot use this function here as it doesn't allow us to define a
cache.

Thx for the review, I've updated the patch with your comment

When both my patches are OK on the dbus side, I'll ask for review on the SELinux one

Comment on attachment 138021 [details] [review]
Stop using selinux_set_mapping() function

Review of attachment 138021 [details] [review]:
-----------------------------------------------------------------

If this is OK from the SELinux perspective then it looks OK from the D-Bus perspective, but I'd really prefer it with a follow-up patch to make the error behaviour better, instead of using _dbus_warn().

At the moment, bus_selinux_check() doesn't raise a DBusError properly, so it has no choice but to do a _dbus_warn(); and bus_selinux_allows_whatever() has three possible results:

* return TRUE: OK
* return FALSE with error not set: caller raises DBUS_ERROR_ACCESS_DENIED
* return FALSE with error set: caller propagates error

This is one result too many: the middle one should not happen. A function that can ever raise a DBusError on failure should *always* raise a DBusError on failure. The nearby AppArmor code paths, which were added years after the SELinux code and with a lot more review, get this right: if bus_apparmor_allows_whatever() denies permission to do whatever, then it always sets the error.

bus_selinux_check() would still have to return TRUE without setting the error, and perhaps call _dbus_warn(), if security_deny_unknown() returns 0 (presumably some sort of semi-permissive mode). The _dbus_warn() call here might not be necessary if log_callback() is already expected to write to the same place as _dbus_warn() (the syslog).

(In reply to Simon McVittie from comment #10)
> If this is OK from the SELinux perspective

Is it?

> I'd really prefer it with a follow-up patch to make the
> error behaviour better

Have you been able to do this?

Created attachment 141393 [details] [review]
Stop using selinux_set_mapping() function

Currently, if the "dbus" security class or the associated AV doesn't
exist, dbus-daemon fails to initialize and exits immediately. Also the
security classes or access vector cannot be reordered in the policy.
This can be a problem for people developping their own policy or trying
to access a machine where, for some reasons, there is not policy defined
at all.

The code here copy the behaviour of the selinux_check_access() function.
We cannot use this function here as it doesn't allow us to define the
AVC entry reference.

See the discussion at https://marc.info/?l=selinux&m=152163374332372&w=2

Created attachment 141394 [details] [review]
Stop using selinux_set_mapping() function

Currently, if the "dbus" security class or the associated AV doesn't
exist, dbus-daemon fails to initialize and exits immediately. Also the
security classes or access vector cannot be reordered in the policy.
This can be a problem for people developping their own policy or trying
to access a machine where, for some reasons, there is not policy defined
at all.

The code here copy the behaviour of the selinux_check_access() function.
We cannot use this function here as it doesn't allow us to define the
AVC entry reference.

See the discussion at https://marc.info/?l=selinux&m=152163374332372&w=2

Created attachment 141395 [details] [review]
Set a DBusError in all failing paths of bus_selinux_check()

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/198.