this might be a good opportunity to replace calls to avc_has_perm() with calls to selinux_access_check() in selinux.c about dbus-broker

[dvdhrm]

Thanks for the hint!

selinux_access_check() resolves the contexts on every call. avc_has_perm() allows us to cache the lookups by storing the sid pointers rather than the context-strings. Is this intentional?

On Wed, Aug 16, 2017 at 04:07:14PM +0000, David Herrmann wrote: Thanks for the hint! `selinux_access_check()` resolves the contexts on every call. `avc_has_perm()` allows us to cache the lookups by storing the sid pointers rather than the context-strings. Is this intentional?

I am not very familiar with all the reasons why selinux_access_check() should be used over avc_has_perm() and if the above has anything to do with the following: the avc_has_perm() causes "user space object managers" (like dbus) to become confused when there is interference with the ordering of "access vectors", and this might actually be due to that caching aspect (although i am not sure) what that means in practical terms is that with `avc_has_perm()` one can, at runtime, make dbus confused and block all access if one "interferes with the ordering of access vectors" by for example adding a new access vector at runtime. this, today, is becoming a greater issue since systemd for example to a greater extend relies on dbus. so if i interfere with the ordering of access vectors at runtime by adding a new policy module that declares a new access vector for example, then dbus potentially becomes confused (possibly due to the caching aspect you described) and blocks all access. that means that systemd can no longer do its job properly and well, if systemd can't do its job then theres a problem. By the way, systemd also uses `selinux_access_check()`. So you may find examples of its usage there

…

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #16 (comment)

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

[bigon]

If you are caching the security contexts I guess you should at least invalidate the cache if the policy is reloaded, see selinux_set_callback(3) manpage (and SELINUX_CB_POLICYLOAD)

[teg]

@bigon could you expand a bit on that? This is my understanding, please correct me if I'm wrong:

1. The security context of a peer (which is the SO_SECLABEL of its socket) is pinned in the file description at open() and stays fixed regardless of any change to security policy.
2. The SID returned by avc_context_to_sid() is valid for the lifetime of the daemon (we only call avc_destroy() when shutting down the daemon).
3. We need to listen for SELINUX_CB_POLICYLOAD to reload the security contexts of names, and then obviously get a new SID if the context changes (we currently do not do this).

Are you saying that we also need to re-resolve all SIDs when the policy changes? Could we get a different SID back, even if the context is guaranteed to be the same?

[bigon]

@teg I'm far from being an expert here, but I think that security context can be invalidated (the label become unlabeled_t) when a module is disabled or a policy is updated. But the sid itself might still be valid.

For the original question from @doverride (avc_has_perm vs selinux_check_access) I guess this could be discussed on the SELinux from the NSA

[dvdhrm]

Lets just switch to selinux_check_access(). Makes the code simpler, and we follow the upstream recommendation.

If this ends up slowing down the message bus considerably, we will need an upstream solution, anyway. Relying on deprecated interfaces sounds not right.

[teg]

Yeah, I'm fine with that.

[dvdhrm]

Fixed!

Thanks for the report!