Le 22/03/18 à 17:09, Stephen Smalley a écrit :
On 03/21/2018 07:58 AM, Laurent Bigonville wrote:
Hello,
Could somebody have a quick look at the two patches that I opened for two dbus bugs:
https://bugs.freedesktop.org/show_bug.cgi?id=92831 (stop using avc_init())
https://bugs.freedesktop.org/attachment.cgi?id=138021 (stop using selinux_set_mapping())
I'm also wondering whether the call to avc_add_callback() shouldn't be replaced by selinux_set_callback(), an opinion on this?
Patches look sane to me although I'm not really familiar with dbus code.
Thanks for the review, Simon already had a look at the dbus part of the code
Looks like the callback is only used to trigger a reload of the dbus configuration (for dbus_contexts updates), and thus selinux_set_callback(SELINUX_CB_POLICYLOAD) is more appropriate than avc_add_callback(AVC_CALLBACK_RESET), since the latter is called upon setenforce 1 as well. However, if it were truly only for that purpose, one might argue that it ought to be a watch on the dbus_contexts file instead and not be tied to selinux at all.
I really don't know the original rational of this. But I guess that if
somebody is modifying dbus_contexts file, there are big chances that he
will reload the policy as well(?).
I'll change avc_add_callback() by selinux_set_callback(), we could say
that as the file is in the SELinux path it's its responsibility.
NB This still won't fix the case where dbusd has already performed a string_to_security_class/av_perm lookup and the result has been cached by the libselinux class cache and then a subsequent policy update alters those values. That is what was fixed for systemd's usage of selinux_check_access() by selinux commit b408d72ca9104cb0c1bc4e154d8732cc7c0a9190. Offhand, I'm now wondering why I didn't just call flush_class_cache() from avc_reset() itself. That would fix it for other users of the AVC. You can't directly call flush_class_cache() from the dbus selinux policyload callback because it is hidden presently. If we can fix it directly in libselinux, then that is better. If not, we'd need to export it and probably give it a more unique name, ala selinux_flush_class_cache().
Right, that's a really good point, that I apparently overlooked...
Is that cache really supposed to substantially speedup things? Would it
be possible to create a version of selinux_check_access() that allows to
pass a reference the cache or let selinux_check_access() create that
cache itself? If it's the case I guess that dbus-broker would benefit of
that as well as they are using selinux_check_access().
Otherwise we can indeed clean up the cache our self, but wasn't the goal
of selinux_check_access() to be an "easy" interface to use, asking the
applications to do this kind of housekeeping is defeating that purpose,
isn't it?
