| Message ID | f4e6ddb4-66ac-45d1-04a6-67bfd9fd225e@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libselinux: mount selinuxfs nodev,noexec,nosuid | expand |
Topi Miettinen <toiwoton@gmail.com> writes: > Mount selinuxfs with mount flags nodev,noexec and nosuid. It's not > likely that this has any effect, but it's visually more pleasing. will nodev interfere with this? File: /sys/fs/selinux/null Size: 0 Blocks: 0 IO Block: 4096 character special file Device: 15h/21d Inode: 23 Links: 1 Device type: 1,3 Access: (0666/crw-rw-rw-) Uid: ( 0/ root) Gid: ( 0/ root) Context: sys.id:sys.role:null.isid:s0 Access: 2020-03-28 13:04:05.578999988 +0100 Modify: 2020-03-28 13:04:05.578999988 +0100 Change: 2020-03-28 13:04:05.578999988 +0100 Birth: - /sys/fs/selinux/null: character special (1/3) > > Signed-off-by: Topi Miettinen <toiwoton@gmail.com> > --- > libselinux/src/load_policy.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c > index fa1a3bf1..3e4020a9 100644 > --- a/libselinux/src/load_policy.c > +++ b/libselinux/src/load_policy.c > @@ -279,7 +279,7 @@ int selinux_init_load_policy(int *enforce) > const char *mntpoint = NULL; > /* First make sure /sys is mounted */ > if (mount("sysfs", "/sys", "sysfs", 0, 0) == 0 || errno == EBUSY) { > - if (mount(SELINUXFS, SELINUXMNT, SELINUXFS, 0, 0) == 0 > || errno == EBUSY) { > + if (mount(SELINUXFS, SELINUXMNT, SELINUXFS, MS_NODEV | > MS_NOEXEC | MS_NOSUID, 0) == 0 || errno == EBUSY) { > mntpoint = SELINUXMNT; > } else { > /* check old mountpoint */
On 29.3.2020 12.27, Dominick Grift wrote: > Topi Miettinen <toiwoton@gmail.com> writes: > >> Mount selinuxfs with mount flags nodev,noexec and nosuid. It's not >> likely that this has any effect, but it's visually more pleasing. > > will nodev interfere with this? > > File: /sys/fs/selinux/null > Size: 0 Blocks: 0 IO Block: 4096 character special file > Device: 15h/21d Inode: 23 Links: 1 Device type: 1,3 > Access: (0666/crw-rw-rw-) Uid: ( 0/ root) Gid: ( 0/ root) > Context: sys.id:sys.role:null.isid:s0 > Access: 2020-03-28 13:04:05.578999988 +0100 > Modify: 2020-03-28 13:04:05.578999988 +0100 > Change: 2020-03-28 13:04:05.578999988 +0100 > Birth: - > > /sys/fs/selinux/null: character special (1/3) That device does not give me joy. Yes, the patch prevents it from being used. But I didn't see any problems in the logs, even with something else mounted over it (adding InaccessiblePaths=/sys/fs/selinux/null to systemd unit files). The device file was added pretty early to Linux, perhaps it was needed then, but not anymore? Judging from internet searches, maybe it's only used by Android. They seem to use a forked version of libselinux anyway. -Topi
On Sun, Mar 29, 2020 at 7:30 AM Topi Miettinen <toiwoton@gmail.com> wrote: > > On 29.3.2020 12.27, Dominick Grift wrote: > > Topi Miettinen <toiwoton@gmail.com> writes: > > > >> Mount selinuxfs with mount flags nodev,noexec and nosuid. It's not > >> likely that this has any effect, but it's visually more pleasing. > > > > will nodev interfere with this? > > > > File: /sys/fs/selinux/null > > Size: 0 Blocks: 0 IO Block: 4096 character special file > > Device: 15h/21d Inode: 23 Links: 1 Device type: 1,3 > > Access: (0666/crw-rw-rw-) Uid: ( 0/ root) Gid: ( 0/ root) > > Context: sys.id:sys.role:null.isid:s0 > > Access: 2020-03-28 13:04:05.578999988 +0100 > > Modify: 2020-03-28 13:04:05.578999988 +0100 > > Change: 2020-03-28 13:04:05.578999988 +0100 > > Birth: - > > > > /sys/fs/selinux/null: character special (1/3) > > That device does not give me joy. Yes, the patch prevents it from being > used. But I didn't see any problems in the logs, even with something > else mounted over it (adding InaccessiblePaths=/sys/fs/selinux/null to > systemd unit files). The device file was added pretty early to Linux, > perhaps it was needed then, but not anymore? > > Judging from internet searches, maybe it's only used by Android. They > seem to use a forked version of libselinux anyway. /sys/fs/selinux/null is used by the kernel; SELinux closes any open file descriptors not authorized for the new process context upon a context-changing exec, and replaces them with a reference to /sys/fs/selinux/null. This was introduced because /dev/null couldn't be guaranteed to exist or be available at all times. nodev likely has no effect on this usage because it is probably only checked when a userspace process tries to open it. That said, I don't really understand what you think you are gaining by adding these mount options to selinuxfs. What threat are you mitigating? It is a kernel pseudo filesystem that doesn't support dynamic file creation by userspace and whose contents are entirely determined by the kernel.
On 29.3.2020 23.44, Stephen Smalley wrote: > On Sun, Mar 29, 2020 at 7:30 AM Topi Miettinen <toiwoton@gmail.com> wrote: >> >> On 29.3.2020 12.27, Dominick Grift wrote: >>> Topi Miettinen <toiwoton@gmail.com> writes: >>> >>>> Mount selinuxfs with mount flags nodev,noexec and nosuid. It's not >>>> likely that this has any effect, but it's visually more pleasing. >>> >>> will nodev interfere with this? >>> >>> File: /sys/fs/selinux/null >>> Size: 0 Blocks: 0 IO Block: 4096 character special file >>> Device: 15h/21d Inode: 23 Links: 1 Device type: 1,3 >>> Access: (0666/crw-rw-rw-) Uid: ( 0/ root) Gid: ( 0/ root) >>> Context: sys.id:sys.role:null.isid:s0 >>> Access: 2020-03-28 13:04:05.578999988 +0100 >>> Modify: 2020-03-28 13:04:05.578999988 +0100 >>> Change: 2020-03-28 13:04:05.578999988 +0100 >>> Birth: - >>> >>> /sys/fs/selinux/null: character special (1/3) >> >> That device does not give me joy. Yes, the patch prevents it from being >> used. But I didn't see any problems in the logs, even with something >> else mounted over it (adding InaccessiblePaths=/sys/fs/selinux/null to >> systemd unit files). The device file was added pretty early to Linux, >> perhaps it was needed then, but not anymore? >> >> Judging from internet searches, maybe it's only used by Android. They >> seem to use a forked version of libselinux anyway. > > /sys/fs/selinux/null is used by the kernel; SELinux closes any open > file descriptors not authorized for the new process context upon a > context-changing exec, and replaces them with a reference to > /sys/fs/selinux/null. This was introduced because /dev/null couldn't > be guaranteed to exist or be available at all times. nodev likely has > no effect on this usage because it is probably only checked when a > userspace process tries to open it. Perhaps then the device should not be visible to user space at all, or at least not usable (which is the effect of MS_NODEV)? The file descriptor replacement thing seems to work also when /sys{,/fs/selinux} is not mounted in the mount namespace of the process, at least I haven't seen any problems. > That said, I don't really understand what you think you are gaining by > adding these mount options to selinuxfs. What threat are you > mitigating? It is a kernel pseudo filesystem that doesn't support > dynamic file creation by userspace and whose contents are entirely > determined by the kernel. I don't think there's any change to threat situation (a process which shouldn't have access to /dev/null, gains access by using /sys/fs/selinux/null? Not very credible) or even any other effect from this change, but I don't like it when selinuxfs always shows up when I grep for filesystems without nodev/noexec/nosuid. So the gain is visual. What's the purpose and gain of having the filesystem mounted with dev,exec,suid, which for other filesystems than selinuxfs are the more dangerous options? -Topi
On Mon, Mar 30, 2020 at 2:14 AM Topi Miettinen <toiwoton@gmail.com> wrote: > > On 29.3.2020 23.44, Stephen Smalley wrote: > > On Sun, Mar 29, 2020 at 7:30 AM Topi Miettinen <toiwoton@gmail.com> wrote: > >> > >> On 29.3.2020 12.27, Dominick Grift wrote: > >>> Topi Miettinen <toiwoton@gmail.com> writes: > >>> > >>>> Mount selinuxfs with mount flags nodev,noexec and nosuid. It's not > >>>> likely that this has any effect, but it's visually more pleasing. > >>> > >>> will nodev interfere with this? > >>> > >>> File: /sys/fs/selinux/null > >>> Size: 0 Blocks: 0 IO Block: 4096 character special file > >>> Device: 15h/21d Inode: 23 Links: 1 Device type: 1,3 > >>> Access: (0666/crw-rw-rw-) Uid: ( 0/ root) Gid: ( 0/ root) > >>> Context: sys.id:sys.role:null.isid:s0 > >>> Access: 2020-03-28 13:04:05.578999988 +0100 > >>> Modify: 2020-03-28 13:04:05.578999988 +0100 > >>> Change: 2020-03-28 13:04:05.578999988 +0100 > >>> Birth: - > >>> > >>> /sys/fs/selinux/null: character special (1/3) > >> > >> That device does not give me joy. Yes, the patch prevents it from being > >> used. But I didn't see any problems in the logs, even with something > >> else mounted over it (adding InaccessiblePaths=/sys/fs/selinux/null to > >> systemd unit files). The device file was added pretty early to Linux, > >> perhaps it was needed then, but not anymore? > >> > >> Judging from internet searches, maybe it's only used by Android. They > >> seem to use a forked version of libselinux anyway. > > > > /sys/fs/selinux/null is used by the kernel; SELinux closes any open > > file descriptors not authorized for the new process context upon a > > context-changing exec, and replaces them with a reference to > > /sys/fs/selinux/null. This was introduced because /dev/null couldn't > > be guaranteed to exist or be available at all times. nodev likely has > > no effect on this usage because it is probably only checked when a > > userspace process tries to open it. > > Perhaps then the device should not be visible to user space at all, or > at least not usable (which is the effect of MS_NODEV)? The file > descriptor replacement thing seems to work also when /sys{,/fs/selinux} > is not mounted in the mount namespace of the process, at least I haven't > seen any problems. > > > That said, I don't really understand what you think you are gaining by > > adding these mount options to selinuxfs. What threat are you > > mitigating? It is a kernel pseudo filesystem that doesn't support > > dynamic file creation by userspace and whose contents are entirely > > determined by the kernel. > > I don't think there's any change to threat situation (a process which > shouldn't have access to /dev/null, gains access by using > /sys/fs/selinux/null? Not very credible) or even any other effect from > this change, but I don't like it when selinuxfs always shows up when I > grep for filesystems without nodev/noexec/nosuid. So the gain is visual. > > What's the purpose and gain of having the filesystem mounted with > dev,exec,suid, which for other filesystems than selinuxfs are the more > dangerous options? I don't think we can switch to nodev since we have been exposing that null device node forever and we know of at least one user (Android). Android started with a complete fork of libselinux but went back to following upstream, although they still retain their own custom additions. So I think at most we could add noexec/nosuid here with no risk of userspace breakage.
diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c index fa1a3bf1..3e4020a9 100644 --- a/libselinux/src/load_policy.c +++ b/libselinux/src/load_policy.c @@ -279,7 +279,7 @@ int selinux_init_load_policy(int *enforce) const char *mntpoint = NULL; /* First make sure /sys is mounted */ if (mount("sysfs", "/sys", "sysfs", 0, 0) == 0 || errno == EBUSY) { - if (mount(SELINUXFS, SELINUXMNT, SELINUXFS, 0, 0) == 0 || errno == EBUSY) { + if (mount(SELINUXFS, SELINUXMNT, SELINUXFS, MS_NODEV | MS_NOEXEC | MS_NOSUID, 0) == 0 || errno == EBUSY) { mntpoint = SELINUXMNT;
Mount selinuxfs with mount flags nodev,noexec and nosuid. It's not likely that this has any effect, but it's visually more pleasing. Signed-off-by: Topi Miettinen <toiwoton@gmail.com> --- libselinux/src/load_policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) } else { /* check old mountpoint */