From: Stephen Smalley <stephen.smalley.work@gmail.com>
To: Paul Moore <paul@paul-moore.com>
Cc: Gabriel Ryan <gabe@cs.columbia.edu>,
omosnace@redhat.com, selinux@vger.kernel.org,
dpquiql@davequigley.com
Subject: Re: Race in security/selinux/hooks.c on isec->sclass and isec->sid usage
Date: Fri, 19 Jan 2024 10:43:38 -0500 [thread overview]
Message-ID: <CAEjxPJ5aFRC86k9vFE-TWO=3TQZxD+scaWo9F-cgRUoAnbK2Zg@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhSnbU_0WEFVrgm2rakNg3aeCs2OE1of0BavgOTmJWwnAQ@mail.gmail.com>
On Thu, Jan 18, 2024 at 4:52 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jan 18, 2024 at 3:47 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> > On Thu, Jan 18, 2024 at 3:19 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Jan 18, 2024 at 12:02 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Thu, Jan 18, 2024 at 11:26 AM Gabriel Ryan <gabe@cs.columbia.edu> wrote:
> > > > >
> > > > > We found a race in selinux for kernel v6.6 using a prototype race
> > > > > testing tool based on modified KCSAN we are developing. We are
> > > > > reporting the race because it appears to be a potential bug. The race
> > > > > occurs on isec->sclass and isec->sid, which are set in
> > > > >
> > > > > security/selinux/hooks.c:3329-3330 selinux_inode_post_setxattr
> > > > >
> > > > > isec->sclass = inode_mode_to_security_class(inode->i_mode);
> > > > > isec->sid = newsid;
> > > > >
> > > > > Where isec->lock is held when isec->sclass and isec->sid are set above
> > > > > but not held when they are read in the following 3 locations:
> > > > >
> > > > > security/selinux/hooks.c:1671 inode_has_perm
> > > > > security/selinux/hooks.c:3125 selinux_inode_permission
> > > > > security/selinux/hooks.c:3690 ioctl_has_perm
> > > > >
> > > > >
> > > > > This seems like it could lead to undefined behavior if multiple
> > > > > threads are reading the isec struct and updating it concurrently,
> > > > > (e.g., reading an old isec->sid value but new isec->sclass value).
> > > > >
> > > > > In some other cases in security/selinux/hooks.c, isec->lock is held
> > > > > when isec->sclass and isec->sid are accessed, such as in
> > > > > security/selinux/hooks.c:4942-4945 selinux_socket_accept. Therefore,
> > > > > extending the isec->lock to cover when sclass and sid are read from
> > > > > the isec struct in these three locations might be a suitable fix.
> > > >
> > > > isec->sclass should only really need to be set once when isec is first
> > > > initialized after mode format bits have been set.
> > > > Not sure why it is getting assigned again in post_setxattr.
> > >
> > > There is similar odd behavior in selinux_inode_setsecurity(). Looking
> > > at the other places in hooks.c where we are setting isec->sclass, I'm
> > > wondering if this is a copy-n-paste from one of the other places that
> > > does have a need for it. The pattern of "lock, set the sclass and
> > > SID, mark the inode as initialized, unlock" occurs in at least three
> > > places that I can see in a quick search.
> >
> > git blame indicates that the setting of isec->sclass was added to
> > inode_post_setxattr() and inode_setsecurity() by commit
> > aa9c2669626ca7e5e5ba ("NFS: Client implementation of Labeled-NFS").
> > Not sure why - do NFS inodes get initialized in some manner that skips
> > the usual setting?
>
> I don't know, but I wouldn't be surprised, we've definitely had to do
> some NFS-specific things over the years.
>
> > It's fine to set it in inode_doinit_with_dentry() when initializing
> > the inode security blob for existing inodes and in
> > selinux_inode_init_security() for newly created ones - no other thread
> > can be reading it at that point.
>
> Yes, when I said "odd behavior" I was trying to get at the original
> discussion that it doesn't make much sense there, regardless of the
> safety. I can potentially see a need for it as-is in
> inode_doinit_with_dentry() but I haven't chased down all the call
> chains as that would take a while, and like you say, it should be
> safe.
>
> > selinux_task_to_inode() is a special case for /proc inodes.
>
> Yes, I believe that needs to stay.
>
> > selinux_socket_post_create() and
> > selinux_socket_accept() should set it before the socket can be
> > accessed by userspace or via an incoming packet.
>
> Agreed. (it looks okay there as-is)
Arguably setting it in post_setxattr and _setsecurity is a bug because
it would replace a socket security class with a file security class on
a socket inode if one performed a fsetxattr() on a socket. We don't
really support that anyway since there is no way to propagate it down
to the sock.
Tempting to remove from both and run the NFS tests (tools/nfs.sh) to
check for regressions.
prev parent reply other threads:[~2024-01-19 15:43 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-18 16:26 Race in security/selinux/hooks.c on isec->sclass and isec->sid usage Gabriel Ryan
2024-01-18 17:02 ` Stephen Smalley
2024-01-18 20:19 ` Paul Moore
2024-01-18 20:47 ` Stephen Smalley
2024-01-18 21:52 ` Paul Moore
2024-01-19 15:43 ` Stephen Smalley [this message]
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='CAEjxPJ5aFRC86k9vFE-TWO=3TQZxD+scaWo9F-cgRUoAnbK2Zg@mail.gmail.com' \
--to=stephen.smalley.work@gmail.com \
--cc=dpquiql@davequigley.com \
--cc=gabe@cs.columbia.edu \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).