Re: NFS/LSM: allow NFS to control all of its own mount options

Re: NFS/LSM: allow NFS to control all of its own mount options

[Christoph Hellwig]

Please don't introduce a special case for just nfs.  All filesystems
should control their mount options, so please provide some library
helpers for context= handling and move it into all filesystems that
can support selinux.
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: NFS/LSM: allow NFS to control all of its own mount options

[Eric Paris]

On Tue, 2008-02-19 at 17:24 -0500, Christoph Hellwig wrote:
> Please don't introduce a special case for just nfs.  All filesystems
> should control their mount options, so please provide some library
> helpers for context= handling and move it into all filesystems that
> can support selinux.

A library helper that looks like what?

Only NFS knows how it is storing that mount option in its blobs.  Only
NFS knows how to translate its blob into the generic LSM interface
needed to set security options.  I'd say the solution is going to have
to be very much NFS specific.

Both in kernel LSMs already provide methods for dealing with mount
options for filesystems that use text strings (see the
security_sb_copy_data stuff called from vfs_kern_mount()).  How is this
'library' going to deal with anything other than a text string, and if
that's all it deals with we already have that.  NFS just can't use it
because it isn't using a string for mount data.  I'm sure I'm just
misunderstanding how to design your solution...

-Eric

Re: NFS/LSM: allow NFS to control all of its own mount options

[Casey Schaufler]

--- Christoph Hellwig <hch@infradead.org> wrote:

> Please don't introduce a special case for just nfs.  All filesystems
> should control their mount options, so please provide some library
> helpers for context= handling and move it into all filesystems that
> can support selinux.

Smack has options that are filesystem independent
(smackfsdef= smackfsroot= smackfsfloor= smackfshat=)
instead of the context= SELinux seems happy with.
Since there is no reason that a file system even
really needs to know what these values are it would
be completely unreasonable to teach every filesystem
about them. The information is completely controlled
and used by the LSM.

Of course, we could use something other than mount options
(vfsctl? sorry - only kidding) to set the LSM specific
information, and that might be the right approach.


Casey Schaufler
casey@schaufler-ca.com

Re: NFS/LSM: allow NFS to control all of its own mount options

[James Morris]

On Tue, 19 Feb 2008, Christoph Hellwig wrote:

> Please don't introduce a special case for just nfs.  All filesystems
> should control their mount options, so please provide some library
> helpers for context= handling and move it into all filesystems that
> can support selinux.

It's not so much a special case for NFS, just that NFS happens to use 
binary mount options.  So, I guess it could be put into a library for 
other potential filesystems with binary mount options.

To clarify:

The SELinux options are indeed filesystem independent, and the FS should 
really not need to be concerned at all with them.  For everything except 
NFS, we parse text options looking for context=, then use that value from 
within SELinux as the label for all files in the mount.

Previously, as Eric mentions, we were using a method initially approved by 
the NFS folk, where, for NFS, SELinux was peeking around inside the binary 
options.  We were then asked to change that so that NFS (or other 
binary-option FS) would obtain the values itself and call into LSM with 
them.  This is what Eric's latest patch enables (a previous patch 
installed the infrastructure for it).

While this code could be put into a library if desired, there is no need 
to make any changes for filesystems with text options (i.e. the general 
case).



- James
-- 
James Morris
<jmorris@namei.org>

Re: NFS/LSM: allow NFS to control all of its own mount options

[Stephen Smalley]

On Wed, 2008-02-20 at 11:25 +1100, James Morris wrote:
> On Tue, 19 Feb 2008, Christoph Hellwig wrote:
> 
> > Please don't introduce a special case for just nfs.  All filesystems
> > should control their mount options, so please provide some library
> > helpers for context= handling and move it into all filesystems that
> > can support selinux.
> 
> It's not so much a special case for NFS, just that NFS happens to use 
> binary mount options.  So, I guess it could be put into a library for 
> other potential filesystems with binary mount options.
> 
> To clarify:
> 
> The SELinux options are indeed filesystem independent, and the FS should 
> really not need to be concerned at all with them.  For everything except 
> NFS, we parse text options looking for context=, then use that value from 
> within SELinux as the label for all files in the mount.
> 
> Previously, as Eric mentions, we were using a method initially approved by 
> the NFS folk, where, for NFS, SELinux was peeking around inside the binary 
> options.  We were then asked to change that so that NFS (or other 
> binary-option FS) would obtain the values itself and call into LSM with 
> them.  This is what Eric's latest patch enables (a previous patch 
> installed the infrastructure for it).
> 
> While this code could be put into a library if desired, there is no need 
> to make any changes for filesystems with text options (i.e. the general 
> case).

And to be clear:  this patch fixes a real bug in the nfs/selinux
interaction on nohide mounts, a bug that needs to be fixed upstream as
soon as possible.  A bug that was introduced by changes in nfs, not
changes in selinux AFAIK, given that the original approach to context
mounts was introduced and approved by nfs folks long ago.  So regardless
of what happens wrt the text mount options, this patch needs to get
merged.

-- 
Stephen Smalley
National Security Agency

Re: NFS/LSM: allow NFS to control all of its own mount options

[Miklos Szeredi]

> Please don't introduce a special case for just nfs.  All filesystems
> should control their mount options, so please provide some library
> helpers for context= handling and move it into all filesystems that
> can support selinux.

Hmm, looks like selinux is not showing it's mount options in
/proc/mounts.  Well, actually there's no infrastructure for it either.
Here's a template patch (completely untested).

Selinux guys, please fill in the details and submit, thanks.

Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>

Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2008-02-20 10:51:11.000000000 +0100
+++ linux/fs/namespace.c	2008-02-20 10:51:25.000000000 +0100
@@ -385,6 +385,7 @@ static int show_vfsmnt(struct seq_file *
 		if (mnt->mnt_flags & fs_infop->flag)
 			seq_puts(m, fs_infop->str);
 	}
+	security_sb_show_options(m, mnt->mnt_sb);
 	if (mnt->mnt_sb->s_op->show_options)
 		err = mnt->mnt_sb->s_op->show_options(m, mnt);
 	seq_puts(m, " 0 0\n");
Index: linux/include/linux/security.h
===================================================================
--- linux.orig/include/linux/security.h	2008-02-18 21:20:03.000000000 +0100
+++ linux/include/linux/security.h	2008-02-20 11:02:04.000000000 +0100
@@ -80,6 +80,7 @@ struct xfrm_selector;
 struct xfrm_policy;
 struct xfrm_state;
 struct xfrm_user_sec_ctx;
+struct seq_file;
 
 extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
 extern int cap_netlink_recv(struct sk_buff *skb, int cap);
@@ -1226,6 +1227,7 @@ struct security_operations {
 	int (*sb_copy_data)(struct file_system_type *type,
 			    void *orig, void *copy);
 	int (*sb_kern_mount) (struct super_block *sb, void *data);
+	int (*sb_show_options) (struct seq_file *, struct super_block *sb);
 	int (*sb_statfs) (struct dentry *dentry);
 	int (*sb_mount) (char *dev_name, struct nameidata * nd,
 			 char *type, unsigned long flags, void *data);
@@ -1487,6 +1489,7 @@ int security_sb_alloc(struct super_block
 void security_sb_free(struct super_block *sb);
 int security_sb_copy_data(struct file_system_type *type, void *orig, void *copy);
 int security_sb_kern_mount(struct super_block *sb, void *data);
+int security_sb_show_options(struct seq_file *, struct super_block *sb);
 int security_sb_statfs(struct dentry *dentry);
 int security_sb_mount(char *dev_name, struct nameidata *nd,
                        char *type, unsigned long flags, void *data);
@@ -1744,6 +1747,12 @@ static inline int security_sb_kern_mount
 	return 0;
 }
 
+static inline int security_sb_show_options (struct seq_file *m,
+					    struct super_block *sb)
+{
+	return 0;
+}
+
 static inline int security_sb_statfs (struct dentry *dentry)
 {
 	return 0;
Index: linux/security/security.c
===================================================================
--- linux.orig/security/security.c	2008-02-18 21:20:06.000000000 +0100
+++ linux/security/security.c	2008-02-20 10:56:16.000000000 +0100
@@ -252,6 +252,14 @@ int security_sb_kern_mount(struct super_
 	return security_ops->sb_kern_mount(sb, data);
 }
 
+int security_sb_show_options (struct seq_file *m, struct super_block *sb)
+{
+	if (security_ops->sb_show_options)
+		return security_ops->sb_show_options(m, sb);
+	else
+		return 0;
+}
+
 int security_sb_statfs(struct dentry *dentry)
 {
 	return security_ops->sb_statfs(dentry);
Index: linux/security/selinux/hooks.c
===================================================================
--- linux.orig/security/selinux/hooks.c	2008-02-18 21:20:06.000000000 +0100
+++ linux/security/selinux/hooks.c	2008-02-20 10:58:57.000000000 +0100
@@ -590,6 +590,12 @@ out:
 	return rc;
 }
 
+static int selinux_sb_show_options(struct seq_file *m, struct super_block *sb)
+{
+	/* ... */
+	return 0;
+}
+
 static int superblock_doinit(struct super_block *sb, void *data)
 {
 	struct superblock_security_struct *sbsec = sb->s_security;
@@ -4797,6 +4803,7 @@ static struct security_operations selinu
 	.sb_free_security =		selinux_sb_free_security,
 	.sb_copy_data =			selinux_sb_copy_data,
 	.sb_kern_mount =	        selinux_sb_kern_mount,
+	.sb_show_options =		selinux_sb_show_options,
 	.sb_statfs =			selinux_sb_statfs,
 	.sb_mount =			selinux_mount,
 	.sb_umount =			selinux_umount,

Re: NFS/LSM: allow NFS to control all of its own mount options

[Stephen Smalley]

On Wed, 2008-02-20 at 11:08 +0100, Miklos Szeredi wrote:
> > Please don't introduce a special case for just nfs.  All filesystems
> > should control their mount options, so please provide some library
> > helpers for context= handling and move it into all filesystems that
> > can support selinux.
> 
> Hmm, looks like selinux is not showing it's mount options in
> /proc/mounts.  Well, actually there's no infrastructure for it either.
> Here's a template patch (completely untested).

I think the intent is to use the security_sb_get_mnt_opts() hook for
this purpose.

> 
> Selinux guys, please fill in the details and submit, thanks.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
> 
> Index: linux/fs/namespace.c
> ===================================================================
> --- linux.orig/fs/namespace.c	2008-02-20 10:51:11.000000000 +0100
> +++ linux/fs/namespace.c	2008-02-20 10:51:25.000000000 +0100
> @@ -385,6 +385,7 @@ static int show_vfsmnt(struct seq_file *
>  		if (mnt->mnt_flags & fs_infop->flag)
>  			seq_puts(m, fs_infop->str);
>  	}
> +	security_sb_show_options(m, mnt->mnt_sb);
>  	if (mnt->mnt_sb->s_op->show_options)
>  		err = mnt->mnt_sb->s_op->show_options(m, mnt);
>  	seq_puts(m, " 0 0\n");
> Index: linux/include/linux/security.h
> ===================================================================
> --- linux.orig/include/linux/security.h	2008-02-18 21:20:03.000000000 +0100
> +++ linux/include/linux/security.h	2008-02-20 11:02:04.000000000 +0100
> @@ -80,6 +80,7 @@ struct xfrm_selector;
>  struct xfrm_policy;
>  struct xfrm_state;
>  struct xfrm_user_sec_ctx;
> +struct seq_file;
>  
>  extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
>  extern int cap_netlink_recv(struct sk_buff *skb, int cap);
> @@ -1226,6 +1227,7 @@ struct security_operations {
>  	int (*sb_copy_data)(struct file_system_type *type,
>  			    void *orig, void *copy);
>  	int (*sb_kern_mount) (struct super_block *sb, void *data);
> +	int (*sb_show_options) (struct seq_file *, struct super_block *sb);
>  	int (*sb_statfs) (struct dentry *dentry);
>  	int (*sb_mount) (char *dev_name, struct nameidata * nd,
>  			 char *type, unsigned long flags, void *data);
> @@ -1487,6 +1489,7 @@ int security_sb_alloc(struct super_block
>  void security_sb_free(struct super_block *sb);
>  int security_sb_copy_data(struct file_system_type *type, void *orig, void *copy);
>  int security_sb_kern_mount(struct super_block *sb, void *data);
> +int security_sb_show_options(struct seq_file *, struct super_block *sb);
>  int security_sb_statfs(struct dentry *dentry);
>  int security_sb_mount(char *dev_name, struct nameidata *nd,
>                         char *type, unsigned long flags, void *data);
> @@ -1744,6 +1747,12 @@ static inline int security_sb_kern_mount
>  	return 0;
>  }
>  
> +static inline int security_sb_show_options (struct seq_file *m,
> +					    struct super_block *sb)
> +{
> +	return 0;
> +}
> +
>  static inline int security_sb_statfs (struct dentry *dentry)
>  {
>  	return 0;
> Index: linux/security/security.c
> ===================================================================
> --- linux.orig/security/security.c	2008-02-18 21:20:06.000000000 +0100
> +++ linux/security/security.c	2008-02-20 10:56:16.000000000 +0100
> @@ -252,6 +252,14 @@ int security_sb_kern_mount(struct super_
>  	return security_ops->sb_kern_mount(sb, data);
>  }
>  
> +int security_sb_show_options (struct seq_file *m, struct super_block *sb)
> +{
> +	if (security_ops->sb_show_options)
> +		return security_ops->sb_show_options(m, sb);
> +	else
> +		return 0;
> +}
> +
>  int security_sb_statfs(struct dentry *dentry)
>  {
>  	return security_ops->sb_statfs(dentry);
> Index: linux/security/selinux/hooks.c
> ===================================================================
> --- linux.orig/security/selinux/hooks.c	2008-02-18 21:20:06.000000000 +0100
> +++ linux/security/selinux/hooks.c	2008-02-20 10:58:57.000000000 +0100
> @@ -590,6 +590,12 @@ out:
>  	return rc;
>  }
>  
> +static int selinux_sb_show_options(struct seq_file *m, struct super_block *sb)
> +{
> +	/* ... */
> +	return 0;
> +}
> +
>  static int superblock_doinit(struct super_block *sb, void *data)
>  {
>  	struct superblock_security_struct *sbsec = sb->s_security;
> @@ -4797,6 +4803,7 @@ static struct security_operations selinu
>  	.sb_free_security =		selinux_sb_free_security,
>  	.sb_copy_data =			selinux_sb_copy_data,
>  	.sb_kern_mount =	        selinux_sb_kern_mount,
> +	.sb_show_options =		selinux_sb_show_options,
>  	.sb_statfs =			selinux_sb_statfs,
>  	.sb_mount =			selinux_mount,
>  	.sb_umount =			selinux_umount,
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Stephen Smalley
National Security Agency

Re: NFS/LSM: allow NFS to control all of its own mount options

[Eric Paris]

On Wed, 2008-02-20 at 08:50 -0500, Stephen Smalley wrote:
> On Wed, 2008-02-20 at 11:08 +0100, Miklos Szeredi wrote:
> > > Please don't introduce a special case for just nfs.  All filesystems
> > > should control their mount options, so please provide some library
> > > helpers for context= handling and move it into all filesystems that
> > > can support selinux.
> > 
> > Hmm, looks like selinux is not showing it's mount options in
> > /proc/mounts.  Well, actually there's no infrastructure for it either.
> > Here's a template patch (completely untested).
> 
> I think the intent is to use the security_sb_get_mnt_opts() hook for
> this purpose.

It was.  I already knew about this issue and its 'on my list.'  Although
I guess we need a something ?new LSM hook? which will translate the
sb_get_mnt_opts stuff into a single text string.  Or I guess really that
can be done in you sb_show_options and I can just use sb_get_mnt_opts
under the covers.  Anyway, unrelated issue that will get fixed as soon
as this real BUG() is fixed.

-Eric

-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html