[PATCH] NFS: Ensure security label is set for root inode

[PATCH] NFS: Ensure security label is set for root inode

[Scott Mayhew]

When using NFSv4.2, the security label for the root inode should be set
via a call to nfs_setsecurity() during the mount process, otherwise the
inode will appear as unlabeled for up to acdirmin seconds.  Currently
the label for the root inode is allocated, retrieved, and freed entirely
witin nfs4_proc_get_root().

Add a field for the label to the nfs_fattr struct, and allocate & free
the label in nfs_get_root(), where we also add a call to
nfs_setsecurity().  Note that for the call to nfs_setsecurity() to
succeed, it's necessary to also move the logic calling
security_sb_{set,clone}_security() from nfs_get_tree_common() down into
nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the
super_block's security flags and nfs_setsecurity() will silently fail.

Reported-by: Richard Haines <richard_c_haines@btinternet.com>
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 fs/nfs/getroot.c        | 39 +++++++++++++++++++++++++++++++++++----
 fs/nfs/nfs4proc.c       | 12 +++---------
 fs/nfs/super.c          | 25 -------------------------
 include/linux/nfs_xdr.h |  1 +
 4 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
index b012c2668a1f..6d0628149390 100644
--- a/fs/nfs/getroot.c
+++ b/fs/nfs/getroot.c
@@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc)
 	struct inode *inode;
 	char *name;
 	int error = -ENOMEM;
+	unsigned long kflags = 0, kflags_out = 0;
 
 	name = kstrdup(fc->source, GFP_KERNEL);
 	if (!name)
@@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc)
 	if (fsinfo.fattr == NULL)
 		goto out_name;
 
+	fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL);
+	if (IS_ERR(fsinfo.fattr->label))
+		goto out_fattr;
 	error = server->nfs_client->rpc_ops->getroot(server, ctx->mntfh, &fsinfo);
 	if (error < 0) {
 		dprintk("nfs_get_root: getattr error = %d\n", -error);
 		nfs_errorf(fc, "NFS: Couldn't getattr on root");
-		goto out_fattr;
+		goto out_label;
 	}
 
 	inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL);
@@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc)
 		dprintk("nfs_get_root: get root inode failed\n");
 		error = PTR_ERR(inode);
 		nfs_errorf(fc, "NFS: Couldn't get root inode");
-		goto out_fattr;
+		goto out_label;
 	}
 
 	error = nfs_superblock_set_dummy_root(s, inode);
 	if (error != 0)
-		goto out_fattr;
+		goto out_label;
 
 	/* root dentries normally start off anonymous and get spliced in later
 	 * if the dentry tree reaches them; however if the dentry already
@@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc)
 		dprintk("nfs_get_root: get root dentry failed\n");
 		error = PTR_ERR(root);
 		nfs_errorf(fc, "NFS: Couldn't get root dentry");
-		goto out_fattr;
+		goto out_label;
 	}
 
 	security_d_instantiate(root, inode);
@@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct fs_context *fc)
 	}
 	spin_unlock(&root->d_lock);
 	fc->root = root;
+	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
+		kflags |= SECURITY_LSM_NATIVE_LABELS;
+	if (ctx->clone_data.sb) {
+		if (d_inode(fc->root)->i_fop != &nfs_dir_operations) {
+			error = -ESTALE;
+			goto error_splat_root;
+		}
+		/* clone any lsm security options from the parent to the new sb */
+		error = security_sb_clone_mnt_opts(ctx->clone_data.sb, s, kflags,
+				&kflags_out);
+	} else {
+		error = security_sb_set_mnt_opts(s, fc->security,
+							kflags, &kflags_out);
+	}
+	if (error)
+		goto error_splat_root;
+	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
+		!(kflags_out & SECURITY_LSM_NATIVE_LABELS))
+		NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
+
+	nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label);
 	error = 0;
 
+out_label:
+	nfs4_label_free(fsinfo.fattr->label);
 out_fattr:
 	nfs_free_fattr(fsinfo.fattr);
 out_name:
 	kfree(name);
 out:
 	return error;
+error_splat_root:
+	dput(fc->root);
+	fc->root = NULL;
+	goto out_label;
 }
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 69b7ab7a5815..cb34e840e4fb 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4002,7 +4002,7 @@ static int nfs4_proc_get_root(struct nfs_server *server, struct nfs_fh *mntfh,
 {
 	int error;
 	struct nfs_fattr *fattr = info->fattr;
-	struct nfs4_label *label = NULL;
+	struct nfs4_label *label = fattr->label;
 
 	error = nfs4_server_capabilities(server, mntfh);
 	if (error < 0) {
@@ -4010,23 +4010,17 @@ static int nfs4_proc_get_root(struct nfs_server *server, struct nfs_fh *mntfh,
 		return error;
 	}
 
-	label = nfs4_label_alloc(server, GFP_KERNEL);
-	if (IS_ERR(label))
-		return PTR_ERR(label);
-
 	error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL);
 	if (error < 0) {
 		dprintk("nfs4_get_root: getattr error = %d\n", -error);
-		goto err_free_label;
+		goto out;
 	}
 
 	if (fattr->valid & NFS_ATTR_FATTR_FSID &&
 	    !nfs_fsid_equal(&server->fsid, &fattr->fsid))
 		memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid));
 
-err_free_label:
-	nfs4_label_free(label);
-
+out:
 	return error;
 }
 
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index dada09b391c6..bb14bede6da5 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc)
 	struct super_block *s;
 	int (*compare_super)(struct super_block *, struct fs_context *) = nfs_compare_super;
 	struct nfs_server *server = ctx->server;
-	unsigned long kflags = 0, kflags_out = 0;
 	int error;
 
 	ctx->server = NULL;
@@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc)
 		goto error_splat_super;
 	}
 
-	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
-		kflags |= SECURITY_LSM_NATIVE_LABELS;
-	if (ctx->clone_data.sb) {
-		if (d_inode(fc->root)->i_fop != &nfs_dir_operations) {
-			error = -ESTALE;
-			goto error_splat_root;
-		}
-		/* clone any lsm security options from the parent to the new sb */
-		error = security_sb_clone_mnt_opts(ctx->clone_data.sb, s, kflags,
-				&kflags_out);
-	} else {
-		error = security_sb_set_mnt_opts(s, fc->security,
-							kflags, &kflags_out);
-	}
-	if (error)
-		goto error_splat_root;
-	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
-		!(kflags_out & SECURITY_LSM_NATIVE_LABELS))
-		NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
-
 	s->s_flags |= SB_ACTIVE;
 	error = 0;
 
@@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc)
 out_err_nosb:
 	nfs_free_server(server);
 	goto out;
-
-error_splat_root:
-	dput(fc->root);
-	fc->root = NULL;
 error_splat_super:
 	deactivate_locked_super(s);
 	goto out;
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index 94c77ed55ce1..6838c149f335 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -75,6 +75,7 @@ struct nfs_fattr {
 	struct nfs4_string	*owner_name;
 	struct nfs4_string	*group_name;
 	struct nfs4_threshold	*mdsthreshold;	/* pNFS threshold hints */
+	struct nfs4_label	*label;
 };
 
 #define NFS_ATTR_FATTR_TYPE		(1U << 0)
-- 
2.24.1

Re: [PATCH] NFS: Ensure security label is set for root inode

[Richard Haines]

On Tue, 2020-03-03 at 17:58 -0500, Scott Mayhew wrote:
> When using NFSv4.2, the security label for the root inode should be
> set
> via a call to nfs_setsecurity() during the mount process, otherwise
> the
> inode will appear as unlabeled for up to acdirmin seconds.  Currently
> the label for the root inode is allocated, retrieved, and freed
> entirely
> witin nfs4_proc_get_root().
> 
> Add a field for the label to the nfs_fattr struct, and allocate &
> free
> the label in nfs_get_root(), where we also add a call to
> nfs_setsecurity().  Note that for the call to nfs_setsecurity() to
> succeed, it's necessary to also move the logic calling
> security_sb_{set,clone}_security() from nfs_get_tree_common() down
> into
> nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in
> the
> super_block's security flags and nfs_setsecurity() will silently
> fail.

I built and tested this patch on selinux-next (note that the NFS module
is a few patches behind).
The unlabeled problem is solved, however using:

mount -t nfs -o
vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0
localhost:$TESTDIR /mnt/selinux-testsuite

I get the message:
    mount.nfs: an incorrect mount option was specified
with a log entry:
    SELinux: mount invalid.  Same superblock, different security
settings for (dev 0:42, type nfs)

If I use "fscontext=" instead then works okay. Using no context option
also works. I guess the rootcontext= option should still work ???

> 
> Reported-by: Richard Haines <richard_c_haines@btinternet.com>
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  fs/nfs/getroot.c        | 39 +++++++++++++++++++++++++++++++++++----
>  fs/nfs/nfs4proc.c       | 12 +++---------
>  fs/nfs/super.c          | 25 -------------------------
>  include/linux/nfs_xdr.h |  1 +
>  4 files changed, 39 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
> index b012c2668a1f..6d0628149390 100644
> --- a/fs/nfs/getroot.c
> +++ b/fs/nfs/getroot.c
> @@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct
> fs_context *fc)
>  	struct inode *inode;
>  	char *name;
>  	int error = -ENOMEM;
> +	unsigned long kflags = 0, kflags_out = 0;
>  
>  	name = kstrdup(fc->source, GFP_KERNEL);
>  	if (!name)
> @@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct
> fs_context *fc)
>  	if (fsinfo.fattr == NULL)
>  		goto out_name;
>  
> +	fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL);
> +	if (IS_ERR(fsinfo.fattr->label))
> +		goto out_fattr;
>  	error = server->nfs_client->rpc_ops->getroot(server, ctx-
> >mntfh, &fsinfo);
>  	if (error < 0) {
>  		dprintk("nfs_get_root: getattr error = %d\n", -error);
>  		nfs_errorf(fc, "NFS: Couldn't getattr on root");
> -		goto out_fattr;
> +		goto out_label;
>  	}
>  
>  	inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL);
> @@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct
> fs_context *fc)
>  		dprintk("nfs_get_root: get root inode failed\n");
>  		error = PTR_ERR(inode);
>  		nfs_errorf(fc, "NFS: Couldn't get root inode");
> -		goto out_fattr;
> +		goto out_label;
>  	}
>  
>  	error = nfs_superblock_set_dummy_root(s, inode);
>  	if (error != 0)
> -		goto out_fattr;
> +		goto out_label;
>  
>  	/* root dentries normally start off anonymous and get spliced
> in later
>  	 * if the dentry tree reaches them; however if the dentry
> already
> @@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct
> fs_context *fc)
>  		dprintk("nfs_get_root: get root dentry failed\n");
>  		error = PTR_ERR(root);
>  		nfs_errorf(fc, "NFS: Couldn't get root dentry");
> -		goto out_fattr;
> +		goto out_label;
>  	}
>  
>  	security_d_instantiate(root, inode);
> @@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct
> fs_context *fc)
>  	}
>  	spin_unlock(&root->d_lock);
>  	fc->root = root;
> +	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
> +		kflags |= SECURITY_LSM_NATIVE_LABELS;
> +	if (ctx->clone_data.sb) {
> +		if (d_inode(fc->root)->i_fop != &nfs_dir_operations) {
> +			error = -ESTALE;
> +			goto error_splat_root;
> +		}
> +		/* clone any lsm security options from the parent to
> the new sb */
> +		error = security_sb_clone_mnt_opts(ctx->clone_data.sb,
> s, kflags,
> +				&kflags_out);
> +	} else {
> +		error = security_sb_set_mnt_opts(s, fc->security,
> +							kflags,
> &kflags_out);
> +	}
> +	if (error)
> +		goto error_splat_root;
> +	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
> +		!(kflags_out & SECURITY_LSM_NATIVE_LABELS))
> +		NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
> +
> +	nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label);
>  	error = 0;
>  
> +out_label:
> +	nfs4_label_free(fsinfo.fattr->label);
>  out_fattr:
>  	nfs_free_fattr(fsinfo.fattr);
>  out_name:
>  	kfree(name);
>  out:
>  	return error;
> +error_splat_root:
> +	dput(fc->root);
> +	fc->root = NULL;
> +	goto out_label;
>  }
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 69b7ab7a5815..cb34e840e4fb 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4002,7 +4002,7 @@ static int nfs4_proc_get_root(struct nfs_server
> *server, struct nfs_fh *mntfh,
>  {
>  	int error;
>  	struct nfs_fattr *fattr = info->fattr;
> -	struct nfs4_label *label = NULL;
> +	struct nfs4_label *label = fattr->label;
>  
>  	error = nfs4_server_capabilities(server, mntfh);
>  	if (error < 0) {
> @@ -4010,23 +4010,17 @@ static int nfs4_proc_get_root(struct
> nfs_server *server, struct nfs_fh *mntfh,
>  		return error;
>  	}
>  
> -	label = nfs4_label_alloc(server, GFP_KERNEL);
> -	if (IS_ERR(label))
> -		return PTR_ERR(label);
> -
>  	error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL);
>  	if (error < 0) {
>  		dprintk("nfs4_get_root: getattr error = %d\n", -error);
> -		goto err_free_label;
> +		goto out;
>  	}
>  
>  	if (fattr->valid & NFS_ATTR_FATTR_FSID &&
>  	    !nfs_fsid_equal(&server->fsid, &fattr->fsid))
>  		memcpy(&server->fsid, &fattr->fsid, sizeof(server-
> >fsid));
>  
> -err_free_label:
> -	nfs4_label_free(label);
> -
> +out:
>  	return error;
>  }
>  
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index dada09b391c6..bb14bede6da5 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc)
>  	struct super_block *s;
>  	int (*compare_super)(struct super_block *, struct fs_context *)
> = nfs_compare_super;
>  	struct nfs_server *server = ctx->server;
> -	unsigned long kflags = 0, kflags_out = 0;
>  	int error;
>  
>  	ctx->server = NULL;
> @@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc)
>  		goto error_splat_super;
>  	}
>  
> -	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
> -		kflags |= SECURITY_LSM_NATIVE_LABELS;
> -	if (ctx->clone_data.sb) {
> -		if (d_inode(fc->root)->i_fop != &nfs_dir_operations) {
> -			error = -ESTALE;
> -			goto error_splat_root;
> -		}
> -		/* clone any lsm security options from the parent to
> the new sb */
> -		error = security_sb_clone_mnt_opts(ctx->clone_data.sb,
> s, kflags,
> -				&kflags_out);
> -	} else {
> -		error = security_sb_set_mnt_opts(s, fc->security,
> -							kflags,
> &kflags_out);
> -	}
> -	if (error)
> -		goto error_splat_root;
> -	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
> -		!(kflags_out & SECURITY_LSM_NATIVE_LABELS))
> -		NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
> -
>  	s->s_flags |= SB_ACTIVE;
>  	error = 0;
>  
> @@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc)
>  out_err_nosb:
>  	nfs_free_server(server);
>  	goto out;
> -
> -error_splat_root:
> -	dput(fc->root);
> -	fc->root = NULL;
>  error_splat_super:
>  	deactivate_locked_super(s);
>  	goto out;
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 94c77ed55ce1..6838c149f335 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -75,6 +75,7 @@ struct nfs_fattr {
>  	struct nfs4_string	*owner_name;
>  	struct nfs4_string	*group_name;
>  	struct nfs4_threshold	*mdsthreshold;	/* pNFS threshold
> hints */
> +	struct nfs4_label	*label;
>  };
>  
>  #define NFS_ATTR_FATTR_TYPE		(1U << 0)

Re: [PATCH] NFS: Ensure security label is set for root inode

[Scott Mayhew]

On Wed, 04 Mar 2020, Richard Haines wrote:

> On Tue, 2020-03-03 at 17:58 -0500, Scott Mayhew wrote:
> > When using NFSv4.2, the security label for the root inode should be
> > set
> > via a call to nfs_setsecurity() during the mount process, otherwise
> > the
> > inode will appear as unlabeled for up to acdirmin seconds.  Currently
> > the label for the root inode is allocated, retrieved, and freed
> > entirely
> > witin nfs4_proc_get_root().
> > 
> > Add a field for the label to the nfs_fattr struct, and allocate &
> > free
> > the label in nfs_get_root(), where we also add a call to
> > nfs_setsecurity().  Note that for the call to nfs_setsecurity() to
> > succeed, it's necessary to also move the logic calling
> > security_sb_{set,clone}_security() from nfs_get_tree_common() down
> > into
> > nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in
> > the
> > super_block's security flags and nfs_setsecurity() will silently
> > fail.
> 
> I built and tested this patch on selinux-next (note that the NFS module
> is a few patches behind).
> The unlabeled problem is solved, however using:
> 
> mount -t nfs -o
> vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0
> localhost:$TESTDIR /mnt/selinux-testsuite
> 
> I get the message:
>     mount.nfs: an incorrect mount option was specified
> with a log entry:
>     SELinux: mount invalid.  Same superblock, different security
> settings for (dev 0:42, type nfs)
> 
> If I use "fscontext=" instead then works okay. Using no context option
> also works. I guess the rootcontext= option should still work ???

Thanks for testing.  It definitely wasn't my intention to break
anything, so I'll look into it.

-Scott
> 
> > 
> > Reported-by: Richard Haines <richard_c_haines@btinternet.com>
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfs/getroot.c        | 39 +++++++++++++++++++++++++++++++++++----
> >  fs/nfs/nfs4proc.c       | 12 +++---------
> >  fs/nfs/super.c          | 25 -------------------------
> >  include/linux/nfs_xdr.h |  1 +
> >  4 files changed, 39 insertions(+), 38 deletions(-)
> > 
> > diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
> > index b012c2668a1f..6d0628149390 100644
> > --- a/fs/nfs/getroot.c
> > +++ b/fs/nfs/getroot.c
> > @@ -73,6 +73,7 @@ int nfs_get_root(struct super_block *s, struct
> > fs_context *fc)
> >  	struct inode *inode;
> >  	char *name;
> >  	int error = -ENOMEM;
> > +	unsigned long kflags = 0, kflags_out = 0;
> >  
> >  	name = kstrdup(fc->source, GFP_KERNEL);
> >  	if (!name)
> > @@ -83,11 +84,14 @@ int nfs_get_root(struct super_block *s, struct
> > fs_context *fc)
> >  	if (fsinfo.fattr == NULL)
> >  		goto out_name;
> >  
> > +	fsinfo.fattr->label = nfs4_label_alloc(server, GFP_KERNEL);
> > +	if (IS_ERR(fsinfo.fattr->label))
> > +		goto out_fattr;
> >  	error = server->nfs_client->rpc_ops->getroot(server, ctx-
> > >mntfh, &fsinfo);
> >  	if (error < 0) {
> >  		dprintk("nfs_get_root: getattr error = %d\n", -error);
> >  		nfs_errorf(fc, "NFS: Couldn't getattr on root");
> > -		goto out_fattr;
> > +		goto out_label;
> >  	}
> >  
> >  	inode = nfs_fhget(s, ctx->mntfh, fsinfo.fattr, NULL);
> > @@ -95,12 +99,12 @@ int nfs_get_root(struct super_block *s, struct
> > fs_context *fc)
> >  		dprintk("nfs_get_root: get root inode failed\n");
> >  		error = PTR_ERR(inode);
> >  		nfs_errorf(fc, "NFS: Couldn't get root inode");
> > -		goto out_fattr;
> > +		goto out_label;
> >  	}
> >  
> >  	error = nfs_superblock_set_dummy_root(s, inode);
> >  	if (error != 0)
> > -		goto out_fattr;
> > +		goto out_label;
> >  
> >  	/* root dentries normally start off anonymous and get spliced
> > in later
> >  	 * if the dentry tree reaches them; however if the dentry
> > already
> > @@ -111,7 +115,7 @@ int nfs_get_root(struct super_block *s, struct
> > fs_context *fc)
> >  		dprintk("nfs_get_root: get root dentry failed\n");
> >  		error = PTR_ERR(root);
> >  		nfs_errorf(fc, "NFS: Couldn't get root dentry");
> > -		goto out_fattr;
> > +		goto out_label;
> >  	}
> >  
> >  	security_d_instantiate(root, inode);
> > @@ -123,12 +127,39 @@ int nfs_get_root(struct super_block *s, struct
> > fs_context *fc)
> >  	}
> >  	spin_unlock(&root->d_lock);
> >  	fc->root = root;
> > +	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
> > +		kflags |= SECURITY_LSM_NATIVE_LABELS;
> > +	if (ctx->clone_data.sb) {
> > +		if (d_inode(fc->root)->i_fop != &nfs_dir_operations) {
> > +			error = -ESTALE;
> > +			goto error_splat_root;
> > +		}
> > +		/* clone any lsm security options from the parent to
> > the new sb */
> > +		error = security_sb_clone_mnt_opts(ctx->clone_data.sb,
> > s, kflags,
> > +				&kflags_out);
> > +	} else {
> > +		error = security_sb_set_mnt_opts(s, fc->security,
> > +							kflags,
> > &kflags_out);
> > +	}
> > +	if (error)
> > +		goto error_splat_root;
> > +	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
> > +		!(kflags_out & SECURITY_LSM_NATIVE_LABELS))
> > +		NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
> > +
> > +	nfs_setsecurity(inode, fsinfo.fattr, fsinfo.fattr->label);
> >  	error = 0;
> >  
> > +out_label:
> > +	nfs4_label_free(fsinfo.fattr->label);
> >  out_fattr:
> >  	nfs_free_fattr(fsinfo.fattr);
> >  out_name:
> >  	kfree(name);
> >  out:
> >  	return error;
> > +error_splat_root:
> > +	dput(fc->root);
> > +	fc->root = NULL;
> > +	goto out_label;
> >  }
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 69b7ab7a5815..cb34e840e4fb 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -4002,7 +4002,7 @@ static int nfs4_proc_get_root(struct nfs_server
> > *server, struct nfs_fh *mntfh,
> >  {
> >  	int error;
> >  	struct nfs_fattr *fattr = info->fattr;
> > -	struct nfs4_label *label = NULL;
> > +	struct nfs4_label *label = fattr->label;
> >  
> >  	error = nfs4_server_capabilities(server, mntfh);
> >  	if (error < 0) {
> > @@ -4010,23 +4010,17 @@ static int nfs4_proc_get_root(struct
> > nfs_server *server, struct nfs_fh *mntfh,
> >  		return error;
> >  	}
> >  
> > -	label = nfs4_label_alloc(server, GFP_KERNEL);
> > -	if (IS_ERR(label))
> > -		return PTR_ERR(label);
> > -
> >  	error = nfs4_proc_getattr(server, mntfh, fattr, label, NULL);
> >  	if (error < 0) {
> >  		dprintk("nfs4_get_root: getattr error = %d\n", -error);
> > -		goto err_free_label;
> > +		goto out;
> >  	}
> >  
> >  	if (fattr->valid & NFS_ATTR_FATTR_FSID &&
> >  	    !nfs_fsid_equal(&server->fsid, &fattr->fsid))
> >  		memcpy(&server->fsid, &fattr->fsid, sizeof(server-
> > >fsid));
> >  
> > -err_free_label:
> > -	nfs4_label_free(label);
> > -
> > +out:
> >  	return error;
> >  }
> >  
> > diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> > index dada09b391c6..bb14bede6da5 100644
> > --- a/fs/nfs/super.c
> > +++ b/fs/nfs/super.c
> > @@ -1179,7 +1179,6 @@ int nfs_get_tree_common(struct fs_context *fc)
> >  	struct super_block *s;
> >  	int (*compare_super)(struct super_block *, struct fs_context *)
> > = nfs_compare_super;
> >  	struct nfs_server *server = ctx->server;
> > -	unsigned long kflags = 0, kflags_out = 0;
> >  	int error;
> >  
> >  	ctx->server = NULL;
> > @@ -1239,26 +1238,6 @@ int nfs_get_tree_common(struct fs_context *fc)
> >  		goto error_splat_super;
> >  	}
> >  
> > -	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL)
> > -		kflags |= SECURITY_LSM_NATIVE_LABELS;
> > -	if (ctx->clone_data.sb) {
> > -		if (d_inode(fc->root)->i_fop != &nfs_dir_operations) {
> > -			error = -ESTALE;
> > -			goto error_splat_root;
> > -		}
> > -		/* clone any lsm security options from the parent to
> > the new sb */
> > -		error = security_sb_clone_mnt_opts(ctx->clone_data.sb,
> > s, kflags,
> > -				&kflags_out);
> > -	} else {
> > -		error = security_sb_set_mnt_opts(s, fc->security,
> > -							kflags,
> > &kflags_out);
> > -	}
> > -	if (error)
> > -		goto error_splat_root;
> > -	if (NFS_SB(s)->caps & NFS_CAP_SECURITY_LABEL &&
> > -		!(kflags_out & SECURITY_LSM_NATIVE_LABELS))
> > -		NFS_SB(s)->caps &= ~NFS_CAP_SECURITY_LABEL;
> > -
> >  	s->s_flags |= SB_ACTIVE;
> >  	error = 0;
> >  
> > @@ -1268,10 +1247,6 @@ int nfs_get_tree_common(struct fs_context *fc)
> >  out_err_nosb:
> >  	nfs_free_server(server);
> >  	goto out;
> > -
> > -error_splat_root:
> > -	dput(fc->root);
> > -	fc->root = NULL;
> >  error_splat_super:
> >  	deactivate_locked_super(s);
> >  	goto out;
> > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> > index 94c77ed55ce1..6838c149f335 100644
> > --- a/include/linux/nfs_xdr.h
> > +++ b/include/linux/nfs_xdr.h
> > @@ -75,6 +75,7 @@ struct nfs_fattr {
> >  	struct nfs4_string	*owner_name;
> >  	struct nfs4_string	*group_name;
> >  	struct nfs4_threshold	*mdsthreshold;	/* pNFS threshold
> > hints */
> > +	struct nfs4_label	*label;
> >  };
> >  
> >  #define NFS_ATTR_FATTR_TYPE		(1U << 0)
> 

Re: [PATCH] NFS: Ensure security label is set for root inode

[Stephen Smalley]

On Wed, Mar 4, 2020 at 9:37 AM Scott Mayhew <smayhew@redhat.com> wrote:
>
> On Wed, 04 Mar 2020, Richard Haines wrote:
> > I built and tested this patch on selinux-next (note that the NFS module
> > is a few patches behind).
> > The unlabeled problem is solved, however using:
> >
> > mount -t nfs -o
> > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0
> > localhost:$TESTDIR /mnt/selinux-testsuite
> >
> > I get the message:
> >     mount.nfs: an incorrect mount option was specified
> > with a log entry:
> >     SELinux: mount invalid.  Same superblock, different security
> > settings for (dev 0:42, type nfs)
> >
> > If I use "fscontext=" instead then works okay. Using no context option
> > also works. I guess the rootcontext= option should still work ???
>
> Thanks for testing.  It definitely wasn't my intention to break
> anything, so I'll look into it.

I'm not sure that rootcontext= should be supported or is supportable
over labeled NFS.
It's primary use case is to allow assigning a specific context other
than the default policy-defined one
to the root directory for filesystems that support labeling but don't
have existing labels on their root
directories, e.g. tmpfs mounts.  Even if we set the rootcontext based
on rootcontext= during mount(2),
it would likely get overridden by subsequent attribute fetches from
the server I would think (e.g. it probably
already switches to the context from the server after 30 seconds or
so?). As long as the separate context= option
continues to work correctly on NFS, I'm not overly concerned about this.

I should note that we are getting similar errors though when trying to
specify any context-related
mount options on NFS via the new fsconfig(2) system call, see
https://github.com/SELinuxProject/selinux-kernel/issues/49
I don't know if this change in when security_sb_set_mnt_opts() will
alter that situation any.

Also, FYI, we have recently made it possible to run the
selinux-testsuite without errors within a labeled NFS
mount.  If you clone
https://github.com/SELinuxProject/selinux-testsuite/ and follow the
README.md
instructions including the NFS section and run ./tools/nfs.sh, it will
export and mount the testsuite directory
via labeled NFS over loopback and run all tests that can be supported
over NFS, and then runs a few specific
tests for context= mount options (but not the other mount options at
present).  It still needs some further
enhancements as per
https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492
but it at least provides some degree of regression testing.

Re: [PATCH] NFS: Ensure security label is set for root inode

[Scott Mayhew]

On Wed, 04 Mar 2020, Stephen Smalley wrote:

> On Wed, Mar 4, 2020 at 9:37 AM Scott Mayhew <smayhew@redhat.com> wrote:
> >
> > On Wed, 04 Mar 2020, Richard Haines wrote:
> > > I built and tested this patch on selinux-next (note that the NFS module
> > > is a few patches behind).
> > > The unlabeled problem is solved, however using:
> > >
> > > mount -t nfs -o
> > > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s0
> > > localhost:$TESTDIR /mnt/selinux-testsuite
> > >
> > > I get the message:
> > >     mount.nfs: an incorrect mount option was specified
> > > with a log entry:
> > >     SELinux: mount invalid.  Same superblock, different security
> > > settings for (dev 0:42, type nfs)
> > >
> > > If I use "fscontext=" instead then works okay. Using no context option
> > > also works. I guess the rootcontext= option should still work ???
> >
> > Thanks for testing.  It definitely wasn't my intention to break
> > anything, so I'll look into it.
> 
> I'm not sure that rootcontext= should be supported or is supportable
> over labeled NFS.

Should rootcontext= be supported for NFS versions < 4.2?  If not then
maybe it that option should be rejected for nfs and nfs4 fstypes in
selinux_set_mnt_opts().

> It's primary use case is to allow assigning a specific context other
> than the default policy-defined one
> to the root directory for filesystems that support labeling but don't
> have existing labels on their root
> directories, e.g. tmpfs mounts.  Even if we set the rootcontext based
> on rootcontext= during mount(2),
> it would likely get overridden by subsequent attribute fetches from
> the server I would think (e.g. it probably
> already switches to the context from the server after 30 seconds or

Yes, that's what happens.  If we wanted to retain that behavior moving
forward, then we need to avoid calling nfs_setsecurity() for the root
inode when the rootcontext= option was used.  To do that, I think we'd
need to add a flag that could be passed back to NFS via the
set_kern_flags parameter of selinux_set_mnt_opts().

> so?). As long as the separate context= option
> continues to work correctly on NFS, I'm not overly concerned about this.

Yep, the context= option still works.
> 
> I should note that we are getting similar errors though when trying to
> specify any context-related
> mount options on NFS via the new fsconfig(2) system call, see
> https://github.com/SELinuxProject/selinux-kernel/issues/49
> I don't know if this change in when security_sb_set_mnt_opts() will
> alter that situation any.
> 
> Also, FYI, we have recently made it possible to run the
> selinux-testsuite without errors within a labeled NFS
> mount.  If you clone
> https://github.com/SELinuxProject/selinux-testsuite/ and follow the
> README.md
> instructions including the NFS section and run ./tools/nfs.sh, it will
> export and mount the testsuite directory
> via labeled NFS over loopback and run all tests that can be supported
> over NFS, and then runs a few specific
> tests for context= mount options (but not the other mount options at
> present).  It still needs some further
> enhancements as per
> https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492
> but it at least provides some degree of regression testing.

Thanks.  I ran this a few times and aside from an exportfs bug
everything passed.  I'll be sure to run this in the future too.

-Scott

Re: [PATCH] NFS: Ensure security label is set for root inode

[Richard Haines]

On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote:
> On Wed, 04 Mar 2020, Stephen Smalley wrote:
> 
> > On Wed, Mar 4, 2020 at 9:37 AM Scott Mayhew <smayhew@redhat.com>
> > wrote:
> > > On Wed, 04 Mar 2020, Richard Haines wrote:
> > > > I built and tested this patch on selinux-next (note that the
> > > > NFS module
> > > > is a few patches behind).
> > > > The unlabeled problem is solved, however using:
> > > > 
> > > > mount -t nfs -o
> > > > vers=4.2,rootcontext=system_u:object_r:test_filesystem_file_t:s
> > > > 0
> > > > localhost:$TESTDIR /mnt/selinux-testsuite
> > > > 
> > > > I get the message:
> > > >     mount.nfs: an incorrect mount option was specified
> > > > with a log entry:
> > > >     SELinux: mount invalid.  Same superblock, different
> > > > security
> > > > settings for (dev 0:42, type nfs)
> > > > 
> > > > If I use "fscontext=" instead then works okay. Using no context
> > > > option
> > > > also works. I guess the rootcontext= option should still work
> > > > ???
> > > 
> > > Thanks for testing.  It definitely wasn't my intention to break
> > > anything, so I'll look into it.
> > 
> > I'm not sure that rootcontext= should be supported or is
> > supportable
> > over labeled NFS.
> 
> Should rootcontext= be supported for NFS versions < 4.2?  If not then
> maybe it that option should be rejected for nfs and nfs4 fstypes in
> selinux_set_mnt_opts().
> 
> > It's primary use case is to allow assigning a specific context
> > other
> > than the default policy-defined one
> > to the root directory for filesystems that support labeling but
> > don't
> > have existing labels on their root
> > directories, e.g. tmpfs mounts.  Even if we set the rootcontext
> > based
> > on rootcontext= during mount(2),
> > it would likely get overridden by subsequent attribute fetches from
> > the server I would think (e.g. it probably
> > already switches to the context from the server after 30 seconds or
> 
> Yes, that's what happens.  If we wanted to retain that behavior
> moving
> forward, then we need to avoid calling nfs_setsecurity() for the root
> inode when the rootcontext= option was used.  To do that, I think
> we'd
> need to add a flag that could be passed back to NFS via the
> set_kern_flags parameter of selinux_set_mnt_opts().
> 
> > so?). As long as the separate context= option
> > continues to work correctly on NFS, I'm not overly concerned about
> > this.
> 
> Yep, the context= option still works.
> > I should note that we are getting similar errors though when trying
> > to
> > specify any context-related
> > mount options on NFS via the new fsconfig(2) system call, see
> > https://github.com/SELinuxProject/selinux-kernel/issues/49
I've done further testing and found that with this patch the
fsconfig(2) problem is also resolved for nfs (provided the rootcontext
is not specified).

> > I don't know if this change in when security_sb_set_mnt_opts() will
> > alter that situation any.
> > 
> > Also, FYI, we have recently made it possible to run the
> > selinux-testsuite without errors within a labeled NFS
> > mount.  If you clone
> > https://github.com/SELinuxProject/selinux-testsuite/ and follow the
> > README.md
> > instructions including the NFS section and run ./tools/nfs.sh, it
> > will
> > export and mount the testsuite directory
> > via labeled NFS over loopback and run all tests that can be
> > supported
> > over NFS, and then runs a few specific
> > tests for context= mount options (but not the other mount options
> > at
> > present).  It still needs some further
> > enhancements as per
> > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492
> > but it at least provides some degree of regression testing.
Could you describe how I could test these, also are there any other
SELinux tests that may be useful (with howto's). I'm almost ready to
post another set of RFC test patches, but can add more.

> 
> Thanks.  I ran this a few times and aside from an exportfs bug
> everything passed.  I'll be sure to run this in the future too.
> 
> -Scott
> 

Re: [PATCH] NFS: Ensure security label is set for root inode

[Stephen Smalley]

On Sun, Mar 8, 2020 at 1:47 PM Richard Haines
<richard_c_haines@btinternet.com> wrote:
>
> On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote:
> > On Wed, 04 Mar 2020, Stephen Smalley wrote:
> > > I should note that we are getting similar errors though when trying
> > > to
> > > specify any context-related
> > > mount options on NFS via the new fsconfig(2) system call, see
> > > https://github.com/SELinuxProject/selinux-kernel/issues/49
> I've done further testing and found that with this patch the
> fsconfig(2) problem is also resolved for nfs (provided the rootcontext
> is not specified).

Excellent, two bugs fixed with one patch!

>>> It still needs some further
> > > enhancements as per
> > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492
> > > but it at least provides some degree of regression testing.
> Could you describe how I could test these, also are there any other
> SELinux tests that may be useful (with howto's). I'm almost ready to
> post another set of RFC test patches, but can add more.

The ones identified in that github issue comment would simply be
additional tests in tools/nfs.sh
unless they happen to already be covered by your fs/filesystem tests
once those are applied to the
host/native filesystem instead of just ext4.  The test cases are:

0. Test the bug fixed by this patch, i.e. perform mount of a
security_label exported filesystem, check the label of the mounted
directory to confirm it isn't unlabeled.
That's a NFS-specific test, goes in tools/nfs.sh.

1. Mount the same filesystem twice with two different sets of context
mount options, check that mount(2) fails with errno EINVAL.
Test cases might include a) mount first without any context mount
options, then try mounting a 2nd time with context mount options and
vice versa, b) mounting
with the same set of context options (e.g. both using context=) but
different context values, c) mounting with different sets of context
options (e.g. one uses
context=, the other uses fscontext=).  This test could be done in
fs/filesystem for any filesystem type, not NFS-specific.

2. Mount a security_label exported NFS filesystem twice, confirm that
NFS security labeling support isn't silently disabled by trying to
set a label on a file and confirm it is set (fixed by  kernel commit
3815a245b50124f0865415dcb606a034e97494d4).  This would go in
tools/nfs.sh
since it is NFS-specific.

3. Perform a context= mount of a security_label exported NFS
filesystem, check that pre-existing files within the mount show up
with the context= value
not the underlying xattr value (fixed by kernel commit
0b4d3452b8b4a5309b4445b900e3cec022cca95a). My original version of
tools/nfs.sh actually would have caught this because it was testing
the context of the nfs.sh script file itself within the context mount
but I dropped it back to only checking the top-level mount directory
when I moved tools/nfs.sh to avoid depending on a fixed location for
it, so it won't be caught currently. We could just change it back to
testing the context of a pre-existing file within the mount; any file
will do.  This would go in tools/nfs.sh, NFS-specific.

4. Ensuring that all of the tests/filesystem and tests/fs_filesystem
tests that make sense for NFS are being run on the labeled NFS mount
itself when run via tools/nfs.sh and not just on an ext4 mount created
by the test script.

Re: [PATCH] NFS: Ensure security label is set for root inode

[Richard Haines]

On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote:
> On Sun, Mar 8, 2020 at 1:47 PM Richard Haines
> <richard_c_haines@btinternet.com> wrote:
> > On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote:
> > > On Wed, 04 Mar 2020, Stephen Smalley wrote:
> > > > I should note that we are getting similar errors though when
> > > > trying
> > > > to
> > > > specify any context-related
> > > > mount options on NFS via the new fsconfig(2) system call, see
> > > > https://github.com/SELinuxProject/selinux-kernel/issues/49
> > I've done further testing and found that with this patch the
> > fsconfig(2) problem is also resolved for nfs (provided the
> > rootcontext
> > is not specified).
> 
> Excellent, two bugs fixed with one patch!
> 
> > > > It still needs some further
> > > > enhancements as per
> > > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492
> > > > but it at least provides some degree of regression testing.
> > Could you describe how I could test these, also are there any other
> > SELinux tests that may be useful (with howto's). I'm almost ready
> > to
> > post another set of RFC test patches, but can add more.
> 
> The ones identified in that github issue comment would simply be
> additional tests in tools/nfs.sh
> unless they happen to already be covered by your fs/filesystem tests
> once those are applied to the
> host/native filesystem instead of just ext4.  The test cases are:
> 
> 0. Test the bug fixed by this patch, i.e. perform mount of a
> security_label exported filesystem, check the label of the mounted
> directory to confirm it isn't unlabeled.
> That's a NFS-specific test, goes in tools/nfs.sh.
> 
> 1. Mount the same filesystem twice with two different sets of context
> mount options, check that mount(2) fails with errno EINVAL.

I've tests for the first part already, however with NFS it returns
EBUSY (using mount(2) or the fixed fsconfig(2)). On ext4, xfs & vfat it
does return EINVAL. I guess another NFS bug. Also mount(8) ignores the
error and just carries on. Here is a test using the testsuite mount(2):

tools/nfs-mount2.sh
#!/bin/sh -e
MOUNT=`stat --print %m .`
TESTDIR=`pwd`
NET="nfsvers=4.2,proto=tcp,clientaddr=127.0.0.1,addr=127.0.0.1"

function err_exit() {
    echo "Error on line: $1 - Closing down NFS"
    umount /mnt/selinux-testsuite
    exportfs -u localhost:$MOUNT
    rmdir /mnt/selinux-testsuite
    systemctl stop nfs-server
    exit 1
}

trap 'err_exit $LINENO' ERR

systemctl start nfs-server
exportfs -orw,no_root_squash,security_label localhost:$MOUNT
mkdir -p /mnt/selinux-testsuite

tests/filesystem/mount -v -f nfs -o vers=4.2,$NET -s localhost:$TESTDIR
-t /mnt/selinux-testsuite

tests/filesystem/mount -v -f nfs -o
vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s localhost:$TESTDIR
-t /mnt/selinux-testsuite
echo "Testing context mount of a security_label export."
fctx=`secon -t -f /mnt/selinux-testsuite`
if [ "$fctx" != "etc_t" ]; then
    echo "Context mount failed: got $fctx instead of etc_t."
    err_exit $LINENO
fi
umount /mnt/selinux-testsuite
echo "Done"
exportfs -u localhost:$MOUNT
rmdir /mnt/selinux-testsuite
systemctl stop nfs-server

> Test cases might include a) mount first without any context mount
> options, then try mounting a 2nd time with context mount options and
> vice versa, b) mounting
> with the same set of context options (e.g. both using context=) but
> different context values, c) mounting with different sets of context
> options (e.g. one uses
> context=, the other uses fscontext=).  This test could be done in
> fs/filesystem for any filesystem type, not NFS-specific.
> 
> 2. Mount a security_label exported NFS filesystem twice, confirm that
> NFS security labeling support isn't silently disabled by trying to
> set a label on a file and confirm it is set (fixed by  kernel commit
> 3815a245b50124f0865415dcb606a034e97494d4).  This would go in
> tools/nfs.sh
> since it is NFS-specific.
> 
> 3. Perform a context= mount of a security_label exported NFS
> filesystem, check that pre-existing files within the mount show up
> with the context= value
> not the underlying xattr value (fixed by kernel commit
> 0b4d3452b8b4a5309b4445b900e3cec022cca95a). My original version of
> tools/nfs.sh actually would have caught this because it was testing
> the context of the nfs.sh script file itself within the context mount
> but I dropped it back to only checking the top-level mount directory
> when I moved tools/nfs.sh to avoid depending on a fixed location for
> it, so it won't be caught currently. We could just change it back to
> testing the context of a pre-existing file within the mount; any file
> will do.  This would go in tools/nfs.sh, NFS-specific.
> 
> 4. Ensuring that all of the tests/filesystem and tests/fs_filesystem
> tests that make sense for NFS are being run on the labeled NFS mount
> itself when run via tools/nfs.sh and not just on an ext4 mount
> created
> by the test script.

Re: [PATCH] NFS: Ensure security label is set for root inode

[Stephen Smalley]

On Mon, Mar 9, 2020 at 12:41 PM Richard Haines
<richard_c_haines@btinternet.com> wrote:
>
> On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote:
> > 1. Mount the same filesystem twice with two different sets of context
> > mount options, check that mount(2) fails with errno EINVAL.
>
> I've tests for the first part already, however with NFS it returns
> EBUSY (using mount(2) or the fixed fsconfig(2)). On ext4, xfs & vfat it
> does return EINVAL. I guess another NFS bug. Also mount(8) ignores the
> error and just carries on. Here is a test using the testsuite mount(2):

Looks like selinux_cmp_sb_context() returns -EBUSY on error instead of -EINVAL.
This goes back to 094f7b69ea738d7d619cba449d2af97159949459 ("selinux:
make security_sb_clone_mnt_opts return an error on context mismatch").
I guess you can just make the test accept either -EINVAL or -EBUSY for
the time being and we'll have to consider
whether we want to change it and what would break if we did.

Re: [PATCH] NFS: Ensure security label is set for root inode

[Richard Haines]

On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote:
> On Sun, Mar 8, 2020 at 1:47 PM Richard Haines
> <richard_c_haines@btinternet.com> wrote:
> > On Fri, 2020-03-06 at 17:01 -0500, Scott Mayhew wrote:
> > > On Wed, 04 Mar 2020, Stephen Smalley wrote:
> > > > I should note that we are getting similar errors though when
> > > > trying
> > > > to
> > > > specify any context-related
> > > > mount options on NFS via the new fsconfig(2) system call, see
> > > > https://github.com/SELinuxProject/selinux-kernel/issues/49
> > I've done further testing and found that with this patch the
> > fsconfig(2) problem is also resolved for nfs (provided the
> > rootcontext
> > is not specified).
> 
> Excellent, two bugs fixed with one patch!
> 
> > > > It still needs some further
> > > > enhancements as per
> > > > https://github.com/SELinuxProject/selinux-testsuite/issues/32#issuecomment-582992492
> > > > but it at least provides some degree of regression testing.
> > Could you describe how I could test these, also are there any other
> > SELinux tests that may be useful (with howto's). I'm almost ready
> > to
> > post another set of RFC test patches, but can add more.
> 
> The ones identified in that github issue comment would simply be
> additional tests in tools/nfs.sh
> unless they happen to already be covered by your fs/filesystem tests
> once those are applied to the
> host/native filesystem instead of just ext4.  The test cases are:
> 
> 0. Test the bug fixed by this patch, i.e. perform mount of a
> security_label exported filesystem, check the label of the mounted
> directory to confirm it isn't unlabeled.
> That's a NFS-specific test, goes in tools/nfs.sh.
> 
> 1. Mount the same filesystem twice with two different sets of context
> mount options, check that mount(2) fails with errno EINVAL.
> Test cases might include a) mount first without any context mount
> options, then try mounting a 2nd time with context mount options and
> vice versa, b) mounting
> with the same set of context options (e.g. both using context=) but
> different context values, c) mounting with different sets of context
> options (e.g. one uses
> context=, the other uses fscontext=).  This test could be done in
> fs/filesystem for any filesystem type, not NFS-specific.
> 
> 2. Mount a security_label exported NFS filesystem twice, confirm that
> NFS security labeling support isn't silently disabled by trying to
> set a label on a file and confirm it is set (fixed by  kernel commit
> 3815a245b50124f0865415dcb606a034e97494d4).  This would go in
> tools/nfs.sh
> since it is NFS-specific.

And another one. If you run the same mount twice using mount(2) you get
EBUSY. If you run with fsmount(2) it works. A simple test below, just
set $1 to fs for fsmount(2)

Otherwise I've completed the remaining tests with no problems.

#!/bin/sh -e
MOUNT=`stat --print %m .`
TESTDIR=`pwd`
NET="nfsvers=4.2,proto=tcp,clientaddr=127.0.0.1,addr=127.0.0.1"

function err_exit() {
    echo "Error on line: $1 - Closing down NFS"
    umount /mnt/selinux-testsuite
    exportfs -u localhost:$MOUNT
    rmdir /mnt/selinux-testsuite
    systemctl stop nfs-server
    exit 1
}

trap 'err_exit $LINENO' ERR

systemctl start nfs-server
exportfs -orw,no_root_squash,security_label localhost:$MOUNT
mkdir -p /mnt/selinux-testsuite

if [ $1 ] && [ $1 = 'fs' ]; then
    RUN="tests/fs_filesystem/fsmount"
else
    RUN="tests/filesystem/mount"
fi

$RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s
localhost:$TESTDIR -t /mnt/selinux-testsuite
$RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s
localhost:$TESTDIR -t /mnt/selinux-testsuite
echo "Testing context mount of a security_label export."
fctx=`secon -t -f /mnt/selinux-testsuite`
if [ "$fctx" != "etc_t" ]; then
    echo "Context mount failed: got $fctx instead of etc_t."
    err_exit $LINENO
fi
umount /mnt/selinux-testsuite
umount /mnt/selinux-testsuite

echo "Done"
exportfs -u localhost:$MOUNT
rmdir /mnt/selinux-testsuite
systemctl stop nfs-server


> 
> 3. Perform a context= mount of a security_label exported NFS
> filesystem, check that pre-existing files within the mount show up
> with the context= value
> not the underlying xattr value (fixed by kernel commit
> 0b4d3452b8b4a5309b4445b900e3cec022cca95a). My original version of
> tools/nfs.sh actually would have caught this because it was testing
> the context of the nfs.sh script file itself within the context mount
> but I dropped it back to only checking the top-level mount directory
> when I moved tools/nfs.sh to avoid depending on a fixed location for
> it, so it won't be caught currently. We could just change it back to
> testing the context of a pre-existing file within the mount; any file
> will do.  This would go in tools/nfs.sh, NFS-specific.
> 
> 4. Ensuring that all of the tests/filesystem and tests/fs_filesystem
> tests that make sense for NFS are being run on the labeled NFS mount
> itself when run via tools/nfs.sh and not just on an ext4 mount
> created
> by the test script.

Re: [PATCH] NFS: Ensure security label is set for root inode

[Stephen Smalley]

On Tue, Mar 10, 2020 at 9:27 AM Richard Haines
<richard_c_haines@btinternet.com> wrote:
>
> On Mon, 2020-03-09 at 09:35 -0400, Stephen Smalley wrote:
> > 2. Mount a security_label exported NFS filesystem twice, confirm that
> > NFS security labeling support isn't silently disabled by trying to
> > set a label on a file and confirm it is set (fixed by  kernel commit
> > 3815a245b50124f0865415dcb606a034e97494d4).  This would go in
> > tools/nfs.sh
> > since it is NFS-specific.
>
> And another one. If you run the same mount twice using mount(2) you get
> EBUSY. If you run with fsmount(2) it works. A simple test below, just
> set $1 to fs for fsmount(2)

I don't know if that's a bug or just an inconsistency between mount(2)
and fsmount(2).
Question for David, Al, and/or fsdevel (cc'd).

>
> Otherwise I've completed the remaining tests with no problems.
>
> #!/bin/sh -e
> MOUNT=`stat --print %m .`
> TESTDIR=`pwd`
> NET="nfsvers=4.2,proto=tcp,clientaddr=127.0.0.1,addr=127.0.0.1"
>
> function err_exit() {
>     echo "Error on line: $1 - Closing down NFS"
>     umount /mnt/selinux-testsuite
>     exportfs -u localhost:$MOUNT
>     rmdir /mnt/selinux-testsuite
>     systemctl stop nfs-server
>     exit 1
> }
>
> trap 'err_exit $LINENO' ERR
>
> systemctl start nfs-server
> exportfs -orw,no_root_squash,security_label localhost:$MOUNT
> mkdir -p /mnt/selinux-testsuite
>
> if [ $1 ] && [ $1 = 'fs' ]; then
>     RUN="tests/fs_filesystem/fsmount"
> else
>     RUN="tests/filesystem/mount"
> fi
>
> $RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s
> localhost:$TESTDIR -t /mnt/selinux-testsuite
> $RUN -v -f nfs -o vers=4.2,$NET,context=system_u:object_r:etc_t:s0 -s
> localhost:$TESTDIR -t /mnt/selinux-testsuite
> echo "Testing context mount of a security_label export."
> fctx=`secon -t -f /mnt/selinux-testsuite`
> if [ "$fctx" != "etc_t" ]; then
>     echo "Context mount failed: got $fctx instead of etc_t."
>     err_exit $LINENO
> fi
> umount /mnt/selinux-testsuite
> umount /mnt/selinux-testsuite
>
> echo "Done"
> exportfs -u localhost:$MOUNT
> rmdir /mnt/selinux-testsuite
> systemctl stop nfs-server

Re: [PATCH] NFS: Ensure security label is set for root inode

[Stephen Smalley]

On Fri, Mar 6, 2020 at 5:01 PM Scott Mayhew <smayhew@redhat.com> wrote:
>
> On Wed, 04 Mar 2020, Stephen Smalley wrote:
> > I'm not sure that rootcontext= should be supported or is supportable
> > over labeled NFS.
>
> Should rootcontext= be supported for NFS versions < 4.2?  If not then
> maybe it that option should be rejected for nfs and nfs4 fstypes in
> selinux_set_mnt_opts().

Looks like it gets ignored currently?
$ sudo exportfs -orw,no_root_squash localhost:/home
$ sudo mkdir -p /mnt/selinux-testsuite
$ sudo mount -t nfs -o vers=4.0,rootcontext=system_u:object_r:etc_t:s0
localhost:/home/sds/selinux-testsuite /mnt/selinux-testsuite
$ ls -Zd /mnt/selinux-testsuite
system_u:object_r:nfs_t:s0 /mnt/selinux-testsuite
$ mount | grep testsuite
localhost:/home/sds/selinux-testsuite on /mnt/selinux-testsuite type
nfs4 (rw,relatime,rootcontext=system_u:object_r:etc_t:s0,vers=4.0,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp6,timeo=600,retrans=2,sec=sys,clientaddr=::1,local_lock=none,addr=::1)

I don't think we need to support it, but I don't know if we explicitly
need to test and reject it for nfs/nfs4.

> > It's primary use case is to allow assigning a specific context other
> > than the default policy-defined one
> > to the root directory for filesystems that support labeling but don't
> > have existing labels on their root
> > directories, e.g. tmpfs mounts.  Even if we set the rootcontext based
> > on rootcontext= during mount(2),
> > it would likely get overridden by subsequent attribute fetches from
> > the server I would think (e.g. it probably
> > already switches to the context from the server after 30 seconds or
>
> Yes, that's what happens.  If we wanted to retain that behavior moving
> forward, then we need to avoid calling nfs_setsecurity() for the root
> inode when the rootcontext= option was used.  To do that, I think we'd
> need to add a flag that could be passed back to NFS via the
> set_kern_flags parameter of selinux_set_mnt_opts().

Doesn't seem justified.

> > so?). As long as the separate context= option
> > continues to work correctly on NFS, I'm not overly concerned about this.
>
> Yep, the context= option still works.

Great, then I have no objections to this patch.

Re: [PATCH] NFS: Ensure security label is set for root inode

[Stephen Smalley]

On Tue, Mar 3, 2020 at 5:59 PM Scott Mayhew <smayhew@redhat.com> wrote:
>
> When using NFSv4.2, the security label for the root inode should be set
> via a call to nfs_setsecurity() during the mount process, otherwise the
> inode will appear as unlabeled for up to acdirmin seconds.  Currently
> the label for the root inode is allocated, retrieved, and freed entirely
> witin nfs4_proc_get_root().
>
> Add a field for the label to the nfs_fattr struct, and allocate & free
> the label in nfs_get_root(), where we also add a call to
> nfs_setsecurity().  Note that for the call to nfs_setsecurity() to
> succeed, it's necessary to also move the logic calling
> security_sb_{set,clone}_security() from nfs_get_tree_common() down into
> nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the
> super_block's security flags and nfs_setsecurity() will silently fail.
>
> Reported-by: Richard Haines <richard_c_haines@btinternet.com>
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Tested-by: Stephen Smalley <sds@tycho.nsa.gov>

Re: [PATCH] NFS: Ensure security label is set for root inode

[Paul Moore]

On Tue, Mar 10, 2020 at 11:53 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Tue, Mar 3, 2020 at 5:59 PM Scott Mayhew <smayhew@redhat.com> wrote:
> >
> > When using NFSv4.2, the security label for the root inode should be set
> > via a call to nfs_setsecurity() during the mount process, otherwise the
> > inode will appear as unlabeled for up to acdirmin seconds.  Currently
> > the label for the root inode is allocated, retrieved, and freed entirely
> > witin nfs4_proc_get_root().
> >
> > Add a field for the label to the nfs_fattr struct, and allocate & free
> > the label in nfs_get_root(), where we also add a call to
> > nfs_setsecurity().  Note that for the call to nfs_setsecurity() to
> > succeed, it's necessary to also move the logic calling
> > security_sb_{set,clone}_security() from nfs_get_tree_common() down into
> > nfs_get_root()... otherwise the SBLABEL_MNT flag will not be set in the
> > super_block's security flags and nfs_setsecurity() will silently fail.
> >
> > Reported-by: Richard Haines <richard_c_haines@btinternet.com>
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Tested-by: Stephen Smalley <sds@tycho.nsa.gov>

This all looks reasonable to me so I've merged it into selinux/next
(with some minor line width fixes); I was hoping some of the NFS guys
would lend an ACK but it has been well over a week with no objections
so I figure it is fair game.

Thanks for the patch, testing, and review everyone!

-- 
paul moore
www.paul-moore.com