From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C4C9DC433E0 for ; Thu, 11 Mar 2021 04:32:36 +0000 (UTC) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 07CFA64FA7 for ; Thu, 11 Mar 2021 04:32:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 07CFA64FA7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=paul-moore.com Authentication-Results: mail.kernel.org; spf=tempfail smtp.mailfrom=linux-audit-bounces@redhat.com Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-454-q8JRP_FbMai3m-vTi2kseA-1; Wed, 10 Mar 2021 23:32:29 -0500 X-MC-Unique: q8JRP_FbMai3m-vTi2kseA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C84421005D4C; Thu, 11 Mar 2021 04:32:25 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DB65F60C05; Thu, 11 Mar 2021 04:32:24 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id CE04E5002F; Thu, 11 Mar 2021 04:32:22 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id 12B4WKHm017639 for ; Wed, 10 Mar 2021 23:32:21 -0500 Received: by smtp.corp.redhat.com (Postfix) id AD62E208C17A; Thu, 11 Mar 2021 04:32:20 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast03.extmail.prod.ext.rdu2.redhat.com [10.11.55.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A525D208C187 for ; Thu, 11 Mar 2021 04:32:18 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id EAFFE85521E for ; Thu, 11 Mar 2021 04:32:17 +0000 (UTC) Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-520-ZEnaakHEPuqPg7zdaj4MCQ-1; Wed, 10 Mar 2021 23:32:15 -0500 X-MC-Unique: ZEnaakHEPuqPg7zdaj4MCQ-1 Received: by mail-ed1-f54.google.com with SMTP id j3so768682edp.11 for ; Wed, 10 Mar 2021 20:32:14 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qHOhGjmvzrB2gk4cBTa5YvyzwnxpAyMed1nNXQc3Z4g=; b=gTOwlsAx8b6MKd8qfTvMLp11kBXjJlqaagu0hVyUJotBgylrHxHQk54BUVRZIe6trz xn4rtfui5zA65IH86zokcmd1JyFGHTSkw/MqeDORQr6JDQ8TENGcTv14WecXV8HQIGEM 7OF75M9IbMQ6uIQzw02knTxn+c4BZPzrTn6cg420VXs7jFHpQVHDgORN50wdwlK+VuZ9 x+NQOBOV9wEsohcJiiUZ9yVpEBXMwQkIOpPQUT6nT8tZXYo8uikd1l72HqNQx+/zwy7r /Toph3RhsutpY7CfwK0IGeUY/rlF/lFIJhGPGsZ3cdivSFtwrEk/7tOaYIXGCZBe+VvI ccpA== X-Gm-Message-State: AOAM53344gRJRhM89oFvqaUEuNG9jyMitHi2/1pvJmu7BQXjHTV4ycZc dbCbY8sEU61zrS8EAGaixjFD+tTunnMAufc3u/ET X-Google-Smtp-Source: ABdhPJx/YG3XP2UAuESOwSLpj64PL3u9CvRelFs99xuHBzzOiR3nWkSYxJxeLSHiEP/BKNcEPynWtODAArxRXIkHQY0= X-Received: by 2002:a05:6402:c96:: with SMTP id cm22mr6632426edb.128.1615437133890; Wed, 10 Mar 2021 20:32:13 -0800 (PST) MIME-Version: 1.0 References: <161377712068.87807.12246856567527156637.stgit@sifl> <161377735153.87807.7492842242100187888.stgit@sifl> In-Reply-To: From: Paul Moore Date: Wed, 10 Mar 2021 23:32:02 -0500 Message-ID: Subject: Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials To: John Johansen , Jeffrey Vander Stoep X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-loop: linux-audit@redhat.com Cc: selinux@vger.kernel.org, linux-security-module@vger.kernel.org, linux-audit@redhat.com X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=linux-audit-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tue, Mar 9, 2021 at 10:06 PM John Johansen wrote: > On 2/19/21 3:29 PM, Paul Moore wrote: > > SELinux has a function, task_sid(), which returns the task's > > objective credentials, but unfortunately is used in a few places > > where the subjective task credentials should be used. Most notably > > in the new security_task_getsecid_subj() LSM hook. > > > > This patch fixes this and attempts to make things more obvious by > > introducing a new function, task_sid_subj(), and renaming the > > existing task_sid() function to task_sid_obj(). > > > > Signed-off-by: Paul Moore > > I have a couple of questions below but the rest looks good > > > --- > > security/selinux/hooks.c | 85 +++++++++++++++++++++++++++------------------- > > 1 file changed, 49 insertions(+), 36 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index f311541c4972e..1c53000d28e37 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -229,10 +229,23 @@ static inline u32 cred_sid(const struct cred *cred) > > return tsec->sid; > > } > > > > +/* > > + * get the subjective security ID of a task > > + */ > > +static inline u32 task_sid_subj(const struct task_struct *task) > > +{ > > + u32 sid; > > + > > + rcu_read_lock(); > > + sid = cred_sid(rcu_dereference(task->cred)); > > + rcu_read_unlock(); > > + return sid; > > +} > > + > > /* > > * get the objective security ID of a task > > */ > > -static inline u32 task_sid(const struct task_struct *task) > > +static inline u32 task_sid_obj(const struct task_struct *task) > > { > > u32 sid; > > > > @@ -2034,11 +2047,8 @@ static inline u32 open_file_to_av(struct file *file) > > > > static int selinux_binder_set_context_mgr(struct task_struct *mgr) > > { > > - u32 mysid = current_sid(); > > - u32 mgrsid = task_sid(mgr); > > - > > return avc_has_perm(&selinux_state, > > - mysid, mgrsid, SECCLASS_BINDER, > > + current_sid(), task_sid_obj(mgr), SECCLASS_BINDER, > > BINDER__SET_CONTEXT_MGR, NULL); > > } > > > > @@ -2046,8 +2056,8 @@ static int selinux_binder_transaction(struct task_struct *from, > > struct task_struct *to) > > { > > u32 mysid = current_sid(); > > - u32 fromsid = task_sid(from); > > - u32 tosid = task_sid(to); > > + u32 fromsid = task_sid_subj(from); > > fromsid potentially gets used as both the subject and the object the following > permission checks. It makes sense to use the same cred for both checks but > what I am not sure about yet is whether its actually safe to use the subject > sid when the task isn't current. > > ie. I am still trying to determine if there is a race here between the transaction > request and the permission check. Okay, I see what you are concerned about now ... and unfortunately I'm not seeing a lot of precedence in the kernel for this type of usage either; the closest I can find is something like task_lock(), but that doesn't seem to cover the subjective creds. In fact, looking at override_creds(), there is nothing preventing a task from changing it's subjective creds at any point in time. Beyond the task_sid_subj() code here, looking back at patch 1 and the use of security_task_getsecid_subj() we look to be mostly safe (where safe means we are only inspecting the current task) with the exception of the binder code once again. There are some other exceptions but they are in the ptrace and audit code, both of which should be okay given the nature and calling context of the code. The problem really does seem to be just binder, and as I look at binder userspace example code, I'm starting to wonder if binder is setup properly to operate sanely in a situation where a process overrides its subject creds. It may be that we always need to use the objective/real creds with binder. Jeff, any binder insight here you can share with us? > > + u32 tosid = task_sid_subj(to); > its not clear to me that using the subj for to is correct Yes, I believe you are correct. Jeff, I know you looked at this code already, but I'm guessing you may have missed this (just as I did when I wrote it); are you okay with changing 'tosid' in selinux_binder_transaction() to the task's objective credentials? -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit