Linux-audit Archive on lore.kernel.org
 help / color / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: John Johansen <john.johansen@canonical.com>,
	Jeffrey Vander Stoep <jeffv@google.com>
Cc: selinux@vger.kernel.org, linux-security-module@vger.kernel.org,
	linux-audit@redhat.com
Subject: Re: [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials
Date: Wed, 10 Mar 2021 23:32:02 -0500
Message-ID: <CAHC9VhQmwFHFYZ2yCPDLWanjc1hzof7G3XO4fqPEX2ykiHCN3g@mail.gmail.com> (raw)
In-Reply-To: <b27662cf-4bcf-ec23-92f5-49a5b2f8c119@canonical.com>

On Tue, Mar 9, 2021 at 10:06 PM John Johansen
<john.johansen@canonical.com> 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 <paul@paul-moore.com>
>
> 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


  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-19 23:28 [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Paul Moore
2021-02-19 23:29 ` [RFC PATCH 1/4] lsm: separate security_task_getsecid() into subjective and objective variants Paul Moore
2021-02-20  2:55   ` James Morris
2021-02-20 14:44     ` Paul Moore
2021-03-04 10:04       ` Jeffrey Vander Stoep
2021-03-04 23:43         ` Paul Moore
2021-03-10  8:21           ` Jeffrey Vander Stoep
2021-03-11  1:56             ` Paul Moore
2021-02-21 12:51   ` John Johansen
2021-02-21 22:09     ` Paul Moore
2021-03-04  0:44     ` Paul Moore
2021-03-10  0:28       ` Paul Moore
2021-03-10  3:09         ` John Johansen
2021-02-24 16:49   ` Mimi Zohar
2021-03-08 19:25   ` Richard Guy Briggs
2021-03-10  0:23     ` Paul Moore
2021-03-10  1:03   ` John Johansen
2021-03-11  1:55     ` Paul Moore
2021-02-19 23:29 ` [RFC PATCH 2/4] selinux: clarify task subjective and objective credentials Paul Moore
2021-02-21 12:55   ` John Johansen
2021-03-08 19:26   ` Richard Guy Briggs
2021-03-10  3:05   ` John Johansen
2021-03-11  4:32     ` Paul Moore [this message]
2021-03-17 22:56       ` Paul Moore
2021-02-19 23:29 ` [RFC PATCH 3/4] smack: differentiate between subjective and objective task credentials Paul Moore
2021-02-21 12:56   ` John Johansen
2021-03-08 19:26   ` Richard Guy Briggs
2021-03-10  1:04   ` John Johansen
2021-02-19 23:29 ` [RFC PATCH 4/4] apparmor: " Paul Moore
2021-02-21 12:57   ` John Johansen
2021-02-21 22:12     ` Paul Moore
2021-02-20  1:49 ` [RFC PATCH 0/4] Split security_task_getsecid() into subj and obj variants Casey Schaufler
2021-02-20 14:41   ` Paul Moore
2021-02-22 23:58     ` Casey Schaufler
2021-02-23 14:14       ` Mimi Zohar
2021-02-24  0:03         ` Paul Moore
2021-03-04  0:46       ` Paul Moore
2021-03-04  2:21         ` Casey Schaufler
2021-03-04 23:41           ` Paul Moore

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHC9VhQmwFHFYZ2yCPDLWanjc1hzof7G3XO4fqPEX2ykiHCN3g@mail.gmail.com \
    --to=paul@paul-moore.com \
    --cc=jeffv@google.com \
    --cc=john.johansen@canonical.com \
    --cc=linux-audit@redhat.com \
    --cc=linux-security-module@vger.kernel.org \
    --cc=selinux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-audit Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-audit/0 linux-audit/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-audit linux-audit/ https://lore.kernel.org/linux-audit \
		linux-audit@redhat.com
	public-inbox-index linux-audit

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.linux-audit


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git