Re: [PATCH v2 2/2] NFSv4 account for selinux security context when deciding to share superblock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 18, 2021 at 6:17 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>
> On 2/18/2021 2:39 PM, Olga Kornievskaia wrote:
> > On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> >> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> >>> From: Olga Kornievskaia <kolga@xxxxxxxxxx>
> >>>
> >>> Keep track of whether or not there was an selinux context mount
> >>> options during the mount.
> >> This may be the intention, but it's not what the change you're
> >> introducing here does.
> > Can you explain why, as I must be doing something wrong? I'm familiar
> > with NFS but not with Selinux and I thought I was doing the correct
> > changes to the Selinux. If that's not the case would you share what
> > has been done incorrectly.
>
> The code in this patch is generic for any LSM that wants to provide
> a security_sb_mnt_opts_compat() hook. Your 1/2 patch deals with how
> the hook provided by SELinux behaves.  All the behavior that is specific
> to SELinux should be in the SELinux hook. If you can state the behavior
> generically you should do that here.

Hopefully by removing specific reference to the selinux and sticking
with LSM addresses your comment. To NFS it's a security context it
doesn't care if it's selinux or something else.

> >>>  While deciding if the superblock can be
> >>> shared for the new mount, check for if we had selinux context on
> >>> the existing mount and call into selinux to tell if new passed
> >>> in selinux context is compatible with the existing mount's options.
> >> You're describing how you expect the change to be used, not
> >> what it does. If I am the author of a security module other
> >> than SELinux (which, it turns out, I am) what would I use this
> >> for? How might this change interact with my security module?
> >> Is this something I might exploit? If I am the author of a
> >> filesystem other than NFS (which I am not) should I be doing
> >> the same thing?
> > I'm not sure I'm understanding your questions. I'm solving a bug that
> > exists when NFS interacts with selinux.
>
> Right, but you're introducing an LSM interface to do so. LSM interfaces
> are expected to be usable by any security module. Smack ought to be able
> to provide NFS support, so might be expected to provide a hook for
> security_sb_mnt_opts_compat().

So I thought to address how a filesystem uses the hooks should have
been in the first patch and I added it here. But addressing how
another LSM module (like Smack) would use it again should be coming
from the first patch. It's a new hook to compare mount options and if
Smack were to implement some security mount options it would implement
a comparison function of the two.


>
> >  This is really not a feature
> > that I'm introducing. I thought it was somewhat descriptive with an
> > example that if you want to mount with different security contexts but
> > whatever you are mounting has a possibility of sharing the same
> > superblock, we need to be careful and take security contexts that are
> > being used by those mount into account to decide whether or not to
> > share the superblock. Again I thought that's exactly what the commit
> > states. A different security module, if it acts on security context,
> > would have to have the same ability to compare if one context options
> > are compatible with anothers.
>
> Not everyone is going to extrapolate the general behavior from
> the SELinux behavior. Your description of the SELinux behavior in 1/2
> is fine. I'm looking for how a module other than SELinux would use
> this.
>
> >  Another filesystem can decide if it
> > wants to share superblocks based on compatibility of existing security
> > context and new one. Is that what you are asking?
>
> What I'm asking is whether this should be a concern for filesystems
> in general, in which case this isn't the right place to make this change.
>
> >
> >
> >>> Previously, NFS wasn't able to do the following 2mounts:
> >>> mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
> >>> <serverip>:/ /mnt
> >>> mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
> >>> <serverip>:/scratch /scratch
> >>>
> >>> 2nd mount would fail with "mount.nfs: an incorrect mount option was
> >>> specified" and var log messages would have:
> >>> "SElinux: mount invalid. Same superblock, different security
> >>> settings for.."
> >>>
> >>> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> >>> ---
> >>>  fs/nfs/fs_context.c       | 3 +++
> >>>  fs/nfs/internal.h         | 1 +
> >>>  fs/nfs/super.c            | 4 ++++
> >>>  include/linux/nfs_fs_sb.h | 1 +
> >>>  4 files changed, 9 insertions(+)
> >>>
> >>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> >>> index 06894bcdea2d..8067f055d842 100644
> >>> --- a/fs/nfs/fs_context.c
> >>> +++ b/fs/nfs/fs_context.c
> >>> @@ -448,6 +448,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
> >>>       if (opt < 0)
> >>>               return ctx->sloppy ? 1 : opt;
> >>>
> >>> +     if (fc->security)
> >>> +             ctx->has_sec_mnt_opts = 1;
> >>> +
> >>>       switch (opt) {
> >>>       case Opt_source:
> >>>               if (fc->source)
> >>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> >>> index 62d3189745cd..08f4f34e8cf5 100644
> >>> --- a/fs/nfs/internal.h
> >>> +++ b/fs/nfs/internal.h
> >>> @@ -96,6 +96,7 @@ struct nfs_fs_context {
> >>>       char                    *fscache_uniq;
> >>>       unsigned short          protofamily;
> >>>       unsigned short          mountfamily;
> >>> +     bool                    has_sec_mnt_opts;
> >>>
> >>>       struct {
> >>>               union {
> >>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >>> index 4034102010f0..0a2d252cf90f 100644
> >>> --- a/fs/nfs/super.c
> >>> +++ b/fs/nfs/super.c
> >>> @@ -1058,6 +1058,7 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
> >>>                                                &sb->s_blocksize_bits);
> >>>
> >>>       nfs_super_set_maxbytes(sb, server->maxfilesize);
> >>> +     server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
> >>>  }
> >>>
> >>>  static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
> >>> @@ -1174,6 +1175,9 @@ static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
> >>>               return 0;
> >>>       if (!nfs_compare_userns(old, server))
> >>>               return 0;
> >>> +     if ((old->has_sec_mnt_opts || fc->security) &&
> >>> +                     security_sb_mnt_opts_compat(sb, fc->security))
> >>> +             return 0;
> >>>       return nfs_compare_mount_options(sb, server, fc);
> >>>  }
> >>>
> >>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> >>> index 38e60ec742df..3f0acada5794 100644
> >>> --- a/include/linux/nfs_fs_sb.h
> >>> +++ b/include/linux/nfs_fs_sb.h
> >>> @@ -254,6 +254,7 @@ struct nfs_server {
> >>>
> >>>       /* User namespace info */
> >>>       const struct cred       *cred;
> >>> +     bool                    has_sec_mnt_opts;
> >>>  };
> >>>
> >>>  /* Server capabilities */
>



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux