search

SELinuxProject / refpolicy

SELinux Reference Policy v2
https://github.com/SELinuxProject/refpolicy/wiki
GNU General Public License v2.0
304 stars 135 forks source link

unconfined domains aren't allowed to fully use dbus #18

Closed CendioOssman closed 5 years ago

CendioOssman commented 5 years ago

Originally reported to Red Hat since it was seen on modern Fedora:

https://bugzilla.redhat.com/show_bug.cgi?id=1647920

Seems to be a general refpolicy issue though.

The basic issue is that a domain with unconfined_domain(my_domain_t) will be allowed to send messages over dbus without issue. However the responses will often be rejected because there is no rule allowing the other domain to send_msg to my_domain_t.

Example AVC where thinlinc_webaccess_t is unconfined:

type=USER_AVC msg=audit(1541681954.605:398): pid=788 uid=81 auid=4294967295 ses=4294967295 subj=system_u:system_r:system_dbusd_t:s0-s0:c0.c1023 msg='avc:  denied  { send_msg } for msgtype=method_return dest=:1.730 spid=1 tpid=5844 scontext=system_u:system_r:init_t:s0 tcontext=system_u:system_r:thinlinc_webaccess_t:s0 tclass=dbus permissive=0  exe="/usr/bin/dbus-daemon" sauid=81 hostname=? addr=? terminal=?'UID="dbus" AUID="unset" SAUID="dbus"

It looks like this bug was fixed here:

https://github.com/TresysTechnology/refpolicy-contrib/commit/6bef7a14757124c56fadc08c255e9dd6c29a15f9

But then reverted because of security issues here:

https://github.com/TresysTechnology/refpolicy-contrib/commit/bc147412848c53b2150490a2c5090a17a40fab82

Does anyone have any reference for those security problems?

If unconfined domains cannot use dbus by default, then this should be clearly documented for unconfined_domain(), and there should be some information on how to enable dbus for such domains. Explicitly listing every other domain (or using equivalent macros such as init_dbus_chat()) defeats the whole purpose of unconfined_domain().

What risk am I taking by adding this and allowing full dbus communication to my domain:

allow { dbusd_session_bus_client dbusd_system_bus_client } thinlinc_webaccess_t:dbus send_msg;
ghost commented 5 years ago

I am responsible for adding and then reverting the commit. It was requested that i revert this by @stephensmalley. I am not sure what his specific reasons were but it is obviously very broad access to allow. Its not least privilege.

stephensmalley commented 5 years ago

The problem is that if you allow all domains to send messages to unconfined domains, then you are permitting all confined domains to escape confinement by sending messages to unconfined domains that trigger operations the confined domain shouldn't be allowed to perform. This was creating unintentional security holes in policy where a confined domain could easily escalate its access through some dbus service provided by an unconfined domain.

CendioOssman commented 5 years ago

So could a documentation note be added about this gotcha?

And what would be the recommended step for us? We're not in a position to properly confine these domains right now, and this restriction is breaking them to the point of being entirely unusable.

We don't have any dbus services, so allowing send_msg to specifically our domains should then be secure as it can only be used for replies?

ghost commented 5 years ago

My personal opinion: You have to start somewhere. DBUS is the main issue. So focus on the dbus avc denials and fix what you can when you can. You dont have to "properly confine" these domains, you just have to address this particular challenge.

Allowing the rule you suggested above is better than my commit, but it is still broad and it sets a bad precedent, especially knowing that in fedora these third party modules aren't really reviewed.

CendioOssman commented 5 years ago

I'd like to avoid too much of whack-a-mole as we are shipping this policy to customers and we need to avoid things break randomly as Red Hat updates things. In this case we are not doing the dbus calls from our code, but it is something in the system libraries (seems to be user lookups). As such it is difficult to determine what the appropriate thing to add here is.

ghost commented 5 years ago

ok so its probably glibc nss, that is pretty limited generally init_dbus_chat (for nss_systemd) and theres a few optionals like resolved_dbus_chat (nss_resolve) and machined_dbus_chat (nss_mymachines)

If you address those three then i suspect you might have the majority covered.

CendioOssman commented 5 years ago

Alright, thanks.

Any thoughts of grouping those under some basic user lookup macro so that not everyone needs to keep track of the latest nss modules? :)

ghost commented 5 years ago

Ideally these should arguably be covered in the auth_use_nsswitch() interface but i am not sure if that is the case in Fedora. In reference policy it is still a point of discussion on how to address nss in the future because it is also getting very broad.

ghost commented 5 years ago

... Also make it a dbus_system_bus_client() if needed.

CendioOssman commented 5 years ago

Hah. And after an update the bug went away.

I turns out I got in dbus-broker instead of the older dbus-daemon, and dbus-broker does not seem to do an SELinux check on replies. So this is a good thing I guess and means the policy will now be applied more sanely on such systems? I.e. no more bi-directional allow rules just to make responses work.

Ah well, I still need to add something for systems that aren't running dbus-broker.

ghost commented 5 years ago

I doubt that. The thing with these userns lookups is that it only wants to translate them if there is something to translate. Meaning that if theres no files or processes with these userns UID/GIDs in the way of your process then it also wont need to request translation. In other words it make take some time to reproduce the event

ghost commented 5 years ago

hmm it seems that you are right ...

CendioOssman commented 5 years ago

I could reproduce the issue every time before the upgrade, so I'm fairly sure the calls were made constantly.

These are the traces I got:

From sudo dbus-monitor --system

method call time=1546964053.400922 sender=:1.115 -> destination=org.freedesktop.DBus serial=1 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=Hello
method return time=1546964053.401056 sender=org.freedesktop.DBus -> destination=:1.115 serial=4294967295 reply_serial=1
   string ":1.115"
signal time=1546964053.401170 sender=org.freedesktop.DBus -> destination=(null destination) serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string ":1.115"
   string ""
   string ":1.115"
signal time=1546964053.401713 sender=org.freedesktop.DBus -> destination=:1.115 serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.115"
method call time=1546964053.401805 sender=:1.115 -> destination=org.freedesktop.systemd1 serial=2 path=/org/freedesktop/systemd1; interface=org.freedesktop.systemd1.Manager; member=GetDynamicUsers
method return time=1546964053.402059 sender=:1.1 -> destination=:1.115 serial=705 reply_serial=2
   array [
   ]
signal time=1546964053.403557 sender=org.freedesktop.DBus -> destination=:1.115 serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameLost
   string ":1.115"
signal time=1546964053.403709 sender=org.freedesktop.DBus -> destination=(null destination) serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string ":1.115"
   string ":1.115"
   string ""
method call time=1546964053.520837 sender=:1.116 -> destination=org.freedesktop.DBus serial=1 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=Hello
method return time=1546964053.520892 sender=org.freedesktop.DBus -> destination=:1.116 serial=4294967295 reply_serial=1
   string ":1.116"
signal time=1546964053.520916 sender=org.freedesktop.DBus -> destination=(null destination) serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string ":1.116"
   string ""
   string ":1.116"
signal time=1546964053.520951 sender=org.freedesktop.DBus -> destination=:1.116 serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameAcquired
   string ":1.116"
method call time=1546964053.520973 sender=:1.116 -> destination=org.freedesktop.systemd1 serial=2 path=/org/freedesktop/systemd1; interface=org.freedesktop.systemd1.Manager; member=GetDynamicUsers
method return time=1546964053.521718 sender=:1.1 -> destination=:1.116 serial=706 reply_serial=2
   array [
   ]
signal time=1546964053.522650 sender=org.freedesktop.DBus -> destination=:1.116 serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameLost
   string ":1.116"
signal time=1546964053.522805 sender=org.freedesktop.DBus -> destination=(null destination) serial=4294967295 path=/org/freedesktop/DBus; interface=org.freedesktop.DBus; member=NameOwnerChanged
   string ":1.116"
   string ":1.116"
   string ""

It shows four call:s with corresponding returns. And I see only four calls to selinux_check_access() in dbus-broker:

Breakpoint 1, selinux_check_access (scon=0x555d4fde4d20 "system_u:system_r:thinlinc_webaccess_t:s0", tcon=0x555d4fc74358 "system_u:system_r:system_dbusd_t:s0-s0:c0.c1023", class=0x555d4e2d995e "dbus", 
    perm=0x555d4e2d9963 "send_msg", aux=0x0) at checkAccess.c:35
35  int selinux_check_access(const char *scon, const char *tcon, const char *class, const char *perm, void *aux) {
(gdb) 
Continuing.

Breakpoint 1, selinux_check_access (scon=0x555d4fde4d20 "system_u:system_r:thinlinc_webaccess_t:s0", tcon=0x555d4fd5ad10 "system_u:system_r:init_t:s0", class=0x555d4e2d995e "dbus", 
    perm=0x555d4e2d9963 "send_msg", aux=0x0) at checkAccess.c:35
35  int selinux_check_access(const char *scon, const char *tcon, const char *class, const char *perm, void *aux) {
(gdb) 
Continuing.

Breakpoint 1, selinux_check_access (scon=0x555d4fde4d20 "system_u:system_r:thinlinc_webaccess_t:s0", tcon=0x555d4fc74358 "system_u:system_r:system_dbusd_t:s0-s0:c0.c1023", class=0x555d4e2d995e "dbus", 
    perm=0x555d4e2d9963 "send_msg", aux=0x0) at checkAccess.c:35
35  int selinux_check_access(const char *scon, const char *tcon, const char *class, const char *perm, void *aux) {
(gdb) 
Continuing.

Breakpoint 1, selinux_check_access (scon=0x555d4fde4d20 "system_u:system_r:thinlinc_webaccess_t:s0", tcon=0x555d4fd5ad10 "system_u:system_r:init_t:s0", class=0x555d4e2d995e "dbus", 
    perm=0x555d4e2d9963 "send_msg", aux=0x0) at checkAccess.c:35
35  int selinux_check_access(const char *scon, const char *tcon, const char *class, const char *perm, void *aux) {
(gdb) 
Continuing.

This also corresponds well with how I'm reading the dbus-broker code:

https://github.com/bus1/dbus-broker/blob/66a3e764c4df9f555dd55bdb96e69336f07d07bc/src/bus/peer.c

You can see it doing a policy check (where the SELinux check is) in peer_queue_unicast() for calls, but no such check in peer_queue_reply() for the returns.

ghost commented 5 years ago

This makes it more in line with traditional unix domain sock behavior, but i wonder whether this is by design and whether this was discussed somewhere...

CendioOssman commented 5 years ago

It is kinda mentioned here in the form of "Reply policies":

https://github.com/bus1/dbus-broker/wiki/Deviations

But it doesn't sound like a terribly firm design choice. And I'm not sure if they considered the SELinux aspect of it.

ghost commented 5 years ago

I am seeing bugs in dbus-broker:

[root@brutus ~]# date
Tue Jan  8 17:53:22 CET 2019
[root@brutus ~]# sesearch --auditallow
auditallow xserver.subj xserver.subj:x_server debug; [ xserver_object_manager ]:False
[root@brutus ~]# busctl get-property org.freedesktop.systemd1 /org/freedesktop/systemd1 org.freedesktop.systemd1.Manager LogLevel
s "info"
[root@brutus ~]# journalctl -rb --grep granted | head -n 2
-- Logs begin at Sat 2018-12-29 11:53:13 CET, end at Tue 2019-01-08 17:53:33 CET. --
Jan 08 17:53:33 brutus audit[1327]: USER_AVC pid=1327 uid=81 auid=4294967295 ses=4294967295 subj=sys.id:sys.role:dbus.system.subj:s0 msg='avc:  granted  { send_msg } for  scontext=wheel.id:sysadm.role:busctl.sysadm.subj:s0 tcontext=sys.id:sys.role:systemd.system.subj:s0 tclass=dbus

That auditallow rule is no longer loaded but dbus-broker still enforces it

ghost commented 5 years ago

@teg @dvdhrm any insights on the above?

ghost commented 5 years ago

seems it does not receive policyload notices anymore?

teg commented 5 years ago

@CendioOssman this was intentional, I have now added a couple of sentences to the wiki to make it clearer. Let me know if it could still be improved.

@doverride sounds like we are missing some policy reload (which I thought libselinux was taking care of for us, but I'll have to refresh my memory some more). I take it you observe this after changing the SELinux policy at runtime, but not after a reboot?

stephensmalley commented 5 years ago

Should be handled automagically by libselinux if using selinux_check_access(). If not, then that seems like a bug in libselinux or policy or kernel. Please confirm that compute_av reports the right result from the kernel for the auditallow vector?

ghost commented 5 years ago

Yes I thought selinux_access_check() would take care of that as well.

I observe this after changing the policy at runtime but i do not observe it on a reboot... indeed

ghost commented 5 years ago

I haven't tested compute_av because fedora does not ship that in its libselinux-utils package. Instead i tried the same with systemd (systemd is also an object manager so that is comparable)

It works as expected with systemd so that indicates to me that its not a policy, selinux or kernel issue.

ghost commented 5 years ago

dbus-broker: https://www.youtube.com/watch?v=f97fi7qFET8 systemd: https://www.youtube.com/watch?v=iY6_kGz01vg

CendioOssman commented 5 years ago

@CendioOssman this was intentional, I have now added a couple of sentences to the wiki to make it clearer. Let me know if it could still be improved.

@teg, ehm... which wiki? I could not find anything. :)

And I was really thinking about this documentation, as that was what I checked:

https://github.com/SELinuxProject/refpolicy/blob/dda33fde949cc22cac74d4301684c71741c37131/policy/modules/system/unconfined.if#L105-L127

teg commented 5 years ago

@CendioOssman https://github.com/bus1/dbus-broker/wiki/Deviations/_compare/f63716a88fc2a8d42c6ef8fff1e460bc5080b21c...ea851e81bbe8f9dab9bbbbb275f526b00ed240e4

CendioOssman commented 5 years ago

Ah. Too many things discussed on this issue. :)

That change looks good to me. :+1:

ghost commented 5 years ago

Feel free to send a PR with what you think should be added to the XML

CendioOssman commented 5 years ago

I'm not entirely sure what to add though. The warning part is fairly straightforward, but what should be the recommended alternative? It would be nice with something less vague than this:

diff --git a/policy/modules/system/unconfined.if b/policy/modules/system/unconfined.if
index 565e7129e..31c9eff94 100644
--- a/policy/modules/system/unconfined.if
+++ b/policy/modules/system/unconfined.if
@@ -111,13 +111,21 @@ interface(`unconfined_domain_noaudit',`
 ##     <p>
 ##     Make the specified domain unconfined and
 ##     audit executable heap usage.  With exception
-##     of memory protections, usage of this interface
-##     will result in the level of access the domain has
-##     is like SELinux was not being used.
+##     of memory protections and D-Bus access, usage of this
+##     interface will result in the level of access the domain
+##     has is like SELinux was not being used.
 ##     </p>
 ##     <p>
 ##     Only completely trusted domains should use this interface.
 ##     </p>
+##     <p>
+##     Note that the specified domain will be allowed to acquire
+##     a D-Bus service name, and send messages to any other
+##     domain. However, other domains will not automatically be
+##     allowed to send responses back to the specified domain.
+##     Any such communication must be explicitly allowed in
+##     addition to making the specified domain unconfined.
+##     </p>
 ## </desc>
 ## <param name="domain">
 ##     <summary>
teg commented 5 years ago

@CendioOssman at the risk of making this even more complicated: the original text was correct when dbus-broker was in use for DBus communication, and as that will be the default at least in Fedora with the next release, I would think that changing the text might confuse more than it enlightens?

CendioOssman commented 5 years ago

Possibly. But it really depends on how quickly and totally dbus-daemon will be abandoned. E.g. when will the RHEL versions switch over or reach EOL?

ghost commented 5 years ago

This stuff potentially applies to more user space object managers. It is potentially not dbus specific. We cannot tell beforehand what user space decides to enforce. So it is hard to make any guarantees about user space object managers.

teg commented 5 years ago

Fair enough, makes sense. Though mabe make the docs more generic then? So people don't assume this only applies to DBus, nor that it applies to all DBus implementations?

stephensmalley commented 5 years ago

I haven't tested compute_av because fedora does not ship that in its libselinux-utils package. Instead i tried the same with systemd (systemd is also an object manager so that is comparable)

It works as expected with systemd so that indicates to me that its not a policy, selinux or kernel issue.

I would have preferred to replicate the exact same check via compute_av or alternatively calling selinux.selinux_check_access() from the python bindings and verifying correct behavior. In your systemd test case, was the rule also conditional and was the boolean initial value the same?

ghost commented 5 years ago

stephensmalley notifications@github.com writes:

I haven't tested compute_av because fedora does not ship that in its libselinux-utils package. Instead i tried the same with systemd (systemd is also an object manager so that is comparable)

It works as expected with systemd so that indicates to me that its not a policy, selinux or kernel issue.

I would have preferred to replicate the exact same check via compute_av or alternatively calling selinux.selinux_check_access() from the python bindings and verifying correct behavior. In your systemd test case, was the rule also conditional and was the boolean initial value the same?

Neither were conditional. I mess with policy pretty much all day every day. I would notice something like this. This dbus-broker issue is likely pretty new.

This dbus-broker version 17 is pretty recent and i wouldnt be surprised if the issue was introduced there.

I will see if i can find the time to look into this further maybe tomorrow or over the weekend but i am pretty confident that its just a bug in dbus-broker

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

-- Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02 https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02 Dominick Grift

ghost commented 5 years ago

Fair enough, makes sense. Though mabe make the docs more generic then? So people don't assume this only applies to DBus, nor that it applies to all DBus implementations?

I will leave that decision to others. It is not that simple either. It seems that not everything it considered a "reply". I was all hopeful about this and set out to try it out here: https://github.com/DefenSec/dssp2-standard/commit/e5bae290654d7d9970229971aa615e1a4f1e1c5c

Turns out that either i didnt fully understand it, and/or that it practically makes little difference. In a lot of cases it seems to not just be a matter of just a "reply". Take for example this:

Jan 10 18:09:50 brutus dbus-broker[1327]: A security policy denied :1.9 to send signal /org/freedesktop/login1/session/_32:org.freedesktop.login1.Session.PauseDevice to :1.93.
Jan 10 18:09:50 brutus audit[1327]: USER_AVC pid=1327 uid=81 auid=4294967295 ses=4294967295 subj=sys.id:sys.role:dbus.system.subj:s0 msg='avc:  denied  { send_msg } for  scontext=sys.id:sys.role:logind.subj:s0 tcontext=wheel.id:wheel.role:xserver.subj:s0 tclass=dbus permissive=0
                                     exe="/usr/bin/dbus-broker" sauid=81 hostname=? addr=? terminal=?'

So systemd-logind has a name on the bus and provides services. xserver seemingly consumes such a service where i suppose it asks logind to keep it posted on something. So logind "replies" (i suppose indirectly), but the check is performed.

In the case of unconfined_domains you can't realy determine beforehand if the request an unconfined domain does can be replied to immediately or whether that requires some indirect reply that is checked after all.

In other words yes sometimes a rule to allow "replies" can be ommited but especially in the case of unconfined domains there is no way to know for sure if that is always the case. So youre practically most of the time better off explicitly allowing a reply.

ghost commented 5 years ago

I haven't tested compute_av because fedora does not ship that in its libselinux-utils package. Instead i tried the same with systemd (systemd is also an object manager so that is comparable) It works as expected with systemd so that indicates to me that its not a policy, selinux or kernel issue.

I would have preferred to replicate the exact same check via compute_av or alternatively calling selinux.selinux_check_access() from the python bindings and verifying correct behavior. In your systemd test case, was the rule also conditional and was the boolean initial value the same?

I think have jumped to gun and pointed fingers at dbus-broker too soon, because i cannot consistently produce this. I need more time to look into this

teg commented 5 years ago

@doverride Only replies (including errors) to method calls are implicitly allowed whenever their triggering method call wasy. The reason for this is that there is a one-to-one correspondence between the request (the method call) and the reply, where the request clearly indicates that a reply is expected and should be allowed.

What you are seeing is a signal being denied, and signals are treated the same in dbus-broker and dbus-daemon. A signal is not sent in reply to a method, but is broadcast to the bus, and anyone having installed a match to it will receive it (unless there are policies in place to deny it). One could argue that maybe there should be a policy mechanism making it possible to restrict who can match on what, and hence removing the need for policies on the actual signals, but that is not the case, so the only way to put a policy on who can receive what signals is to require explicit permissions for a sender of a signal to send it to a given receiver.

pebenito commented 5 years ago

As for the policy comments, it should be something along the lines that the interface does not allow return communications from confined domains via message based mechanisms such as dbus or SysV message queues.

ghost commented 5 years ago

@doverride Only replies (including errors) to method calls are implicitly allowed whenever their triggering method call wasy. The reason for this is that there is a one-to-one correspondence between the request (the method call) and the reply, where the request clearly indicates that a reply is expected and should be allowed.

What you are seeing is a signal being denied, and signals are treated the same in dbus-broker and dbus-daemon. A signal is not sent in reply to a method, but is broadcast to the bus, and anyone having installed a match to it will receive it (unless there are policies in place to deny it). One could argue that maybe there should be a policy mechanism making it possible to restrict who can match on what, and hence removing the need for policies on the actual signals, but that is not the case, so the only way to put a policy on who can receive what signals is to require explicit permissions for a sender of a signal to send it to a given receiver.

Thanks. I am going to try it again and implement signal subscribers. It is not easy to do policy development if dbus-broker (or maybe all object managers) only reloads policy one time at runtime (that seems to be the case but i am still looking into it)

ghost commented 5 years ago

@doverride Only replies (including errors) to method calls are implicitly allowed whenever their triggering method call wasy. The reason for this is that there is a one-to-one correspondence between the request (the method call) and the reply, where the request clearly indicates that a reply is expected and should be allowed. What you are seeing is a signal being denied, and signals are treated the same in dbus-broker and dbus-daemon. A signal is not sent in reply to a method, but is broadcast to the bus, and anyone having installed a match to it will receive it (unless there are policies in place to deny it). One could argue that maybe there should be a policy mechanism making it possible to restrict who can match on what, and hence removing the need for policies on the actual signals, but that is not the case, so the only way to put a policy on who can receive what signals is to require explicit permissions for a sender of a signal to send it to a given receiver.

Thanks. I am going to try it again and implement signal subscribers. It is not easy to do policy development if dbus-broker (or maybe all object managers) only reloads policy one time at runtime (that seems to be the case but i am still looking into it)

Welp, can't say i did not try. There's just too many signal providers. Need a way to differentiate between sending signals and method calls.

I will have another look at the policyload issue again. It sucks that doing a setenforce 0 has no effect on dbus-broker (and maybe others).

ghost commented 5 years ago

Must really be dbus-broker only issue

when you load a policy at runtime that affects dbus, all subsequent reloads are "ignored" by dbus-broker regardless of the changes

ghost commented 5 years ago

and even mac status changes are ignored in that case. So setenforce no longer affects dbus then.

ghost commented 5 years ago

@teg I think its this: https://github.com/bus1/dbus-broker/commit/5ad77724275cbb7831465cef0d93ca5b3084bd58#diff-5f25f6e3571b4ef6c9938932a3d8680f

I heve not exactly narrowed it down to one or more specific directives but commenting the above all out makes it work again.

BTW: systemctl edit --full is broken in 240....

ghost commented 5 years ago

It is the PrivateNetwork=yes, dbus-broker needs access to the netlink-selinux-socket

teg commented 5 years ago

@doverride thanks a lot for figuring this out! I'll follow up on the dbus-broker issue you created.