| Message ID | 20190305211758.GA27437@fieldses.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | security/selinux: fix SECURITY_LSM_NATIVE_LABELS on reused superblock | expand |
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 >
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); > >
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); > >
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); >>>
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.
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.
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
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);