[PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

In the case when we're reusing a superblock, selinux_sb_clone_mnt_opts()
fails to set set_kern_flags, with the result that
nfs_clone_sb_security() incorrectly clears NFS_CAP_SECURITY_LABEL.

The result is that if you mount the same NFS filesystem twice, NFS
security labels are turned off, even if they would work fine if you
mounted the filesystem only once.

("fixes" may be not exactly the right tag, it may be more like
"fixed-other-cases-but-missed-this-one".)

Cc: Scott Mayhew <smayhew@redhat.com>
Cc: stable@vger.kernel.org
Fixes: 0b4d3452b8b4 "security/selinux: allow security_sb_clone_mnt_opts..."
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 security/selinux/hooks.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f0e36c3492ba..5e9304567233 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -959,8 +959,11 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
 	BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
 
 	/* if fs is reusing a sb, make sure that the contexts match */
-	if (newsbsec->flags & SE_SBINITIALIZED)
+	if (newsbsec->flags & SE_SBINITIALIZED) {
+		if ((kern_flags & SECURITY_LSM_NATIVE_LABELS) && !set_context)
+			*set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
 		return selinux_cmp_sb_context(oldsb, newsb);
+	}
 
 	mutex_lock(&newsbsec->lock);
 
-- 
2.20.1

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[J. Bruce Fields]

Whoops, probably should have cc'd linux-nfs as well.--b.

On Tue, Mar 05, 2019 at 04:17:58PM -0500, bfields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> In the case when we're reusing a superblock, selinux_sb_clone_mnt_opts()
> fails to set set_kern_flags, with the result that
> nfs_clone_sb_security() incorrectly clears NFS_CAP_SECURITY_LABEL.
> 
> The result is that if you mount the same NFS filesystem twice, NFS
> security labels are turned off, even if they would work fine if you
> mounted the filesystem only once.
> 
> ("fixes" may be not exactly the right tag, it may be more like
> "fixed-other-cases-but-missed-this-one".)
> 
> Cc: Scott Mayhew <smayhew@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: 0b4d3452b8b4 "security/selinux: allow security_sb_clone_mnt_opts..."
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  security/selinux/hooks.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f0e36c3492ba..5e9304567233 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -959,8 +959,11 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>  	BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>  
>  	/* if fs is reusing a sb, make sure that the contexts match */
> -	if (newsbsec->flags & SE_SBINITIALIZED)
> +	if (newsbsec->flags & SE_SBINITIALIZED) {
> +		if ((kern_flags & SECURITY_LSM_NATIVE_LABELS) && !set_context)
> +			*set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
>  		return selinux_cmp_sb_context(oldsb, newsb);
> +	}
>  
>  	mutex_lock(&newsbsec->lock);
>  
> -- 
> 2.20.1
> 

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[Stephen Smalley]

On 3/5/19 4:17 PM, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> In the case when we're reusing a superblock, selinux_sb_clone_mnt_opts()
> fails to set set_kern_flags, with the result that
> nfs_clone_sb_security() incorrectly clears NFS_CAP_SECURITY_LABEL.
> 
> The result is that if you mount the same NFS filesystem twice, NFS
> security labels are turned off, even if they would work fine if you
> mounted the filesystem only once.
> 
> ("fixes" may be not exactly the right tag, it may be more like
> "fixed-other-cases-but-missed-this-one".)
> 
> Cc: Scott Mayhew <smayhew@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: 0b4d3452b8b4 "security/selinux: allow security_sb_clone_mnt_opts..."
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

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

Do you have some tests you are using for the selinux nfs support?  I 
have an open issue on the selinux-testsuite with an example script for 
running the regular selinux tests on a NFS mount but it can't fully 
succeed as noted there,
https://github.com/SELinuxProject/selinux-testsuite/issues/32

I've also have another script to test context= mount handling for nfs 
since that should take precedence over native labels; it looks like that 
might be broken again:

#!/bin/sh
cat > /etc/exports <<EOF
/home localhost(rw,no_root_squash,security_label)
EOF
exportfs -a
systemctl start nfs-server
mkdir -p /mnt/home
mount -t nfs -o vers=4.0,context=system_u:object_r:etc_t:s0 
localhost:/home /mnt/home
echo "Under NFSv4.0:"
ls -Za /mnt/home
touch /mnt/home/foo
ls -Z /mnt/home/foo
ls -Z /home/foo
rm /mnt/home/foo
umount /mnt/home
mount -t nfs -o vers=4.2,context=system_u:object_r:etc_t:s0 
localhost:/home /mnt/home
echo "Under NFSv4.2:"
ls -Za /mnt/home
touch /mnt/home/foo
ls -Z /mnt/home/foo
ls -Z /home/foo
rm /home/foo
umount /mnt/home
rmdir /mnt/home
rm /etc/exports
exportfs -ua
systemctl stop nfs-server

> ---
>   security/selinux/hooks.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f0e36c3492ba..5e9304567233 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -959,8 +959,11 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>   	BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>   
>   	/* if fs is reusing a sb, make sure that the contexts match */
> -	if (newsbsec->flags & SE_SBINITIALIZED)
> +	if (newsbsec->flags & SE_SBINITIALIZED) {
> +		if ((kern_flags & SECURITY_LSM_NATIVE_LABELS) && !set_context)
> +			*set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
>   		return selinux_cmp_sb_context(oldsb, newsb);
> +	}
>   
>   	mutex_lock(&newsbsec->lock);
>   
> 

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[J. Bruce Fields]

On Wed, Mar 06, 2019 at 09:34:43AM -0500, Stephen Smalley wrote:
> On 3/5/19 4:17 PM, J. Bruce Fields wrote:
> >From: "J. Bruce Fields" <bfields@redhat.com>
> >
> >In the case when we're reusing a superblock, selinux_sb_clone_mnt_opts()
> >fails to set set_kern_flags, with the result that
> >nfs_clone_sb_security() incorrectly clears NFS_CAP_SECURITY_LABEL.
> >
> >The result is that if you mount the same NFS filesystem twice, NFS
> >security labels are turned off, even if they would work fine if you
> >mounted the filesystem only once.
> >
> >("fixes" may be not exactly the right tag, it may be more like
> >"fixed-other-cases-but-missed-this-one".)
> >
> >Cc: Scott Mayhew <smayhew@redhat.com>
> >Cc: stable@vger.kernel.org
> >Fixes: 0b4d3452b8b4 "security/selinux: allow security_sb_clone_mnt_opts..."
> >Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 
> Do you have some tests you are using for the selinux nfs support?

No.  I'll ask around.  Trond or Anna, do either of you do any selinux
testing?

> I have an open issue on the selinux-testsuite with an example script
> for running the regular selinux tests on a NFS mount but it can't
> fully succeed as noted there,
> https://github.com/SELinuxProject/selinux-testsuite/issues/32

So if I just clone
https://github.com/SELinuxProject/selinux-testsuite.git and filter out
those known failures, would that be a good starting point?

> I've also have another script to test context= mount handling for
> nfs since that should take precedence over native labels; it looks
>  like that might be broken again:

Thanks for the report, I'll take a look.  That's before or after
applying this patch?  Assuming the former, do you have an idea how
recent a regression it is?

--b.

> #!/bin/sh
> cat > /etc/exports <<EOF
> /home localhost(rw,no_root_squash,security_label)
> EOF
> exportfs -a
> systemctl start nfs-server
> mkdir -p /mnt/home
> mount -t nfs -o vers=4.0,context=system_u:object_r:etc_t:s0
> localhost:/home /mnt/home
> echo "Under NFSv4.0:"
> ls -Za /mnt/home
> touch /mnt/home/foo
> ls -Z /mnt/home/foo
> ls -Z /home/foo
> rm /mnt/home/foo
> umount /mnt/home
> mount -t nfs -o vers=4.2,context=system_u:object_r:etc_t:s0
> localhost:/home /mnt/home
> echo "Under NFSv4.2:"
> ls -Za /mnt/home
> touch /mnt/home/foo
> ls -Z /mnt/home/foo
> ls -Z /home/foo
> rm /home/foo
> umount /mnt/home
> rmdir /mnt/home
> rm /etc/exports
> exportfs -ua
> systemctl stop nfs-server
> 
> >---
> >  security/selinux/hooks.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> >diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >index f0e36c3492ba..5e9304567233 100644
> >--- a/security/selinux/hooks.c
> >+++ b/security/selinux/hooks.c
> >@@ -959,8 +959,11 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
> >  	BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
> >  	/* if fs is reusing a sb, make sure that the contexts match */
> >-	if (newsbsec->flags & SE_SBINITIALIZED)
> >+	if (newsbsec->flags & SE_SBINITIALIZED) {
> >+		if ((kern_flags & SECURITY_LSM_NATIVE_LABELS) && !set_context)
> >+			*set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
> >  		return selinux_cmp_sb_context(oldsb, newsb);
> >+	}
> >  	mutex_lock(&newsbsec->lock);
> >

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[Stephen Smalley]

On 3/6/19 10:34 AM, J. Bruce Fields wrote:
> On Wed, Mar 06, 2019 at 09:34:43AM -0500, Stephen Smalley wrote:
>> On 3/5/19 4:17 PM, J. Bruce Fields wrote:
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>>
>>> In the case when we're reusing a superblock, selinux_sb_clone_mnt_opts()
>>> fails to set set_kern_flags, with the result that
>>> nfs_clone_sb_security() incorrectly clears NFS_CAP_SECURITY_LABEL.
>>>
>>> The result is that if you mount the same NFS filesystem twice, NFS
>>> security labels are turned off, even if they would work fine if you
>>> mounted the filesystem only once.
>>>
>>> ("fixes" may be not exactly the right tag, it may be more like
>>> "fixed-other-cases-but-missed-this-one".)
>>>
>>> Cc: Scott Mayhew <smayhew@redhat.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 0b4d3452b8b4 "security/selinux: allow security_sb_clone_mnt_opts..."
>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>>
>> Do you have some tests you are using for the selinux nfs support?
> 
> No.  I'll ask around.  Trond or Anna, do either of you do any selinux
> testing?
> 
>> I have an open issue on the selinux-testsuite with an example script
>> for running the regular selinux tests on a NFS mount but it can't
>> fully succeed as noted there,
>> https://github.com/SELinuxProject/selinux-testsuite/issues/32
> 
> So if I just clone
> https://github.com/SELinuxProject/selinux-testsuite.git and filter out
> those known failures, would that be a good starting point?

Yes, although you might want to remove the inet_socket and sctp tests 
from tests/Makefile before running it over NFS; it looks like there are 
some interactions between NFS and labeled networking (netlabel and 
ipsec/xfrm) that require further test policy changes to permit.

> 
>> I've also have another script to test context= mount handling for
>> nfs since that should take precedence over native labels; it looks
>>   like that might be broken again:
> 
> Thanks for the report, I'll take a look.  That's before or after
> applying this patch?  Assuming the former, do you have an idea how
> recent a regression it is?

Now I'm having difficulty reproducing it entirely.  I thought on stock 
Fedora 29 (4.20.x) I was seeing the actual underlying security labels 
leaking through on files within the NFS mount despite using a context= 
mount, while correctly seeing the context mount values with your patch, 
but now I can't seem to repro.  It was this bug that originally 
motivated Scott's commit that you are further fixing IIUC,
https://github.com/SELinuxProject/selinux-kernel/issues/35

> 
> --b.
> 
>> #!/bin/sh
>> cat > /etc/exports <<EOF
>> /home localhost(rw,no_root_squash,security_label)
>> EOF
>> exportfs -a
>> systemctl start nfs-server
>> mkdir -p /mnt/home
>> mount -t nfs -o vers=4.0,context=system_u:object_r:etc_t:s0
>> localhost:/home /mnt/home
>> echo "Under NFSv4.0:"
>> ls -Za /mnt/home
>> touch /mnt/home/foo
>> ls -Z /mnt/home/foo
>> ls -Z /home/foo
>> rm /mnt/home/foo
>> umount /mnt/home
>> mount -t nfs -o vers=4.2,context=system_u:object_r:etc_t:s0
>> localhost:/home /mnt/home
>> echo "Under NFSv4.2:"
>> ls -Za /mnt/home
>> touch /mnt/home/foo
>> ls -Z /mnt/home/foo
>> ls -Z /home/foo
>> rm /home/foo
>> umount /mnt/home
>> rmdir /mnt/home
>> rm /etc/exports
>> exportfs -ua
>> systemctl stop nfs-server
>>
>>> ---
>>>   security/selinux/hooks.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index f0e36c3492ba..5e9304567233 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -959,8 +959,11 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>>>   	BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>>>   	/* if fs is reusing a sb, make sure that the contexts match */
>>> -	if (newsbsec->flags & SE_SBINITIALIZED)
>>> +	if (newsbsec->flags & SE_SBINITIALIZED) {
>>> +		if ((kern_flags & SECURITY_LSM_NATIVE_LABELS) && !set_context)
>>> +			*set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
>>>   		return selinux_cmp_sb_context(oldsb, newsb);
>>> +	}
>>>   	mutex_lock(&newsbsec->lock);
>>>

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[J. Bruce Fields]

On Wed, Mar 06, 2019 at 11:49:36AM -0500, Stephen Smalley wrote:
> On 3/6/19 10:34 AM, J. Bruce Fields wrote:
> >On Wed, Mar 06, 2019 at 09:34:43AM -0500, Stephen Smalley wrote:
> >>I've also have another script to test context= mount handling for
> >>nfs since that should take precedence over native labels; it looks
> >>  like that might be broken again:
> >
> >Thanks for the report, I'll take a look.  That's before or after
> >applying this patch?  Assuming the former, do you have an idea how
> >recent a regression it is?
> 
> Now I'm having difficulty reproducing it entirely.  I thought on
> stock Fedora 29 (4.20.x) I was seeing the actual underlying security
> labels leaking through on files within the NFS mount despite using a
> context= mount, while correctly seeing the context mount values with
> your patch, but now I can't seem to repro.  It was this bug that
> originally motivated Scott's commit that you are further fixing
> IIUC,
> https://github.com/SELinuxProject/selinux-kernel/issues/35

For what it's worth, I can't reproduce.  (If I mount with
-overs=4.2,context=system_u:object_r:etc_t:s0 then ls -Z, I only see
system_u:object_r:etc_t:s0.)

--b.

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[Stephen Smalley]

On 3/6/19 12:25 PM, J. Bruce Fields wrote:
> On Wed, Mar 06, 2019 at 11:49:36AM -0500, Stephen Smalley wrote:
>> On 3/6/19 10:34 AM, J. Bruce Fields wrote:
>>> On Wed, Mar 06, 2019 at 09:34:43AM -0500, Stephen Smalley wrote:
>>>> I've also have another script to test context= mount handling for
>>>> nfs since that should take precedence over native labels; it looks
>>>>   like that might be broken again:
>>>
>>> Thanks for the report, I'll take a look.  That's before or after
>>> applying this patch?  Assuming the former, do you have an idea how
>>> recent a regression it is?
>>
>> Now I'm having difficulty reproducing it entirely.  I thought on
>> stock Fedora 29 (4.20.x) I was seeing the actual underlying security
>> labels leaking through on files within the NFS mount despite using a
>> context= mount, while correctly seeing the context mount values with
>> your patch, but now I can't seem to repro.  It was this bug that
>> originally motivated Scott's commit that you are further fixing
>> IIUC,
>> https://github.com/SELinuxProject/selinux-kernel/issues/35
> 
> For what it's worth, I can't reproduce.  (If I mount with
> -overs=4.2,context=system_u:object_r:etc_t:s0 then ls -Z, I only see
> system_u:object_r:etc_t:s0.)

Yes, sorry for the noise.

Re: [PATCH] security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock

[Paul Moore]

On Tue, Mar 5, 2019 at 4:18 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
>
> In the case when we're reusing a superblock, selinux_sb_clone_mnt_opts()
> fails to set set_kern_flags, with the result that
> nfs_clone_sb_security() incorrectly clears NFS_CAP_SECURITY_LABEL.
>
> The result is that if you mount the same NFS filesystem twice, NFS
> security labels are turned off, even if they would work fine if you
> mounted the filesystem only once.
>
> ("fixes" may be not exactly the right tag, it may be more like
> "fixed-other-cases-but-missed-this-one".)
>
> Cc: Scott Mayhew <smayhew@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: 0b4d3452b8b4 "security/selinux: allow security_sb_clone_mnt_opts..."
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  security/selinux/hooks.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Thanks for the fix.  I just merged this into selinux/stable-5.1 and
I'll send this up to Linus tomorrow.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f0e36c3492ba..5e9304567233 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -959,8 +959,11 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
>         BUG_ON(!(oldsbsec->flags & SE_SBINITIALIZED));
>
>         /* if fs is reusing a sb, make sure that the contexts match */
> -       if (newsbsec->flags & SE_SBINITIALIZED)
> +       if (newsbsec->flags & SE_SBINITIALIZED) {
> +               if ((kern_flags & SECURITY_LSM_NATIVE_LABELS) && !set_context)
> +                       *set_kern_flags |= SECURITY_LSM_NATIVE_LABELS;
>                 return selinux_cmp_sb_context(oldsb, newsb);
> +       }
>
>         mutex_lock(&newsbsec->lock);
>
> --
> 2.20.1

-- 
paul moore
www.paul-moore.com