Bug 105330 - Stop using selinux_set_mapping() function
Summary: Stop using selinux_set_mapping() function
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-03-02 18:32 UTC by Laurent Bigonville
Modified: 2018-10-12 21:32 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Stop using selinux_set_mapping() function (4.39 KB, patch)
2018-03-06 09:52 UTC, Laurent Bigonville
Details | Splinter Review
Stop using selinux_set_mapping() function (4.51 KB, patch)
2018-03-12 13:31 UTC, Laurent Bigonville
Details | Splinter Review
Stop using selinux_set_mapping() function (4.93 KB, patch)
2018-08-31 10:37 UTC, Laurent Bigonville
Details | Splinter Review
Stop using selinux_set_mapping() function (4.95 KB, patch)
2018-08-31 10:53 UTC, Laurent Bigonville
Details | Splinter Review
Set a DBusError in all failing paths of bus_selinux_check() (1.34 KB, patch)
2018-08-31 10:53 UTC, Laurent Bigonville
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laurent Bigonville 2018-03-02 18:32:39 UTC
Hi,

ATM, when selinux_set_mapping() fails due to an incomplete policy (or no policy at all) dbus-daemon exits even in permissive mode.

This is a bit a problem for people that are developing their policy or for people that need to recover their machines after a miss configuration as dbus-daemon is part of the boot and login process via logind.

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.

selinux_set_mapping() was introduced by:

commit ba088208bc0c35ca418a097a8482c4a7705f4a43
Author: osmond sun <osmond.sun@gmail.com>
Date:   Wed Nov 6 00:53:18 2013 +0800

    selinux: Use selinux_set_mapping() to avoid hardcoded constants for policy
    
    Previous to the introduction of selinux_set_mapping(), DBus pulled
    constants generated from the system's policy at build time.  But this
    means it's impossible to replace the system policy without rebuilding
    userspace components.
    
    This patch maps from arbitrary class/perm indices used by D-Bus and
    the policy values and handles all the translation at runtime on
    avc_has_perm() calls.
    
    Bug: https://bugs.freedesktop.org/attachment.cgi?id=88719
    Reviewed-By: Colin Walters <walters@verbum.org>
    Tested-By: Colin Walters <walters@verbum.org>
Comment 1 Laurent Bigonville 2018-03-03 08:53:30 UTC
In addition to that it seems that when the security classes and/or AV are reordered in the policy that dbus misbehave.
Comment 2 Simon McVittie 2018-03-05 12:46:17 UTC
(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>).
Comment 3 Simon McVittie 2018-03-05 12:46:53 UTC
(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.)
Comment 4 Laurent Bigonville 2018-03-05 12:50:21 UTC
I've a non-tested patch waiting here on top of my patch for #92831
Comment 5 Laurent Bigonville 2018-03-06 09:52:31 UTC
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.
Comment 6 Laurent Bigonville 2018-03-06 09:57:40 UTC
(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 7 Simon McVittie 2018-03-12 12:42:46 UTC
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".
Comment 8 Laurent Bigonville 2018-03-12 13:31:50 UTC
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.
Comment 9 Laurent Bigonville 2018-03-12 13:50:37 UTC
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 10 Simon McVittie 2018-04-17 16:59:40 UTC
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).
Comment 11 Simon McVittie 2018-08-30 19:51:24 UTC
(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?
Comment 12 Laurent Bigonville 2018-08-31 10:37:23 UTC
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
Comment 13 Laurent Bigonville 2018-08-31 10:53:15 UTC
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
Comment 14 Laurent Bigonville 2018-08-31 10:53:19 UTC
Created attachment 141395 [details] [review]
Set a DBusError in all failing paths of bus_selinux_check()
Comment 15 GitLab Migration User 2018-10-12 21:32:47 UTC
-- 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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.