| Message ID | 20200604200831.28866-1-stephen.smalley.work@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | libselinux: fix selinux_restorecon() statfs bug | expand |
Patch looks good to me, and I can confirm that it fixes the reported
problem upstream.
Tested-by: Jonathan Lebon <jlebon@redhat.com>
On Thu, Jun 4, 2020 at 4:08 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > As reported in https://github.com/SELinuxProject/selinux/issues/248, > setfiles -r (rootpath) fails when the alternate root contains a symlink > that is correct relative to the alternate root but not in the current root. > This is a regression introduced by commit e016502c0a26 ("libselinux: Save > digest of all partial matches for directory"). Do not call statfs(2) here > if acting on a symbolic link. Unfortunately there is no lstatfs() call. > Ensure that we initialize the statfs buffer always. If the supplied > file is a symlink, then we don't need to worry about the later tests of > filesystem type because we wouldn't be setting the digest anyway and > we are not performing a full sysfs relabel. While here, fix the earlier > test for a directory to use the correct test. > > Reproducer: > $ mkdir /root/my-chroot && echo foo > /root/my-chroot/link-target && ln -s /link-target /root/my-chroot/symlink > $ echo "/root/my-chroot/symlink" | setfiles -vFi -r /root/my-chroot -f - /etc/selinux/targeted/contexts/files/file_contexts > > Before: > setfiles: statfs(/root/my-chroot/symlink) failed: No such file or directory > > After: > Relabeled /root/my-chroot/symlink from unconfined_u:object_r:admin_home_t:s0 to system_u:object_r:default_t:s0 > > Fixes: https://github.com/SELinuxProject/selinux/issues/248 > Fixes: e016502c0a26 ("libselinux: Save digest of all partial matches for directory") > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> Anyone want to ack or object to this patch? > --- > libselinux/src/selinux_restorecon.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c > index 91dfeb66..d1ce830c 100644 > --- a/libselinux/src/selinux_restorecon.c > +++ b/libselinux/src/selinux_restorecon.c > @@ -938,7 +938,7 @@ int selinux_restorecon(const char *pathname_orig, > } > > /* Skip digest if not a directory */ > - if ((sb.st_mode & S_IFDIR) != S_IFDIR) > + if (!S_ISDIR(sb.st_mode)) > setrestorecondigest = false; > > if (!flags.recurse) { > @@ -952,7 +952,8 @@ int selinux_restorecon(const char *pathname_orig, > } > > /* Obtain fs type */ > - if (statfs(pathname, &sfsb) < 0) { > + memset(&sfsb, 0, sizeof sfsb); > + if (!S_ISLNK(sb.st_mode) && statfs(pathname, &sfsb) < 0) { > selinux_log(SELINUX_ERROR, > "statfs(%s) failed: %s\n", > pathname, strerror(errno)); > -- > 2.23.3 >
On Wed, Jun 10, 2020 at 11:56:28AM -0400, Stephen Smalley wrote: > On Thu, Jun 4, 2020 at 4:08 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > As reported in https://github.com/SELinuxProject/selinux/issues/248, > > setfiles -r (rootpath) fails when the alternate root contains a symlink > > that is correct relative to the alternate root but not in the current root. > > This is a regression introduced by commit e016502c0a26 ("libselinux: Save > > digest of all partial matches for directory"). Do not call statfs(2) here > > if acting on a symbolic link. Unfortunately there is no lstatfs() call. > > Ensure that we initialize the statfs buffer always. If the supplied > > file is a symlink, then we don't need to worry about the later tests of > > filesystem type because we wouldn't be setting the digest anyway and > > we are not performing a full sysfs relabel. While here, fix the earlier > > test for a directory to use the correct test. > > > > Reproducer: > > $ mkdir /root/my-chroot && echo foo > /root/my-chroot/link-target && ln -s /link-target /root/my-chroot/symlink > > $ echo "/root/my-chroot/symlink" | setfiles -vFi -r /root/my-chroot -f - /etc/selinux/targeted/contexts/files/file_contexts > > > > Before: > > setfiles: statfs(/root/my-chroot/symlink) failed: No such file or directory > > > > After: > > Relabeled /root/my-chroot/symlink from unconfined_u:object_r:admin_home_t:s0 to system_u:object_r:default_t:s0 > > > > Fixes: https://github.com/SELinuxProject/selinux/issues/248 > > Fixes: e016502c0a26 ("libselinux: Save digest of all partial matches for directory") > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > Anyone want to ack or object to this patch? Acked-by: Petr Lautrbach <plautrba@redhat.com> > > --- > > libselinux/src/selinux_restorecon.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c > > index 91dfeb66..d1ce830c 100644 > > --- a/libselinux/src/selinux_restorecon.c > > +++ b/libselinux/src/selinux_restorecon.c > > @@ -938,7 +938,7 @@ int selinux_restorecon(const char *pathname_orig, > > } > > > > /* Skip digest if not a directory */ > > - if ((sb.st_mode & S_IFDIR) != S_IFDIR) > > + if (!S_ISDIR(sb.st_mode)) > > setrestorecondigest = false; > > > > if (!flags.recurse) { > > @@ -952,7 +952,8 @@ int selinux_restorecon(const char *pathname_orig, > > } > > > > /* Obtain fs type */ > > - if (statfs(pathname, &sfsb) < 0) { > > + memset(&sfsb, 0, sizeof sfsb); > > + if (!S_ISLNK(sb.st_mode) && statfs(pathname, &sfsb) < 0) { > > selinux_log(SELINUX_ERROR, > > "statfs(%s) failed: %s\n", > > pathname, strerror(errno)); > > -- > > 2.23.3 > > >
On Wed, Jun 10, 2020 at 06:19:22PM +0200, Petr Lautrbach wrote: > On Wed, Jun 10, 2020 at 11:56:28AM -0400, Stephen Smalley wrote: > > On Thu, Jun 4, 2020 at 4:08 PM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > > > > As reported in https://github.com/SELinuxProject/selinux/issues/248, > > > setfiles -r (rootpath) fails when the alternate root contains a symlink > > > that is correct relative to the alternate root but not in the current root. > > > This is a regression introduced by commit e016502c0a26 ("libselinux: Save > > > digest of all partial matches for directory"). Do not call statfs(2) here > > > if acting on a symbolic link. Unfortunately there is no lstatfs() call. > > > Ensure that we initialize the statfs buffer always. If the supplied > > > file is a symlink, then we don't need to worry about the later tests of > > > filesystem type because we wouldn't be setting the digest anyway and > > > we are not performing a full sysfs relabel. While here, fix the earlier > > > test for a directory to use the correct test. > > > > > > Reproducer: > > > $ mkdir /root/my-chroot && echo foo > /root/my-chroot/link-target && ln -s /link-target /root/my-chroot/symlink > > > $ echo "/root/my-chroot/symlink" | setfiles -vFi -r /root/my-chroot -f - /etc/selinux/targeted/contexts/files/file_contexts > > > > > > Before: > > > setfiles: statfs(/root/my-chroot/symlink) failed: No such file or directory > > > > > > After: > > > Relabeled /root/my-chroot/symlink from unconfined_u:object_r:admin_home_t:s0 to system_u:object_r:default_t:s0 > > > > > > Fixes: https://github.com/SELinuxProject/selinux/issues/248 > > > Fixes: e016502c0a26 ("libselinux: Save digest of all partial matches for directory") > > > Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> > > > > Anyone want to ack or object to this patch? > > Acked-by: Petr Lautrbach <plautrba@redhat.com> Applied. > > > --- > > > libselinux/src/selinux_restorecon.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c > > > index 91dfeb66..d1ce830c 100644 > > > --- a/libselinux/src/selinux_restorecon.c > > > +++ b/libselinux/src/selinux_restorecon.c > > > @@ -938,7 +938,7 @@ int selinux_restorecon(const char *pathname_orig, > > > } > > > > > > /* Skip digest if not a directory */ > > > - if ((sb.st_mode & S_IFDIR) != S_IFDIR) > > > + if (!S_ISDIR(sb.st_mode)) > > > setrestorecondigest = false; > > > > > > if (!flags.recurse) { > > > @@ -952,7 +952,8 @@ int selinux_restorecon(const char *pathname_orig, > > > } > > > > > > /* Obtain fs type */ > > > - if (statfs(pathname, &sfsb) < 0) { > > > + memset(&sfsb, 0, sizeof sfsb); > > > + if (!S_ISLNK(sb.st_mode) && statfs(pathname, &sfsb) < 0) { > > > selinux_log(SELINUX_ERROR, > > > "statfs(%s) failed: %s\n", > > > pathname, strerror(errno)); > > > -- > > > 2.23.3 > > > > >
diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c index 91dfeb66..d1ce830c 100644 --- a/libselinux/src/selinux_restorecon.c +++ b/libselinux/src/selinux_restorecon.c @@ -938,7 +938,7 @@ int selinux_restorecon(const char *pathname_orig, } /* Skip digest if not a directory */ - if ((sb.st_mode & S_IFDIR) != S_IFDIR) + if (!S_ISDIR(sb.st_mode)) setrestorecondigest = false; if (!flags.recurse) { @@ -952,7 +952,8 @@ int selinux_restorecon(const char *pathname_orig, } /* Obtain fs type */ - if (statfs(pathname, &sfsb) < 0) { + memset(&sfsb, 0, sizeof sfsb); + if (!S_ISLNK(sb.st_mode) && statfs(pathname, &sfsb) < 0) { selinux_log(SELINUX_ERROR, "statfs(%s) failed: %s\n", pathname, strerror(errno));
As reported in https://github.com/SELinuxProject/selinux/issues/248, setfiles -r (rootpath) fails when the alternate root contains a symlink that is correct relative to the alternate root but not in the current root. This is a regression introduced by commit e016502c0a26 ("libselinux: Save digest of all partial matches for directory"). Do not call statfs(2) here if acting on a symbolic link. Unfortunately there is no lstatfs() call. Ensure that we initialize the statfs buffer always. If the supplied file is a symlink, then we don't need to worry about the later tests of filesystem type because we wouldn't be setting the digest anyway and we are not performing a full sysfs relabel. While here, fix the earlier test for a directory to use the correct test. Reproducer: $ mkdir /root/my-chroot && echo foo > /root/my-chroot/link-target && ln -s /link-target /root/my-chroot/symlink $ echo "/root/my-chroot/symlink" | setfiles -vFi -r /root/my-chroot -f - /etc/selinux/targeted/contexts/files/file_contexts Before: setfiles: statfs(/root/my-chroot/symlink) failed: No such file or directory After: Relabeled /root/my-chroot/symlink from unconfined_u:object_r:admin_home_t:s0 to system_u:object_r:default_t:s0 Fixes: https://github.com/SELinuxProject/selinux/issues/248 Fixes: e016502c0a26 ("libselinux: Save digest of all partial matches for directory") Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com> --- libselinux/src/selinux_restorecon.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)