selinux_check_access is not thread-safe

selinux_check_access is not thread-safe

[Seth Moore]

Originally posted here: https://github.com/SELinuxProject/selinux/issues/287

By default, selinux_check_access does not appear to be thread-safe. It
calls avc_open, which then calls avc_init, passing NULL for all
callback function tables. The result is that no locking is done for
the AVC, which can corrupt the cache if multiple threads are calling
selinux_check_access.

It looks like calling avc_init, supplying lock callbacks, is the
"easy" answer. However, the avc_init man page says that avc_init is
deprecated. There's a new function for setting callbacks,
selinux_set_callback, but it does not seem to support locking.

I see a few possible solutions:
1. Update selinux_set_callback to support AVC lock functions.
2. Update the man pages to indicate selinux is not intended to be
thread-safe anymore.
3. Update the avc_init man page, indicating it's safe to use (un-deprecate?)

Note that we have observed buggy behavior with Android keystore2. Our
quick-n-dirty fix was a serializing lock around all selinux calls:
https://android.googlesource.com/platform/system/security/+/ff188d3a6ca38919e568f0c89f74d90c011526e9

My prefered fix is either #1 or #3, as they provide slightly
finger-grained locking than our fix.

Cheers,
Seth

Re: selinux_check_access is not thread-safe

[Stephen Smalley]

On Mon, May 24, 2021 at 1:24 PM Seth Moore <sethmo@google.com> wrote:
>
> Originally posted here: https://github.com/SELinuxProject/selinux/issues/287
>
> By default, selinux_check_access does not appear to be thread-safe. It
> calls avc_open, which then calls avc_init, passing NULL for all
> callback function tables. The result is that no locking is done for
> the AVC, which can corrupt the cache if multiple threads are calling
> selinux_check_access.
>
> It looks like calling avc_init, supplying lock callbacks, is the
> "easy" answer. However, the avc_init man page says that avc_init is
> deprecated. There's a new function for setting callbacks,
> selinux_set_callback, but it does not seem to support locking.
>
> I see a few possible solutions:
> 1. Update selinux_set_callback to support AVC lock functions.
> 2. Update the man pages to indicate selinux is not intended to be
> thread-safe anymore.
> 3. Update the avc_init man page, indicating it's safe to use (un-deprecate?)
>
> Note that we have observed buggy behavior with Android keystore2. Our
> quick-n-dirty fix was a serializing lock around all selinux calls:
> https://android.googlesource.com/platform/system/security/+/ff188d3a6ca38919e568f0c89f74d90c011526e9
>
> My prefered fix is either #1 or #3, as they provide slightly
> finger-grained locking than our fix.

I would recommend #1.