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?
