On Wed, Jul 28, 2021 at 10:03 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> Current code contains a lot of racy patterns when converting an
> ocontext's context structure to an SID. This is being done in a "lazy"
> fashion, such that the SID is looked up in the SID table only when it's
> first needed and then cached in the "sid" field of the ocontext
> structure. However, this is done without any locking or memory barriers
> and is thus unsafe.
>
> Between commits 24ed7fdae669 ("selinux: use separate table for initial
> SID lookup") and 66f8e2f03c02 ("selinux: sidtab reverse lookup hash
> table"), this race condition lead to an actual observable bug, because a
> pointer to the shared sid field was passed directly to
> sidtab_context_to_sid(), which was using this location to also store an
> intermediate value, which could have been read by other threads and
> interpreted as an SID. In practice this caused e.g. new mounts to get a
> wrong (seemingly random) filesystem context, leading to strange denials.
> This bug has been spotted in the wild at least twice, see [1] and [2].
>
> Fix the race condition by making all the racy functions use a common
> helper that ensures the ocontext::sid accesses are made safely using the
> appropriate SMP constructs.
>
> Note that security_netif_sid() was populating the sid field of both
> contexts stored in the ocontext, but only the first one was actually
> used. The SELinux wiki's documentation on the "netifcon" policy
> statement [3] suggests that using only the first context is intentional.
> I kept only the handling of the first context here, as there is really
> no point in doing the SID lookup for the unused one.
The security_netif_sid() change looks fine to me. I *think* that the
second netif label goes back to some of the original SELinux network
labeling, I believe the original intent may even predate my
involvement in the project but that is far enough back I can't say for
certain without doing some heavy mailing list searches :)
> I wasn't able to reproduce the bug mentioned above on any kernel that
> includes commit 66f8e2f03c02, even though it has been reported that the
> issue occurs with that commit, too, just less frequently. Thus, I wasn't
> able to verify that this patch fixes the issue, but it makes sense to
> avoid the race condition regardless.
>
> [1] https://github.com/containers/container-selinux/issues/89
> [2] https://lists.fedoraproject.org/archives/list/selinux@xxxxxxxxxxxxxxxxxxxxxxx/thread/6DMTAMHIOAOEMUAVTULJD45JZU7IBAFM/
> [3] https://selinuxproject.org/page/NetworkStatements#netifcon
>
> Reported-by: Sujithra Periasamy <sujithra@xxxxxxxxxx>
> Cc: Xinjie Zheng <xinjie@xxxxxxxxxx>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> security/selinux/ss/services.c | 162 ++++++++++++++++-----------------
> 1 file changed, 77 insertions(+), 85 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 0a5ce001609b..c8db8a3432e4 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2369,6 +2369,43 @@ err_policy:
> return rc;
> }
>
> +/**
> + * ocontext_to_sid - Helper to safely get sid for an ocontext
> + * @sidtab: SID table
> + * @c: ocontext structure
> + * @index: index of the context entry (0 or 1)
> + * @out_sid: pointer to the resulting SID value
> + *
> + * For all ocontexts except OCON_ISID the SID fields are populated
> + * on-demand when needed. Since updating the SID value is an SMP-sensitive
> + * operation, this helper must be used to do that safely.
> + *
> + * WARNING: This function may return -ESTALE, indicating that the caller
> + * must retry the operation after re-acquiring the policy pointer!
> + */
> +static int ocontext_to_sid(struct sidtab *sidtab, struct ocontext *c,
> + size_t index, u32 *out_sid)
> +{
> + int rc;
> + u32 sid;
> +
> + /* Ensure the associated sidtab entry is visible to this thread. */
> + sid = smp_load_acquire(&c->sid[index]);
Is there a reason why you decided to use smp_{load,store} to guard
ocontext.sid[] as opposed to RCU/spinlock? RCU would allow us to
avoid the memory barrier in smp_load_acquire() on every call to
ocontext_to_sid() and it looks like all of the non-exclusive callers
are already in a RCU protected section so the additional impact on an
already cached value should be next to nothing. The spinlock would
make things slightly more complicated (take the lock, recheck
ocontext.sid, set/bail, unlock, etc.) but we are already in the slow
path at that point.
> + if (!sid) {
> + rc = sidtab_context_to_sid(sidtab, &c->context[index], &sid);
> + if (rc)
> + return rc;
> +
> + /*
> + * Ensure the new sidtab entry is visible to other threads
> + * when they see the SID.
> + */
> + smp_store_release(&c->sid[index], sid);
> + }
> + *out_sid = sid;
> + return 0;
> +}
--
paul moore
www.paul-moore.com
